Page MenuHomePhabricator

Liberate the @ for AtEase
Closed, ResolvedPublic

Description

Background
  • What is AtEase? – It is a micro-library developed for MediaWiki as alternative to the native @ operator.
@ operator
$content = @file_get_contents( $path );
wikimedia/at-ease
use Wikimedia\AtEase\AtEase;

AtEase::suppressWarnings();
$content = file_get_contents( $path );
AtEase::restoreWarnings();
  • What does it do? – It surpresses warnings from the PHP engine. You can think of these as console warnings. They are not exceptions or fatal errors, but just additional information emitted from the program.
  • When is it useful? – To cleanly handle expected errorrs. It is considered bad practice to check if a file exists before opening it (it is subject to race conditions, and is inefficient, see EAFP). When unhandled, these errors would cause PHPUnit to mark code as unstable, and would pollute production error logs with unhelpful messages. Hence, these can be surpressed in PHP.
  • Why did we create AtEase? – There was a serious problem with the @ operator, which is that it not only disabled error messages for warnings, but also disabled error messages for fatal errors/exceptions. This is especially unexpected given that it did not catch or prevent those fatals, so a CLI process or web request woudl still abort fatally, but just not say anywhere why. (See also Coding conventions.)

For example, if you used @file_get_contenst() and then misspelled the function name, there would be no explaination as to why, where or how the process failed. Which, if it happens randomly in production in some rare code paths would be pretty miserable to try and figure out.

What happened

As of PHP 7, fatal errors and stack traces are no longer hidden by the @ operator. (Example: https://3v4l.org/t8i9q).

So?
  1. There is non-trivial cost and complexity involved in supporting AtEase due to how easy it is to accidentally forget to call AtEase::restoreWarnings();. When that happens, it could go unnoticed for months or years and hide a lot of errors from production.
  1. Another common mistake is not realising that exceptions may abort the flow between suppressWarnings and restoreWarnings. Which means even if the author wrote the code seemingly right, if there's any way that an indirectly called code path can throw, it can still lead to AtEase and native error_reporting being left in a dirty state, which can cause all kinds of problems. For example, as part of change 590684 it took several days to trace down why a test case was not passing in WMF CI but seemingly passing in isolation.
  1. It can't be easily used in unit tests due to assertions using exceptions to communicate with PHPUnit. (Example)
  1. In tight-loops there is non-trivial overhead from using AtEase, which it it sometimes moved to higher level code to ecompas an entire batch, thus tolerate far more potential warnings, which is undesirable. (Note: It is assumed but unconfimed that @ would have less overhead.)
  1. (Minor) More verbose code. Another library to maintain.
Status quo
use Wikimedia\AtEase\AtEase;

AtEase::suppressWarnings();
$content = file_get_contents( $path );
AtEase::restoreWarnings();
Proposal 1
  • Remove the PHPCS rule for Generic.PHP.NoSilencedErrors.Discouraged.
  • Update coding conventions to remove any mention of AtEase.
  • Update coding conventions to remove discouragement to use @.
$content = @file_get_contents( $path );

Beyond that, we could make a PHPCS rule to sniff against AtEase. This rule would auto-disabled by library-upgrader in all existing repos' phpcs.xml file (given there are existing violations, and is not easily auto-fixed).

For third parties I suggest we keep bundling wikimedia/at-ease for at least one LTS. It is in wide use and I don't think code generally gains anything from removing existing uses. Let's take it easy on the deprecation and migration :)

I'm not sure yet whether run-time deprecation warnings are a good idea for this. Probably not right away, but maybe after that LTS, we can do one final release of AtEase with deprecation warnings, then freeze/archive the library and remove it MW in the next release after that LTS.

Proposal 2
  • Keep coding conventions that discourage use of @.
  • Update coding conventions that "when error supressing is really needed" to no longer mention AtEase, but instead mention phpcs:ignore.
// phpcs:ignore Generic.PHP.NoSilencedErrors.Discouraged
$content = @file_get_contents( $path );

For third parties I suggest we keep bundling wikimedia/at-ease for at least two LTS releases. It is in wide use and I don't think we should force developers to waste time on these one-line replacements when for most code it would gain basically nothing from that.

See also

Event Timeline

