I'm about to look at a project's PHP codebase for the first time, and I'd like to perform a semi-formal audit. Because I'm new to the project, this will not only give me a chance to get properly acquainted with the code, but also provide the organization an external opinion about the state of the project.
As PHP is one of my "secondary" languages (one I don't eat, drink and breathe on a daily basis) I'd like some feedback on how to proceed. Specifically, I'm wondering about:
I've never audited code, but I can think of a few code smells that should help you on your way.
Error Suppression — From php.net:
PHP supports one error control operator: the at sign (@). When prepended to an expression in PHP, any error messages that might be generated by that expression will be ignored.
Do yourself a favor and perform a find->all in your IDE for the @ symbol and divide and conquer.
Error Notices — Along the same vein as above, PHP allows you to set your error reporting level at runtime. Many developers suppress simple warnings and notices. You should use the
error_reporting() function with a parameter of
E_STRICT to display all PHP errors:
The global keyword — PHP allows programmers to define global variables to break function scope. Scope is a little upside down in PHP... If you create a variable outside of a function, it can be modified inside the function if it has been declared inside the function as global. That smells.
Deprecated Features — PHP is rife with functions and features, and many of them have been deprecated.
mysql_query() — If the devs haven't used or developed some kind of database abstraction, you're going to see a bunch of hard-coded database queries littered all over the place. This should probably be moved to Code Smell #1.
Lots of Static Methods — maybe a code smell in any language, a developer can effectively globalize everything by making all of his classes/methods static. Per Jeremy Walton's suggestion, search for the word static and the Paamayim Nekudotayim operator (
Complex String Syntax and Variable Parsing — hard to explain on a dime, check out the reference here: http://php.net/manual/en/language.types.string.php Look for the section on Variable parsing a few screens down. If this is abused it can be a nightmare to debug and/or understand.
Variable Variables — another whopper. It's a technique whereby the value of a variable is used to lookup a reference to another variable. Just search for two dollar signs
$$ to identify if they've succumbed to the Dark Lord.
require_once() — These functions are fine, to be sure. They are very useful when templating and separating view logic. They ensure that a file is included only once when scripts are parsed. However, if you see them used liberally in the business logic portion of the application, this is a red flag that the developer has written nested/circular includes, and may not be aware what portions of code have already been included. This isn't so bad per se, but it's definitely a window into the competency-level of the developer(s). Finally, @Phil suggests the use of autoloaders instead of
require_once(). This article explains autoloading in good detail.
String and Redundant Booleans — a lot of bad PHP code looks like this
if ($is_true == "true"). This is easy to search for, and harder to fix. Something else to look out for--though not a PHP specific issue--is Redundant booleans:
return ($isBad) ? true : false;.
Short Tags — Here's what PHP.net has to say about short tags:
Using short tags should be avoided when developing applications or libraries that are meant for redistribution, or deployment on PHP servers which are not under your control, because short tags may not be supported on the target server. For portable, redistributable code, be sure not to use short tags.
ASP style tags are even worse.
Accidental Assignment — More often than not, you'll see this happen in some non-critical component of the code, where it can sleep and/or lurk until you get an unexpected result one day:
if ($foo = "Some Value"). This always evaluates
true, of course, and was most likely due to a typo. However, in rare circumstances you'll actually want to test for assignment. If that is the case you should use double parenthesis to indicate intent:
if (($foo = $this->bar()))