search

contributte / apitte

:wrench: An opinionated and enjoyable API framework based on Nette Framework. Supporting content negotiation, debugging, middlewares, attributes, annotations and loving openapi/swagger.
https://contributte.org/packages/contributte/apitte/
MIT License
61 stars 36 forks source link

TryCatchMiddleware is redundant because of error handling in ApiMiddleware #173

Open SoTeKie opened 3 years ago

SoTeKie commented 3 years ago

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 commented 3 years ago

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

f3l1x commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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.