Drush overrides PHP's error_reporting settings defined in the php.ini. This happens in drush_errors_on():

  $errors =& drush_get_context('DRUSH_ERROR_REPORTING', E_ALL ^ E_NOTICE);
  $errors = error_reporting($errors);

There and drush_errors_off() is the only time that context is used, so in effect, that piece of code overrides the builtin error_reporting() settings with E_ALL ^ E_NOTICE, which yields deprecation warnings and all bunch of silly stuff.

I think a better way to do this would be:

  $errors =& drush_get_context('DRUSH_ERROR_REPORTING', error_reporting());
  $errors = error_reporting($errors);

drush_errors_off() should also be changed to:

  $errors =& drush_get_context('DRUSH_ERROR_REPORTING', error_reporting());
  $errors = error_reporting(0);

... so that the default DRUSH_ERROR_REPORTING context is set correctly regardless of which one is called first.

Note that a workaround for this is to have a hook_drush_init() implementation that explicitely sets that context for you beforehand. But by POLA (Principle Of Least Astonishment), I think we should respect existing error_reporting settings.

This is coming from #1075322: Creating a site gives many "Function ereg() is deprecated file.inc:941" errors and a broken site and relatives.

CommentFileSizeAuthor
#3 1129332_error_reporting_improved.patch1.66 KBanarcat
#2 1129332_error_reporting.patch1.7 KBanarcat
#1 test_errors.txt1.42 KBanarcat

Comments

anarcat’s picture

anarcat commented
StatusFileSize
new test_errors.txt1.42 KB

Here is a test script to demonstrate how the builtin errors are overriden.

anarcat’s picture

anarcat commented
Status: Active » Needs review
StatusFileSize
new 1129332_error_reporting.patch1.7 KB

And here's a patch that fixes it all. The code is much more readable to me that way too.

The output of the test script before:

anarcat@marcos:drush$ drush scr test_errors.txt
original errors: 30711                                                                                   [success]
original context: 22527                                                                                  [success]
warning 1 test_errors.txt:6                                                                              [warning]
turning errors on                                                                                        [success]
errors: 22527                                                                                            [success]
context: 30711                                                                                           [success]
warning 1 test_errors.txt:13                                                                             [warning]
turning errors off                                                                                       [success]
errors: 0                                                                                                [success]
context: 22527                                                                                           [success]
turning errors back on                                                                                   [success]
warning 3 test_errors.txt:25                                                                             [warning]
errors: 22527                                                                                            [success]
context: 0                                                                                               [success]
then off again                                                                                           [success]
then back on                                                                                             [success]
errors: 22527                                                                                            [success]
context: 0                                                                                               [success]
warning 5 test_errors.txt:38                                                                             [warning]

the output of the test script after:

original errors: 22527                                                                                   [success]
original context: 22527                                                                                  [success]
warning 1 test_errors.txt:6                                                                              [warning]
turning errors on                                                                                        [success]
errors: 22527                                                                                            [success]
context: 22527                                                                                           [success]
warning 1 test_errors.txt:13                                                                             [warning]
turning errors off                                                                                       [success]
errors: 0                                                                                                [success]
context: 22527                                                                                           [success]
turning errors back on                                                                                   [success]
warning 3 test_errors.txt:25                                                                             [warning]
errors: 22527                                                                                            [success]
context: 22527                                                                                           [success]
then off again                                                                                           [success]
then back on                                                                                             [success]
errors: 22527                                                                                            [success]
context: 22527                                                                                           [success]
warning 5 test_errors.txt:38                                                                             [warning]

note, for clarity, that the "correct" error reporting level is 22527:

$ php -a
Interactive mode enabled

<?php print error_reporting();
22527

Patch has more information regarding the logic of the change.

Note that another improvement we could do we be to accept arguments in drush_errors_on() to allow changing the default error level, because 'just set error_reporting()' works, but only if drush_errors_on/off was never called.

anarcat’s picture

anarcat commented
StatusFileSize
new 1129332_error_reporting_improved.patch1.66 KB

And this second patch does just that. This patch is completely optional and not necessary to fix the actual bug here. Just an API improvement.

It does how depend on the other one.

anarcat’s picture

anarcat commented

It seems nobody objects here, I'll just commit this after some more stress tests on my side.

msonnabaum’s picture

msonnabaum commented
Status: Needs review » Reviewed & tested by the community

Looks reasonable to me.

moshe weitzman’s picture

moshe weitzman commented

No objection. Just coding standards stuff. Code comments should start with capital letter and end in a period. Also, } else { does not follow coding standards

anarcat’s picture

anarcat commented
Status: Reviewed & tested by the community » Fixed

I have fixed the commits to follow the coding standards, thanks!

Pushed to master.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

j0nathan’s picture

j0nathan commented

Subscribing (archives).

anarcat’s picture

anarcat commented
Version: » All-versions-4.x-dev
Assigned: Unassigned » msonnabaum
Status: Closed (fixed) » Needs review

Actually, I think that we should refactor this into 4.x - it's a small patch that doesn't break anything... without this, I need to patch core to install openatrium with aegir.

the two patches i want to merge into 4.x are:

d001faeb3f00f2c0a60f69213272166f6a1c14e2
ce7c0a21eee7bda270b56d4dfe22c48f1afb132a

anarcat’s picture

anarcat commented

Another workaround has been posted in #1120848: Don't log E_DEPRECATED errors OR check error_reporting(), but I think we should still merge those in for completeness.

tinefin’s picture

tinefin commented

subscribing

msonnabaum’s picture

msonnabaum commented
Assigned: msonnabaum » Unassigned
Status: Needs review » Fixed

This looks reasonable to me. Backported.

Automatically closed -- issue fixed for 2 weeks with no activity.