Call not-found/bad-method handlers on invalid HTTP method #2141
Conversation
|
I'll work on improving coverage back to where it should be. No need to merge until I do that. |
Partially resolves slimphp#2005. Router map() calls don't have method checking, and we still don't link the valid methods in the request to valid methods at the application level (which IMO should be since the router doesn't enforce any sort of method limitation at this point), but now you won't get an uncaught exception when someone hits you with an HTTP message with verb CHICKEN. Also revised tests to check app run handling rather than request create handling, since internally to the request a bad method should still be invalid/throw an exception, so we don't want to weaken that guarantee. But we do want the application not to 500 at runtime. Along with that, behavior (as tested) differs between whether we have a route or not, since in any other case the former would give you a bad method exception, while the latter would give you a not-found. So these tweaks match up that behavior for obscure HTTP methods as well. Finally, RouterInterface now has constants for route info array returns. They're only used on the exceptional path, but they make it a bit clearer why we're pulling various numeric keys out of the route, rather than having a non-obvious contract with FastRoute. Took extra care here to respect order of execution for the existing code so anything relying on that order will still work. Likewise, InvalidMethodException subclasses InvalidArgumentException and returns the same message and code, so anything checking for exception strings etc., as well as instanceof checks, will still work. Credit to @nickdnk for building the first red test for this issue.
|
...and by "improving coverage" I actually mean "fixing a completely broken first implementation", as it turns out. Now we're good to go though :) |
Slim/App.php
Outdated
| $response = $this->container->get('response'); | ||
|
|
||
| $response = $this->process($request, $response); | ||
| $response = !isset($e) ? $this->process($request, $response) : $this->processInvalidMethod($request, $response); |
There was a problem hiding this comment.
Please refactor this ternary into the try/catch
There was a problem hiding this comment.
I assume that means duplicating $response = $this->container->get('response'); to maintain existing application flow?
There was a problem hiding this comment.
I think that it can go before the try?
There was a problem hiding this comment.
Existing flow gets the request before the response so I wanted to maintain that order of execution. But sounds like we're cool with swapping order since the two should never be related anyway. Will push a rev once this talk's over.
There was a problem hiding this comment.
I don't think it should matter. If it does, I'm not sure what I'll think!
There was a problem hiding this comment.
Cleaned up! Keep up with the feedback :)
Changes execution order a bit, but won't matter unless folks are doing something really weird interaction-wise between container request and response objects. And maybe not even then!
|
Thanks! |
Partially resolves #2005. Router map() calls don't have method checking,
and we still don't link the valid methods in the request to valid
methods at the application level (which IMO should be since the router
doesn't enforce any sort of method limitation at this point), but now
you won't get an uncaught exception when someone hits you with an HTTP
message with verb CHICKEN.
Also revised tests to check app run handling rather than request create
handling, since internally to the request a bad method should still be
invalid/throw an exception, so we don't want to weaken that guarantee.
But we do want the application not to 500 at runtime.
Along with that, behavior (as tested) differs between whether we have a
route or not, since in any other case the former would give you a bad
method exception, while the latter would give you a not-found. So these
tweaks match up that behavior for obscure HTTP methods as well.
Finally, RouterInterface now has constants for route info array returns.
They're only used on the exceptional path, but they make it a bit
clearer why we're pulling various numeric keys out of the route, rather
than having a non-obvious contract with FastRoute.
Took extra care here to respect order of execution for the existing code
so anything relying on that order will still work. Likewise,
InvalidMethodException subclasses InvalidArgumentException and returns
the same message and code, so anything checking for exception strings
etc., as well as instanceof checks, will still work.
Credit to @nickdnk for building the first red test for this issue.