Page MenuHomePhabricator

Remove uses of the error suppression operator
Closed, ResolvedPublic

Description

"Error suppression operators have lots of implementation issues in PHP, they are evil and should almost never be used."

and

"Don't use the error suppression (@) operator for any reason ever. It's broken when E_STRICT is enabled and it causes an unlogged, unexplained error if there is a fatal, which is hard to support. Use wfSuppressWarnings() and wfRestoreWarnings() instead."

Somebody should go through MediaWiki and remove the 87 uses of it and replace it with appropriate error checking. Things like array indexes can be checked with isset(), some things should actually get wfSuppressWarnings() and wfRestoreWarnings() (permission errors on file operations are a good example).

Tagging this easy for somebody with some spare time.


Version: unspecified
Severity: minor

Event Timeline

bzimport raised the priority of this task from to Low.Nov 21 2014, 11:06 PM
bzimport set Reference to bz24159.
bzimport added a subscriber: Unknown Object (MLST).
demon created this task.Jun 28 2010, 3:04 PM
Reedy added a comment.Jun 29 2010, 6:30 AM

Dump of the what files/lines for the lazy? :P

Created attachment 7523
List for the lazy

$ grep -irPn '@[a-z]+\(' . | grep -v .svn | grep -v 'Binary file' > ~/uses-of-@.txt

attachment uses-of-@.txt ignored as obsolete

It has one false positive, an @import in CSS, but it's commented anyway :)

Created attachment 7524
Try 2

Forgot to include _ in [a-z]+. This version includes it :)

Attached:

./includes/normal/UtfNormalTest.php:77: if( preg_match( '/@Part([\d])/', $data, $matches ) ) {

Would be another ;)

demon added a comment.Jul 2 2010, 7:46 PM

$ grep -irPn '@[a-z_]+\(' .

This only includes global function calls, which isn't the only place @ can be used :)

$ grep -irPn '@[a-z_]+::'

Yields 1 static method call. Similar grep for method on object calls came back negative, yay :)

$ grep -irn '@\$' . | grep -v .svn | grep -v 'Binary file'

Gives you a bunch of variable accessing with @ that I didn't include above :(

thetooth wrote:

A better solution is to use php's scream flag which overrides the suppression symbol.

Add a statement to the main construct after the configuration has been loaded and have it call "ini_set('scream.enabled', true);" if a debug flag is set, that way developers can catch all errors regardless of another dev's coding style.

It's an PECL extension which requires PHP 5.2.0. I don't see how that is a solution to this bug.

demon added a comment.Oct 3 2010, 6:48 PM

