Why Suppressing Notices is Wrong

Tweet

When I first came to work for my current employer and opened up Zend Studio (don’t worry, they’ve mostly abandoned that atrocity of an IDE), around 2,000 of the 10,000 PHP files that made up our application lit up with errors. The developers before me never properly maintained it, and the horrors of legacy code I was exposed to were unfathomable. I knew right then and there that this wasn’t going to be an easy trip.

After spending weeks trying (and failing) to fix the mess, I gave up and focused on the more contemporary tasks implementing the new requested features on top of the chaos – a bad idea as most will agree. A job is a job though, and for a while it all worked well. In time, we started to notice the app’s write operations peaking beyond normal limits. Opening the error log revealed why: notices and warnings.

The PHP notice suppression operator is somewhat of a controversial topic in many circles. Some overuse it, some don’t use it at all, and some don’t even know it exists. Apologies in advance for the horrible code you’re about to witness in this article, but it serves a purpose to illustrate the usefulness (or lack thereof) of the suppression operator.

Notices and Warnings

PHP has different error levels categorized by severity. Notices and warnings are the weakest type which means they don’t break the execution of your application – instead, they’re just echoed out to the screen or written to the error log (or both, depending on your settings), and execution continues.

An example would be the following. If you have an empty array $aArray, and try to access a key that doesn’t exist like so:

<?php
$aArray = array();
$aArray['someKey'];

You get a notice that the key doesn’t exist.

Notice: Undefined index: someKey in /var/www/index.php on line 3

Call Stack:
    0.0009     328920   1. {main}() /var/www/index.php:0 

You get a notice that the key doesn’t exist. But this doesn’t break your code – in fact, if you try storing $aArray['someKey'] into a variable, that variable will actually get initialized as expected with a null value.

Let’s take a look at a more concrete example. Assume we have a controller that passes into its view an array of messages; the array can look like this (and in a lot of cases in this legacy code I was introduced to, it did!):

<?php
// $oView = ... our view object
// $aMessages = array();

if (/* some expression that signifies an error */) {
    $aMessages['error'] = 'Error no.2 occurred, contact developer!'; 
}

$aMessages['status'] = 'Finished operation';
$oView->aMessages = $aMessages;

You’ll notice that the “error” key is only set if the error actually occurs, that is, if the if-expression in the parentheses evaluates true.

The view code looked something like this (simplified for the purposes of this article):

<h1>Operation completed</h1>
<?php 
    echo @$this->aMessages['error'];
    echo '<hr />';
    echo $this->aMessages['status'];
    echo '<hr />';
    // ...

The status of the operation and the error is echoed out. But seeing as they were aware of the fact that “error” might not be set, they used the suppression operator @ to prevent the notice from being printed on screen.

A surprising amount of PHP developers don’t actually know that even if you suppress a notice on screen that it still gets logged if logging is turned on – so your error log is getting littered with pointless and relatively large lines of text. Developers often think @ is just legit shorthand for “ignore”, but it’s not – it’s shorthand for “I know my code here sucks, but let’s pretend everything is OK because I’m not going to make it better.”

When to Use It?

So when is using suppressions actually acceptable?

Never!

When you use suppressions, it means you’re aware your code might suck but you just don’t care. This tells developers who run into your code after you that you’re a bad developer.

It also slows down your app because it constantly has to write something into the error log. Imagine the code above being called by 1,000,000 visitors on a high traffic day. That’s one million lines of text in the error log! The impact on performance is hardly negligible.

Really? Never?

While developing, the error messages are there for a reason. They help you find bugs in your code before you inflict the same fate on your users and customers. They point you in the right direction as to what’s wrong and what needs to be fixed. This is true for notices as much as it is for fatal errors – they all serve to help you, the developer, make the best product possible. Sometimes, however, you might need to test some functionality before handling any remaining notices that are left over from before – when outputting generated files for example. If a notice gets printed before the file generation begins, that means headers were already sent (because without headers your browser has no idea how to echo the content it is being given) and that means your file won’t display properly because it needs to send its own headers to the browser in order for the browser to recognize we’re dealing with an actual file.

There’s also the rare case of having to deal with some of PHP’s iffy implementation details – for example, using the mail() function will throw an error if the mail server isn’t up, so doing @mail() is justified.

But these are all exceptions to the general rule. With training, you’ll be able to recognize the instances in which using a suppressor is unavoidable. Then and only then should you suppress anything, but you should strive to avoid even this. A good developer fixes ALL possible notices as soon as they detect a chance of encountering them.

How can you force yourself to do this, you might ask? By doing the same thing I did when I built my framework:

<?php
function myErrorHandler($errno, $errstr, $errfile, $errline)
{
    if (!(error_reporting() & $errno)) {
        // This error code is not included in error_reporting         
        return;
    }
    echo "<b>ERROR!</b> [$errno] $errstr<br>n";
    echo "  Fatal error on line $errline in file $errfile";
    echo ", PHP " . PHP_VERSION . " (" . PHP_OS . ")<br>n";
    echo "Aborting...<br>n";
    exit(1);
    break;
}
$old_error_handler = set_error_handler("myErrorHandler");

The snippet above replaces the default error handler with a custom function, which makes the application die on absolutely any error, warning, or notice.

