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

Add throws Exception #975

Closed
wants to merge 1 commit into from
Closed

Add throws Exception #975

wants to merge 1 commit into from

Conversation

harikt
Copy link
Contributor

@harikt harikt commented Dec 6, 2017

@weierophinney , @shadowhand

Adding Exception to the interface

RequestHandler may throw an exception if request conditions prevent it from producing a response

RequestHandler may throw an exception if request conditions prevent it from producing a response
@@ -93,6 +93,8 @@ interface RequestHandlerInterface
*
* @param ServerRequestInterface $request
*
* @throws \Exception MAY throw an exception if request conditions prevent it from producing a response
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see why this would be necessary. Request conditions shouldn't trigger an exception, domain code could. The latter might be out of scope.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/php-fig/fig-standards/blob/52a479b55f5c235ab44aed2254de7ef1f085982e/proposed/http-handlers/request-handlers.md#11-request-handlers

A request handler MAY throw an exception if request conditions prevent it from producing a response. The type of exception is not defined.

What does this sentence means ? ( It is mentioning RequestHandler not Request itself ) If you think this doesn't throw an exception I believe this line should be removed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should you not at least use a common exception interface if you want exceptions in the handlers? otherwise the consumer can't know what to catch and might mix up their domain exceptions or exceptions from the http layer with the handler exceptions...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dbu Let me turn that around on you: would you expect all user created controllers in an MVC framework to throw a framework-defined exception? That would mean they would have to catch any domain exceptions and re-cast them as MVC-layer exceptions...

Request handlers and middleware fall under a similar umbrella, which is why we are not defining any exception types or limiting what types they can throw.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A common exception type is rather useful when it carries some context, but I can't think of any case when I want to handle an exception differently purely based on it's type.

For example my controller might throw an MVC specific 404 exception in which case my error middleware should catch the request and respond with a 404 page. When a domain exception leaks reaches my error middleware, I probably convert it to a 500 response and show the internal server error page.

That would mean they would have to catch any domain exceptions and re-cast them as MVC-layer exceptions

To be honest, I believe that would be ideal, because that means all errors are handled and no domain exception leaks to the transport level, but I know that's not how things work nowadays.....

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that would be ideal, because that means all errors are handled and no domain exception leaks to the transport level,

Except it's far from ideal. Many developers register exception handlers with their application so that they can handle these (to log them, to return a response, etc.), and doing try/catch blocks in every request handler and/or middleware means:

  • Problems when exceptions are not caught (e.g., none are expected, but one is raised)
  • Problems due to throwing a new exception without binding the previous exception (lost information)
  • Problems due to nested exceptions (recursion, etc.)

Re-casting introduces far more errors than it solves, in my experience.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW I think this is just scope creep. We don't need to define any error handling behaviour because that's entirely up to implementors.
In my opinion, the use case for annotating @throws in a interface is when you actually have some error behaviour you want to formalise, you want to dictate or give indications about how implementors should handle error conditions, and that usually happens by delivering specific exception types too. We don't have such a requirement here, it's not among the goals of the spec, so there is nothing to add in this regard.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree @stefanotorresi. Every piece of code might throw an exception, so there is no reason to formalize this.

@weierophinney I believe we should reject this PR, but it's your call.

@weierophinney
Copy link
Contributor

Closing. As noted in several comments, the WG feels that annotating a @throws declaration formalizes behavior we do not wish to formalize within this specification. Essentially, it would introduce scope creep, and introduce an onus on middleware implementors to implement error handling in each and every middleware, something that has largely been avoided in reference implementations to great success.

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

Successfully merging this pull request may close these issues.

7 participants