bug #31107 [Routing] fix trailing slash redirection with non-greedy t…

…railing vars (nicolas-grekas) This PR was merged into the 4.2 branch. Discussion ---------- [Routing] fix trailing slash redirection with non-greedy trailing vars | Q | A | ------------- | --- | Branch? | 4.2 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #30863, #31066 | License | MIT | Doc PR | - Fixes redirecting `/123/` to `/123` when the route is defined as `/{foo<\d+>}` Commits ------- d88833d [Routing] fix trailing slash redirection with non-greedy trailing vars
symfony · Apr 17, 2019 · 2d2ff38 · 2d2ff38
2 parents 74a18bc + d88833d
commit 2d2ff38
Show file tree

Hide file tree

Showing 3 changed files with 28 additions and 8 deletions.
diff --git a/src/Symfony/Component/Routing/Matcher/Dumper/PhpMatcherTrait.php b/src/Symfony/Component/Routing/Matcher/Dumper/PhpMatcherTrait.php
@@ -131,17 +131,22 @@ private function doMatch(string $pathinfo, array &$allow = [], array &$allowSche
                     }
 
                     $hasTrailingVar = $trimmedPathinfo !== $pathinfo && $hasTrailingVar;
+
+                    if ($hasTrailingVar && ($hasTrailingSlash || '/' !== substr($matches[\count($vars)], -1)) && preg_match($regex, $this->matchHost ? $host.'.'.$trimmedPathinfo : $trimmedPathinfo, $n) && $m === (int) $n['MARK']) {
+                        if ($hasTrailingSlash) {
+                            $matches = $n;
+                        } else {
+                            $hasTrailingVar = false;
+                        }
+                    }
+
                     if ('/' !== $pathinfo && !$hasTrailingVar && $hasTrailingSlash === ($trimmedPathinfo === $pathinfo)) {
                         if ($supportsRedirections && (!$requiredMethods || isset($requiredMethods['GET']))) {
                             return $allow = $allowSchemes = [];
                         }
                         continue;
                     }
 
-                    if ($hasTrailingSlash && $hasTrailingVar && preg_match($regex, $this->matchHost ? $host.'.'.$trimmedPathinfo : $trimmedPathinfo, $n) && $m === (int) $n['MARK']) {
-                        $matches = $n;
-                    }
-
                     foreach ($vars as $i => $v) {
                         if (isset($matches[1 + $i])) {
                             $ret[$v] = $matches[1 + $i];

diff --git a/src/Symfony/Component/Routing/Matcher/UrlMatcher.php b/src/Symfony/Component/Routing/Matcher/UrlMatcher.php
@@ -158,6 +158,14 @@ protected function matchCollection($pathinfo, RouteCollection $routes)
 
             $hasTrailingVar = $trimmedPathinfo !== $pathinfo && preg_match('#\{\w+\}/?$#', $route->getPath());
 
+            if ($hasTrailingVar && ($hasTrailingSlash || '/' !== substr($matches[(\count($matches) - 1) >> 1], -1)) && preg_match($regex, $trimmedPathinfo, $m)) {
+                if ($hasTrailingSlash) {
+                    $matches = $m;
+                } else {
+                    $hasTrailingVar = false;
+                }
+            }
+
             if ('/' !== $pathinfo && !$hasTrailingVar && $hasTrailingSlash === ($trimmedPathinfo === $pathinfo)) {
                 if ($supportsTrailingSlash && (!$requiredMethods || \in_array('GET', $requiredMethods))) {
                     return $this->allow = $this->allowSchemes = [];
@@ -166,10 +174,6 @@ protected function matchCollection($pathinfo, RouteCollection $routes)
                 continue;
             }
 
-            if ($hasTrailingSlash && $hasTrailingVar && preg_match($regex, $trimmedPathinfo, $m)) {
-                $matches = $m;
-            }
-
             $hostMatches = [];
             if ($compiledRoute->getHostRegex() && !preg_match($compiledRoute->getHostRegex(), $this->context->getHost(), $hostMatches)) {
                 continue;

diff --git a/src/Symfony/Component/Routing/Tests/Matcher/RedirectableUrlMatcherTest.php b/src/Symfony/Component/Routing/Tests/Matcher/RedirectableUrlMatcherTest.php
@@ -187,6 +187,17 @@ public function testSlashAndVerbPrecedenceWithRedirection()
         $this->assertEquals($expected, $matcher->match('/api/customers/123/contactpersons'));
     }
 
+    public function testNonGreedyTrailingRequirement()
+    {
+        $coll = new RouteCollection();
+        $coll->add('a', new Route('/{a}', [], ['a' => '\d+']));
+
+        $matcher = $this->getUrlMatcher($coll);
+        $matcher->expects($this->once())->method('redirect')->with('/123')->willReturn([]);
+
+        $this->assertEquals(['_route' => 'a', 'a' => '123'], $matcher->match('/123/'));
+    }
+
     protected function getUrlMatcher(RouteCollection $routes, RequestContext $context = null)
     {
         return $this->getMockForAbstractClass('Symfony\Component\Routing\Matcher\RedirectableUrlMatcher', [$routes, $context ?: new RequestContext()]);