[Routing] Throw 405 instead of 404 when redirect is not possible #26100
Conversation
|
👍 but... that's a BC break. |
|
A 301/302 in response of a POST to add a trailing slash doesn't work already. It's just silent now, but still badly broken. |
d433520 to
828d6fe
Compare
828d6fe to
45e477c
Compare
|
This now fixes the same issue with scheme-based redirections. |
|
For this there is 307 HTTP status code
I'm ok to change it to 405 as bug fix but in master it should be changed to 307 IMO. |
| throw $e; | ||
| } | ||
| if (!in_array($this->context->getMethod(), array('HEAD', 'GET'))) { | ||
| throw new MethodNotAllowedException(array('GET')); |
There was a problem hiding this comment.
I don't get it. When there is no route for the current uri path (ResourceNotFoundException) and the request method was POST, then it throws a MethodNotAllowedException? But we don't know at this point that a GET to the same URI would be allowed, do we?
There was a problem hiding this comment.
correct! I've moved the check a few lines below where it belongs
45e477c to
e10b2c7
Compare
|
|
||
| if ($hasTrailingSlash) { | ||
| $code .= <<<EOF | ||
| if ('/' === substr(\$pathinfo, -1)) { | ||
| // no-op | ||
| } elseif (!in_array(\$this->context->getMethod(), array('HEAD', 'GET'))) { | ||
| \$allow[] = 'GET'; |
There was a problem hiding this comment.
How do we know that GET is actually allowed? If we do a POST /foo on a PUT /foo/ route, it will say method not allowed but GET is allowed? We don't know if GET is allowed at all.
There was a problem hiding this comment.
Yes it is: the redirect handles the GET
There was a problem hiding this comment.
I guess you need to check if get is in the allowed methods of the route first.
There was a problem hiding this comment.
and if not we can mark the request with 307 response in master and here we just continue matching
There was a problem hiding this comment.
GET is allowed: it triggers a redirect. There is nothing else to consider on this very path without slash. The verbs allowed on the route apply only when there is a slash.
There was a problem hiding this comment.
retry the same request with GET and he could still receive a 404
Why? to my understanding, it would then get a 301/302, because of this very "if" which will be hit again.
There was a problem hiding this comment.
@Tobion note that if you're fine with the 404, but not with the 405, I'm OK to remove this line. It's only a minor thing on the HTTP side, and I doubt it will make any real difference in practice.
There was a problem hiding this comment.
ping @Tobion let's agree on this and move on :)
There was a problem hiding this comment.
If you replace HEAD with GET in $supportsTrailingSlash = $supportsRedirections && (!$methods || in_array('HEAD', $methods)); this works for PhpMatcherDumper. But the logic in RedirectableUrlMatcher is broken. See below.
| throw $e; | ||
| } | ||
|
|
||
| try { | ||
| parent::match($pathinfo.'/'); | ||
|
|
||
| if (!in_array($this->context->getMethod(), array('HEAD', 'GET'))) { | ||
| throw new MethodNotAllowedException(array('GET')); |
There was a problem hiding this comment.
Same here, we don't know if GET is allowed at all.
There was a problem hiding this comment.
This cannot work because we don't know if GET for the route is allowed. We would need to make a similar check as in the PhpMatcherDumper like in_array('GET', $route-getMethods()). But we don't have access to the route, so that is difficult to do.
Example:
Route definition: POST /foo/
Request: POST /foo -> throws ResourceNotFoundException, then we check again with slash, i.e. POST /foo/
This works so we return MethodNotAllowedException with allow GET
But GET /foo is not allowed. It will just return a 404 again if someone tries that.
So I don't see how to implement this logic in the RedirectableUrlMatcher. We could just say, RedirectableUrlMatcher does not support that feature. And revert this here but keep the logic in PhpMatcherDumper which most people use anyway.
e10b2c7 to
9284281
Compare
|
@Tobion comments addressed. |
|
Thank you @nicolas-grekas. |
…ssible (nicolas-grekas) This PR was merged into the 2.7 branch. Discussion ---------- [Routing] Throw 405 instead of 404 when redirect is not possible | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - Finishes #25962. Commits ------- 9284281 [Routing] Throw 405 instead of 404 when redirect is not possible
Finishes #25962.