The code at the linked to page looks like it was pulled together from elsewhere (the registration and login code are different programming styles.) The line in question was probably attempting to convert any single-quotes to their html entity value (which should have been done via the htmlentities call, with an appropriate flag, a few lines prior to this line of code), which when it got published and then copied to the getcodeify… site resulted in the html entity for a single-quote looking like an actual single-quote.
The following is a laundry list of practices that the linked to code should/should-not be doing -
- Don’t use defined constants for database connection credentials. This limits your code to ONE database connection.
- Any variable/defined constant should be uniquely named as to what it is for. For the database credentials, the name should include ‘db’ or similar.
- Use ‘require’ for things that your code must have for it to work.
- There’s no consistent or useful error handing for database statements that can fail - connection, query, prepare, and execute. The easiest way of adding error handling for all the database statements, without adding code at each statement (which will also let you remove the existing error handling logic), is to use exceptions for database statement errors and in most cases let php catch and handle the exception, where php will use its error settings to control what happens with the actual error information (database statement errors will ‘automatically’ get displayed/logged the same as php errors.) The exception to this rule is when inserting/updating user submitted data. In this case, your code should catch the exception, detect if the error number is for something that your code is designed to handle, setup and display a message telling the user what was wrong with the data. For all other error numbers, just re-throw the exception and let php handle it.
- Switch to the much simpler and more consistent PDO database extension.
- If you build the sql query statement in a php variable, it makes debugging easier. This also separates the sql syntax as much as possible from the php code, reducing mistakes, and also lets you see the common php code being used for queries that are candidates for reduction by extending the database extension you are using with useful methods.
- Other than trimming user submitted data, mainly for detecting if all white-space characters were entered, do NOT modify user submitted data, as this changes the meaning of the data. Validate the data and only use it if it is valid. If it is not valid, tell the user what was wrong with it and let them correct it.
- Don’t waste time attempting to ‘sanitize’ data to try to make it safe. Instead, use data safely in whatever context it is being used in - html, sql, mail header, …
- The Author of this script apparently missed the point that external data can be anything and cannot be trusted. You MUST validate data on the server before using it.
- A great point about BOOLEAN values is you can directly test them in conditional statements. This code has a number of loose tests against a true value that are just needless typing.
- This code is using preg_replace on values being fetched from a query. Simply amazing. See the above points about not modifying data and using data safely in context (at the point where it is being used.)
- The only piece of user data that should be stored in a session variable upon successful login is the user’s id (auto-increment primary index.) Then, query to get any other user data on each page request. This will allow the user data to be edited and the change will take effect on the very next page request.
- The code is filled with copying variables to other variables for nothing. Programming is not an exercise to see how much unnecessary typing you can do.
- The whole esc_url function is pointless. To get a form to submit to the same page, just leave the entire action=‘…’ attribute out of the form tag. Don’t even attempt to echo PHP_SELF/REQUEST_URI values.
- A session can be used for more than just remembering who the logged in user is. The logout code should only unset the session variable(s) that the login code was responsible for.
- The logout code should detect if a post method form was submitted before doing anything. Attempting to delete cookies is pointless. All this does is set cookie’s time in the past so that the browser won’t send it with requests. The cookie still exists on the client until it is physically deleted on the client computer.
- This code has some of the form and the form processing on separate pages. This results in a poor user experience, is less secure, takes more code, and is redirecting all over the place (which opens a site up to phishing). Put the form and form processing on the same page. This will simplify the code and allow you to re-populate appropriate form fields with their existing values so that the user doesn’t need to keep reentering the same things over and over.
- The only redirect in any post method form processing should be upon successful completion of the form processing code and it should be to the exact same url of the current page to cause a get request for that page. If you want to display a one-time success message, store it in a session variable, then test, display, and unset the session variable at the appropriate location in the html document.
- Validation/user error messages should be stored in an array, using the field name as the array index. This will let you test and perform dependent validation steps on inputs and will let you output error messages adjacent to the inputs they go with. Only the text of the message should be stored in the array. Any markup/styling should be handled at the point of displaying the error.
- The database MUST enforce uniqueness. Any column that must contain unique values must be defined as a unique index. This will cause an error upon inserting/updating duplicate values. To avoid repetitive logic, you can and should use this in your code to detect if duplicate values where entered by simply trying to insert the data, rather then first trying to use a select query to find if data already exist.
- Any dynamic value that is output on a web page should have htmlentities applied to it, right before being output.