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

TryCatchMiddleware is redundant because of error handling in ApiMiddleware #173

Open
SoTeKie opened this issue Jan 19, 2021 · 4 comments
Open

Comments

@SoTeKie
Copy link

SoTeKie commented Jan 19, 2021

Correct me if I'm wrong as I'm new to apitte and nette in general, but from testing and what I can see, this commit makes the TryCatchMiddleware redundant as it eats up any exceptions that would go the TryCatchMiddleware before this change. I'd appreciate some input even if I'm wrong about this as I'm failing to understand or replicate how TryCatchMiddleware can catch any exceptions with this change.

@SoTeKie
Copy link
Author

SoTeKie commented Jan 19, 2021

If I'm correct about my assumption I think the TryCatchMiddleware should be removed and the documentation updated.

@f3l1x
Copy link
Member

f3l1x commented Jan 19, 2021

Hi. Thx for question. I think it depends on how you're setup chain of middlewares, you can build your own chain and there will be a place for TryCatchMiddleware.

@SoTeKie
Copy link
Author

SoTeKie commented Jan 19, 2021

From what I understand TryCatchMiddleware will still catch an exception in other middleware, but not ones thrown in the controller for example. I'm not sure if this is intended but this is the behavior I could replicate so far, maybe it would be a good idea to emphasize that it won't handle errors thrown in controllers. The documentation now is worded in a way that makes it seem like it's the place to handle exceptions in any part of the application.

@f3l1x
Copy link
Member

f3l1x commented Feb 15, 2021

You're right @SoTeKie, TryCatchMiddleware will catch exceptions throwed in other middleware, generally every other exceptions then in dispatcher.

I would love to see PR with correct wording as you said.

@f3l1x f3l1x transferred this issue from contributte/apitte-middlewares Nov 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants