Drupal overriding error_reporting setting in php.ini

Sub (I need to know more about this stuff in any case)

Subscribe.

I was started a similar issue #1138464: Error reporting slow down drupal

Would someone please commit this !

this is the wrong approach. System settings are not for user-space applications to thumb their nose at, and as a system administrator, I was appalled to learn that Drupal thinks it knows better how to configure my server.

Goes double for me.

f-ing annoying as hell to not be able to change this in the proper place,

Has this not yet been committed? It's a pain having to go in and fix this (let alone remember to) manually every time.

edeloa,

Maybe we're not using the issue queue correctly?

Changed status to tested and reviewed.

Hmmm, I think it would be preferable to add it to default.settings.php. Removing the forced setting might be appropriate, though; let's see if I can find the original issue where this was added...
Edit: See #348448: Always report E_STRICT errors. We'd be reverting that issue, so would need to justify that, or at a minimum bump the error reporting during simpletest execution.

Also, this should go into D8 first. Thanks!

For those looking for a quick fix to this, just add ini_set('error_reporting', E_ERROR); to the top of one of your module files. No hacking core required.

Hi Jess,

I can't believe that the original intent is to force, against a user's will and server settings, anything. Much less something that near cripples a large module site.

Should we re-open #348448: Always report E_STRICT errors as a bugged resolution to that problem?

Best,

PS: Thanks Nick, that's a bit easier, safer, and more permanent.

For those of you who are trying to override this without hacking core, or other versioned code, I think I have found a solution. Just add a line like this to settings.php:

error_reporting(E_ALL & ~E_NOTICE);

If I am reading the code correctly, the settings.php file is included after the call to error_reporting() in bootstrap.php. I have tried this on one of my sites and it seems to be working.

Of course, you can set the actual error reporting value to whatever you need for your server.

Can someone explain in precisely which cases it makes sense to hide errors under the carpet?

When said errors are known to be useless and are causing performance problems.

The issue is that this is a configuration setting that's being made in a non-configurable location (i.e. core). It doesn't matter what it is, it simply shouldn't be hard-coded. There's already a number of ini_set calls that are in settings.php, if it's the strongly held opinion of core maintainers that E_ALL be the default, then that should be put into the default settings.php just like any other php.ini override. At least then people know where to look for it, and expect Drupal to override any previously defined settings in .htaccess or php.ini. In my case I had to grep core to figure out what was going on, which led me to this issue.

So no, it's not about "hiding errors under the carpet", it's about overriding ANY php settings in a non-obvious way. This is configuration, and should be handled in the same way as any other configuration.

Added “unnecessarily”/

Subscribe.
I was unable to remove E_DEPRECATED error messages because ((E_ALL & ~E_DEPRECATED) | E_ALL) == E_ALL,

I'm moving this back to D7 as it no longer seems to be done this way in D8.

resetting to 8.0.x-dev as it actually does still happen.

Now we are setting it in core/lib/Drupal/Core/DrupalKernel.php

    // Enforce E_STRICT, but allow users to set levels not part of E_STRICT.
    error_reporting(E_STRICT | E_ALL);

Since we are now requiring php-5.5.9+ and E_STRICT has been part of E_ALL since 5.4.0

turns out that error_reporting(E_STRICT | E_ALL); is exactly the same as error_reporting(E_ALL); in php-5.4.0+

We should simply remove this and allow folks to set error_reporting in php.ini when they can. Setting it in settings.php will still work for those who do not have access to their php.ini files.

Setting it in settings.php will still work for those who do not have access to their php.ini files

Is it worth, then, including

# error_reporting(E_ALL)

in default.settings.php (commented out) so that people can quickly find it, and help them get the syntax correct?

I worry that there are a number of php ini settings that we could have commented in settings.php to teach people about good php settings, but it's a slippery slope to determine which ones are worth calling out specifically.

I think we could definitely add it to settings.php but would prefer the PHP default as the starting point which is actually:

PHP 5.3 or later, the default value is E_ALL & ~E_NOTICE & ~E_STRICT & ~E_DEPRECATED. This setting does not show E_NOTICE, E_STRICT and E_DEPRECATED level errors.

http://php.net/manual/en/errorfunc.configuration.php#ini.error-reporting

E_ALL seems like quite a bit of verbosity for the default.

Adding a patch to remove these.

+1

The point in including this here would not be to teach people good PHP settings. (And, I quite agree, that E_ALL is not a good setting - in fact, the reason I'm following this issue is because I have started patching every D7 installation I manage to remove that line from the bootstrap, because I don't want every Drush call filling up my PHP error log).

