If you edit PHP code, please work with E_NOTICE turned 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

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?

by rfay on Tue, 2010-09-14 02:43

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.

by Jochen on Tue, 2010-09-14 03:24

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

error_reporting = E_ALL & E_NOTICE

???

by rfay on Tue, 2010-09-14 03:29

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

by Jochen on Tue, 2010-09-14 04:28

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

by rfay on Tue, 2010-09-14 04:41

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

by Jochen on Tue, 2010-09-14 05:53

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 ...

by rfay on Tue, 2010-09-14 05:57

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).

by Jochen on Tue, 2010-09-14 05:55

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   

by rfay on Tue, 2010-09-14 06:00

Time to help out the maintainer of supercron with a patch.

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);

by Benjamin Melançon on Tue, 2010-09-14 04:56

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;
?>

by FGM on Tue, 2010-09-14 05:53

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 ?

by rfay on Tue, 2010-09-14 05:54

I think your approach is great. Cleaner is better.

by Jochen on Tue, 2010-09-14 05:57

Thanks for the tip, I agree!

by Jochen on Tue, 2010-09-14 05:56

Ah, this seems to be better then using the variable :-)

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)...

by Benjamin Melançon on Tue, 2010-09-14 13:37

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

by Blake Senftner on Tue, 2010-09-14 16:23

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...

by rfay on Tue, 2010-09-14 16:25

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.

by Blake Senftner on Tue, 2010-09-14 17:00

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!!

by Nick Mathew on Tue, 2010-09-14 13:46

Since we're setting it at runtime, PHP recommends using the error_reporting function.

So just add ini_set('error_reporting', 30719); to the ini_set section of settings.php

Other values for the function can be found at
http://www.php.net/manual/en/errorfunc.constants.php

by rfay on Tue, 2010-09-14 17:25

I can't get any of the settings.php related approaches suggested today to work on a stock Drupal 6.19 instance.

by Blake Senftner on Tue, 2010-09-14 17:34

I just went with this as the last line of settings.php:

error_reporting(E_NOTICE);

by Scott Reynen on Tue, 2010-09-14 18:02

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?

error_reporting(-1); // Via Ryan below.
$conf['error_level'] = 2; // The constant ERROR_REPORTING_DISPLAY_ALL, as error.inc is not yet loaded.

by Scott Reynen on Tue, 2010-09-14 06:41

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.

by Ryan Parman on Tue, 2010-09-14 07:38

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!

by Sean on Tue, 2010-09-14 09:57

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!

by dalin on Tue, 2010-09-14 23:10

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.

by Ronald on Wed, 2010-09-15 04:11

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.

by Lucas Weeks on Wed, 2010-09-15 08:56

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

Drupal theme by Kiwi Themes.