Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[Routing] Throw 405 instead of 404 when redirect is not possible#26100

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
fabpot merged 1 commit intosymfony:2.7fromnicolas-grekas:route-405
Feb 12, 2018

Conversation

@nicolas-grekas
Copy link
Member

QA
Branch?2.7
Bug fix?yes
New feature?no
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets-
LicenseMIT
Doc PR-

Finishes#25962.

@sroze
Copy link
Contributor

sroze commentedFeb 9, 2018
edited
Loading

👍 but... that's a BC break.

@nicolas-grekas
Copy link
MemberAuthor

nicolas-grekas commentedFeb 9, 2018
edited
Loading

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.

@nicolas-grekasnicolas-grekas changed the title[Routing] Throw 405 instead of 404 when trailing slash redirection is not possible[Routing] Throw 405 instead of 404 when trailing slash/scheme redirection is not possibleFeb 9, 2018
@nicolas-grekasnicolas-grekas changed the title[Routing] Throw 405 instead of 404 when trailing slash/scheme redirection is not possible[Routing] Throw 405 instead of 404 when redirect is not possibleFeb 9, 2018
@nicolas-grekas
Copy link
MemberAuthor

nicolas-grekas commentedFeb 9, 2018
edited
Loading

This now fixes the same issue with scheme-based redirections.

@Tobion
Copy link
Contributor

For this there is 307 HTTP status code

This status code is similar to 302 (Found), except that it
does not allow changing the request method from POST to GET.
https://tools.ietf.org/html/rfc7231#section-6.4.7

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'))) {
thrownewMethodNotAllowedException(array('GET'));
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 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?

Copy link
MemberAuthor

@nicolas-grekasnicolas-grekasFeb 11, 2018
edited
Loading

Choose a reason for hiding this comment

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

correct! I've moved the check a few lines below where it belongs

if ('/' === substr(\$pathinfo, -1)) {
// no-op
} elseif (!in_array(\$this->context->getMethod(), array('HEAD', 'GET'))) {
\$allow[] = 'GET';
Copy link
Contributor

@TobionTobionFeb 11, 2018
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Yes it is: the redirect handles the GET

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess you need to check if get is in the allowed methods of the route first.

Copy link
Contributor

Choose a reason for hiding this comment

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

and if not we can mark the request with 307 response in master and here we just continue matching

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

GETis 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.

Copy link
MemberAuthor

@nicolas-grekasnicolas-grekasFeb 11, 2018
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
MemberAuthor

@nicolas-grekasnicolas-grekasFeb 11, 2018
edited
Loading

Choose a reason for hiding this comment

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

@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.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

ping@Tobion let's agree on this and move on :)

Copy link
Contributor

@TobionTobionFeb 12, 2018
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

done

parent::match($pathinfo.'/');

if (!in_array($this->context->getMethod(),array('HEAD','GET'))) {
thrownewMethodNotAllowedException(array('GET'));
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, we don't know if GET is allowed at all.

Copy link
Contributor

@TobionTobionFeb 12, 2018
edited
Loading

Choose a reason for hiding this comment

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

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 likein_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.

@nicolas-grekas
Copy link
MemberAuthor

@Tobion comments addressed.

@fabpot
Copy link
Member

Thank you@nicolas-grekas.

@fabpotfabpot merged commit9284281 intosymfony:2.7Feb 12, 2018
fabpot added a commit that referenced this pull requestFeb 12, 2018
…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
@nicolas-grekasnicolas-grekas deleted the route-405 branchFebruary 12, 2018 17:56
This was referencedFeb 28, 2018
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@fabpotfabpotfabpot approved these changes

+1 more reviewer

@TobionTobionTobion approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

2.7

Development

Successfully merging this pull request may close these issues.

5 participants

@nicolas-grekas@sroze@Tobion@fabpot@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp