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.

In T253461#6162104, @Daimona wrote:

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.

In T253461#6162294, @MaxSem wrote:

In T253461#6162104, @Daimona wrote:

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"

In T253461#6162104, @Daimona wrote:

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.

In T253461#6162294, @MaxSem wrote:

In T253461#6162104, @Daimona wrote:

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...

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 ] );

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 >

In T253461#6161850, @thiemowmde wrote:

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.

In T253461#6161850, @thiemowmde wrote:

[…] 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.

In T253461#6163975, @hoo wrote:

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 ) );

In T253461#6163975, @hoo wrote:

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.

In T253461#6161850, @thiemowmde wrote:

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.

In T253461#6164288, @thiemowmde wrote:

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.

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.

In T253461#6163439, @Tgr wrote:

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

In T253461#6191082, @Legoktm wrote:

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.

In T253461#6191082, @Legoktm wrote:

In T253461#6163439, @Tgr wrote:

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.

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.

This should probably have a custom sniff to spot use of AtEase and require use of phpcs:ignore instead.

I modified mw:at-ease to reflect this decision, even if the library is strictly-speaking independent of MediaWiki.

Change 957993 had a related patch set uploaded (by Krinkle; author: Krinkle):

[IPSet@master] IPSet: Remove AtEase overhead from hot code path

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

Change 957993 merged by jenkins-bot:

[IPSet@master] IPSet: Remove AtEase overhead from hot code path

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