change E_NOTICE to warning and allow selection of error level

The reason it's flipped off on stable versions is exactly because module authors' modules are full of notices. Now imagine 90% of the Internet trying to install Drupal modules and getting screaming red errors that are harmless but they've no way of knowing that.

OTOH, E_ALL notifications absolutely *need* to be on for development versions of Drupal so that we don't accidentally introduce notices during core development. Developers also need it for when they're coding modules so that they don't spew notices to begin with.

The thought was that developers are developing against CVS checkouts anyway, and thus would get the E_ALL errors screaming at them and fix them in their modules well before D6 stable shipped. It turns out, this isn't the case.

I'm not sure that a toggle is going to improve matters, really. But if we add one we'd still need to change make sure its default state is "on" for dev and "off" for stable.

I strongly disagree. Our end users are held hostage to the responsibility (or lack thereof) of our module maintainers. Again, 90% of the Internet can't just make a patch to a module to get rid of notices. They don't know what an issue queue is. They don't know what a module maintainer is. If they've ever seen PHP, it was to copy/paste a snippet out of the handbook. They simply know that red error == broken.

As *developers*, it's *our* obligation to fix E_ALL compliance in both core and contrib. But we should not pass that responsibility to our end users.

I should mention too that if you come across a module that's spewing notices, you should submit a patch to fix them, along with a note that educates the module maintainer on how to view these notices themselves so that they don't introduce them again. If all developers do this, we'll have contrib-wide E_ALL compliance in no time. :)

That, my friend, is collaboration. :)

I'm coming around to this way of thinking.

+1 on yellow warnings too, rather than red.
Let's see a toggle option for error severity under admin/settings/error-reporting as well.
I still think it's too late to change this in 6.x though. :\

I'm 100% behind the approach (being a maintainer whose code is NOT entirely E_ALL compliant). PHP notices certainly fall into the category of "out of sight, out of mind". As webchick suggests, an option would be very helpful though for those poor souls that have no power over their contributed modules.

On a side note, we should consider the pure performance impact of notices... during Rasmus's keynote during DrupalCon, registering a notice requires loading several PHP error handling libraries and a write to disk. Fixing the notices Drupal-wide is in truth a performance enhancement.

Subscribing. Will try to come back and comment on this in more detail soon.

More accurate title.

Rather than changing the color of PHP notices, wouldn't it be better to just stick them in the watchdog without printing them to screen? or to only print them to screen if you have permission to see PHP errors?

Thank you goes to Pasqualle for pointing me in this direction. Might be able to test sometime soon.

Requires update to HEAD. Also doesn't really look right as a select box. Radio buttons must look a bit better.

Refactored the text a bit and stuck it in a radios widget instead. Looks a bit nicer.

$errno does not exist anymore. Will require more refactoring.

Whoops!

Yay +1 for helping other module developers fix PHP notices. Following...

Pasqualle is right in #21.

See: #335122: Test clean HEAD after every commit and http://pastebin.ca/1258476

Reroll.

Please, do not credit me.

It seems awkward that all options mention that errors are written to the log. The setting doesn't affect this.

This should be two settings - log vs. screen, and whether notices get written.

I like those changes - maybe 'all error messages are logged' but otherwise very nice!

#30 seems like a good direction and simplification in text.

subscribe

#30/31 sounds like a reasonable solution. Let's do, so we can stop shipping non-E_NOTICE-friendly code. :-)

Here's #30 implemented. I think I rather like it. Also attached screenshots for before/after comparision and UX review. Also removes the second parameter from system_settings_form() since TRUE is the default parameter.

I committed this patch to CVS HEAD. Thanks all.

There is room for improvement in terms of settings pages. After I applied this patch, I looked for the settings in "Administer › Site configuration › Logging and alerts" but they were in "Administer › Site configuration › Error handling". There is a case to be made for merging these.

I wonder if the 404/403 setting should be moved to admin/settings/site-information and move the error-reporting to a condensed admin/settings/logging.

404/403 to admin/settings/site-information - they're 'custom stuff which affects the whole site' and very much equivalent to a different front page path etc. Moving error reporting settings to admin/settings/logging also makes sense - will probably need to re-jig that page a bit, but there's not much there to re-jig anyway.

So both these changes sound great, and that means one less settings page in the admin screen!

Here's an attempt to implement the followup in #37 with screenshots of the new pages. The 'before' screenshot is still in #35.

Sorry, my IDE caught trailing whitespace.

I like it that way a lot, and the patch looks good on a quick scan.

Quick change from #39, if the VERSION constant defined in system.module contains 'dev', error reporting is defaulted to showing PHP notice errors (value 2), otherwise not (value 1). That way, whenever we change the version constant in-between releases, we won't need to go in and change this default variable every time.

I'm responding to suggestions in #36 and onward:

