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

New PHP Extender: Registering custom exception handler #1781

Closed
matteocontrini opened this issue May 13, 2019 · 4 comments · Fixed by #1970
Closed

New PHP Extender: Registering custom exception handler #1781

matteocontrini opened this issue May 13, 2019 · 4 comments · Fixed by #1970
Assignees
Milestone

Comments

@matteocontrini
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Member

luceos 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
Copy link
Contributor

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

@franzliedke
Copy link
Contributor

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

@Ralkage Ralkage modified the milestones: 0.1.0-beta.10, 0.1.0-beta.11 Sep 10, 2019
@franzliedke franzliedke mentioned this issue Sep 23, 2019
78 tasks
@franzliedke franzliedke changed the title Extensions aren't allowed to register a custom exception handler New PHP Extender: Registering custom exception handler Nov 15, 2019
franzliedke added a commit that referenced this issue Jan 17, 2020
This extender implements several methods for extending the new error
handling stack implemented in #1843.

Most use-cases should be covered, but I expect some challenges for more
complex setups. We can tackle those once they come up, though. Basic
use-cases should be covered.

Fixes #1781.
@franzliedke franzliedke self-assigned this Jan 17, 2020
franzliedke added a commit that referenced this issue Jan 18, 2020
This extender implements several methods for extending the new error
handling stack implemented in #1843.

Most use-cases should be covered, but I expect some challenges for more
complex setups. We can tackle those once they come up, though. Basic
use-cases should be covered.

Fixes #1781.
franzliedke added a commit that referenced this issue Jan 31, 2020
This extender implements several methods for extending the new error
handling stack implemented in #1843.

Most use-cases should be covered, but I expect some challenges for more
complex setups. We can tackle those once they come up, though. Basic
use-cases should be covered.

Fixes #1781.
luceos pushed a commit that referenced this issue Feb 4, 2020
This extender implements several methods for extending the new error
handling stack implemented in #1843.

Most use-cases should be covered, but I expect some challenges for more
complex setups. We can tackle those once they come up, though. Basic
use-cases should be covered.

Fixes #1781.
wzdiyb pushed a commit to wzdiyb/core that referenced this issue Feb 16, 2020
This extender implements several methods for extending the new error
handling stack implemented in flarum#1843.

Most use-cases should be covered, but I expect some challenges for more
complex setups. We can tackle those once they come up, though. Basic
use-cases should be covered.

Fixes flarum#1781.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants