Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Routing] support scheme requirement without redirectable dumped matcher#26304
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
5b3e320 to2bcc2a9Compare
nicolas-grekas left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
the $supportsRedirections property should be turned back to a local variable
| return\$this->redirect(\$rawPathinfo,\$ret['_route'], key(\$requiredSchemes)) +\$ret; | ||
| if (\$requiredMethods && !isset(\$requiredMethods[\$canonicalMethod]) && !isset(\$requiredMethods[\$requestMethod])) { | ||
| \$allow +=\$requiredMethods; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
By checking requirements before methods, we fixed an issue where a 405 is triggered while a scheme requirement is not met. Shouldn't we keep this fixed behavior, ie check methods last?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I also thought about this alot. There are cases were it's not optimal in both cases.
Request GET http /
Route POST https /
If you check the scheme first instead of the method. It will try to make a redirect from http to https although the route requires POST which is not met.
So I think checking method first is better as getting a 405 instead of a 404 for the case you explained does not really matter. Also this order is what was implemented by UrlMatcher. So it makes it more consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| $pathinfo = '/' !== $pathinfo[-1] ? $pathinfo.'/' : substr($pathinfo, 0, -1); | ||
| if ($ret = $this->doMatch($pathinfo)) { | ||
| return $this->redirect($pathinfo, $ret['_route']) + $ret; | ||
| if (in_array($this->context->getMethod(), array('HEAD', 'GET'), true)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
\in_array can be used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this file is dumped in the root namespace
Tobion commentedFeb 25, 2018
We would still need to pass the var to |
nicolas-grekas commentedFeb 26, 2018
ready for rebase |
91ecd89 tocbbc3e2CompareTobion commentedFeb 26, 2018
Rebased |
nicolas-grekas left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
with minor comment
| if (\$requiredMethods && !isset(\$requiredMethods[\$canonicalMethod]) && !isset(\$requiredMethods[\$requestMethod])) { | ||
| \$allow +=\$requiredMethods; | ||
| if (\$requiredSchemes && !\$hasRequiredScheme) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
if (!\$hasRequiredScheme) { would be enough here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
fixed
| 'httpPort' =>$context->getHttpPort(), | ||
| 'httpsPort' =>$context->getHttpsPort(), | ||
| '_route' =>'foo', | ||
| '_route' =>null, |
nicolas-grekasFeb 27, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this change breaks the 3.4 test suite, can't we do without?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We don't know the route that would match, unless we do a new match like for trailing slash. Also the way it worked was not reliable anyway. When you have a condition that didn't match because of the scheme but now matches with a different scheme, this route can be before the route that caused the redirect. So the actual route that will match might not be the one that was passed here at all.
nicolas-grekasFeb 27, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
we still need to make the currently failing test green before merging...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
fixed :)
735bc1d to7154ec1Compare7154ec1 to8c36250Comparebeec4f7 toddb5be6Comparenicolas-grekas commentedFeb 28, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@Tobion scheme redirection now does a match before, similar to trailing slash redirection. I also added another redirection, doing a trailing slash + scheme redirection when possible. |
8b3bc06 toafc6254Compare| } | ||
| return$this->getAttributes($route,$name,array_replace($matches,$hostMatches,isset($status[1]) ?$status[1] :array())); | ||
| $ret =$this->getAttributes($route,$name,array_replace($matches,$hostMatches,isset($status[1]) ?$status[1] :array())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
what's the reason for doing that before the $hasRequiredScheme check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
reverted, was a leftover from previous iteration
afc6254 to2e27658CompareTobion commentedFeb 28, 2018
Please add a test for scheme + trailing slash redirect. Then good to merge. |
2e27658 tof9b54c5Comparenicolas-grekas commentedFeb 28, 2018
Thank you@Tobion. |
…ble dumped matcher (Tobion)This PR was merged into the 4.1-dev branch.Discussion----------[Routing] support scheme requirement without redirectable dumped matcher| Q | A| ------------- | ---| Branch? | master| Bug fix? | yes| New feature? | yes| BC breaks? | no| Deprecations? | no <!-- don't forget to update UPGRADE-*.md files -->| Tests pass? | yes <!-- please add some, will be required by reviewers -->| Fixed tickets || License | MIT| Doc PR |The scheme handling was just redirecting immediately without testing the other routes at all for potential matches. This can cause Problems when you try to have different routes/controllers for the same path but different schemes. See added test.``` $coll = new RouteCollection(); $coll->add('https_route', new Route('/', array(), array(), array(), '', array('https'))); $coll->add('http_route', new Route('/', array(), array(), array(), '', array('http'))); $matcher = $this->getUrlMatcher($coll); $this->assertEquals(array('_route' => 'http_route'), $matcher->match('/'));```Instead of matching the right route, it would redirect immediatly as soon as it hits the first route.This does not make sense and is not consistent with the other logic. Redirection should only happen when nothing matches. While fixing this I could also remove the limitation> throw new \LogicException('The "schemes" requirement is only supported for URL matchers that implement RedirectableUrlMatcherInterface.');If redirection is not possible, the wrong scheme will just not match the route. This is the same as already implemented by UrlMatcher (not RedirecableUrlMatcher). If redirection is supported, it will redirect to the first supported scheme if no other route matches. This makes the implementation similar to redirection for trailing slash and handling not allowed methods.Also previously, the scheme redirection was done for non-safe verbs which shouldn't happen as well, ref.#25962Commits-------f9b54c5 [Routing] support scheme requirement without redirectable dumped matcher
Uh oh!
There was an error while loading.Please reload this page.
The scheme handling was just redirecting immediately without testing the other routes at all for potential matches. This can cause Problems when you try to have different routes/controllers for the same path but different schemes. See added test.
Instead of matching the right route, it would redirect immediatly as soon as it hits the first route.
This does not make sense and is not consistent with the other logic. Redirection should only happen when nothing matches. While fixing this I could also remove the limitation
If redirection is not possible, the wrong scheme will just not match the route. This is the same as already implemented by UrlMatcher (not RedirecableUrlMatcher). If redirection is supported, it will redirect to the first supported scheme if no other route matches. This makes the implementation similar to redirection for trailing slash and handling not allowed methods.
Also previously, the scheme redirection was done for non-safe verbs which shouldn't happen as well, ref.#25962