vlucas / bulletphp

A resource-oriented micro PHP framework
http://bulletphp.com
BSD 3-Clause "New" or "Revised" License
418 stars 50 forks source link

Enhancement: Catch-all exception handling #71

Open ellotheth opened 9 years ago

ellotheth commented 9 years ago

When a Bullet app throws an exception, it triggers two handlers: the handler for Exception, and the handler for the exception subclass (i.e. RuntimeException or MyAwesomeCustomException). The handler for Exception always runs first:

// Always trigger base 'Exception', plus actual exception class
$events = array_unique(array('Exception', get_class($e)));

As a result, the behavior in the Exception handler applies to every exception. Subclass handlers can overwrite behavior that manipulates the Response, but any error logging or notification will apply to every exception.

In most of my Bullet apps, I use custom exceptions to handle all kinds of error conditions, including invalid user input. I don't necessarily want a stack trace in my error log every time a user mistypes their query, but I do want logs for otherwise unhandled or unexpected exceptions. In other words, I want to control my error logging on a per-exception (or per handler) basis. At the moment, that means I've got a giant switch statement (with a default case) in my Exception handler.

Swapping the order of exception handling so Exception is last would be a big backwards-compatibility break. What if a catch-all exception handler (maybe except, in the same category as before and after) were added that only ran if no other exception handlers were present? If App::filter() returned false when no handlers were triggered, it could look something like this:

// Run and get result
try {
    $response = $this->_runPath($this->_requestMethod, $path);
} catch(\Exception $e) {
    // Always trigger base 'Exception', plus actual exception class
    $events = array_unique(array('Exception', get_class($e)));

    // Default status is 500 and content is Exception object
    $this->response()->status(500)->content($e);

    // Run filters and assign response; returns false if nothing was run
    if (!$this->filter($events, array($e))) {
        $this->filter('except', array($e));
    }

    $response = $this->response();
    break;
}
vlucas commented 9 years ago

Hhhmmm... what an unfortunate mistake. I think the user expectation is that SpecificException be handled first, and then fallback to a generic Exception handler.

In practice, I do wonder how large of a BC break it would be to change this. It seems like most people would just use Exception itself with a switch due to the current order of how events are fired, but it would be really hard to guess how this is actually used and depended on in the wild.

vlucas commented 6 years ago

Seems like @netom should fix this in v2 :)