Here are some comments about the code -
Site specific values (domain, base title - vms vs vcs, …) should be defined in a config (configuration) file, not hard-coded throughout the project.
There are two main programming differences when updating old database code -
- Php no longer provides any attempt to protect against sql special characters in string data being able to break the sql syntax,
- Exceptions are used for errors by default, for the connection, query(), exec(), prepare(), and execute() calls.
For item #1, the simplest, general solution, that provides protection for all data types, not just strings, is to use prepared queries. If it seems like using the mysqli extension with prepared queries is overly complicated and inconsistent, it is. This would be a good time to switch to the much simpler and better designed PDO extension.
For item #2, using exceptions for errors actually simplifies your code, since you can remove any existing discrete error checking logic, because it will never get executed upon an error. With exceptions, if execution continues past a statement that can throw an exception, you know there was no error without needing any logic to test the result of the statement. The only database exceptions you should catch and handle in your code are for user recoverable errors, such as when inserting/updating duplicate user submitted data. For all other database errors and all other types of queries, simply do nothing in your code and let php catch and handle any database exception.
Security -
- The admin operations don’t seem to have any user authentication requirement. Anyone can request one of the admin pages and can see any data and perform any admin operation.
- Sql queries - see the point above about prepared queries.
- Apply htmlentities() when outputting dynamic values in a html context - as already posted.
Operational -
- Do NOT select the highest data value (member_number), increment it, and use it. This is not concurrent safe. Use an autoincrement primary index.
Implementation -
- The form processing code and the form, for any operation, should be on the same page.
- The code for any page should be laid out in this general order - 1) initialization, 2) post method form processing, 3) get method business logic - get/produce data needed to display the page, 4) Html document.
- Post method form processing code should only deal with processing the form data. After successful completion of the post method form processing code, you should perform a redirect to the exact same URL of the current page to cause a get request for that page. This will prevent the browser from trying to resubmit the form data should that page get browsed back to or reloaded. To display a one-time success message, store the message or a flag value in a session variable, then test for the session variable and display the success message at the appropriate location in the html document.
- Post method form processing code needs to detect if a post method form was submitted before referencing any of the form data.
- You need to trim input data, mainly so that you can detect if all white-space characters were entered, before validating it.
- You need to separately validate all inputs before using them.
- You should use an array to hold user/validation errors, with the array index being the field name.
- After the end of the validation logic, if there are no errors (the array holding the user/validation errors is empty), use the submitted data. If there are errors, the code will continue on to (re)display the html document, where you can test for and either display all the user/validation errors at once, or display them adjacent to the field they correspond with.
- Don’t use a loop to fetch what will be at most one row of data from a query. Just directly fetch, test if there is data, and use the single row of data.
- If your data is properly indexed, there’s no need for LIMIT 1 in the queries.
- If a query doesn’t match any data, output a message stating so, instead of doing nothing.
- The foreach() loop at the start of access.php does nothing and should be removed.
- Use ‘require’ for things the code must have.
- Include/require are not functions. The () around the path/file don’t do anything and should be removed.
- Don’t copy variables to other variables for nothing. Just use the original variable that data is in.
- You need to validate the resulting web pages at validator.w3.org
- Don’t echo blocks of static markup. Simply drop out of php mode and put the markup in-line.
- There’s no need to use multiple versions of variable names, such as $sql_c, $result_c, $row_c for every different operation. You should deal with the result from one query before going on to the next query.
- Don’t run quires inside of loops. Use an appropriate type of JOIN query.
- There’s generally no need to close database connections in your code since php destroys all resources on a page when your script ends.
- It is current practice to leave out any closing ?> php tag at the end of files.
- To cause a form to submit to the same page it is on, simply leave out the entire action attribute.
- Don’t change the name/meaning of data. The tag_search_field is actually the rfid_tag. Just use rfid_tag throughout the code.
- Don’t name databases, tables, columns, variables, … with part of the name of the application, e.g. ‘vms’. Just use general names.
- Date or date/time columns should be a proper date or datetime data type, not a character data type.
- You should always list out the columns in an INSERT query.
- You can put php variables directly into over-all double-quoted strings.
