[Routing] support scheme requirement without redirectable dumped matcher #26304

Tobion · 2018-02-25T02:15:47Z

Q	A
Branch?	master
Bug fix?	yes
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
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. #25962

nicolas-grekas

the $supportsRedirections property should be turned back to a local variable

nicolas-grekas · 2018-02-25T08:25:35Z

src/Symfony/Component/Routing/Matcher/Dumper/PhpMatcherDumper.php


-                return \$this->redirect(\$rawPathinfo, \$ret['_route'], key(\$requiredSchemes)) + \$ret;
+            if (\$requiredMethods && !isset(\$requiredMethods[\$canonicalMethod]) && !isset(\$requiredMethods[\$requestMethod])) {
+                \$allow += \$requiredMethods;


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?

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.

Here is why I was talking about that: report in #22739, fix just sent in #26312.

Simperfit · 2018-02-25T07:11:49Z

src/Symfony/Component/Routing/Matcher/Dumper/PhpMatcherDumper.php

-            $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)) {


\in_array can be used

this file is dumped in the root namespace

Tobion · 2018-02-25T13:28:25Z

the $supportsRedirections property should be turned back to a local variable

We would still need to pass the var to generateMatchMethod. As a property it's more flexible IMO.

nicolas-grekas · 2018-02-26T17:43:46Z

ready for rebase

Tobion · 2018-02-26T22:22:41Z

Rebased

nicolas-grekas

with minor comment

nicolas-grekas · 2018-02-27T08:32:38Z

src/Symfony/Component/Routing/Matcher/Dumper/PhpMatcherDumper.php

-
-            if (\$requiredMethods && !isset(\$requiredMethods[\$canonicalMethod]) && !isset(\$requiredMethods[\$requestMethod])) {
-                \$allow += \$requiredMethods;
+            if (\$requiredSchemes && !\$hasRequiredScheme) {


if (!\$hasRequiredScheme) { would be enough here

nicolas-grekas · 2018-02-27T08:35:31Z

src/Symfony/Bundle/FrameworkBundle/Tests/Routing/RedirectableUrlMatcherTest.php

@@ -53,7 +53,7 @@ public function testSchemeRedirect()
                'scheme' => 'https',
                'httpPort' => $context->getHttpPort(),
                'httpsPort' => $context->getHttpsPort(),
-                '_route' => 'foo',
+                '_route' => null,


this change breaks the 3.4 test suite, can't we do without?

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.

we still need to make the currently failing test green before merging...

nicolas-grekas · 2018-02-28T09:21:11Z

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

Tobion · 2018-02-28T10:25:24Z

src/Symfony/Component/Routing/Matcher/UrlMatcher.php

                        $this->allow = array_merge($this->allow, $requiredMethods);
                    }

                    continue;
                }
            }

-            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()));


what's the reason for doing that before the $hasRequiredScheme check?

reverted, was a leftover from previous iteration

Tobion · 2018-02-28T10:59:51Z

Please add a test for scheme + trailing slash redirect. Then good to merge.

nicolas-grekas · 2018-02-28T11:39:10Z

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  | Tests pass? | yes  | 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. #25962 Commits ------- f9b54c5 [Routing] support scheme requirement without redirectable dumped matcher

carsonbot added Status: Needs Review Routing Feature labels Feb 25, 2018

Tobion force-pushed the simplify-scheme-redirect branch from 5b3e320 to 2bcc2a9 Compare February 25, 2018 02:43

nicolas-grekas reviewed Feb 25, 2018

View reviewed changes

Simperfit reviewed Feb 25, 2018

View reviewed changes

nicolas-grekas added this to the 4.1 milestone Feb 26, 2018

Tobion force-pushed the simplify-scheme-redirect branch 2 times, most recently from 91ecd89 to cbbc3e2 Compare February 26, 2018 22:22

nicolas-grekas approved these changes Feb 27, 2018

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Feb 27, 2018

nicolas-grekas reviewed Feb 27, 2018

View reviewed changes

Tobion force-pushed the simplify-scheme-redirect branch 2 times, most recently from 735bc1d to 7154ec1 Compare February 27, 2018 14:37

nicolas-grekas force-pushed the simplify-scheme-redirect branch from 7154ec1 to 8c36250 Compare February 28, 2018 07:56

nicolas-grekas approved these changes Feb 28, 2018

View reviewed changes

nicolas-grekas force-pushed the simplify-scheme-redirect branch 3 times, most recently from beec4f7 to ddb5be6 Compare February 28, 2018 09:18

nicolas-grekas force-pushed the simplify-scheme-redirect branch 6 times, most recently from 8b3bc06 to afc6254 Compare February 28, 2018 09:51

nicolas-grekas mentioned this pull request Feb 28, 2018

[Routing] allow no-slash root on imported routes #26284

Merged

Tobion commented Feb 28, 2018

View reviewed changes

nicolas-grekas force-pushed the simplify-scheme-redirect branch from afc6254 to 2e27658 Compare February 28, 2018 10:50

[Routing] support scheme requirement without redirectable dumped matcher

f9b54c5

nicolas-grekas force-pushed the simplify-scheme-redirect branch from 2e27658 to f9b54c5 Compare February 28, 2018 11:35

nicolas-grekas merged commit f9b54c5 into symfony:master Feb 28, 2018

Tobion deleted the simplify-scheme-redirect branch February 28, 2018 12:06

javiereguiluz mentioned this pull request Mar 2, 2018

[Routing] support scheme requirement without redirectable dumped matcher symfony/symfony-docs#9376

Closed

fabpot mentioned this pull request May 7, 2018

Release v4.1.0-BETA1 #27181

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Routing] support scheme requirement without redirectable dumped matcher #26304

[Routing] support scheme requirement without redirectable dumped matcher #26304

Tobion commented Feb 25, 2018 •

edited by javiereguiluz

Loading

nicolas-grekas left a comment

nicolas-grekas Feb 25, 2018

Tobion Feb 25, 2018

nicolas-grekas Feb 25, 2018

Simperfit Feb 25, 2018

nicolas-grekas Feb 25, 2018

Tobion commented Feb 25, 2018

nicolas-grekas commented Feb 26, 2018

Tobion commented Feb 26, 2018

nicolas-grekas left a comment

nicolas-grekas Feb 27, 2018

Tobion Feb 27, 2018

nicolas-grekas Feb 27, 2018 •

edited

Loading

Tobion Feb 27, 2018

nicolas-grekas Feb 27, 2018 •

edited

Loading

nicolas-grekas Feb 28, 2018

nicolas-grekas commented Feb 28, 2018 •

edited

Loading

Tobion Feb 28, 2018

nicolas-grekas Feb 28, 2018

Tobion commented Feb 28, 2018

nicolas-grekas commented Feb 28, 2018

[Routing] support scheme requirement without redirectable dumped matcher #26304

[Routing] support scheme requirement without redirectable dumped matcher #26304

Conversation

Tobion commented Feb 25, 2018 • edited by javiereguiluz Loading

nicolas-grekas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tobion commented Feb 25, 2018

nicolas-grekas commented Feb 26, 2018

Tobion commented Feb 26, 2018

nicolas-grekas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas Feb 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas Feb 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Feb 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tobion commented Feb 28, 2018

nicolas-grekas commented Feb 28, 2018

Tobion commented Feb 25, 2018 •

edited by javiereguiluz

Loading

nicolas-grekas Feb 27, 2018 •

edited

Loading

nicolas-grekas Feb 27, 2018 •

edited

Loading

nicolas-grekas commented Feb 28, 2018 •

edited

Loading