Last modified: 2014-02-12 23:32:51 UTC
Currently wfSuppressWarnings() does not remove E_STRICT notices from the error output, causing them to be generated even when warnings are supposed to be suppressed. E_STRICT should be added to the list of ORed error levels in wfSuppressWarnings() (GlobalFunctions.php line 2331, as of 1.20.2).
Note that this is only an issue on PHP 5.4 and above, where E_ALL now includes E_STRICT. In earlier versions of PHP, E_ALL excluded E_STRICT and so this did not cause any issues. In other words, this is a fix for a regression caused by changes in PHP 5.4.
Why should it suppress them? Those kind of errors usually are problems with the PHP code and not with some random i/o type errors that you usually want to suppress.
(In reply to comment #2) > Why should it suppress them? A) Because that's what wfSuppressWarnings() is for - to suppress warnings! B) Because that's how the function works in all previous PHP versions. This is a regression.
Whether it changed or not, why should we hide notices about using the wrong calling conventions in PHP or some other similar type things? I'm just not sold on this, though it's probably not a big deal.
This has been already done on bug 43092, where Bawolff did Gerrit change #38650 which kills E_STRICT on wfSuppressWarnings. (In reply to comment #4) > Whether it changed or not, why should we hide notices about using the wrong > calling conventions in PHP or some other similar type things? I'm just not > sold > on this, though it's probably not a big deal. Because those are for developers, not for production environments, and it has been proved that some E_STRICT warnings arise before LocalSettings.php is actually parsed, so they need to be supressed there for people that can't or don't want to change global configuration about error_reporting. I'm still not sure if I should close this bug as FIXED or as DUPLICATE of bug 43092 (and probably close bug 43092 also)
Here's one concrete example, which is where I noticed the issue. I'm sure it is not the only situation that it will occur in. I have an extension (WikiDB) that is compatible with older versions of MediaWiki, including MW1.6 which runs on PHP4. I have existing users who still use PHP4 and for whatever reason are unwilling/unable to upgrade, and I still support those users, from a single code-base. The biggest issue with E_STRICT is that it throws a warning if you make a static call to a function which doesn't have the static keyword. However, in order to be compatible with PHP4, we need to omit it. There are no ill-effects by doing this in PHP5, except the annoying warnings. Users of WikiDB are instructed to disable E_STRICT via a call to error_reporting() in their LocalSettings.php, so the code works without these messages being output. However if there are any places in code which suppress warnings, E_STRICT is temporarily re-enabled. This manifests in PHP notices in the search box, for example. Even if the MW code-base is clean, this is something that is likely to occur in extensions, and if warnings are being suppressed, then they should be - well - suppressed. On a more general note, best practice is to disable this kind of error on production machines, however in PHP 5.4, the deliberate error suppression code is actually un-suppressing errors! That seems very broken to me. At the very least it should look at whether E_STRICT is currently being reported or not, and only omit it from the suppression if it is not already suppressed.
Also, this should be back-ported to any previous MW versions that are still supported.
OK, re-enabling it temporarily is definitely bad behavior if it was off before.
Closing this bug as already resolved.
(In reply to comment #4) > Whether it changed or not, why should we hide notices about using the wrong > calling conventions in PHP or some other similar type things? I'm just not > sold > on this, though it's probably not a big deal. I agree 100% that we should not be surpressing warnings about incorrect calling conventions. However there are other E_STRICT warnings that sometimes need to be surpressed (the timezone thing for example). >The biggest issue with E_STRICT is that it throws a warning if you make a >static call to a function which doesn't have the static keyword. However, in >order to be compatible with PHP4, we need to omit it. From a core prespective, given how long ago PHP4 support has been dropped, that's not the most convincing argument. However functions should work as they are documented, and wfSuppressWarnings() is documented as surpressing all warnings. >On a more general note, best practice is to disable this kind of error on >production machines, however in PHP 5.4, the deliberate error suppression code >is actually un-suppressing errors! That seems very broken to me. Didn't even realize that with the original bug, but yeah that's not good. (In reply to comment #7) > Also, this should be back-ported to any previous MW versions that are still > supported. Yeah probably, especially with it overriding the user's error_reporting settings to enable E_STRICT. /me wonders how to go about doing that/request that it be done in our new git world. Something for me to read up on.
-> major and +milestone per http://lists.wikimedia.org/pipermail/wikitech-l/2013-February/067005.html , will set flag for backporting when available.
(In reply to comment #7) > Also, this should be back-ported to any previous MW versions that are still > supported. Any chance this can be back-ported (or has it already been?)
It was backported to 1.19 and 1.20: https://gerrit.wikimedia.org/r/#/q/Ie1ace158dac1733e6b2b2c1d533004d9bcab8c80,n,z
Change-Id: Ie1ace158dac1733e6b2b2c1d533004d9bcab8c80 1.19 done, 1.20 pending submit, both not released yet.
Great - thanks for the update.
1.20 backport was +2'ed by Platonides but needs verified +2 because tests fail.
Am I allowed to self-verify?
(In reply to comment #17) > Am I allowed to self-verify? It's less than a self-merge and IIRC there aren't big objections for self-merge of backports even.
Merged