(In reply to comment #8)

It's an PECL extension which requires PHP 5.2.0. I don't see how that is a
solution to this bug.

It's not. We shouldn't be using a PECL extension to get around poor coding.

Reedy added a comment.Feb 10 2011, 4:53 PM

How many more of these are still about....?

demon added a comment.Feb 10 2011, 5:04 PM

Too many :(

demon added a comment.May 15 2011, 2:35 PM

Using r88187:

$ php checkSyntax.php
Building file list...done.
Checking syntax (using php -l, this can take a long time)
Warning in file /opt/local/apache2/htdocs/phase3/includes/api/ApiParse.php: Error supression operator (@) found.
Warning in file /opt/local/apache2/htdocs/phase3/includes/api/ApiQuery.php: Error supression operator (@) found.
Warning in file /opt/local/apache2/htdocs/phase3/includes/api/ApiQueryBacklinks.php: Error supression operator (@) found.
Warning in file /opt/local/apache2/htdocs/phase3/includes/api/ApiQueryExternalLinks.php: Error supression operator (@) found.
Warning in file /opt/local/apache2/htdocs/phase3/includes/cache/MessageCache.php: Error supression operator (@) found.
Warning in file /opt/local/apache2/htdocs/phase3/includes/ConfEditor.php: Error supression operator (@) found.
Warning in file /opt/local/apache2/htdocs/phase3/includes/db/Database.php: Error supression operator (@) found.
Warning in file /opt/local/apache2/htdocs/phase3/includes/db/DatabaseIbm_db2.php: Error supression operator (@) found.
Warning in file /opt/local/apache2/htdocs/phase3/includes/db/DatabaseMysql.php: Error supression operator (@) found.
Warning in file /opt/local/apache2/htdocs/phase3/includes/db/DatabasePostgres.php: Error supression operator (@) found.
Warning in file /opt/local/apache2/htdocs/phase3/includes/diff/DairikiDiff.php: Error supression operator (@) found.
Warning in file /opt/local/apache2/htdocs/phase3/includes/filerepo/ForeignAPIFile.php: Error supression operator (@) found.
Warning in file /opt/local/apache2/htdocs/phase3/includes/filerepo/FSRepo.php: Error supression operator (@) found.
Warning in file /opt/local/apache2/htdocs/phase3/includes/filerepo/LocalFile.php: Error supression operator (@) found.
Warning in file /opt/local/apache2/htdocs/phase3/includes/filerepo/LocalRepo.php: Error supression operator (@) found.
Warning in file /opt/local/apache2/htdocs/phase3/includes/GlobalFunctions.php: Error supression operator (@) found.
Warning in file /opt/local/apache2/htdocs/phase3/includes/HistoryBlob.php: Error supression operator (@) found.
Warning in file /opt/local/apache2/htdocs/phase3/includes/Import.php: Error supression operator (@) found.
Warning in file /opt/local/apache2/htdocs/phase3/includes/json/Services_JSON.php: Error supression operator (@) found.
Warning in file /opt/local/apache2/htdocs/phase3/includes/media/DjVu.php: Error supression operator (@) found.
Warning in file /opt/local/apache2/htdocs/phase3/includes/media/Exif.php: Error supression operator (@) found.
Warning in file /opt/local/apache2/htdocs/phase3/includes/media/SVG.php: Error supression operator (@) found.
Warning in file /opt/local/apache2/htdocs/phase3/includes/objectcache/SqlBagOStuff.php: Error supression operator (@) found.
Warning in file /opt/local/apache2/htdocs/phase3/includes/parser/Parser.php: Error supression operator (@) found.
Warning in file /opt/local/apache2/htdocs/phase3/includes/parser/Parser_LinkHooks.php: Error supression operator (@) found.
Warning in file /opt/local/apache2/htdocs/phase3/includes/profiler/Profiler.php: Error supression operator (@) found.
Warning in file /opt/local/apache2/htdocs/phase3/includes/Revision.php: Error supression operator (@) found.
Warning in file /opt/local/apache2/htdocs/phase3/includes/Sanitizer.php: Error supression operator (@) found.
Warning in file /opt/local/apache2/htdocs/phase3/includes/search/SearchMySQL.php: Error supression operator (@) found.
Warning in file /opt/local/apache2/htdocs/phase3/includes/search/SearchSqlite.php: Error supression operator (@) found.
Warning in file /opt/local/apache2/htdocs/phase3/includes/Setup.php: Error supression operator (@) found.
Warning in file /opt/local/apache2/htdocs/phase3/includes/specials/SpecialLockdb.php: Error supression operator (@) found.
Warning in file /opt/local/apache2/htdocs/phase3/includes/specials/SpecialUnlockdb.php: Error supression operator (@) found.
Warning in file /opt/local/apache2/htdocs/phase3/includes/specials/SpecialWantedpages.php: Error supression operator (@) found.
Warning in file /opt/local/apache2/htdocs/phase3/includes/StreamFile.php: Error supression operator (@) found.
Warning in file /opt/local/apache2/htdocs/phase3/includes/templates/Userlogin.php: Error supression operator (@) found.
Warning in file /opt/local/apache2/htdocs/phase3/includes/User.php: Error supression operator (@) found.
Warning in file /opt/local/apache2/htdocs/phase3/languages/Language.php: Error supression operator (@) found.
Warning in file /opt/local/apache2/htdocs/phase3/maintenance/backup.inc: Error supression operator (@) found.
Warning in file /opt/local/apache2/htdocs/phase3/maintenance/checkImages.php: Error supression operator (@) found.
Warning in file /opt/local/apache2/htdocs/phase3/maintenance/checkSyntax.php: Error supression operator (@) found.
Warning in file /opt/local/apache2/htdocs/phase3/maintenance/importImages.php: Error supression operator (@) found.
Warning in file /opt/local/apache2/htdocs/phase3/maintenance/importUseModWiki.php: Error supression operator (@) found.
Warning in file /opt/local/apache2/htdocs/phase3/maintenance/language/StatOutputs.php: Error supression operator (@) found.
Warning in file /opt/local/apache2/htdocs/phase3/maintenance/Maintenance.php: Error supression operator (@) found.
Warning in file /opt/local/apache2/htdocs/phase3/maintenance/proxy_check.php: Error supression operator (@) found.
Warning in file /opt/local/apache2/htdocs/phase3/maintenance/rebuildFileCache.php: Error supression operator (@) found.
Warning in file /opt/local/apache2/htdocs/phase3/maintenance/storage/fixBug20757.php: Error supression operator (@) found.
Warning in file /opt/local/apache2/htdocs/phase3/maintenance/updateSearchIndex.php: Error supression operator (@) found.
Warning in file /opt/local/apache2/htdocs/phase3/maintenance/updateSpecialPages.php: Error supression operator (@) found.
Warning in file /opt/local/apache2/htdocs/phase3/maintenance/userOptions.inc: Error supression operator (@) found.

thetooth wrote:

haha, oh wow

demon added a comment.Jul 6 2011, 10:02 PM

As of r91609:

$ php maintenance/checkSyntax.php
Building file list...done.
Checking syntax (using php -l, this can take a long time)
Warning in file /www/phase3/includes/filerepo/ForeignAPIFile.php: Error supression operator (@) found.
Warning in file /www/phase3/includes/json/Services_JSON.php: Error supression operator (@) found.
Warning in file /www/phase3/includes/libs/jsminplus.php: trailing ?> found.
Warning in file /www/phase3/includes/media/Exif.php: Error supression operator (@) found.
Warning in file /www/phase3/includes/parser/Parser.php: Error supression operator (@) found.
Warning in file /www/phase3/includes/parser/Parser_LinkHooks.php: Error supression operator (@) found.
Warning in file /www/phase3/includes/profiler/Profiler.php: Error supression operator (@) found.
Warning in file /www/phase3/includes/Revision.php: Error supression operator (@) found.
Warning in file /www/phase3/includes/Sanitizer.php: Error supression operator (@) found.
Warning in file /www/phase3/includes/search/SearchMySQL.php: Error supression operator (@) found.
Warning in file /www/phase3/includes/search/SearchSqlite.php: Error supression operator (@) found.
Warning in file /www/phase3/includes/Setup.php: Error supression operator (@) found.
Warning in file /www/phase3/includes/specials/SpecialWantedpages.php: Error supression operator (@) found.
Warning in file /www/phase3/includes/StreamFile.php: Error supression operator (@) found.
Warning in file /www/phase3/includes/User.php: Error supression operator (@) found.
Warning in file /www/phase3/maintenance/backup.inc: Error supression operator (@) found.
Warning in file /www/phase3/maintenance/checkImages.php: Error supression operator (@) found.
Warning in file /www/phase3/maintenance/importImages.php: Error supression operator (@) found.
Warning in file /www/phase3/maintenance/importUseModWiki.php: Error supression operator (@) found.
Warning in file /www/phase3/maintenance/language/StatOutputs.php: Error supression operator (@) found.
Warning in file /www/phase3/maintenance/Maintenance.php: Error supression operator (@) found.
Warning in file /www/phase3/maintenance/proxy_check.php: Error supression operator (@) found.
Warning in file /www/phase3/maintenance/storage/fixBug20757.php: Error supression operator (@) found.
Warning in file /www/phase3/maintenance/userOptions.inc: Error supression operator (@) found.

Done! 1134 files checked, 0 failures and 24 warnings found

demon added a comment.Jul 6 2011, 10:07 PM

(In reply to comment #14)

As of r91609:

$ php maintenance/checkSyntax.php
Building file list...done.
Checking syntax (using php -l, this can take a long time)
Warning in file /www/phase3/includes/filerepo/ForeignAPIFile.php: Error
supression operator (@) found.

Scratch that one, fixed in r91611.

Krinkle set Security to None.
Krinkle removed a subscriber: Unknown Object (MLST).
lcawte added a subscriber: lcawte.EditedDec 11 2014, 7:21 PM

As of 90fd90d8ce3926e448c525138f644b74231e6402, we have four core files using the @ operator.

lewiscawte@lewis-laptop:/code/mediawiki$ php maintenance/checkSyntax.php
Building file list...done.
Checking syntax (using  php -l, this can take a long time)
Warning in file /code/mediawiki/includes/GlobalFunctions.php: Error supression operator (@) found.
Warning in file /code/mediawiki/includes/Setup.php: Error supression operator (@) found.
Warning in file /code/mediawiki/maintenance/userOptions.inc: Error supression operator (@) found.
Warning in file /code/mediawiki/maintenance/Maintenance.php: Error supression operator (@) found.
found.

Done! 1511 files checked, 0 failures and 4 warnings found
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 10 2015, 3:41 PM

Change 516619 had a related patch set uploaded (by D3r1ck01; owner: Derick Alangi):
[mediawiki/core@master] Avoid the use of silence operator (@) and use AtEase methods

https://gerrit.wikimedia.org/r/516619

Change 516619 merged by jenkins-bot:
[mediawiki/core@master] Avoid the use of silence operator (@) and use AtEase methods

https://gerrit.wikimedia.org/r/516619

CCicalese_WMF closed this task as Resolved.Apr 28 2020, 8:49 PM
CCicalese_WMF claimed this task.
CCicalese_WMF added a subscriber: CCicalese_WMF.

Marking as Resolved as it is in the Done column. Feel free to reopen if there is remaining work.