1
\$\begingroup\$

This is my second serious project using php, I made a groupme bot before but the only php was to write the post data to a file and run a bash script that did all of the work. I want to know if I did anything in this that is considered bad practice (I most likely did) and if anything I did caused potential security issues. I commented the code a ton so it should be pretty easy to follow. I'll post the github link because it is several files. Before someone mentions it, I made the storage and settings files php files so they cant be read by anyone knowing the call url and so they are easier to include.

I plan on adding support for responses based on the user ID and adding an ability to get the user ID of someone by doing a command like /getuserid "name". I don't really want someone to write it for me because I like making this all from scratch but I'd appreciate advice on the implementation.

Is there a better method for storing the admin and responses array in php without using SQL? I don't know storage methods in php besides making include files. Also, is there a better method for containing strings in the command function? I used a method that takes whatever is between quotes but that means that I can't use a quote in the string...

There are probably many spelling errors in the readme and comments because I commented most of it when I was dead tired so it would be done before I went to sleep.


bot.php

<?php
//Includes
include 'config.php';
include 'admins.php';
include 'responses.php';
//The following lines get the message info from groupme's post to this url and put them into variables
$data = json_decode(file_get_contents('php://input'));
$name = $data->name;
$text = $data->text;
$sender_type = $data->sender_type;
$sender_id = $data->sender_id;
//Logging
if ($log == 1) {
    //Checks to make sure logging dir exists
        if (!is_dir($logdir)) {
        //Makes the logging dir if it doesn't exist and sets it to the specified chmod
                mkdir($logdir, $logdirchmod);
        }
    //Puts the message in log file in format "Sender ID(Name) : Message"
    file_put_contents($logdir.$logfile, "$sender_id($name) : $text \n", FILE_APPEND | LOCK_EX);
}
//Checks to see if the type is a user and only handles messages from users to prevent infinite loops, also doesn't respond to commands
if ($sender_type == "user" && $text[0] !== "/") {
    basic_response($text, $responses);
    //Checks to see if text contains weather and WU variables are set before calling the weather reaponse fucntion
    if (stripos($text, "weather") !== FALSE) {
        if (isset($wutoken) && isset($wulocation)) {
            weather_response($wutoken, $wulocation);
        }
    }
}
//commands function, first checks to see if a message contains a / at the beginning
if ($text[0] == "/" && in_array($sender_id,$admins)) {
    //Initialezes message as null
    $message = null;
    //Splits the input contained in quotes into elements of array "commands"
    preg_match_all('`"([^"]*)"`', escapeshellcmd($text), $command);
    if ($text == "/help") {
        //Help message
        $message = '
        /responses lists current respones
        /addresponse makes a new response in the format "/addresponse "phrase" "response""
        /delresponse deletes a reaponse in the format "/delresponse "phrase""
        /editresponse edits a response in the format "/editresponse "phrase" "response""
        /admins lists current admins by user id
        /addadmin adds an admin in the format "/addadmin "userid""
        /deladmin delets an admin in the format "/deladmin "userid""
        ';
    } else if ($text == "/responses") {
        //Goes through the array of responses and puts each set on one line
        foreach($responses as $val) {
            $message .= "$val[0] -> $val[1]\n";
        }
    } else if (strpos($text, '/addresponse') !== FALSE) {
        //Checks to see if the arguments are set correctly before continuing
        if (isset($command[1][0]) && $command[1][0] !== "" && isset($command[1][1]) && $command[1][1] !== "" && !isset($command[1][2])) {
            //Checks to see if that response is already in the responses file first
            if (search_array($command[1][0], $responses)) {
                $message = 'There is already a response for that phrase or a response that contains that phrase';
            } else {
                //Creates and formats the new response
                $newresponse = '['.$command[0][0].', '.$command[0][1].'],';
                //Adds the new response to the responses file
                file_put_contents('responses.php',str_replace('$responses = [', "\$responses = [\n\t$newresponse", file_get_contents('responses.php')));
				$message = 'Added response '.$command[0][0].' -> '.$command[0][1];
			}
		} else {
			$message = 'Invalid input';
		}
	} else if (strpos($text, '/delresponse') !== FALSE) {
		//Checks to see if the arguements are set correctly
		if (isset($command[1][0]) && $command[1][0] !== "" && !isset($command[1][1])) {
			//Checks to see if that response is already in the responses file first
			if (search_array($command[1][0], $responses)) {
				//loads current responses by reading the responses.php file
				$currentresponses = file('responses.php');
				//Reads each line of the responses file (now in array currentresponses)
				foreach ($currentresponses as $linenumber => $line) {
					//Checks to see if the line contains the response that is being deleted
					if (strpos($line, $command[0][0]) !== FALSE) {
						//Once that response is found, loads the contents of the responses file into new variable newresponses
						$newresponses = file_get_contents('responses.php');
						//Deletes appropriate line
						$newresponses = str_replace($line, '', $newresponses);
						//Writes the modified version back to the responses.php file
						file_put_contents('responses.php', $newresponses);
						$message = 'Deleted response for '.$command[0][0];
					}
				}
			} else {
				$message = 'There is no response for the phrase '.$command[0][0].', nothing to delete';
			}
		} else {
			$message = 'Invalid input';
		}
	} else if (strpos($text, '/editresponse') !== FALSE) {
		//Checks to see if the arguments are set correctly
		if (isset($command[1][0]) && $command[1][0] !== "" && isset($command[1][1]) && $command[1][1] !== "" && !isset($command[1][2])) {
			//Checks to see if that response is already in the responses file first
			if (search_array($command[1][0], $responses)) {
				//If the response is found in the responses file, reads it into array currentresponses
				$currentresponses = file('responses.php');
				//Reads responses.php file line by line
				foreach ($currentresponses as $linenumber => $line) {
					//Finds the line containing the response to be modified
					if (strpos($line, $command[0][0]) !== FALSE) {
						//Once the response is found, loads the contents of the responses file into new variable newresponses
						$newresponses = file_get_contents('responses.php');
						//Replaces the line with a new line containing the appropriate response information
						$newresponses = str_replace($line, "\t[".$command[0][0].', '.$command[0][1]."],\n", $newresponses);
						//Writes the modified version back to the responses.php file
						file_put_contents('responses.php', $newresponses);
						$message = 'Edited response '.$command[0][0].' -> '.$command[0][1];
					}
				}
			} else {
				$message = 'That phrase does not have a response to edit';
			}
		} else {
			$message = 'Invalid input';
		}
	} else if ($text == "/admins") {
		//Goes through the array of admins and puts each user id on one line
		foreach($admins as $val) {
			$message .= "$val\n";
		}
	} else if (strpos($text, '/addadmin') !== FALSE) {
		//Checks to see if the arguments are set correctly before continuing
		if (isset($command[1][0]) && $command[1][0] !== "" && !isset($command[1][1])) {
			//Checks to see if that user is already in the admins file first
			if (in_array($command[1][0], $admins)) {
				$message = 'That user is already an admin';
			} else {
				//Adds the new user id to admin file
				file_put_contents('admins.php',str_replace('$admins = [', "\$admins = [\n\t".$command[0][0].', ', file_get_contents('admins.php')));
                $message = 'Added admin with user id '.$command[1][0].' to the admins file';
            }
        } else {
            $message = 'Invalid input';
        }
    } else if (strpos($text, '/deladmin') !== FALSE) {
        //Checks to see if the arguements are set correctly
        if (isset($command[1][0]) && $command[1][0] !== "" && !isset($command[1][1])) {
            //Checks to see if that user id is an admin
            if (in_array($command[1][0], $admins)) {
                //loads current admins by reading the admins.php file
                $currentadmins = file('admins.php');
                //Reads each line of the admins file (now in array currentadmins)
                foreach ($currentadmins as $linenumber => $line) {
                    //Checks to see if the line contains the user id that is being deleted
                    if (strpos($line, $command[0][0]) !== FALSE) {
                        //Once that user id is found, loads the contents of the admins file into new variable newadmins
                        $newadmins = file_get_contents('admins.php');
                        //Deletes appropriate line
                        $newadmins = str_replace($line, '', $newadmins);
                        //Writes the modified version back to the admin.php file
                        file_put_contents('admins.php', $newadmins);
                        $message = 'Removed admin with user id '.$command[0][0];
                    }
                }
            } else {
                $message = 'User '.$command[0][0].' is not an admin, nothing to delete';
            }
        } else {
            $message = 'Invalid input';
        }
    } else {
        $message = 'Command not found';
    }
    send($message, null, null);
}
//Send function, token should be the groupme bot token
//Message is the message you want to send
//attachments should be the attachments you want to send, this should be a string if it's a url and an array where the 0 element is the user id and the 1 element is the location of the username in format [start,end]
//attachmenttype should be 'image' or 'mentions
function send($message, $attachments, $attachmenttype) {
    include 'config.php';
    //Sets the basic postdata arguments
    $postdata = [
        'bot_id' => $token,
        'text' => $message,
    ];
    //This is only run if there are attachments
    if (isset($attachments)) {
        //Sets the attachment type to image and sets the url to the $attachments argument
        if ($attachmenttype == 'image') {
            $attachments = [
                'type' => 'image',
                'url' => $attachments
            ];
        } else if ($attachmenttype == 'mentions') {
            //Sets the attachments type to mentions and adds the user id's and location
            $attachments = [
                'type' => 'mentions',
                'user_ids' => $attachments[0],
                'loci' => attachments[1]
            ];
        }
        //Adds the attachments element to the postdata and then adds the attachments to that element
        $postdata['attachments'] = [$attachments];
    }
    //Encodes the postdata in json format then adds single quotes around it
    $postdata = escapeshellarg(json_encode($postdata));
    `curl -s -X POST -d $postdata -H 'Content-Type: application/json' https://api.groupme.com/v3/bots/post`;
}
//This function is used for the commands ability, it is designed to simply read the first element of each element in an array and check to see if that is equal to the supplied needle
function search_array($needle, $haystack) {
    $exists = 0;
    foreach ($haystack as $element) {
        if (stripos($needle, $element[0]) !== FALSE) {
            $exists = 1;
        }
    }
    return $exists;
}
//Basic response function
function basic_response($text, $responses) {
    $element = 0;
    //Goes through responses array
    foreach ($responses as $catch) {
        //Finds if a phrase you are looking for is in the text to a message
        if (stripos($text, $catch[0]) !== FALSE) {
            //If a phrase you have set is found, this sets the message to be the appropriate response
            $message = $responses[$element][1];
            send($message, null, null);
        }
        $element++;
    }
}
//Weather function
function weather_response($token, $location) {
    //Gets the conditions for your area
    $rawweather = json_decode(`curl -s http://api.wunderground.com/api/$token/conditions/q/$location.json`);
    //makes variables for condition information
    $temperature = $rawweather->current_observation->feelslike_string;
    $weather = $rawweather->current_observation->weather;
    $icon = $rawweather->current_observation->icon_url;
    //Compiles this information into a post for the forecast
    $forecast = "The weather is $weather with a temperature of $temperature";
    //Encodes the forecast into json and adds the token and the weather icon
    send($forecast, $icon, 'image');
}