Rather, the issue here is that this would be a change of behaviour. We'd simply be showing people how to get their site to behave how previous newly configured Drupal sites used to behave.

+1 (sorta)

Like James (JamesOakley) I've been following this for years as it's a pita.

And like Erik (eporama) I would like to see a proper instructional version of this "commented in settings.php to teach people about good php settings."

# # #

Erik, would you mind adding a corresponding comment patch for settings.php? If it's not done now, I'm pretty sure it'll never get done. :(

Best All,
Michael

"but it's a slippery slope to determine which ones are worth calling out specifically."

Not really the point of this particular Issue, but it'd be nice if there were an entire dedicated section, either inside settings.php or in its own settings companion file, for just these types of informational 'real world' good php settings. (Yes there are a bunch already there, but it seems haphazardly scattered throughout, intermixed with must have settings like $databases and default turned on things like 404_fast_paths)

What made this issue higher priority for me is the (imminent) introduction of PHP 7 (See https://www.drupal.org/node/2454439) which moves a whole load of new pieces of syntax into deprecated / warning status. The most common one is the planned change that PHP classes will not automatically interpret as a constructor any method whose name matches its class. Lots of modules seem to trigger that warning ... but I don't want an error log full of those. It took a lot of debugging to realise, finally, that the changes I was making to php.ini was having no effect because Drupal stamps on my virtual host / server settings.

But that's just an aside. Technically, I think I've already expressed my view on this.

I have written a small section for default.settings.php (including here just for easier reading/commenting)

/**
 * During development, it is recommended to increase the level of PHP's error
 * reporting. Drupal will respect the value for error_reporting set by the
 * php.ini configuration or by overrides in the webserver configuration or
 * .htaccess. To enable the maximum verbosity in PHP logging, uncomment the
 * line below.
 *
 * Note: This setting can cause a performance impact if set too verbose.
 * See https://www.drupal.org/node/1267246 for more information.
 */
# ini_set('error_reporting', E_ALL);

Visual inspection only: +1

No D8 to patch on :(

Thank you Erik for adding the info section, perfect blend of easy instructions in settings with a details link for the whole issue.

Best,
Michael

Well, now that I look for it, we do have a handbook page for "PHP Configuration Settings". This would be more appropriate place than linking to a bug/issue. We would need to change this part:

error_reporting set to E_ALL & ~E_NOTICE. Work is ongoing to change this to E_ALL for Drupal 6 and Drupal 7.

As a) E_ALL is not ongoing work, but already done and b) we're trying to undo that work ;-).

However, we can't change that page unless we actually commit this patch, so we should discuss what the wording should be. The line above is from a section of "PHP needs the following configuration directives for Drupal to work" which is patently false for the "error_reporting" value, I'm not sure why it was deemed necessary.

In the next section "Recommended PHP configuration settings", I would suggest we use a variation on the small section I had put in for inclusion in the default.settings.php:

Error Reporting

During development, it is recommended to increase the level of PHP's error reporting. The default value of "E_ALL & ~E_NOTICE & ~E_STRICT & ~E_DEPRECATED" does not show E_NOTICE, E_STRICT and E_DEPRECATED level errors. However, this can cause a performance impact if set too verbose.

The reasoning makes sense.

About testing environment, I guess it is not necessary to have it set from drupal core, i.e. testbots instructions should mention to configure php with E_ALL, and that should be enough.

The patch here needs a re-roll, hence marking as NW.

BTW not sure the right component here is "base system" or "bootstrap system".

Applying a new patch that is a reroll. Only the line numbers really changed, so no interdiff for that.

The patch should not link back to this issue, it should link to a documentation page on Drupal.org

I don't think we should change this in a patch release, so moving to 8.3.x

Agreed with making this configurable, but the default for both new sites and if possible existing sites should remain the same as now.

Catch,

Can we at least get D7 patched? Do what you want with 8, but 6 years man....

@Michael-IDA: D8 has to be fixed first, then the change can be backported.

@catch: I don't know how existing sites could be handled besides a note in the changelog and maybe a change notice?

I cannot find the docs. But I think that's no more "Do in D8 first and then backported". Now, we can create a separate issue for D7.

How in holy heck is this not done yet?
I just woke up to a 14GB syslog!
I'll be handling it module-side, but seriously, this is creating over 50GB of disk writes every day from notices alone, on just 10 sites.
Don't override my php.ini!!

I have already forgotten about this!

