Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extensions aren't allowed to register a custom exception handler #1781

Open
matteocontrini opened this issue May 13, 2019 · 4 comments

Comments

@matteocontrini
Copy link

commented May 13, 2019

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.

$this->app->singleton(ErrorHandler::class, function () {
$handler = new ErrorHandler;
$handler->registerHandler(new ExceptionHandler\FloodingExceptionHandler);
$handler->registerHandler(new ExceptionHandler\IlluminateValidationExceptionHandler);
$handler->registerHandler(new ExceptionHandler\InvalidAccessTokenExceptionHandler);
$handler->registerHandler(new ExceptionHandler\InvalidConfirmationTokenExceptionHandler);
$handler->registerHandler(new ExceptionHandler\MethodNotAllowedExceptionHandler);
$handler->registerHandler(new ExceptionHandler\ModelNotFoundExceptionHandler);
$handler->registerHandler(new ExceptionHandler\PermissionDeniedExceptionHandler);
$handler->registerHandler(new ExceptionHandler\RouteNotFoundExceptionHandler);
$handler->registerHandler(new ExceptionHandler\TokenMismatchExceptionHandler);
$handler->registerHandler(new ExceptionHandler\ValidationExceptionHandler);
$handler->registerHandler(new InvalidParameterExceptionHandler);
$handler->registerHandler(new ExceptionHandler\FallbackExceptionHandler($this->app->inDebugMode(), $this->app->make('log')));
return $handler;
});

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

Thanks.

@tobyzerner

This comment has been minimized.

Copy link
Member

commented May 13, 2019

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

This comment has been minimized.

Copy link
Member

commented May 17, 2019

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 franzliedke added this to the 0.1.0-beta.10 milestone Jun 2, 2019

@franzliedke

This comment has been minimized.

Copy link
Member

commented Jul 6, 2019

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

@franzliedke

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

#1843 made this very easy, now we just need the extenders. 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.