Krinkle created this task.May 23 2020, 7:41 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMay 23 2020, 7:41 PM
TK-999 added a subscriber: TK-999.May 24 2020, 5:12 PM
Krinkle updated the task description. (Show Details)May 24 2020, 5:38 PM
Demian added a subscriber: Demian.May 24 2020, 6:08 PM
MaxSem added a subscriber: MaxSem.May 24 2020, 6:49 PM

I would like to veto the idea of allowing the silencing operator again. For a single reason: The @ prefix is probably the most ugly syntax we have in PHP. It's an extreme code smell condensed in a single character, and one of the reasons why people hate PHP. It's very hard to spot. Very hard to understand if you don't know already what it means. The character doesn't tell a story. The contrary. It looks like something else, e.g. an annotation, a mention, or a type prefix.

Marking code with something that is literally named suppressWarnings is sooooo much better:

  • It says what it does. You don't need to know much about PHP to understand it.
  • You can look up what it does and will find a nice README explaining everything. Try to find the @ sign anywhere.
  • It's much more to type. You really have to convince yourself and others if and why you want to use it.
  • It's much, much more visible in code reviews.
  • We can track it much more easily.
  • … the list goes on, but I think you get the point.

One concern I have is that this could potentially be abused. There are cases where using the @ operator is almost the "standard way"™ to do things, e.g. file_get_contents as in the example. However, I fear that if we remove the PHPCS rule, people may start using the operator even when unnecessary, e.g. when accessing array offsets, or other basic operations where saner error handling should be preferred. At that point, I'd have more or less the same concerns expressed by Thiemo above.

However, I fear that if we remove the PHPCS rule, people may start using the operator even when unnecessary, e.g. when accessing array offsets

You might be shocked, but I've seen AtEase used for precisely this somewhere in our codebase.

However, I fear that if we remove the PHPCS rule, people may start using the operator even when unnecessary, e.g. when accessing array offsets

You might be shocked, but I've seen AtEase used for precisely this somewhere in our codebase.

Perhaps at https://gerrit.wikimedia.org/g/mediawiki/skins/CologneBlue/+/7b435b050d12389826602696bbb1ed61be8924e8/includes/CologneBlueTemplate.php#32 - "Suppress warnings to prevent notices about missing indexes in $this->data"

people may start using the operator even when unnecessary, e.g. when accessing array offsets...

On a tangent: the ?? (null-coalescing) operator should be popularized.

However, I fear that if we remove the PHPCS rule, people may start using the operator even when unnecessary, e.g. when accessing array offsets

You might be shocked, but I've seen AtEase used for precisely this somewhere in our codebase.

Oh crap, this is dreadful. I couldn't even imagine AtEase being used like that. Because, well, like Thiemo also pointed out above, AtEase forces you to use a longer syntax, it's two function calls (and a use statement, if you want), so you must really think before using it. It's not as easy as just lazily adding a '@' character.

I still think that allowing '@' could encourage misuse even more (because it's shorter), but the initial conditions are worse than I thought...

Tgr added a subscriber: Tgr.May 25 2020, 4:39 PM

The common concern about @ is that it encourages misuse because it seems very lightweight but it's actually not cheap (it changes php.ini settings dynamically, all the error message generation still happens even though the result is discarded, allegedly it even impacts compiler optimizations - although those are PHP 5 era claims, not sure if they still hold up).

Maybe we could keep the PHPCS rule against @ and require people who use it to manually override?

@Tgr I think that would be a good compromise yeah, perhaps even better than we have today as doing it for the sniff is more explicily in the mental context of doing something questionable (as opposed to merely requiring more boilerplate).

mediawiki/extensions Examples:

	// phpcs:ignore Generic.PHP.NoSilencedErrors.Discouraged
	$value = @unserialize( $row->pp_value, [ 'allowed_classes' => false ] );
hoo added a subscriber: hoo.May 25 2020, 10:53 PM

I can see @thiemowmde's concerns, but I also see the trouble with using at-ease. As a compromise, it might be worthwhile changing at-ease to follow a more functional style:

php > function withAt( callable $callback ) { @$callback(); }
php > withAt( function() { echo file_get_contents( '/nothing-here' ); } );
php > withAt( function() { echo file_get_contents1( '/nothing-here' ); } );
PHP Warning:  Uncaught Error: Call to undefined function file_get_contents1() in php shell code:1
Stack trace:
#0 php shell code(1): {closure}()
#1 php shell code(1): withAt(Object(Closure))
#2 {main}
  thrown in php shell code on line 1
php > withAt( function() { echo file_get_contents( '/tmp/test' ); } );
Some text
php >
Krinkle added a comment.EditedMay 25 2020, 11:05 PM

It says what it does. You don't need to know much about PHP to understand it.

This cuts both ways. In general, I think PHP is better understood, documented, and known to contributors than obscure things MW invents.

[…] Try to find the @ sign (Search: PHP @) anywhere.

I don't think we can make the argument against all symbols in a programming language. That would also rule out the use of -> for members, :: for static etc. The way one would generally have to search for these, for any construct in any language, is by naming the symbol which generally works well. Search: PHP at, yields the expected result at the top.

it might be worthwhile changing at-ease to follow a more functional style:

php > function withAt( callable $callback ) { @$callback(); }
php > withAt( function() { echo file_get_contents( '/tmp/test' ); } );

A more realistic example:

use Wikimedia\AtEase\AtEase;

 $filename =;
 $obj = ;

$mtime = AtEase::withAt( function () use ( $obj, $filename ) {
   filemtime( $obj->pathFor( $filename ) );
} );

This adds closure management, import each relevant variable, managing the return value, .. I think this would add quite significant complexity and more oppertunities to make mistakes. (Did you notice I forgot a return statement?)

Why have custom-made indirection at all? If we want verbosy for verbosity sake, we can require any number of comments or no-op functions to be called (enforced by a sniff). But we can require them without actually doing anything functional. E.g. we could require a call to a void function AtEase::theBelowIsIntentional(); above any @-using line of code.

I think the phpcs:ignore comment achieves this already, whilst still being not so complex so as to require 5 new concepts being introduced in an existing line of code that may already have its own complexity. I would actually like us to do the phpcs comment even if we end up using something different than the @ operator, as it semantically ties in very well with the fact that it is an anti-pattern. Calling only some function is very easy to become numb to after a while, perceived as just unquestioned boilerplate that is perhaps needed for some reason.

	// phpcs:ignore Generic.PHP.NoSilencedErrors.Discouraged
	$mtime = @filemtime( $obj->pathFor( $filename ) );
Tgr added a comment.May 25 2020, 11:18 PM

As a compromise, it might be worthwhile changing at-ease to follow a more functional style

There's AtEase::qiuetCall. Of course, that comes with even more overhead and verbosity (in most cases you can write something like AtEase::qiuetCall( 'file_get_contents', '/nothing-here' ) instead of AtEase::qiuetCall( function() { file_get_contents( '/nothing-here' ); } ), but then you lose some static type checking capability instead).

As of PHP 7, fatal errors and stack traces are no longer hidden by the @ operator. (Example: https://3v4l.org/t8i9q).

The PHP manual still says that fatal errors are hidden. I have commit access and can theoretically edit it, although I'd appreciate it if someone could do the research necessary to write a new note, precisely specifying the new behaviour and when it was introduced.

I would like to veto the idea of allowing the silencing operator again. For a single reason: The @ prefix is probably the most ugly syntax we have in PHP. It's an extreme code smell condensed in a single character, and one of the reasons why people hate PHP. It's very hard to spot. Very hard to understand if you don't know already what it means. The character doesn't tell a story. The contrary. It looks like something else, e.g. an annotation, a mention, or a type prefix.

It is quite easy to find it in the PHP manual section on operators. I think basic understanding of PHP should be a prerequisite for reading MediaWiki code.

Marking code with something that is literally named suppressWarnings is sooooo much better:

The issues with AtEase that @Krinkle listed in the task description are real and significant.

I would be fine with removing the PHPCS sniff, after the PHP manual has been updated. I don't think the performance or readability concerns above are sufficient to warrant keeping it. It's slower than ?? but faster than AtEase. Readability is no worse than any other operator. For anyone with a basic understanding of PHP, it is the most obvious way to do scoped error suppression.

[…] PHP is better understood, documented, and known to contributors than obscure things MW invents.

How is code that says "suppress warnings" obscure?

I don't think we can make the argument against all symbols in a programming language.

I'm not making arguments against "all symbols".

That would also rule out the use of -> for members, :: for static etc.

I'm sorry? Who started arguing against these?

The issues with AtEase […] are real and significant.

I never said these issues are not real.

What are you two trying to achieve here? Attacking my arguments like this is cheap and heavily disrespectful.

I just said that simply disabling the PHPCS sniff and allowing @ again (which is what this ticket proposes) is not and can not be the only possible solution. I can see that enforcing people to add an // phpcs:ignore line probably resolves the issues I have.

The issues with AtEase […] are real and significant.

I never said these issues are not real.

My point is that you didn't address those issues.

What are you two trying to achieve here? Attacking my arguments like this is cheap and heavily disrespectful.

Declaring a "veto" in your first comment struck me as disrespectful. Your language was generally very strident. It seemed like you were not prepared to consider Krinkle's proposal.

daniel added a subscriber: daniel.
Krinkle triaged this task as Medium priority.Jun 3 2020, 3:43 PM
Krinkle moved this task from Inbox to Watching on the TechCom board.

I haven't fully reviewed the technical implications of @, but as the original creator of at-ease, I fully support replacing it with a PHP language feature.

Maybe we could keep the PHPCS rule against @ and require people who use it to manually override?

Having tried to do this in the past with some documentation rules, having PHPCS rules that people are supposed to disable is just a frustrating experience. How does a newcomer know that the lint tool that's complaining at them is wrong? There are plenty ways to do bad things in PHP, but that's why we have code review.
If @ becomes the recommended way to silence errors, then PHPCS shouldn't warn about it, just like it didn't for at-ease.

My two cents:

  • Has any sort of profiling done on @ and AtEase, one might be quite faster. We should check.
  • I'm always supportive of reducing code base that we need to maintain, one less repository to maintain, no matter how small, is potential for more work in critical areas for us
Tgr added a comment.Jun 4 2020, 10:02 AM

Having tried to do this in the past with some documentation rules, having PHPCS rules that people are supposed to disable is just a frustrating experience.

Fair point. So maybe a warning then (via SonarQube)?

Having tried to do this in the past with some documentation rules, having PHPCS rules that people are supposed to disable is just a frustrating experience.

@Legoktm, do you remember the exact example you are talking about? Without knowing what you are referring to I'm afraid this can't really be an argument. PHPCS rules are meant to frustrate when you violate them.

How does a newcomer know that the lint tool that's complaining at them is wrong?

The point is: the tool is not wrong. Prefixing arbitrary code with an @ prefix is a code smell, and almost certainly wrong.

[…] that's why we have code review.

As somebody who does a ton of code reviews I disagree with this. The @ prefix is way to easy to miss.

Maybe we could keep the PHPCS rule against @ and require people who use it to manually override?

Having tried to do this in the past with some documentation rules, having PHPCS rules that people are supposed to disable is just a frustrating experience. How does a newcomer know that the lint tool that's complaining at them is wrong? There are plenty ways to do bad things in PHP, but that's why we have code review.
If @ becomes the recommended way to silence errors, then PHPCS shouldn't warn about it, just like it didn't for at-ease.

I support having a PHPCS rule against @, and making people manually silence it when they need to use it. We do this in JavaScript for some things too, and it works well in my experience. A newcomer would find out about this the same way that they would currently find out that they're supposed to use AtEase::suppressWarnings() rather than @: reading documentation about how to suppress warnings, or looking at other code for examples.

Krinkle updated the task description. (Show Details)Aug 21 2020, 9:37 PM
Krinkle updated the task description. (Show Details)Aug 21 2020, 9:40 PM

Updated proposal to reflect feedback:

Proposal 2
  • Keep coding conventions that discourage use of @.
  • Update coding conventions that "when error supressing is really needed" to no longer mention AtEase, but instead mention phpcs:ignore.

For third parties I suggest we keep bundling wikimedia/at-ease for at least two LTS releases. It is in wide use and I don't think we should force developers to waste time on these one-line replacements when for most code it would gain basically nothing from that.

Krinkle updated the task description. (Show Details)Aug 25 2020, 6:09 PM
Krinkle moved this task from Watching to Inbox on the TechCom board.Sep 9 2020, 1:36 PM
Krinkle closed this task as Resolved.Sep 11 2020, 7:27 PM
Krinkle claimed this task.
Krinkle moved this task from Inbox to Watching on the TechCom board.

Done (diff).