Yay for removing a whole admin page! Very good.
Site information page is getting busy but those 403/404 settings *do* belong there.

So, +1 and yes please.

There is some possibility to backport it into 6.x?
Sorry, I've duplicated the question at #1 :)

Ok, so we broke HEAD with #35. :) There's an undefined $type variable in the error handler that should have been $error['%type']. Here's another try for the testing bot.

Roll this back and won't fix. When core E_ALL was first tried, some of our most respected core contributors created a patch that was chock full of errors. The run of the mill PHP coder wont know when to use isset and when to use empty. Do not even try getting contrib notice free.

If I'm working on contrib, I want to be able to show E_ALL errors to myself, this patch lets me does that, and it doesn't force it down the throats of contrib authors if they don't want it.

If it's in the UI then someone might flip it and get a screen full of it. If you are a contrib author then you might try to fix them and you will fail.

Here's the patch to fix borken HEAD only.

It seems we wont get anywhere without me linking http://drupal.org/node/112715 ... is this what you want for contrib? broken patches galore.

Re-rolled now that HEAD is not broken.

At least can we put this in settings.php so that the usual one bit webmaster who tries everything wont face a wall of red errors and come to whine to drupal.org saying Drupal is broken? this will be a support nightmare if people can switch it.

I tend to agree with Pasqualle. If D7 outputs a sheet of errors by default, contrib authors will see it instantly, and fix notices as they're porting their modules. So by the time end users are using modules, they shouldn't ever run into this.

usual one bit webmaster who tries everything wont face a wall of red errors and come to whine to drupal.org saying Drupal is broken

If people see real errors and report bugs, that's good. We have stuff in core which can completely hose your site - PHP filter, Full HTML, running without cron set up for a year - all these can do damage in various ways, seeing a big wall of red errors doesn't do you any harm at all. Some people will whine, but they whine anyway, and if we get more valid bug reports and cleaner contrib modules then it's worth some pain.

Expanded rationale:

* In D6 and below, error handling severity is hard-coded into common.inc. You must *hack core* to see notices in a stable release of Drupal. This is stupid. This alone makes the patch a welcome improvement.
* We need to accept the fact that until we put every single module you need to build a "major" site into Drupal core, contributed modules are an integral part of the "Drupal" package. This is a small step toward ensuring higher code quality for *all* of Drupal.
* Core and contrib are completely different ball-parks. With core, you're dealing with hundreds of thousands of lines of code, put in by thousands of developers over almost a decade. With contrib, you're normally dealing with a few hundred lines, at most, written by one person. I always submit notice fix patches to contributed modules as I come across them, and trust me, we are not dealing with the complexity of form.inc here. 99% of the time it's simply uninstantiated variables or misspelled constants.

I'm all for this as well. This is a good way to distinguish the good contrib from the bad. Maintainers who leave NOTICEs unrepaired are irresponsible IMO. I used to do that, but I've matured as core has.

You all presume that the contrib maintainers can fix this. This is not so. Yes, I firmly believe that quite some people will never grok the difference between isset and empty.

Some will be able to, some won't. The ones that can't will eventually see the usage of their modules drop.

We're discussing having automated testing running on all of contrib too - just because some maintainers will never write tests (and if we had a 'click through' test just testing for exceptions, might fail that baseline test too), isn't a good reason not to offer the option to those who can/will.

I am still +1 on this, and on encouraging contrib authors to write proper code. Those that don't will scare away users until they fix their code, same as any other bug. That is how it should be. Making this an admin toggle makes absolute sense, as it gives us the ability to have a pedantic development mode and a silent "don't show big scary errors when the site is live" mode.

I can't make my contrib module E_ALL compliant unless I'm running E_ALL when I write it.

Massively improved the error reporting test that was failing. I think this is finally ready for a good review.

Personally, given all the work that we went through to make core NOTICE free, that it's a mistake to ever turn off E_NOTICE at this point. Contrib should be required to follow suit. A toggle isn't going to help as much as you think since the developer still has to remember it's there.

Getting big red scary warnings in the face. That'll force contrib devs to fix things when they port their module from D6 to D7.

I'm not saying we shouldn't use this patch, but at the very least we should default to E_NOTICE being on. If contrib developers can't make their code notice free then there's a good chance people don't want to use that code anyhow.

Applied patch, reset menu cache, visited the pages and everything seemed to be where I expected it to be. Ran the error handling tests and retrieved all passes. Well done, Dave. This patch is wicked and I'd consider it RTBC.

The only grippe I have with the patch is "Logging, errors and alerts"'s description:

Settings for logging and alerts modules. Various modules can route Drupal's system events to different destination, such as syslog, database, email, ...etc.

Of course I'm okay with that, but a "...etc" might not look right to some. Should we get Keith Smith's documentation awesomeness on this?

