flarum / framework

Simple forum software for building great communities.
http://flarum.org/
6.39k stars 835 forks source link

New PHP Extender: Registering custom exception handler #1781

Closed matteocontrini closed 4 years ago

matteocontrini commented 5 years ago

Currently, extensions cannot register a custom ErrorHandler. For example, an extension cannot throw a custom exception in a controller and also rely on a custom handler to generate an appropriate JSON response.

With the following sample, if MyException is thrown in a controller the registered error handler is not called.

# extend.php
return [
    function (ErrorHandler $handler)
    {
        $handler->registerHandler(new MyErrorHandler);
    },
];
class MyErrorHandler implements ExceptionHandlerInterface
{
    public function manages(Exception $e)
    {
        return $e instanceof MyException;
    }

    public function handle(Exception $e)
    {
        $status = $e->getCode();
        $error = [
            'status' => (string) $status,
            'message' => $e->getMessage()
        ];

        return new ResponseBag($status, [$error]);
    }
}

The problem is that the ErrorHandler is configured with a FallbackHandler that catches all the exceptions (line 82), so if you register a new one it will be appended after the fallback one.

https://github.com/flarum/core/blob/bbe62f400fd849f1f86ce7a4931c59fab4aec6fc/src/Api/ApiServiceProvider.php#L68-L85

I'll leave the discussion and proposals for an improvement to actual PHP/Flarum developers 😄

Thanks.

tobyzerner commented 5 years ago

Thanks for the detailed report! We basically just need to restructure the binding and then add an Extender for this, similar to how the Frontend extender works.

luceos commented 5 years ago

The first example that came into mind is fof/sentry. Which adds it own handler as well. It does by adding the handler into the middleware stack. Check:

https://github.com/FriendsOfFlarum/sentry/blob/6077061d6cd7a4648cf53d4d82c9b3a33ebf8f81/extend.php#L24-L30

franzliedke commented 5 years ago

Linking this to #1641, which also has to do with exception handling, so the two should probably be handled together.

franzliedke commented 5 years ago

1843 made this very easy, now we just need the extenders. :smile: