Please stop (ab)using the silent operator ("@")

Support & Bugs
1

Piwik appears to (ab)use the “@” PHP operator heavily to silence any errors.

This has one ugly side effect: in case of a fatal error, you’ll get a blank page only and no error gets logged anywhere!

I am tracking SVN currently and changeset #5980 was causing this behavior (http://dev.piwik.org/trac/changeset/5980).
While this particular error appears to be fixed (in #5988), the “shutup” operator is still used there.

Another reason to avoid the “@” operator/prefix is for performance reasons: PHP has to toggle its internal error handling every time it when interpreting a silenced expression.

2

Please provide a precise reason for an abuse of @ operator?
in general, they are important to have (since we should catch the error upstream), but we might abuse it in specific places (not catch the error).

3

If you benchmark it, the @ operator has very little overhead.

Also, we have to consider the fact that many users run Piwik in restrictive environments, eg disabled functions, insufficient write permissions, etc.

On the whole, we strive to find a balance between perfection and pragmatism.

4

E.g. the following code:

$clientHeaders = @Piwik_Config::getInstance()->General['proxy_client_headers'];

I assume it’s used here to silence any noticed from an unset “proxy_client_headers” config option.

There’s no reason to suppress any possible notice like that, but instead any settings that might be used should get initialized to NULL or something like that instead.

5

It appears to be twice as slow for an assignment, given the following test code:


% cat test.php
<?php
error_reporting(E_ALL ^ E_NOTICE);

$a = microtime(true);
for( $i=0; $i<10000; $i++) {
  $foo = @$bar['as'];
}

$b = microtime(true);
for( $i=0; $i<10000; $i++) {
  $foo = $bar['as'];
}

$c = microtime(true);

echo "Time with @:    ", $b-$a, "\n";
echo "Time without @: ", $c-$b, "\n";
?>

% php test.php
Time with @:    0.0081470012664795
Time without @: 0.0034759044647217

While that’s not that much in absolute terms, it adds up anyway.

[quote=vipsoft]
Also, we have to consider the fact that many users run Piwik in restrictive environments, eg disabled functions, insufficient write permissions, etc.

On the whole, we strive to find a balance between perfection and pragmatism.[/quote]

There’s a reason for some usage of the @-operator for sure.

It should just be made absolutely sure, that the user won’t get a blank page without the fatal error displayed or logged somewhere.

6

Let’s put this in perspective.

You’re talking about 5 milliseconds total for 10000 iterations. The actual overhead is 0.0000005 seconds, or 0.5 microseconds per occurrence.

There are 237 occurrences of the @ operator spread across the Piwik codebase. None of which appear to be used in any loops, so even if there were a codepath that could exercise every occurrence, it would hypothetically add 0.2 milliseconds to a mythical request that has loaded over 70 different files (including 9 different Controllers and templates/views). This passes my sniff test for reasonable tradeoffs.

In many cases, the alternative wouldn’t be to remove the @ operator but to replace it with a conditional test. This doesn’t always result in better performance. For example:

Let’s say a recurring problem is that some users don’t have the correct write permissions on a file or directory (e.g., umask is set incorrectly). If we changed this:


    @unlink($tmpfile);

to:


    if (is_writable($tmpfile)) {
        unlink($tmpfile);
    }

we might find the overhead in the second case is relatively worse (e.g., 1.1 microseconds of overhead for the conditional vs 0.5 microseconds for the @ operator).

There might be cases where the conditional test is better, but this always adds code, increases cyclomatic complexity, and reduces code clarity. Going back to your excerpt, there’s really no convincing argument to not use the @ operator.


  $clientHeaders = @Piwik_Config::getInstance()->General['proxy_client_headers'];

// vs:

  if (isset(Piwik_Config::getInstance()->General['proxy_client_headers']))
    $clientHeaders = Piwik_Config::getInstance()->General['proxy_client_headers'];
  else
    $clientHeaders = null;

// or:

  $clientHeaders = isset(Piwik_Config::getInstance()->General['proxy_client_headers'])
    ? Piwik_Config::getInstance()->General['proxy_client_headers']
    : null;
7

Thanks for your explanations - yes, it’s mostly nitpicking from my side, but please understand that spending quite some time to debug a blank page without any error being logged/displayed should be avoided after all.

Given the example, I was thinking about fixing the instantiation of the “General” array of the Config instance: you would merge the config once with an array containing all the defaults.
This way you would only have an additional array_merge, but apart from removing some/most of the “@” operators, you will also have a place where the default values and/for all configuration parameters are listed/managed.

8

I think you’re right something have changed recently, now errors are not shown anymore when developing (had the issue yesterday).

Some error are being silenced since 1.7.1

9

I am not talking about errors being not displayed in general (which may be configured via display_errors for example), but the errors hidden by using “@” explicitly.

When using “@” with include/require it would also silence any errors from the included file (don’t do that!).