I believe that Earl's point of making E_NOTICE defaulting to on should be managed in a separate issue as it is somewhat unrelated to this patch.

There is no reason modules can't request the administrator turn off E_NOTICE if they really cannot be notice free.

@66 I don't think defaulting to E_ALL | E_NOTICE is a wise idea because however nice it would be, I don't forsee contrib being E_NOTICE free. I think this is mostly to give developers the easy ability to make sure they are writing E_NOTICE free code without having to adjust PHP ini settings. That's why the patch defaults to showing E_NOTICE errors if the running version of Drupal is x.x-dev, otherwise the sensible E_ALL for releases.

I agree that contrib will never be notice free if we don't push the issue. But I think it can be notice free if Drupal, by default, displays notices. I am really uncomfortable with the two different worlds approach.

I agree with Earl. Flip the default, and lets keep on surfacing the quality contrib modules. We *need* differentiators.

That's a fair point. Here's a revision from #62 with cleanups to the test functions, split into two different defaults. One defaulted to showing all errors and notices, the other using the method in #62 (default to show all errors and notices only using a x.x-dev release).

I can live with either approach, but would prefer E_NOTICE by default. For one, the logic is simpler. For another, as Earl and Moshe already said we need to put contrib authors on the spot to not write buggy code.

Put the interface into Drupal 6 so ppl can prep and release Drupal 7 with E_ALL. Do or do not... there is no try.

So a summary for the changes in #70:
- Merges admin/settings/error-reporting and admin/settings/logging into one screen for "Logging, errors and alerts".
- The error reporting setting is moved to a "settings" local task under admin/settings/logging.
- The dblog/syslog settings are moved to local tasks under admin/settings/logging.
- Moves the 403/404 page settings into admin/settings/site-information since it's a custom site location like the front page location setting.
- Improves the Drupal error handler test in common.test to test the different error reporting settings.
- Improves and simplifies the Drupal error handler test to test for the correct line number and file in the error messages tested.
- Fixes trailing whitespace in the modified files (auto-IDE correction).

The difference between the two patches:
291026-error-level-default-notices-D7.patch: Sets the default error reporting in Drupal to show E_NOTICE errors.
291026-error-level-default-no-notices-D7.patch: If the VERSION constant contains the string 'dev', it sets the default error reporting in Drupal to show E_NOTICE errors, otherwise the current error reporting level (show all errors except E_NOTICE)

Screenshots are posted in http://drupal.org/node/291026#comment-1209569

Pasqualle: I disagree. You may care about these notices. They may be indicative of bugs. As a user you simply do not know whether they are truly bugs or not. That's why E_NOTICE exists, because it is picking up potential bugs.

I committed the version with E_NOTICE on by default. We can always relax this before the release. Let's see how this works, and how it affects contributed module authors before and during the code freeze. Thanks.

Would it be possible to have these notices only visible to certain roles ?

I think it could be useful to have only people in role "administrators" or "testers" to see these notices.

@dag-: Please feel free to open a separate feature request for that. I think it could be useful to wrap a permission around PHP errors, but we should explore that as a separate issue on its own. This one is done. :)

Dries said above (in comment 76):
> I committed the version with E_NOTICE on by default. We can always relax this before the release.

Did you forget to relax this before the final release of Drupal 7.0 or? I strongly think that forcing E_NOTICE on is wrong in production releases/sites - it's just confusing visitors and normal users. Developers can always turn it on (in php.ini) when they are developing. A minimum should be that the notices are only displayed to logged in users.

There will always be modules with undefined indexes - we shouldn't punish our users/visitors because of sloppy coding from Drupal contributors.

PS! Related bug report - #963182: Enforce E_ALL?

This is now configurable under admin/config/development/logging. If you don't want it on in production, turn it off.

There's an interface to not have the errors show up on screen, but there's no interface to prevent Drupal from fiddling with the error levels in PHP.

So now we have to hack core to prevent notices from blanketing the watchdog log, drowing out the actually important information.

I think it might be reasonable to include an error_reporting setting in default.settings.php with a comment about how it works, but having it an unchangable, hard-coded setting that you must have E_ALL on your sites actually has a negative performance impact.

I think this is rather unfortunate instance of “Drupal knows best”. Drupal should not take it upon itself to change the system configuration, hopefully defined by someone who knows what's best for the system.

This is most definetly not critical, and it isn't a bug report it is either a feature request or a task.

Subscribe.

Sorry for bumping your issue changes, marcingy.

If changes to the error logging mechanisms are requested, they should be discussed in a new, follow-up feature request (feel free to cross-link here). This issue has been closed an in core since early 2009 and is already at 80-some replies. Sounds like "Make watchdog respect the error reporting level at admin/config/development/logging" is what you're asking for.

Cross posted to #1267246: Drupal overriding error_reporting setting in php.ini