Hi everyone,
Yesterday, I merged in a branch to re-enable PHP notices in our development environments and to fix quite a few of them (I've updated more than 80 files in the process). I committed a few follow-up branches to fix broken tests. As it currently stands, I believe I've fixed all of the selenium tests, but I can't tell, because the test runner is currently blocked by a few jobs on bigmac. I also can't tell if simpletest is red or green, because I can't access Hudson (it's after hours, so couldn't get Mike to whitelist my IP). I may be blocking you from merging tomorrow, but please don't merge until the tests are resolved. I believe the effort will be worth the benefits. PHP notices have been disabled for quite a long time in our dev environments. I believe that re-enabling PHP notices should lead to much better quality code and result in fewer bugs. As part of our development process, we should keep an eye on two log files where the PHP notices will appear.
- /var/log/drupal.log
- /var/log/php-fpm/www-error.log
As I went through the PHP notices to try and fix the most egregious ones (or ones that resulted in failed tests), there were some really basic patterns I saw that we were taking too many liberties with (and yes, I've been guilty of all of these myself). Below are a few:
- Undefined variables - make sure a variable is defined before you try to reference it. There are a lot of
if ($variable) { }
statements throughout the codebase. If $variable hasn't been declared and initialized, then this will result in a php notice if $variable isn't set. Declare your variables or use isset() or !empty() to check if they are declared.
- Undefined indexes - verify that indexes exist before trying to access it.
if ($_GET['q'] == 'test') {}
This will trigger a php notice if $_GET['q] doesn't exist. Test for the existence of the index -- usually an isset() will do.
- Trying to get property of non-object
if ($node->type == 'article'){}
The issue here is if $node doesn't exist or is not an object. You shouldn't try to reference the "type" property if you haven't checked if the node is an object.
- Variable scope - this is similar to undefined variables.
if (isset($a)) {
$newVar = 'test';
}
$b = $newVar;
If $newVar wasn't declared outside of the if statement, then technically, you'd be setting $b to an undefined variable if $a was not set. PHP is a bit too forgiving here. We should get in the habit of making sure a variable is declared or exists within the scope in which it is being referenced. If $a doesn't exist, this will trigger a notice.
- Call-time pass by reference - as of PHP 5.3.0, a notice will be triggered if you try to pass a variable by reference in a function call. For example:
if (isset($a)) {
foo(&$var);
}
There is no reference on a function call, only on function definitions. So if the function definition was:
function foo(&$var) {}
then you can just do:
if (isset($a)) {
foo($var);
}
The bulk of php notices that I've seen are a result of these 5 patterns that we should avoid. Please let me know if you have questions. Any feedback or thoughts are appreciated.
One last thought on the question of whether it's worthwhile to fix these php notices, here are some opinions from others: -- http://stackoverflow.com/questions/5073642/why-should-i-fix-e-notice-errors -- http://stackoverflow.com/questions/1868874/does-php-run-faster-without-warnings
-- Chhay