This is, of course, simplified. A proper error reporting class is far more advanced than this and outputs much more info, but the gist of it is as follows: break your app on purpose whenever a notice or warning is encountered, or, in other words, treat your warnings and notices as fatal errors.

By doing this, your app will break during development and testing, giving you the chance to write your code as bulletproof as possible. You’re making your life and the lives of those coming into your code after you’re gone all much easier.

Adding a custom error handler which breaks your app with every notice and warning will make sure you and your developers take much more care of the cleanliness of your code. In the long term, it keeps the error log clean and reserved for only the most serious of errors. It might slow down development a bit for rookie programmers, but experienced developers will appreciate this extra effort. After all, it’s better to spend an extra week writing cleaner code than to spend months later down the line trying to fix it.

Conclusion

Good code is a medal you can strap onto your chest. It’s a trophy you wear proudly. Good code follows you after you leave it behind, and the tales it tells to those that inherit it will feature you as the main character. It’s up to you to make sure you’re the protagonist of those stories, and not an antagonist.

Free book: Jump Start HTML5 Basics

Grab a free copy of one our latest ebooks! Packed with hints and tips on HTML5's most powerful new features.

  • Pierre

    Very nice article, I will definitely use this error handler.

    • http://about.me/bruno.skvorc Bruno Skvorc

      Thanks, but you really shouldn’t, it’s just a mock handler, a demo, you probably want something more robust :)

    • KingCrunch

      Oh, please don’t! The error_handler (as well as the exception_handler) is similar bad, because it catches _every_ message regardless whether it is silenced (which is in some cases valid), or caught (in case of exceptions). Worth to say, that the error_handler gets even triggered, when error_reporting is set to 0, and the exception_handler gets triggered, even when the exception is caught (later).

      In my last project we used a error_handler and exception_handler, that “saved” _everything_ in an array. Really: Everything including the corresponding stacktrace. Imagine an array and you iterate over it and within this foreach there is a notice. ;) The memory usage was unnecessary high.
      The main problem was:
      – In development it as appended to the output. This often leads to extremely huge pages the browser receives (and slows down) and then you need to find out, which of this was the relevant error.
      – In production it was written to a log on shutdown_handler. The problem: In the meantime PHP cleans up it’s memory. If there were to many entries, the logger simply disappear and an error during shutdown is bad. Sometimes this leads to segfaults.

      It is soooo simple: Fix it!

      • blacksonic

        then how u know if an error occures in production if u suggest not to use it?

        • http://www.xoogu.com/ Dave

          turn on display_errors, and set error reporting level to E_ALL (also include E_STRICT if you’re on an older version of PHP).

          If you’re dealing with other’s code, you can always search your project for calls to change the error reporting level and use of the suppression operator, and remove them as well.

          After correcting visible errors, keep testing and check the error logs for any other errors.

  • Bearnik

    This article is great and well structure – totally agree with this

    • http://about.me/bruno.skvorc Bruno Skvorc

      Thanks a lot!

  • http://tech.navarr.me/ Navarr

    Really? Never?

    What about file_get_contents() on a remote URL? So long as you account for the error?

    • http://oaass.wordpress.com/ Ole

      For that I would use
      if (!file_get_contents(….)) { /* Error handling */ }

    • blacksonic

      curl is the answer
      file_get_contents doesnt work everywhere for urls, allow_url_fopen directive must be enabled

    • mcc

      something like if(ini_get(‘allow_url_fopen’)) {}, if(function_exists(‘curl_init’)) {}, theres lots of boilerplate code to work around these.

  • http://unassumingphp.com andy

    Very nice article, agree totally.

    • http://about.me/bruno.skvorc Bruno Skvorc

      Thank you!

  • http://blog.mindplay.dk Rasmus Schultz

    The mail() function is actually, in my opinion, the perfect example of when it’s horribly wrong to use the error-suppression operator. What possible real-world case could you have for sending an e-mail, where you don’t care, or don’t need to know, whether it was sent at all?

    For something like email, you really should use a message queue of some sort – add your e-mail to an outgoing queue, say, in an actual message queue service, a database record, or write it to a flat file, or directly to your SMTP servers outgoing folder if possible, so that (A) sending the e-mail doesn’t block the user and force them to wait for an SMTP server to respond, and (B) if the e-mail sending service is currently offline, the e-mail will simply queue up, and gets sent when the SMTP comes back online.

    The error-suppression operator is absolutely a bad idea here, as in most cases I can think. As a rule of thumb, I really only ever use the error-suppression operator for one thing – array lookups, when you’re not sure (and it doesn’t matter) whether the key is present in the array or not.

    For example, instead of if (isset($_GET['email']) && is_valid_email($_GET['email'])) can be written more comfortably as simply if (is_valid_email(@$_GET['email'])) – assuming that is_valid_email() can handle a NULL value and returns false.

    Bottom line, please don’t use error-suppression for anything that could actually cause an error. Suppressing a notice when doing array lookups really is just about the only acceptable example I can think of, and only because you’re not strictly suppressing an error, but merely using this as a way to shorten trivial/repetitive lookups that are *expected* to fail some of the time. Please don’t teach people to suppress actual errors!

  • Tim hawkins

    @mkdir , if the directory already exists

  • http://www.talksonweb.com AK

    @Rasmus Schultz, good point and well explained but don’t use exclamation marks. The article was a good read.