bug #30013 [Routing] dont redirect routes with greedy trailing vars w…

…ith no explicit slash (nicolas-grekas) This PR was merged into the 4.1 branch. Discussion ---------- [Routing] dont redirect routes with greedy trailing vars with no explicit slash | Q | A | ------------- | --- | Branch? | 4.1 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #29673 #29734 #29575 | License | MIT | Doc PR | - From the linked issue: > The current logic is the following: > - when a route is declared with a trailing slash, the trimmed-slash url is redirected to the one with the slash > - when a route is declared with *no* trailing slash, the slashed url is redirected to the trimmed-slash one > > That includes routes with slash-greedy requirements: when the same greedy requirement matches both the slashed and the trimmed-slash URLs, only one of them is considered the canonical one and a redirection happens. > > We could fine tune this logic and make an exception when a trailing slash-greedy requirement is declared with no explicit trailing slash after it. (ie disable any redirections for `/foo/{.*}` but keep it for `/foo/{.*}/`. That would mean `/foo/bar` and `/foo/bar/` wouldn't trigger the redirection for route `/foo/{.*}`, breaking the "not-semantics" property of trailing slashes for catch-all routes. Which might be legit afterall. This PR implements this fine tuning, as that's the most BC behavior (and thus the correct one). See #30012 for `testGreedyTrailingRequirement` in action on 3.4 as a proof. Commits ------- 2bb8890 [Routing] dont redirect routes with greedy trailing vars with no explicit slash
symfony · Jan 29, 2019 · 05071a4 · 05071a4
2 parents 78c23c7 + 2bb8890
commit 05071a4
Show file tree

Hide file tree

Showing 13 changed files with 80 additions and 213 deletions.
diff --git a/src/Symfony/Component/Routing/Matcher/Dumper/PhpMatcherDumper.php b/src/Symfony/Component/Routing/Matcher/Dumper/PhpMatcherDumper.php
@@ -558,34 +558,27 @@ private function compileSwitchDefault(bool $hasVars, bool $matchHost): string
             $code = '';
         }
 
