on May 23rd, 2008Exceptions, you’re doing it wrong
Edit: After a couple of comments and some personal pondering I decided to remove a small part of the article, if you read it before and think something is missing, then you know why.
Once again a post aimed at the PHP community, not so much of a rant but more of something I’ve seen done horribly wrong in a lot of PHP code recently, first let me take a few examples from a couple of well known PHP frameworks and libraries:
symfony, file: lib/database/sfMySQLDatabase.class.php
$error = 'Failed to create a MySQLDatabase connection'; throw new sfDatabaseException($error);
propel, file: runtime/classes/propel/util/BasePeer.php
throw new PropelException("Expecting to delete 1 record, but criteria match multiple.");
doctrine, file: lib/Doctrine/Connection.php
throw new Doctrine_Connection_Exception('First argument should be an instance of
PDO or implement Doctrine_Adapter_Interface');
I could go on and on and list a couple of hundred of these from each of most poplar PHP libraries, and they all make the same assumption: An exception is a fatal error. And by making this assumption and using one monolithic “DatabaseException”-class, it becomes impossible to handle the exceptions in any other manner then as a fatal errors.
For example the first snippet, coming from the symfony framework is thrown when you can’t connect to a database. Since every other error that can happen to any database connection done in symfony also throws a “sfDatabaseException” how is the user of the library supposed try a backup database or supply a custom “database is down”-error page ? By regex:ing the message of the thrown exception?
Exceptions are not, and I repeat not, a fatal error mechanism (they can be, sure - but it’s not their only or primary use). Taken the above symfony code again, it should look something like this:
throw new DatabaseConnectionFailedException('MySQL');
Or something along those lines, making it possible to somehow distinguish between different type of exceptions allows us to do something like this (again assuming the symfony code):
try {
// Try to connect to a database
} catch (DatabaseConnectionFailedException $e) {
// Try backup database
} catch (DatabaseException $e) {
// Generic database error
}
So, to sum it up:
Pleased to see I was *almost* doing it right :) I only throw exceptions when they are meant to be fatal. But I am going to use the inherited exception for sure. Thanks!
Throwing specific exception classes like DatabaseConnectionFailed is spot-on advice, but I wouldn’t recommend a Recoverable- versus Fatal- class hierarchy. What’s recoverable for one app (connection failed) may be fatal for another (no backup DB), so you’ll probably find it difficult to decide which side of the tree any given exception falls under. A shallow but specific hierarchy is probably effective enough. As the exception designer, your job is to tell the calling code what happened, not necessarily how it should be handled.
While for a different language (C#/VB.NET), I found Microsoft’s Framework Design Guidelines to have the most lucid advice for designing exception classes (and APIs in general). I’d highly recommend it for anyone designing an API for a class-based OOP language, even if not C#.
You make a good point there about Exceptions. Symfony, Propel guys should know better than to use exceptions this way.
However, I see this problem not related to PHP, but a problem related to the the coder. It can happen in any language.
Wow, I never looked at the source of the programs. Hard to believe that such well known programs would do that with exceptions. I usually like to have a numeric code to go along with the message, so the caller knows what type of error occurred and can handle it based on the code, if necessary.
Bob: Yes you’re right about the class hierarchy, I think I was trying to show a more abstract model of an exception hierarchy and somehow I managed to make i pretty concrete.
I’ll try to modify my original post a bit to reflect this, later tonight :)
Why differentiate between Recoverable and Fatal exceptions? To decide if an Exception is fatal or recoverable should be the task of who intercepts (if it is) the exception, not of the exception itself. If a DatabaseUnavailableException is thrown, i could decide to connect to the Backup database if I have one, or threat it as a fatal exception if no other database is available.
So, I think you are right: exceptions should be different for different errors, but they should not inherit to differentiate between Fatal or Recoverable. You can inherit from DatabaseException to DatabaseSQLException and DatabaseConnectionException for example, but Fatal and Recoverable is a not really a nice example imho.
The best advice I ever saw was to only use exceptions for ‘exceptional circumstances’ only (from The Pragmatic Programmer, I think). If that’s true then all of the recoverable error handling should actually return through the normal method return. i.e return a value to test to see if the database connection was really made, and create a method to get the error message.
Personally, the reason I don’t like pushing lots of stuff into the exception handling is because it becomes a second logical flow through the system. Usually the main logic is complex enough, you don’t need a whole other set of things quietly buried inside of it. If it’s simpler it is less likely to be wrong.
Paul.
http://theprogrammersparadox.blogspot.com
S2: Yes you’re right, if you look one or two comments above your comment you’ll see that I agreed with Bob when he said the same thing.
I have now edited the post to better reflect this.
Why should a visitor care that your database is down? An “unexpected error” message accompanied by a 500 return code is all the user is supposed to see. The rest of the information should go to a log.
Otherwise, I agree with your article *and* with bob’s comment.
Ovidiu Curcan: Well it could be an intranet site for example, but yes you got a point in your comment if we’re talking about public websites.
i didn’t even KNOW about throw() before i read this.
I have to disagree.
First of all, I hope you realise you can package an id with an exception. Have you also checked the sfDatabaseException class to see if they package the database error code with the exception, my guess is they do.
I see how you could make this assumption, but I am afraid you are wrong. Exceptions should not be created for each and every error case, that is just silly.
The correct method (as used by Symphony) is to use custom exception classes to help identify the “kind” of exception thrown, you can then determine the exception category (i.e. Database exception) and if you want (or need to) you can then determine the exact error type by calling getCode() on the exception: for example, for MySQL it may be a duplicate error or maybe a missing required field error (I forget the exact error code).
My Code for this would be something like
catch (DatabaseException $e)
{
$iMySQLErrorCode = $e->getCode();
// handle different error codes…
}
> First of all, I hope you realise you can package an id with an exception.
> Have you also checked the sfDatabaseException class to see if they
> package the database error code with the exception, my guess is they do.
Yes I’m well aware that you can link an error code with an exception, but no neither symfony (nor any of the other libs I took as example) does this.
> I see how you could make this assumption, but I am afraid you are wrong. > Exceptions should not be created for each and every error case, that is just silly.
I never said that exceptions should be created for every error case, but errors that do stand out and that also make sense to refactor out into their own class should.
> The correct method (as used by Symphony)
Which it isn’t.
I wouldn’t go so far as saying the correct method, but using an error code is also a good way to discriminate exceptions - thought not a method I use personally that much.
I agree with your idea that the code should be used to discriminate different types of errors within the same class of exceptions, but I don’t agree on the fact that all database exceptions should go under one gigantic “DatabaseException” as you suggest.
There is a clear advantage to be able to control the height an exception bubles to through the callstack, which is a lot harder to do with an error code.
> Exceptions should not be created for each and
> every error case, that is just silly.
Why? Every error has it’s exception. That’s how it should be when you write nice clean code. If I throw a GenericException for every error I could possibly have, I will never be able to tell what error was really thrown and what I can do about it. On the other side, if I have an Exception for every error, I can identify every error and handle it the way it should be handled.
Spot on, and I agree that this problem occurs in multiple languages.
Python’s DBAPI uses the exact same generic DatabaseError style.
It annoyed me enough that I wrote a translation layer to catch DatabaseError, run it through some regexps/rules in an external library, and then my apps
can rely on specified exceptions…
sorry this is python not php, but the pattern still works :)
try:
try:
…
except DatabaseError, err:
raise sdutil.adapt_db_error(err)
except sdutil.ConnectionClosedError:
…
found this pattern useful, as it lets the regexp hacks stay in an independantly maintained library.
Eli: Nice trick, but all in all I have to say that Python is pretty good at doing “the right thing” with exceptions, KeyError, IterationDone (or whatever the one that ends an iterator is called, always forget), ValueError, etc.
Jon is absolutely right. The base Exception class in PHP takes a message and an error code for a reason, and I think a lot of people forget that. In fact it seems that my all-time favorite framework, the Zend Framework, even forgets this and passes only an error code in many of it’s libraries.
I think subclassing your Exceptions just makes sense. A database connection error might be fatal, but a DB Query error isn’t neccessarily treated the same way. Of course creating a unique Exception subclass for each of the ways that a DB query can fail would just be ridiculous, so at some point it’s up to the programmer to recognize when it’s time to customize a class via parameters rather than subclassing.
What the argument boils down to? Wether you want to handle this way:
catch(DBConnectionException $dbce) {
…
} catch(DBException $dbe) {
…
} catch(Exception $e) {
…
}
Or this way:
catch(Exception $e) {
if($e->getCode() == DBException::CONNECTION_EXCEPTION) {
….
} else {
….
}
}
The reason they all treat an exception from the database layer as a fatal exception, is that they all make the erroneous assumption that “model” and “database” are synonymous.
Actually, you’re only half right: almost none of these examples should be handled by exceptions at all.
The real abuse of exceptions is using them for domain logic.
There is nothing ‘exceptional’ about not being able to connect to a database, find a row in a table, etc.
These are all situations which fall into the expected logical space of possibilities.
The fact that many frameworks use exceptions incorrectly does not make it acceptable.
This type of exception abuse is part of the downward spiral of ever poorer quality code these days.
Execptions are a mechanism to escape multile levels of a call stack for situations which are so, well, exceptional, that they mostly do not happen.
ALL other use of exceptions is pathlogical and evidence of mental laziness.
The biggest problem I have with exceptions is when to catch them and provide a friendly message for users v.s. when to let the exception speak for itself v.s. when to catch, repackage and raise the exception again. For example, I was playing with some libxml2/libxslt Python bindings a few weeks ago when I found that I was using try-except far too much because I played with XML schema, XSLT and XML all at once, and I parsed each file to make sure it was well-formed XML before trying to actually use it. Coming from C, I was more worried about specific error messages and return codes than letting things degrade gracefully. After all, letting Python say “parseError” rather than catching the exception is far more efficient, and it doesn’t really harm anything because of the fact that every exception is fatal. How would I know which file failed with a parseError? Debugging, of course. ^_^
Great article overall. Sometimes it is too easy to get happy about using exceptions and classes rather than simple errors and global functions. I once saw some code for a PHP style sheet switcher that was created using a classes… Talk about overkill! :P
We have a similar problem a while ago. PropelException was thrown when we are trying to add a duplicate record. I think checking if the duplicate exists before inserting is a bad idea since I don’t want to spend so much SELECT resources just to prevent the 0.05% occurence of a duplicate record in the table. At first I was stumped since PropelException is too generic. Well luckily what Propel did was they actually embedded the PDOException raised by PDO in their PropelException class and could be accessed via the getCause() method. I would then test if this embedded exception is a PDOException and if the PDOException code equals to the 23000 SQLSTATE error code for duplicate records.
The code is:
$e->getCause() instanceof PDOException && 23000 === (int) $e->getCause()->getCode()