Write E_ALL compliant code
Introductory notice
This document is a very recent addition to the coding guidelines, and is still subject to discussion among developers. Do NOT post comments on this page yet. One issue will be created for each point introduced here. Those issues will be used for discussion and patch submission. I'll keep updating this page as the discussion progresses. When a consensus has been reached on each point, this introduction will be deleted. Eventually, when everything has been patched up, I'll delete the mention about Drupal not being E_STRICT compliant.
E_ALL: a better practice
Currently, the Drupal code is not E_STRICT compliant. When running a Drupal site with E_ALL, each page view creates scores of error notices messages. Many developers agree that it would be good if the source of Drupal could be brought up to par with commonly accepted good practice.
The purpose of this document is twofold:
- to show common coding mistakes that prevent Drupal from being E_STRICT compliant.
- to set better coding guidelines to use in new code and patches.
Once those guidelines are accepted, it is only a matter of time and some developers' efforts before all the previous coding mistakes are patched up. Then, we will be able to run Drupal with the E_ALL directive.
Common coding mistakes and new coding practice
1- Use of if (isset($var)) or if (!empty($var))
(Note: this first issue is still under review by the developers. See discussion and patches here: http://drupal.org/node/34434 . This section will be updated according to the discussion there, until a broad consensus is reached and relevant patches committed).
If you want to test if an array has been set to any value, don't use:
<?php
if ($foo) {}
?>but:
<?php
// either
if (isset($foo)) {} // $foo=0 (zero) and $foo= '' return TRUE
// or
if (!empty($foo)) {} // use this when 0 or '' are not expected
// and are not valid values for $foo.
?>The difference between isset() and !empty() is that unlike !empty(), isset() will return TRUE even if the variable is set to an empty string or to the integer 0. In order to decide which one to use, consider whether 0 or '' are valid and expected values for your variable.
The following code is wrong:
<?php
function _form_builder($form, $parents = array(), $multiple = FALSE) {
// (...)
if ($form['#input']) {
// some code (...)
}
}
?>Here, the variable $form is passed on to the function. If $form['#input'] has been set to any value, some code is executed. The problem is that testing this way outputs the following error message:
notice: Undefined index: #input in includes/form.inc on line 194.Even though the array $form is already declared and passed to the function, each array's index must be explicitly declared. The previous code should read:
<?php
function _form_builder($form, $parents = array(), $multiple = FALSE) {
// (...)
if (!empty($form['#input'])) {
// some code (...)
}
}
?>Beware!
The function isset() returns TRUE when the variable is set to the integer 0, but FALSE if the variable is set to NULL. In some cases, is_null() is a better choice, especially when testing the value of a variable returned by an SQL query.
Testing for error notices
If you wish to help to clean up Drupal's code so that it complies with E_STRICT, you can set up a test site and in includes/common.inc change:
<?php
if ($errno & (E_ALL ^ E_NOTICE)) {
?>to:
<?php
if ($errno & (E_ALL )) {
?>