Is error suppressing bad practice?

On a SO question I asked here about some code I was unsure about, someone replied “BTW, horrible code there: it uses the error suppressing symbol (@) a lot.”

Is there a reason why this is bad practice? With things like:

$db=@new mysqli($db_info) or die('Database error');

, it allows me to display just a custom error message. Without error suppressing, then it would still display the typical PHP message of:

Warning: mysqli::mysqli(): php_network_getaddresses: getaddrinfo failed: No such host is known. in somefilepath on line 6

as well as ‘Database error’.

Is error suppressing always bad, and if so, what specifically about the above is bad?

Update: the actual code that I’m using is:

or error('Datatabase error', 'An error occurred with the database' . (($debug_mode) ? '<br />MySQL reported: <b>' . $db->error . '</b><br />Error occurred on line <b>' . __LINE__ . '</b> of <b>' . __FILE__ . '</b>' : ''))

which removes all previous output and displays an error message. So the fact that the error message doesn’t include details about what specifically happened (which people seem to be suggesting as a reason why error suppressing is bad) is irrelevant.

I think you’re doing the right thing by suppressing the error, because you are implementing your own error handling.

It might be easier to consider the equivalent in, say, Java. Suppressing the error using @ is analogous to swallowing an exception. The following code, which is similar to yours, is reasonable:

try {
    db = new MySQLi(dbInfo);
} catch (SQLException connectionFailure) {
    die("Database error");
}

(Well, in Java, you wouldn’t want the servlet engine to die, but you get the idea.)

However, this isn’t acceptable:

try {
    db = new MySQLi(dbInfo);
} catch (Exception e) {
}

Most PHP error suppression is done carelessly, analogous to the latter example, hence the knee-jerk reaction against your code.

Error suppression is bad, because it does not only hide the information you already know (something went wrong). It may also hide information vital for debugging which you aren’t aware of (what, where and why).

In this specific example, “Database error” isn’t a very good error message. Something went wrong with the DB. Not very enlightening. The “Warning: mysqli::mysqli(): php_network_getaddresses: getaddrinfo failed: No such host is known. in somefilepath on line 6” is actually a better error message. It gives you a location and reason for the problem. You can jump right in and start debugging, because the error has already been located.

Of course, library designers sometimes have a tendency of bringing up to many irrelevant warnings. I don’t know PHP sufficiently well to know if it has a way to filter warnings you are not interested in. I know that reading through a hundred-line stacktrace isn’t very enlightening. But the correct response to too much information is not to reduce the amount of information to zero, but to filter out irrelevant parts at some stage. The @ operator does not have this granularity (it’s motto is all or nothing), and is therefore not a suitable tool for effective error management.

There are some cases where you know that a certain error will turn up, and that fixing the actual problem is more expensive than silencing the message (and suffering potential fallout from that). This could be the case in a one-off script, but sticking your fingers into your ears and going “la la la” is not a professional response to (potential) bugs.

Supressing errors is bad because it hides the problem, which many times we are not even aware of and it may result in unexpected behavior that may prove very costly in some critical applications eg in applications that are used in financial, health and defence domains.

It is also not a good idea to have unhandled exceptions in the code and show the actual error messages to the user as it may result in security issues because error messages usually reveal a lot about your code, which may help malicious users and hackers to manipulate the system.

So the good practice is to handle the errors at different levels eg at highest level you may handle the error by just loggin the actual error and showing a simple and user friendly message to the user.

Yes, the error suppressing operator is generally a bad idea.

You should manage error reporting in the configuration (php.ini). So you can choose a different setting for each environment (for ex. leave warnings in development and hide them in production).

The only situation when using @ could make sense is when you develop a library for other developers. If you voluntarily choose to do something which produces a warning, and don’t want to annoy the users of your library with a warning that does not depend on them, @ could be a solution.

I can’t think of any other use.

I think we can formalize the reason that error suppression is bad because unknown unknowns happen.

Today you suppress the error. Then an unknown unknown happens, that line of code now causes a fatal error. And that puts you in the worst situation possible for system maintainers: e.g. the software stops working and nothing shows up in the logs.

Error suppression makes it impossible to maintain the good programming practice that “if things don’t work, there will likely be something in the error logs”.

Don’t suppress errors, instead create code that doesn’t cause any errors.

Additionally, make sure that any errors automatically get emailed to someone, so they can proactively fix them. Errors should be dealt with, not suppressed.

Suppressing errors (exceptions in your example cases, actually) AS WELL as warnings is a double edged sword. It helps your user-end application look clean, as you can basically assign custom error codes to a lot of pre-defined exceptions, but it can create issues when you, as a developer, are trying to debug an application.

Like, take a look at Microsoft Windows for example. When you encounter an exception, like a BSOD, you get an error code, as well as a certain string which lets you get a brief idea on what the exception is (for example, the one which I seem to encounter most often, DPC_WATCHDOG_VIOLATION). Now, I don’t really have an operating system development experience, but it should be pretty clear that the actual exception that was encountered in this case would’ve had a much bigger error message, but it was suppressed and handled by passing through a predefined template, which gave us the error’s Stop Code as well as a QR link to the error support page.

This is what I would say, good exception handling, since they know its an exception that can be encountered, have a template for it to send the user to the support page, and a user-readable error message that one can Google easily.

But what about an exception that does not have a predefined template? Would you throw a generic error message such as “operating system error“? Personally, that would be a design decision then, as to how you want to handle exceptions you don’t have a predefined template for.

A good practice would be to still print the Exception occurred, but somewhere inside a log (systemd allows us to check logs with journalctl wherever systemd is used). For Java, there is Logger. For PHP, there is Monolog.

Also, what about warnings? Warnings can be suppressed in most cases, until they are critical. But its still a good practice to still log them using a logger, into a file.

Two different things: One, an API reports something that it calls an error. Two, an API reports an error.

An example was an API that I used to make sure that a file with some name didn’t exist. The name of the API contained “delete”. It reported an error if a file with the given name didn’t exist, or if the directory where the file would have been didn’t exist. At least the first case was no error to me since after the call, the file didn’t exist; exactly what I wanted. The second case, you have to decide yourself if it is an error to you or not. On the other hand, no permission to read the directory is an error.

So there are rare cases where you ignore some errors. But in general, you don’t. The very least is some logging in a development version, preferably falling into the debugger.

There are also APIs where some errors are “normal”. For example the iOS keychain API. Calls can fail because the user has no password set, has no finger print stored, has old hardware, and lots of things that are “normal”. As a developer you care just about failure or success, not the reasons. And then there are possible errors that you have never seen, and you can’t ignore them. So you write your own API that calls the original API, ignores the ignorable errors, and reports the ones you can’t ignore.

I have a huge amount of methods to which i pass an arbitrary amount of key/value pairs.

in cases where i don’t discriminate between
null,0,false,array() or basically anything empty() or !isset(), i use

    if (@ $params['key'])

without error suppression, a warning is issued when the key does not exist. but i don’t care whether it is unset or empty.

this here is bloaty. i don’t like it at all:

    if (isset($params['key']) && $params['key'])

this is even more bloaty:

    $p_a = $p_b = null;
    extract($attributes, EXTR_OVERWRITE | EXTR_PREFIX_ALL, 'p_');
    if ($p_a) { ; }

personally, i find the @ operator very appealing here. the only warning this would ever suppress is a warning i couldn’t care less about.

but it might still be bad style, and i wonder whether there is a more elegant approach.

Filed under: softwareengineering - @ 17:51

Thẻ: error-handling, php, programming-practices