Let's make Drupal 6 E_ALL compliant

+1 Amen!

I've submitted quite a few e_notice patches over the last couple of weeks, but get a bit frustrated when they get ignored. I *know* it makes the code a bit more ugly, with lots of: if(isset(xxx)) etc, and ?: logic. It's certainly embarassing when Drupal creates such a mess with e_notice enabled.

Just earlier tonight I was in #Drupal_Support and someone came in to ask "Why am I getting all of these extra errors when I moved a site from sandbox to dev server?" The reason was his dev box was set for E_ALL, and Drupal setting itself to E_ALL & !E_NOTICE internally doesn't work in my experience. You have to set it to an obscure number in the .htaccess file. I know because I had to figure out what the corresponding number is for our own dev box at work, which is also set to E_ALL. As soon as he dropped that line into his .htaccess it worked fine.

Setting a dev environment to "pendantic" is recommended in every language. In C/C++, you set your compiler to treat errors as warnings. In PHP, you set error reporting to E_ALL. Same idea.

E_ALL compliance in core means we're compatible with more possible server configurations.

As for making the code uglier, only if you do all your checking late. :-) At work we have default-value input filters for all input that takes a default, similar to variable_get() in Drupal. Internally it just does the usual ternary, plus some type checking, but the resulting code where it's used is quite clean. If you just throw isset()s around, then yes, the code gets ugly and harder to maintain. That's why you structure the code to not need such ugliness, which makes it easier to read as well as more robust. Score. :-)

Huge +1

@lyricnz — empty() is less ugly than isset() to me, and more useful some cases. I completely agree that 'ugly' code is a small price to pay for being E_ALL compliant. Perhaps you can close your other patches, and repost them here?

@webchick — is that an appropriate practice (ie. posting many small related patches to one issue, and having someone in charge of committing them)? Who's in charge?

@webchick - isset() and empty() perform identically: http://www.php.lt/benchmark/phpbench.php

Also, !isset() is not the functionally the same as empty(), since an initialized empty string will return false for !isset() but true for empty(). Hence I think the main consideration should be what functionality is actually needed!

First, THANKS webchick for getting up on your high horse since I was just about to call mine in from pasture on this same issue.

Drupal 4.7 has been causing havoc on my shared server, hogging MYSQL resources, so I thought I would enable the devel module and do XDebug and do some profiling. As soon as I enabled devel, I got Notices by the hundreds. I straightened them out for the views.module and then it went right on to the next module, spitting out notices left and right. Frustrating since I totally agree with points 1-3 and normally I refuse to run any script that isn't E_ALL compliant because it's often a symptom of other underlying problems.

As for isset vs empty, use isset if simply checking that something is set, use empty if you want want to also know whether it is set and set/not set to a zero or zero-length string.

I have a preliminary patch to remove all the E_ALL notices I could find while installing: http://drupal.org/node/103250

+1 on this. It's probably the only way to enfore E_ALL compliance. And there *are* issues caused by sloppy coding (for example http://drupal.org/node/48178).

+1
Big benefits, and something that gets easier with time, and continues to improve our coding skills.

Looking at the patch though, to manually turn off E_ALL, the developer has to hack common.inc?? Would it be kosher for a variable to be set in settings.php? The problem is using contrib modules under a deadline, or where the maintainer is not responsive to E_ALL compliance patches.

Yes this will lead to eventual minor code bloat and yet this is a must to upkeep the high quality of Drupal code. In general, we do not hack just because it'd be smaller, we cherish quality code instead. The price to pay here will be very small so +1.

sime: Making the error level compatibility of Drupal configurable is a very bad idea. You could develop something under a very loose setting (because you didn't want to be bothered when just playing around with something), then forget to tighten it again before uploading. Bam, you just uploaded code that will break for someone with E_ALL & !E_NOTICE (Drupal's current level), to say nothing of proper E_ALL.

By setting Core to E_ALL and leaving it there, it means that developers building modules will get ugly messages thrown in their face while developing until they make their code E_ALL compliant. That means there's no excuse for not writing E_ALL compliant modules, whereas right now it's actually harder to write E_ALL modules because you can't enable E_ALL error reporting without making Core flip out completely.

(Part of me wants to go as far as saying use E_ALL | E_STRICT on PHP 5, but there would be a performance hit for checking the version, most of the E_STRICT stuff is on classes which we don't use anyway, and I don't want to bog down this discussion as happens to too many small-but-good ideas.)

+1 to E_ALL in common.inc, and let module authors do the right thing.

Dries, can we get some weigh-in from you please? :-)

Contrib maintainers will have to become responsive to E_ALL notices because they are now displayed on every single page.

@Crell
Understood, but as long as we recognize that hacking core will be necessary if we want to use some modules. There are two other situations where our control is not absolute:

1) People developing modules on servers that don't allow php.ini / htaccess overrides. They might be blissfully unaware of the problems others face with their code in an E_ALL environment.

2) Slack maintainers will also learn that if Drupal sets E_ALL, that they can set it back again within their modules.

Sorry if I've got some of my technical details wrong (at least I don't think I have). It's just the phrase "you shouldn't hack core" that rings in my ear. I would like us to avoid forcing this situation upon unsuspecting folk.

sime:

To point one, it's no different than we have now. Many many production servers are set to not display errors or warnings at all. If you're developing on one of those, then you could miss a warning (say, doing foreach() on a string instead of an array) and upload broken code now. You'll still be blissfully unaware that you have problems, but once it gets into CVS, things break. Developing under E_ALL has been part of standard good PHP development practices since, um, I think there's been an E_ALL, and the equivalent "treat warnings as errors" switch for gcc has been recommended good practice for as long as I remember.

To point two, again, it's no different than now. A module author could disable errors completely just by putting error_reporting(E_NONE) as the first line of a function in his module. I don't know of any that do so. Were they to do so now or do so with an E_ALL default, in either case it's a very stupid thing to do. :-)

Really, I see this as parallel to requiring that the HTML that core generates be XHTML Strict compliant and, where possible, semantically clean. It sets a high standard of correctness that helps ensure core is as widely-compatible and forward-compatible as possible. Module authors are encouraged to follow suit, or sometimes things don't work right when theming. Our PHP code should be held to no less of a standard.

Hacking core should NOT be necessary to use some modules just because we set to E_ALL. If a module requires core to be hacked to change the error reporting level, then that's a bug that should be filed against the module, just as much as a module that generates badly formed HTML should be treated as buggy and fixed.

Dries, Steven, can we please get some commentary from you here?

Committed to CVS HEAD. Thanks.