If you edit PHP code, please work with E_NOTICE turned on!!!

Submitted by rfay on

It's a Drupal and PHP best practice to develop with E_NOTICE turned on. Please do.

I installed a new contrib project from Drupal.org yesterday and found a whole bunch of warnings emitted to the screen. It turned out that the developer of the project had not (apparently) been working with E_NOTICE properly enabled in their development environment and so had never seen these. Two of them were actually obvious logic failures. You want to know when PHP is complaining about your code! (Make sure you have error/warning reporting to screen turned on during development too!)

So here's the deal: PHP has a flag called E_NOTICE. In release version of Drupal it's ignored (in includes/common.inc, drupal_error_handler()). In dev versions of Drupal and in Pressflow it's enabled. Anybody who ever edits PHP code during development should have this turned on. So you can

  • Use a development version of Drupal
  • Use Pressflow
  • Or apply this patch to your Drupal stable install (works on D6)

Why does this matter to you? Because if you make a single mistake, like using a misspelled variable, you won't know about it! You might do something like:

$x = $varable;

by accident, just a simple typo. If you have E_NOTICE enabled, you'll get a complaint and can fix it right away. If you don't, you've introduced a hard-to-find bug.

What about contrib modules/themes that have warnings? Well, we need to get them fixed. Please, fix them up. We wouldn't have this problem if all if everybody did what's promoted here. But it's easy enough: Fix the problem and file a patch. That's what I did last night.

Please everybody: Run your dev environment with E_NOTICE, and fix problems when you find them. Let's make contrib and our site run clean.

32 Comments

Best practice for turning on?

Submitted by Benjamin Melançon on

What is the best practice for turning on– just using a development version of Drupal, editing the file (hacking core!), or enabling it in one's environment?

I would say, use the dev version or pressflow

Submitted by rfay on

It's easiest to use a Drupal dev version or pressflow, but this is not hacking core. It's just making the one change that's always made between dev and stable releases.

So Randy, is it correct to

Submitted by Jochen on

So Randy, is it correct to put this in my /etc/php5/php.ini file:

error_reporting = E_ALL & E_NOTICE

???

Certainly correct, but Drupal overrides it

Submitted by rfay on

There's certainly nothing wrong with putting it in php.ini, but since Drupal overrides error_reporting, you need to fix this in Drupal.

Hmm, I don't get it. What

Submitted by Jochen on

Hmm, I don't get it. What line do I need to change in includes/common.inc ?

Edited the article to show exactly what to do

Submitted by rfay on

I edited the article to provide a patch.

This is what you need to do:

diff --git includes/common.inc includes/common.inc
index 2ca8c72..39d6499 100644
--- includes/common.inc
+++ includes/common.inc
@@ -631,7 +631,7 @@ function drupal_error_handler($errno, $message, $filename, $line, $context) {
     return;
   }