config.php:

<?php
//Set this to the token/bot ID of your bot, ex: 1252346142346243 or whatever your code is
$token = "yourtoken";
//Set this to your Weather Underground API token, ex: 42362362362 or whatever your code is
//$wutoken = "yourwutoken";
//Wunderground location string in format "STATEABBREVIATION/CITY_NAME", ex: TX/College_Station
//$wulocation = "youtwulocation";
//Set this to 1 to make a chat log under specified dir
$log = 1;
//Name for log file
$logfile = "log";
//Directory for logs, add slash at beginning for absolute path, there must be a slash at the end
$logdir = "logs/";
//chmod settings for dir
$logdirchmod = 0777;

admins.php:

<?php
$admins = [
    "youruserid"
];

responses.php:

<?php
//Add responses here in the format ["catch", "response"],
$responses = [
    ["abc", "123"],
    ["fish", "mmmm"],
        ["test", "It works!"],
];
\$\endgroup\$
1
\$\begingroup\$

Consistency

    if ($text == "/help") {

Sometimes, like here, you use double quotes. Other places, you use single quotes. It's not immediately obvious to me what the rule you are using is.

    if ($text == '/help') {

I'd use single quotes here and everywhere that I can. I only use double quotes for variable interpolation, or rarely, to avoid escaping single quotes in the string.

Some places you use a four-column indent and some places eight. Although eight seems excessive to me, this is inconsistent. It would be better to have eight everywhere, although I would prefer four everywhere.

Multi-line strings

        $message = '
        /responses lists current respones
        /addresponse makes a new response in the format "/addresponse "phrase" "response""
        /delresponse deletes a reaponse in the format "/delresponse "phrase""
        /editresponse edits a response in the format "/editresponse "phrase" "response""
        /admins lists current admins by user id
        /addadmin adds an admin in the format "/addadmin "userid""
        /deladmin delets an admin in the format "/deladmin "userid""
        ';

It is generally considered a best practice to use a nowdoc or heredocfor a multiline string.

        $message = <<<'EOHELP'
  /responses lists current responses
  /addresponse makes a new response in the format "/addresponse "phrase" "response""
  /delresponse deletes a response in the format "/delresponse "phrase""
  /editresponse edits a response in the format "/editresponse "phrase" "response""
  /admins lists current admins by user id
  /addadmin adds an admin in the format "/addadmin "userid""
  /deladmin deletes an admin in the format "/deladmin "userid""
EOHELP;

I used a nowdoc here, as it is the equivalent of a single quoted string.

In this particular case, this also means that we don't have to worry about words with apostrophes here, e.g. "it's" or "can't." Such a word would have broken the original version.

I reduced the indent, as it didn't seem necessary. A side benefit is that it makes that code block not require scrolling on my screen.

I fixed some typos as I went.

Don't repeat work unnecessarily

            foreach ($currentresponses as $linenumber => $line) {
                //Checks to see if the line contains the response that is being deleted
                if (strpos($line, $command[0][0]) !== FALSE) {
                    //Once that response is found, loads the contents of the responses file into new variable newresponses
                    $newresponses = file_get_contents('responses.php');
                    //Deletes appropriate line
                    $newresponses = str_replace($line, '', $newresponses);
                    //Writes the modified version back to the responses.php file
                    file_put_contents('responses.php', $newresponses);
                    $message = 'Deleted response for '.$command[0][0];
                }
            }

There would seem to be two possibilities here. If only one response with the given phrase can exist, then you can add a break at the end.

If there can be multiple matching lines, then why load and save the file each time?

            $newresponses = file_get_contents('responses.php');

            foreach ($currentresponses as $linenumber => $line) {
                //Checks to see if the line contains the response that is being deleted
                if (strpos($line, $command[0][0]) !== FALSE) {
                    $newresponses = str_replace($line, '', $newresponses);
                    $message = 'Deleted response for '.$command[0][0];
                }
            }

            file_put_contents('responses.php', $newresponses);

This loads and saves the file once, no matter how many modifications are made.

If this is often called with a phrase that is not present, you can save time by lazy loading the file only when the first is found.

                    if (!isset($newresponses)) {
                        $newresponses = file_get_contents('responses.php');
                        if (!isset($newresponses)) {
                            // if still not set, the phrase will not be found
                            // so no reason to continue
                            break;
                        }
                    }

This way it just does a simple check rather than loading a file.

I removed some of the comments as being overly obvious. You shouldn't have to comment a line that loads a file saying that it loads a file. Concentrate on commenting why you do things rather than on what you are doing.

Future improvements

Consider making things like deleting a response into their own methods. The current method is quite long. More, shorter methods would be easier to read.

Consider using a database for storage rather than files. Particularly for security reasons, don't name storage files with the .php extension.

Consider making classes to represent things like storage, responses, etc.

\$\endgroup\$
  • \$\begingroup\$ Wow thanks for this amazing answer, it helps a lot. I considered using a sql database as it is much cleaner than a few files but the one reason I don't want to do that is because I am trying to make it purely PHP so anyone can use it on any webhost with PHP. I know many webhosts with PHP will also have SQL but many is not all. I made another project last night (github.com/desultory/RandomYoutubeVideo) and learned how to serialize data for storage which in my opinion is a much cleaner method of array storage and may implement it in this application. The only problem with it is that \$\endgroup\$ – Desultory Aug 16 '17 at 8:05
  • \$\begingroup\$ it doesn't add the <php at the beginning so it can be read by anyone knowing the URL and it is also way less human readable. Do you know a good in-between that is human readable, secure, and a simple/good practice? I know SQL is but like I said, I want to refrain from having SQL in "portable" applications like these. \$\endgroup\$ – Desultory Aug 16 '17 at 8:07

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.