Error handling: proposed update
MarkR
I notice that our error handling system isn't very good - I logged an
issue about it (124).
I propose the following:
1. We scrap the existing error handler because it isn't very useful,
and replace it with one which does the following:
- Wrap all unmasked (by error_reporting) errors in an ErrorException,
which is then thrown
- Write a handler for uncaught exceptions which logs in the PHP error
log, including a stack trace and some context information, and (perhaps
optionally) to the browser in HTML
2. We have it run at E_ALL (not E_STRICT)
3. We fix all issues which are detected by the described setup (I have
found several already)
4. We can remove all code checking for results from DB errors and such
like, these should be throwing exceptions instead
5. Every single instance of die() is removed, an exception should be
thrown instead. Unlike in Perl, die() is not an exception raising
mechanism in PHP.
Does this seem reasonable?
Mark
Matthias Bauer
> I notice that our error handling system isn't very good - I logged an
> issue about it (124).
>
> I propose the following:
>
> [...]
>
> Does this seem reasonable?
+1 from me. If you were to make a patch, I'd be willing to land it (in a
branch to see what breaks, fix things, then merge).
-Matt
Owen Winkler
>
> On 16.01.2007 23:40 MarkR wrote:
>
> > I notice that our error handling system isn't very good - I logged an
> > issue about it (124).
I'm +1 for someone replacing the Error class that I know we need with
one that actually does what I wrote it for.
Though, I would prefer it remain called "Error" to make things simple,
if that's reasonable.
Owen
MarkR
handling was supposed to behave- we've got things like:
Error::raise( sprintf('The directory specified for the SQLite database
is not writable by the web server. Please adjust the permissions on:
%s', dirname($dbfile) ) );
Which implies that Error::raise won't return,
But also stuff like:
return Error::raise( $o->dbh->errorInfo(),
E_USER_WARNING );
Which implies that Error::raise returns a useful value.
I would be in favour of scrapping both these uses and just having it
throw an exception instead. This seems more sensible.
In that case, if we're going to throw an exception when a DB error
occurs, why not just have PDO throw an exception directly.
Doing these things would then mean that we can scrap almost all of the
error handling code in the app (But would need to catch a PDOException
in a few places instead).
Certainly the places we're calling die() wouldn't need to bother any
more.
Mark
Mark
Owen Winkler
>
> I'm having difficulty understanding exactly how the original error
> handling was supposed to behave- we've got things like:
There are reasons to have the Error class return values, especially in
the database class, so that the calling function can handle the error
display.
The Error class should only actually display errors in two cases:
1) When it is explicitly told to do so.
2) When an unhandled error occurs via set_error_handler
Since throwing exceptions can't (I could be wrong, but I don't see a
way) return a value to the place that called the function, I don't see
a way that you could use exceptions in the database class (for
example) to have it return errors reliably to the calling function and
also provide detailed error information for logging and debugging
display.
No, we don't ever want it to die(), but we don't want it outputting
errors from exceptions in places that could better handle the scenario
with a returned value.
Owen
MarkR
and then use it to do something OTHER than drop dead with debug info?
I'm trying to think of ways of radically simplifying the error handling
strategy. Throwing exceptions would seem to be the main one.
Throwing exceptions can't return a value of course, but they can be
caught.
In the unlikely event that we don't want to croak following a database
error, we can catch PDOException (e.g. to invoke the installer if
tables are missing).
I want to try to optimise the common case, and worry about uncommon
ones later. I've developed several apps before, which just let
exceptions happen (and get handled by an uncaught exception handler,
which logs lots of debug info).
In my opinion, doing stuff like :
if ( Error::is_error( $result ) ) {
$result->out();
die();
}
(options.php about line 99)
is just plain wrong. That should happen by default (except more
verbosely), and we should code for the case where we *don't* want it
to croak the error.
Mark
Matthias Bauer
> I'm trying to think of ways of radically simplifying the error handling
> strategy. Throwing exceptions would seem to be the main one.
>
> Throwing exceptions can't return a value of course, but they can be
> caught.
+1 on switching Error handling to Exceptions. I like bubbling.
-Matt
Jay Pipes
>
> Is there any case where we want to return an error from the database,
> and then use it to do something OTHER than drop dead with debug info?
Yes. For instance, a foreign key violation coming from the database can
best be handled by the caller, and is not appropriate to drop dead to debug.
> I'm trying to think of ways of radically simplifying the error handling
> strategy. Throwing exceptions would seem to be the main one.
Exceptions != Errors. They are entirely different. Exceptions are
events which occur infrequently, or outside of the natural occurrence of
an application's domain. For instance, a database connection failure
would be considered an exception, since it is presumed that the natural
state of the application is to be able to connect to the database.
However, errors are different. They represent failures of a type that
can be expected during normal application operation due to incorrect
user input. The example above of a foreign key violation would be a
good example of an error; so would a divide by zero error.
> Throwing exceptions can't return a value of course, but they can be
> caught.
Indeed, though it is useful to separate out thinking on exceptions and
errors in the way described above. Errors can be dealt with by the
caller, exceptions should be handled by the application's overall
process space and either, in debug mode, display debug information, or,
in user mode, gracefully die.