-  if ($errno & (E_ALL ^ E_DEPRECATED ^ E_NOTICE)) {
+  if ($errno & (E_ALL ^ E_DEPRECATED)) {
     $types = array(1 => 'error', 2 => 'warning', 4 => 'parse error', 8 => 'notice', 16 => 'core error', 32 => 'core warning', 64 => 'compile error', 128 => 'compile warning', 256 => 'user error', 512 => 'user warning', 1024 => 'user notice', 2048 => 'strict warning', 4096 => 'recoverable fatal error');

     // For database errors, we want the line number/file name of the place that

Hmm, yes, ... but the problem

Submitted by Jochen on

Hmm, yes, ... but the problem is that our common.inc file is version controlled and I don't want this patch to be running on our PROD server :-)

I could store this in a variable and make it environment independant ...

Settings.php?

Submitted by rfay on

The comment here about doing it in settings.php may work for you. Please give it a shot: http://randyfay.com/node/76#comment-192. (I haven't used that technique).
As far as I can tell, changing the setting in settings.php has no effect.

I actually think that E_NOTIFY probably should be on in production (but with warnings sent to watchdog instead of to the screen).

Applied the patch, but I'm

Submitted by Jochen on

Applied the patch, but I'm really getting a lof of these entries now :p

php 14/09/2010 - 13:53    Use of undefined constant SUPERCRON_SCRIPT_NAME  ...   admin   

Yay Drupal 7- just two lines in settings.php!

Submitted by Benjamin Melançon on

error_reporting(-1);  // Have PHP complain about absolutely everything as noted by Ryan below.
$conf['error_level'] = 2;  // Show all messages on your screen, 2 = ERROR_REPORTING_DISPLAY_ALL.
ini_set('display_errors', TRUE);  // These lines just give you content on WSOD pages.
ini_set('display_startup_errors', TRUE);

Do it in settings.php?

Submitted by Benjamin Melançon on

Sorry, I was being flippant with the hacking core comment, but it would still be better to have a place people can just change, and it shouldn't be required to have a development copy of Drupal to

Also, in Drupal 7, the relevant file is includes/bootstrap.inc for changing error_reporting to all.

However, this in settings.php ought to do it no matter what?

<?php
error_reporting(E_ALL);
$conf['error_level'] = ERROR_REPORTING_DISPLAY_ALL;
?>

E_STRICT ?

Submitted by FGM on

I tend to do most of my dev work (currently with PHP 5.3) with error_reporting(-1); to catch just about everything, including deprecation warnings and other minor issues.

Why do you recommend limiting oneself to notices and ignoring deprecation and strict warnings ?

Not at all

Submitted by rfay on

I think your approach is great. Cleaner is better.

where is "ERROR_REPORTING_DISPLAY_ALL" coming from?

Submitted by Blake Senftner on

error_reporting(E_ALL);
$conf['error_level'] = ERROR_REPORTING_DISPLAY_ALL;

I just added the above to the bottom of a settings.php file I'm using with one of the sites I'm developing. However, the use of "ERROR_REPORTING_DISPLAY_ALL" is triggering this error to display on my Drupal page:

Notice: Use of undefined constant ERROR_REPORTING_DISPLAY_ALL - assumed 'ERROR_REPORTING_DISPLAY_ALL' in C:\wamp\www\ProjectHome\sites\default\settings.php on line 260

I'm getting the error on my page, thanks (I think) to the "error_reporting(E_ALL);"...

Where should "ERROR_REPORTING_DISPLAY_ALL" live? I did a grep of a Drupal 6.19 I have locally, and it did not find it. The site in question is actually Pressflow, (not local)...

Drupal 7

Submitted by Benjamin Melançon on

Hey, i'm sorry, Blake, i completely forgot to mention that that code was written up looking at Drupal 7.

thanks for your reply

Submitted by Blake Senftner on

I tried just going with:

error_reporting(E_ALL);

as the last line of my settings.php, which works, but just traveling to the home page of a fresh installation of drupal 6.19 generates 396 messages, many of them being "Function ereg() is deprecated...", and "Undefined variable: [something]" from various modules.

This many messages is too much to live with, so I am forced to turn this off. Too bad. As I travel around this new and empty site, the messages are overwhelming. The clients already gave me a freaked call wondering what I broke...

PHP 5.3 warnings

Submitted by rfay on

The warnings you're getting are about PHP 5.3 deprecated functions.

Two things to note here:

  1. I actually recommended just turning on E_NOTICE :-) You turned on deprecated as well, which you don't need IMO for D6.

  2. Drupal 6.19 core is free of these notices, so you have some modules that have not been updated to 5.3.

good info

Submitted by Blake Senftner on

Thanks Randy. Changing to E_NOTICE does reduce the number of messages, that's for sure. But I'm still seeing quite a few (over a hundred) generated by the jqueryMenu.module.

They are all complaining about undefined variable usage...

Interesting to note that locating where the 3 variables are being used without any declaration and fixing them causes the jQuery Menu to completely fail to generate any menu at all. Very strange...

On the first case, it is an "output" variable that is used for the first time with a ".=" assignment. Simply setting that to the empty string before its first use completely breaks the menu. I think that's because the routine in question is recursive... very strange, since the $output variable is not one of the variables passed recursively, but is one of the variables built up recursively...

On the other two cases, simply undeclared variables are being passed to another routine. I can set them fine, and the menu is still generated, but with 13 messages about undefined usage of $output...

Oh well. I guess I'll file an issue with the module maintainer... thanks for your help!!

I don't believe settings.php can help in D6

Submitted by Scott Reynen on

I'm pretty sure the error reporting level is irrelevant. Drupal uses set_error_handler(), which - according to the linked description on php.net - disregards the error reporting level. So the only way to change what Drupal does is to set a different error handler function (this is what Pressflow does) or edit the existing function (this is what Randy's patch does).

Unfortunately, you can't set a different error handler function in settings.php, because that's loaded before common.inc, and the last handler set is the one actually used. If people really want to fix this in D6 without touching core, I think it'll require a contrib module. Moshe has already indicated he's not interested in doing something like this in devel. I can't think of another existing module where this would make sense; maybe a new module?

Needs to be easier, will be in D7

Submitted by Scott Reynen on

Having spent an hour last weekend trying to figure out how to change this setting (to fix notices someone kindly reported in an issue queue), I'm happy to see more notice of, er, notices in D7. Unfortunately, I expect this won't do much to solve the problems. First, there is way too much information online about how to turn notices off to actually find anything in a search like this about turning them on. Second, as indicated by the comments here, patching core or using a different version of core in development aren't great solutions for many people.

I was going to suggest adding an interface for changing this to a new or existing module (e.g. devel), but it looks like it's already done in D7 core. Yay.

Actually, Rasmus recommends

Submitted by Ryan Parman on

Actually, Rasmus recommends developing with error_reporting(-1);. This enables ALL possible error reporting in PHP. This includes E_STRICT, E_ALL, E_NOTICE, and everything else!

changed my development efficiency

Submitted by Sean on

I had the pleasure of working on a project with Randy a year ago, and when he walked me through this little gem... it changed my development forever. Great Stuff, Thanks!

AMEN brotha! If everyone was

Submitted by dalin on

AMEN brotha!

I everyone was developing with all errors blaring in their face there wouldn't have been any issue when PHP 5.3 came out.

MAMP on a mac

Submitted by Ronald on

If you work with MAC a nice way to do things is switch all error reporting on, open up the log and have that running alongside your installation.

This way you get all the errors - updated dynamically on the log and your browser screen is clean.

Thanks, Randy. And thanks,

Submitted by Lucas Weeks on

Thanks, Randy. And thanks, Ronald. Works like a charm on the mac.