-        $code .= $hasVars ? '
-                if ($trimmedPathinfo === $pathinfo || !$hasTrailingVar) {
-                    break;
-                }' : '
-                break;';
-
         $code = sprintf(<<<'EOF'
-            if ('/' !== $pathinfo && $hasTrailingSlash === ($trimmedPathinfo === $pathinfo)) {%s
+            if ('/' !== $pathinfo && %s$hasTrailingSlash === ($trimmedPathinfo === $pathinfo)) {%s
+                break;
             }
 
 EOF
             ,
+            $hasVars ? '!$hasTrailingVar && ' : '',
             $code
         );
 
         if ($hasVars) {
             $code = <<<'EOF'
 
-            if ($trimmedPathinfo === $pathinfo || !$hasTrailingVar) {
-                // no-op
-            } elseif (preg_match($regex, rtrim($matchedPathinfo, '/') ?: '/', $n) && $m === (int) $n['MARK']) {
-                $matches = $n;
-            } else {
-                $hasTrailingSlash = true;
-            }
+            $hasTrailingVar = $trimmedPathinfo !== $pathinfo && $hasTrailingVar;
 
 EOF
                 .$code.<<<'EOF'
+            if ($hasTrailingSlash && $hasTrailingVar && preg_match($regex, rtrim($matchedPathinfo, '/') ?: '/', $n) && $m === (int) $n['MARK']) {
+                $matches = $n;
+            }
 
             foreach ($vars as $i => $v) {
                 if (isset($matches[1 + $i])) {
@@ -665,31 +658,10 @@ private function compileRoute(Route $route, string $name, bool $checkHost, bool
         if ('/' !== $pathinfo && preg_match($regex, rtrim($matchedPathinfo, '/') ?: '/', $n) && $m === (int) $n['MARK']) {
             $matches = $n;
         }
-EOF;
-        } elseif ($this->supportsRedirections && (!$methods || isset($methods['GET']))) {
-            $code .= <<<'EOF'
-        $hasTrailingSlash = false;
-        if ($trimmedPathinfo === $pathinfo) {
-            // no-op
-        } elseif (preg_match($regex, rtrim($matchedPathinfo, '/') ?: '/', $n) && $m === (int) $n['MARK']) {
-            $matches = $n;
-        } else {
-            $hasTrailingSlash = true;
-        }
-
-        if ('/' !== $pathinfo && $hasTrailingSlash === ($trimmedPathinfo === $pathinfo)) {%s
-            if ($trimmedPathinfo === $pathinfo) {
-                goto %s;
-            }
-        }
 EOF;
         } else {
             $code .= <<<'EOF'
-        if ($trimmedPathinfo === $pathinfo) {
-            // no-op
-        } elseif (preg_match($regex, rtrim($matchedPathinfo, '/') ?: '/', $n) && $m === (int) $n['MARK']) {
-            $matches = $n;
-        } elseif ('/' !== $pathinfo) {
+        if ($trimmedPathinfo !== $pathinfo) {
             goto %2$s;
         }
 EOF;

diff --git a/src/Symfony/Component/Routing/Matcher/UrlMatcher.php b/src/Symfony/Component/Routing/Matcher/UrlMatcher.php
@@ -156,21 +156,18 @@ protected function matchCollection($pathinfo, RouteCollection $routes)
                 continue;
             }
 
-            if ($trimmedPathinfo === $pathinfo || !$hasTrailingVar = preg_match('#\{\w+\}/?$#', $route->getPath())) {
-                // no-op
-            } elseif (preg_match($regex, $trimmedPathinfo, $m)) {
-                $matches = $m;
-            } else {
-                $hasTrailingSlash = true;
-            }
+            $hasTrailingVar = $trimmedPathinfo !== $pathinfo && preg_match('#\{\w+\}/?$#', $route->getPath());
 
-            if ('/' !== $pathinfo && $hasTrailingSlash === ($trimmedPathinfo === $pathinfo)) {
+            if ('/' !== $pathinfo && !$hasTrailingVar && $hasTrailingSlash === ($trimmedPathinfo === $pathinfo)) {
                 if ($supportsTrailingSlash && (!$requiredMethods || \in_array('GET', $requiredMethods))) {
                     return $this->allow = $this->allowSchemes = [];
                 }
-                if ($trimmedPathinfo === $pathinfo || !$hasTrailingVar) {
-                    continue;
-                }
+
+                continue;
+            }
+
+            if ($hasTrailingSlash && $hasTrailingVar && preg_match($regex, $trimmedPathinfo, $m)) {
+                $matches = $m;
             }
 
             $hostMatches = [];

diff --git a/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher1.php b/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher1.php
@@ -187,11 +187,7 @@ public function match($pathinfo)
                         break;
                     case 160:
                         // foo1
-                        if ($trimmedPathinfo === $pathinfo) {
-                            // no-op
-                        } elseif (preg_match($regex, rtrim($matchedPathinfo, '/') ?: '/', $n) && $m === (int) $n['MARK']) {
-                            $matches = $n;
-                        } elseif ('/' !== $pathinfo) {
+                        if ($trimmedPathinfo !== $pathinfo) {
                             goto not_foo1;
                         }
 
@@ -209,11 +205,7 @@ public function match($pathinfo)
                         break;
                     case 204:
                         // foo2
-                        if ($trimmedPathinfo === $pathinfo) {
-                            // no-op
-                        } elseif (preg_match($regex, rtrim($matchedPathinfo, '/') ?: '/', $n) && $m === (int) $n['MARK']) {
-                            $matches = $n;
-                        } elseif ('/' !== $pathinfo) {
+                        if ($trimmedPathinfo !== $pathinfo) {
                             goto not_foo2;
                         }
 
@@ -225,11 +217,7 @@ public function match($pathinfo)
                         break;
                     case 279:
                         // foo3
-                        if ($trimmedPathinfo === $pathinfo) {
-                            // no-op
-                        } elseif (preg_match($regex, rtrim($matchedPathinfo, '/') ?: '/', $n) && $m === (int) $n['MARK']) {
-                            $matches = $n;
-                        } elseif ('/' !== $pathinfo) {
+                        if ($trimmedPathinfo !== $pathinfo) {
                             goto not_foo3;
                         }
 
@@ -262,17 +250,12 @@ public function match($pathinfo)
 
                         list($ret, $vars, $requiredMethods, $requiredSchemes, $hasTrailingSlash, $hasTrailingVar) = $routes[$m];
 
-                        if ($trimmedPathinfo === $pathinfo || !$hasTrailingVar) {
-                            // no-op
-                        } elseif (preg_match($regex, rtrim($matchedPathinfo, '/') ?: '/', $n) && $m === (int) $n['MARK']) {
-                            $matches = $n;
-                        } else {
-                            $hasTrailingSlash = true;
+                        $hasTrailingVar = $trimmedPathinfo !== $pathinfo && $hasTrailingVar;
+                        if ('/' !== $pathinfo && !$hasTrailingVar && $hasTrailingSlash === ($trimmedPathinfo === $pathinfo)) {
+                            break;
                         }
-                        if ('/' !== $pathinfo && $hasTrailingSlash === ($trimmedPathinfo === $pathinfo)) {
-                            if ($trimmedPathinfo === $pathinfo || !$hasTrailingVar) {
-                                break;
-                            }
+                        if ($hasTrailingSlash && $hasTrailingVar && preg_match($regex, rtrim($matchedPathinfo, '/') ?: '/', $n) && $m === (int) $n['MARK']) {
+                            $matches = $n;
                         }
 
                         foreach ($vars as $i => $v) {

diff --git a/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher10.php b/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher10.php
@@ -2794,17 +2794,12 @@ public function match($pathinfo)
 
                         list($ret, $vars, $requiredMethods, $requiredSchemes, $hasTrailingSlash, $hasTrailingVar) = $routes[$m];
 
-                        if ($trimmedPathinfo === $pathinfo || !$hasTrailingVar) {
-                            // no-op
-                        } elseif (preg_match($regex, rtrim($matchedPathinfo, '/') ?: '/', $n) && $m === (int) $n['MARK']) {
-                            $matches = $n;
-                        } else {
-                            $hasTrailingSlash = true;
+                        $hasTrailingVar = $trimmedPathinfo !== $pathinfo && $hasTrailingVar;
+                        if ('/' !== $pathinfo && !$hasTrailingVar && $hasTrailingSlash === ($trimmedPathinfo === $pathinfo)) {
+                            break;
                         }
-                        if ('/' !== $pathinfo && $hasTrailingSlash === ($trimmedPathinfo === $pathinfo)) {
-                            if ($trimmedPathinfo === $pathinfo || !$hasTrailingVar) {
-                                break;
-                            }
+                        if ($hasTrailingSlash && $hasTrailingVar && preg_match($regex, rtrim($matchedPathinfo, '/') ?: '/', $n) && $m === (int) $n['MARK']) {
+                            $matches = $n;
                         }
 
                         foreach ($vars as $i => $v) {

diff --git a/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher11.php b/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher11.php
@@ -119,20 +119,15 @@ private function doMatch(string $pathinfo, array &$allow = [], array &$allowSche
 
                         list($ret, $vars, $requiredMethods, $requiredSchemes, $hasTrailingSlash, $hasTrailingVar) = $routes[$m];
 
-                        if ($trimmedPathinfo === $pathinfo || !$hasTrailingVar) {
-                            // no-op
-                        } elseif (preg_match($regex, rtrim($matchedPathinfo, '/') ?: '/', $n) && $m === (int) $n['MARK']) {
-                            $matches = $n;
-                        } else {
-                            $hasTrailingSlash = true;
-                        }
-                        if ('/' !== $pathinfo && $hasTrailingSlash === ($trimmedPathinfo === $pathinfo)) {
+                        $hasTrailingVar = $trimmedPathinfo !== $pathinfo && $hasTrailingVar;
+                        if ('/' !== $pathinfo && !$hasTrailingVar && $hasTrailingSlash === ($trimmedPathinfo === $pathinfo)) {
                             if ('GET' === $canonicalMethod && (!$requiredMethods || isset($requiredMethods['GET']))) {
                                 return $allow = $allowSchemes = [];
                             }
-                            if ($trimmedPathinfo === $pathinfo || !$hasTrailingVar) {
-                                break;
-                            }
+                            break;
+                        }
+                        if ($hasTrailingSlash && $hasTrailingVar && preg_match($regex, rtrim($matchedPathinfo, '/') ?: '/', $n) && $m === (int) $n['MARK']) {
+                            $matches = $n;
                         }
 
                         foreach ($vars as $i => $v) {

diff --git a/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher12.php b/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher12.php
@@ -64,17 +64,12 @@ public function match($pathinfo)
 
                         list($ret, $vars, $requiredMethods, $requiredSchemes, $hasTrailingSlash, $hasTrailingVar) = $routes[$m];
 
-                        if ($trimmedPathinfo === $pathinfo || !$hasTrailingVar) {
-                            // no-op
-                        } elseif (preg_match($regex, rtrim($matchedPathinfo, '/') ?: '/', $n) && $m === (int) $n['MARK']) {
-                            $matches = $n;
-                        } else {
-                            $hasTrailingSlash = true;
+                        $hasTrailingVar = $trimmedPathinfo !== $pathinfo && $hasTrailingVar;
+                        if ('/' !== $pathinfo && !$hasTrailingVar && $hasTrailingSlash === ($trimmedPathinfo === $pathinfo)) {
+                            break;
                         }
-                        if ('/' !== $pathinfo && $hasTrailingSlash === ($trimmedPathinfo === $pathinfo)) {
-                            if ($trimmedPathinfo === $pathinfo || !$hasTrailingVar) {
-                                break;
-                            }
+                        if ($hasTrailingSlash && $hasTrailingVar && preg_match($regex, rtrim($matchedPathinfo, '/') ?: '/', $n) && $m === (int) $n['MARK']) {
+                            $matches = $n;
                         }
 
                         foreach ($vars as $i => $v) {

diff --git a/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher13.php b/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher13.php
@@ -44,11 +44,7 @@ public function match($pathinfo)
                 switch ($m = (int) $matches['MARK']) {
                     case 56:
                         // r1
-                        if ($trimmedPathinfo === $pathinfo) {
-                            // no-op
-                        } elseif (preg_match($regex, rtrim($matchedPathinfo, '/') ?: '/', $n) && $m === (int) $n['MARK']) {
-                            $matches = $n;
-                        } elseif ('/' !== $pathinfo) {
+                        if ($trimmedPathinfo !== $pathinfo) {
                             goto not_r1;
                         }
 
@@ -58,11 +54,7 @@ public function match($pathinfo)
                         not_r1:
 
                         // r2
-                        if ($trimmedPathinfo === $pathinfo) {
-                            // no-op
-                        } elseif (preg_match($regex, rtrim($matchedPathinfo, '/') ?: '/', $n) && $m === (int) $n['MARK']) {
-                            $matches = $n;
-                        } elseif ('/' !== $pathinfo) {
+                        if ($trimmedPathinfo !== $pathinfo) {
                             goto not_r2;
                         }
 

diff --git a/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher2.php b/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher2.php
@@ -230,11 +230,7 @@ private function doMatch(string $pathinfo, array &$allow = [], array &$allowSche
                         break;
                     case 160:
                         // foo1
-                        if ($trimmedPathinfo === $pathinfo) {
-                            // no-op
-                        } elseif (preg_match($regex, rtrim($matchedPathinfo, '/') ?: '/', $n) && $m === (int) $n['MARK']) {
-                            $matches = $n;
-                        } elseif ('/' !== $pathinfo) {
+                        if ($trimmedPathinfo !== $pathinfo) {
                             goto not_foo1;
                         }
 
@@ -252,22 +248,8 @@ private function doMatch(string $pathinfo, array &$allow = [], array &$allowSche
                         break;
                     case 204:
                         // foo2
-                        $hasTrailingSlash = false;
-                        if ($trimmedPathinfo === $pathinfo) {
-                            // no-op
-                        } elseif (preg_match($regex, rtrim($matchedPathinfo, '/') ?: '/', $n) && $m === (int) $n['MARK']) {
-                            $matches = $n;
-                        } else {
-                            $hasTrailingSlash = true;
-                        }
-
-                        if ('/' !== $pathinfo && $hasTrailingSlash === ($trimmedPathinfo === $pathinfo)) {
-                            if ('GET' === $canonicalMethod) {
-                                return $allow = $allowSchemes = [];
-                            }
-                            if ($trimmedPathinfo === $pathinfo) {
-                                goto not_foo2;
-                            }
+                        if ($trimmedPathinfo !== $pathinfo) {
+                            goto not_foo2;
                         }
 
                         $matches = ['foo1' => $matches[1] ?? null];
@@ -278,22 +260,8 @@ private function doMatch(string $pathinfo, array &$allow = [], array &$allowSche
                         break;
                     case 279:
                         // foo3
-                        $hasTrailingSlash = false;
-                        if ($trimmedPathinfo === $pathinfo) {
-                            // no-op
-                        } elseif (preg_match($regex, rtrim($matchedPathinfo, '/') ?: '/', $n) && $m === (int) $n['MARK']) {
-                            $matches = $n;
-                        } else {
-                            $hasTrailingSlash = true;
-                        }
-
-                        if ('/' !== $pathinfo && $hasTrailingSlash === ($trimmedPathinfo === $pathinfo)) {
-                            if ('GET' === $canonicalMethod) {
-                                return $allow = $allowSchemes = [];
-                            }
-                            if ($trimmedPathinfo === $pathinfo) {
-                                goto not_foo3;
-                            }
+                        if ($trimmedPathinfo !== $pathinfo) {
+                            goto not_foo3;
                         }
 
                         $matches = ['_locale' => $matches[1] ?? null, 'foo' => $matches[2] ?? null];
@@ -325,20 +293,15 @@ private function doMatch(string $pathinfo, array &$allow = [], array &$allowSche
 
                         list($ret, $vars, $requiredMethods, $requiredSchemes, $hasTrailingSlash, $hasTrailingVar) = $routes[$m];
 
-                        if ($trimmedPathinfo === $pathinfo || !$hasTrailingVar) {
-                            // no-op
-                        } elseif (preg_match($regex, rtrim($matchedPathinfo, '/') ?: '/', $n) && $m === (int) $n['MARK']) {
-                            $matches = $n;
-                        } else {
-                            $hasTrailingSlash = true;
-                        }
-                        if ('/' !== $pathinfo && $hasTrailingSlash === ($trimmedPathinfo === $pathinfo)) {
+                        $hasTrailingVar = $trimmedPathinfo !== $pathinfo && $hasTrailingVar;
+                        if ('/' !== $pathinfo && !$hasTrailingVar && $hasTrailingSlash === ($trimmedPathinfo === $pathinfo)) {
                             if ('GET' === $canonicalMethod && (!$requiredMethods || isset($requiredMethods['GET']))) {
                                 return $allow = $allowSchemes = [];
                             }
-                            if ($trimmedPathinfo === $pathinfo || !$hasTrailingVar) {
-                                break;
-                            }
+                            break;
+                        }
+                        if ($hasTrailingSlash && $hasTrailingVar && preg_match($regex, rtrim($matchedPathinfo, '/') ?: '/', $n) && $m === (int) $n['MARK']) {
+                            $matches = $n;
                         }
 
                         foreach ($vars as $i => $v) {

diff --git a/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher3.php b/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher3.php
@@ -84,17 +84,12 @@ public function match($pathinfo)
 
                         list($ret, $vars, $requiredMethods, $requiredSchemes, $hasTrailingSlash, $hasTrailingVar) = $routes[$m];
 
-                        if ($trimmedPathinfo === $pathinfo || !$hasTrailingVar) {
-                            // no-op
-                        } elseif (preg_match($regex, rtrim($matchedPathinfo, '/') ?: '/', $n) && $m === (int) $n['MARK']) {
-                            $matches = $n;
-                        } else {
-                            $hasTrailingSlash = true;
+                        $hasTrailingVar = $trimmedPathinfo !== $pathinfo && $hasTrailingVar;
+                        if ('/' !== $pathinfo && !$hasTrailingVar && $hasTrailingSlash === ($trimmedPathinfo === $pathinfo)) {
+                            break;
                         }
-                        if ('/' !== $pathinfo && $hasTrailingSlash === ($trimmedPathinfo === $pathinfo)) {
-                            if ($trimmedPathinfo === $pathinfo || !$hasTrailingVar) {
-                                break;
-                            }
+                        if ($hasTrailingSlash && $hasTrailingVar && preg_match($regex, rtrim($matchedPathinfo, '/') ?: '/', $n) && $m === (int) $n['MARK']) {
+                            $matches = $n;
                         }
 
                         foreach ($vars as $i => $v) {