I agree that this is definitely useful in development, but not sure if we want that by default.
In any case, having the ability to modify the setting is at least a first step, so I went ahead with the suggestions at #34, and instead of reverting current behavior, I am keeping it.

I could not find a d8 equivalent of Required PHP configuration settings section on Drupal 7 PHP requirements, but I found a php configuration section on Drupal 8 PHP requirements.

We can always change that documentation page, so I linked https://www.drupal.org/docs/8/system-requirements/drupal-8-php-requireme... on the patch.

One simple solution would be to create a new checkbox on the performance tab.
Check the box to use the php.ini value, uncheck to override (default config).
Existing functionality untouched, easily configurable.
I've been modifying the php.ini value for YEARS, not realizing it did nothing in 7 and 8.

We just launched a new suite of sites that are very ajax-heavy, and suddenly my syslog and messages are 14GB/day each.

Don't think it matters but this was broken by #3151118: Include bootstrap.inc using composer so here's a reroll.

I think this is worth resurfacing because best I can tell it blocks disabling deprecation logging in a production environment. At a minimum its a weird WTF that your PHP configuration for error logging doesn't actually affect your error logging.

Blocking controlling deprecation logging in production can be problematic in 8.1 since deprecation messages can be brutal with the null string deprecation popping up out of the blue. I recently had a scan find a deprecation in a vendor library which would have been great if it hadn't found it by triggering 10G of logs since this bypassed my ini setting. Also,we're on track to have some serious deprecation message spam in 8.2 with the dynamic property deprecation.

Reroll updates the documentation link and format. Also copies changes to scaffold. See the interdiff.

#54 +1

I have started some new tests on patch #54. Fingers crossed...

Test passed, back to needs review

As noted in #54 I've run into this issue as I wanted to disable PHP 8.1 deprecation notices in production.

Do we have to consider backward compatibility here? I think this is too late for 10.0.0, but is this OK to change in 10.1.0 as long as we document it in the release notes? Or should we add some kind of BC notice inside DrupalKernel::bootEnvironment() if the error level is not set sufficiently high?

As a bug it perfectly backportable, more interesting effect happens when error_reporting is added to disabled functions

@andypost I consider this disruptive if it requires site owners to add something to settings.php, we can only do that in a major/minor and not a patch release. If we can figure out how to do this without requiring it in settings.php yet, then perhaps it can be backported.

Settings are initialized after bootEnvironment() is called

  public static function createFromRequest(Request $request, $class_loader, $environment, $allow_dumping = TRUE, $app_root = NULL) {
    $kernel = new static($environment, $class_loader, $allow_dumping, $app_root);
    static::bootEnvironment($app_root);
    $kernel->initializeSettings($request);
    return $kernel;
  }

(just thinking aloud) but how about creating a file in project root via an update hook that MUST BE committed to VCS. By using that file bootEnvironment() could enable a BC mode and if developers want the "latest" behavior they just need to remove it. Reading the file content at this stage of the request should have the same cost (or even less) than fetching a setting from Settings.

@mxr576 while that might work it's probably more effort for both core developers and end users, than documenting a change in settings.php that (likely) won't affect most people anyway.

The worst case scenario I can see is that someone's php.ini error_reporting level is set to something much lower than E_ALL, which means that if we just remove the code from the kernel and they don't update settings.php, they will see less errors than they did before. This isn't actually disruptive to end users, and while it may be disruptive to developers, we can document it in the release notes and all they have to do is change their php.ini or add a line to settings.php?

One other option is add a warning to the status report if error_reporting is not set to E_ALL?

One other option is to add a warning to the status report if error_reporting is not set to E_ALL?

And an update hook could also warn meanwhile deployment if the current error_reporting settings is lower than E_ALL...

I'm not really concerned about when it happens if we can move it forward because its quite frustrating. Is there anything blocking this from going into 10.1 and then we can discuss backporting?

As far as being disruptive, it feels a little weird to consider loosening the requirements around error_reporting disruptive but I guess maybe like you said there is an argument a CI pipeline that expects specific error reporting. Sounds like a fragile pipeline but an easy mistake to make I guess.

One other option is to add a warning to the status report if error_reporting is not set to E_ALL?

Core doing this sounds like a bad idea because we'd incorrectly warn people in production environments that honestly should _not_ be running E_ALL. Sounds like a devel.module feature?

Possibly relevant: #3322606: Does the Drupal error_reporting() check still work?

Change like this seems like it would 100% need a change record maybe even release notes?