bug #25962 [Routing] Fix trailing slash redirection for non-safe verb…

…s (nicolas-grekas) This PR was merged into the 2.7 branch. Discussion ---------- [Routing] Fix trailing slash redirection for non-safe verbs | 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 | - This test dumped matchers using the existing test cases for (Redirectable)UrlMatcher so that we are sure they behave the same. Fixes the differences found while doing so. Commits ------- ad593ae [Routing] Fix trailing slash redirection for non-safe verbs
symfony · Feb 1, 2018 · ceb4e73 · ceb4e73
2 parents 471c410 + ad593ae
commit ceb4e73
Show file tree

Hide file tree

Showing 8 changed files with 203 additions and 61 deletions.
diff --git a/src/Symfony/Component/Routing/Matcher/Dumper/PhpMatcherDumper.php b/src/Symfony/Component/Routing/Matcher/Dumper/PhpMatcherDumper.php
@@ -101,7 +101,7 @@ public function match(\$rawPathinfo)
         \$allow = array();
         \$pathinfo = rawurldecode(\$rawPathinfo);
         \$context = \$this->context;
-        \$request = \$this->request;
+        \$request = \$this->request ?: \$this->createRequest(\$pathinfo);
 
 $code
 
@@ -283,7 +283,11 @@ private function compileRoute(Route $route, $name, $supportsRedirections, $paren
 
         if ($hasTrailingSlash) {
             $code .= <<<EOF
-            if (substr(\$pathinfo, -1) !== '/') {
+            if ('/' === substr(\$pathinfo, -1)) {
+                // no-op
+            } elseif (!in_array(\$this->context->getMethod(), array('HEAD', 'GET'))) {
+                goto $gotoname;
+            } else {
                 return \$this->redirect(\$rawPathinfo.'/', '$name');
             }
 
@@ -329,7 +333,7 @@ private function compileRoute(Route $route, $name, $supportsRedirections, $paren
         }
         $code .= "        }\n";
 
-        if ($methods) {
+        if ($methods || $hasTrailingSlash) {
             $code .= "        $gotoname:\n";
         }
 

diff --git a/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher1.php b/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher1.php
@@ -20,7 +20,7 @@ public function match($rawPathinfo)
         $allow = array();
         $pathinfo = rawurldecode($rawPathinfo);
         $context = $this->context;
-        $request = $this->request;
+        $request = $this->request ?: $this->createRequest($pathinfo);
 
         // foo
         if (0 === strpos($pathinfo, '/foo') && preg_match('#^/foo/(?P<bar>baz|symfony)$#s', $pathinfo, $matches)) {

diff --git a/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher2.php b/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher2.php
@@ -20,7 +20,7 @@ public function match($rawPathinfo)
         $allow = array();
         $pathinfo = rawurldecode($rawPathinfo);
         $context = $this->context;
-        $request = $this->request;
+        $request = $this->request ?: $this->createRequest($pathinfo);
 
         // foo
         if (0 === strpos($pathinfo, '/foo') && preg_match('#^/foo/(?P<bar>baz|symfony)$#s', $pathinfo, $matches)) {
@@ -66,23 +66,33 @@ public function match($rawPathinfo)
 
                 // baz3
                 if ('/test/baz3' === rtrim($pathinfo, '/')) {
-                    if (substr($pathinfo, -1) !== '/') {
+                    if ('/' === substr($pathinfo, -1)) {
+                        // no-op
+                    } elseif (!in_array($this->context->getMethod(), array('HEAD', 'GET'))) {
+                        goto not_baz3;
+                    } else {
                         return $this->redirect($rawPathinfo.'/', 'baz3');
                     }
 
                     return array('_route' => 'baz3');
                 }
+                not_baz3:
 
             }
 
             // baz4
             if (preg_match('#^/test/(?P<foo>[^/]++)/?$#s', $pathinfo, $matches)) {
-                if (substr($pathinfo, -1) !== '/') {
+                if ('/' === substr($pathinfo, -1)) {
+                    // no-op
+                } elseif (!in_array($this->context->getMethod(), array('HEAD', 'GET'))) {
+                    goto not_baz4;
+                } else {
                     return $this->redirect($rawPathinfo.'/', 'baz4');
                 }
 
                 return $this->mergeDefaults(array_replace($matches, array('_route' => 'baz4')), array ());
             }
+            not_baz4:
 
             // baz5
             if (preg_match('#^/test/(?P<foo>[^/]++)/$#s', $pathinfo, $matches)) {
@@ -170,12 +180,17 @@ public function match($rawPathinfo)
 
             // hey
             if ('/multi/hey' === rtrim($pathinfo, '/')) {
-                if (substr($pathinfo, -1) !== '/') {
+                if ('/' === substr($pathinfo, -1)) {
+                    // no-op
+                } elseif (!in_array($this->context->getMethod(), array('HEAD', 'GET'))) {
+                    goto not_hey;
+                } else {
                     return $this->redirect($rawPathinfo.'/', 'hey');
                 }
 
                 return array('_route' => 'hey');
             }
+            not_hey:
 
         }
 

diff --git a/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher3.php b/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher3.php
@@ -20,7 +20,7 @@ public function match($rawPathinfo)
         $allow = array();
         $pathinfo = rawurldecode($rawPathinfo);
         $context = $this->context;
-        $request = $this->request;
+        $request = $this->request ?: $this->createRequest($pathinfo);
 
         if (0 === strpos($pathinfo, '/rootprefix')) {
             // static

diff --git a/src/Symfony/Component/Routing/Tests/Matcher/DumpedRedirectableUrlMatcherTest.php b/src/Symfony/Component/Routing/Tests/Matcher/DumpedRedirectableUrlMatcherTest.php
@@ -0,0 +1,43 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\Routing\Tests\Matcher;
+
+use Symfony\Component\Routing\Matcher\Dumper\PhpMatcherDumper;
+use Symfony\Component\Routing\Matcher\RedirectableUrlMatcherInterface;
+use Symfony\Component\Routing\Matcher\UrlMatcher;
+use Symfony\Component\Routing\RouteCollection;
+use Symfony\Component\Routing\RequestContext;
+
+class DumpedRedirectableUrlMatcherTest extends RedirectableUrlMatcherTest
+{
+    protected function getUrlMatcher(RouteCollection $routes, RequestContext $context = null)
+    {
+        static $i = 0;
+
+        $class = 'DumpedRedirectableUrlMatcher'.++$i;
+        $dumper = new PhpMatcherDumper($routes);
+        $dumpedRoutes = eval('?>'.$dumper->dump(array('class' => $class, 'base_class' => 'Symfony\Component\Routing\Tests\Matcher\TestDumpedRedirectableUrlMatcher')));
+
+        return $this->getMockBuilder($class)
+            ->setConstructorArgs(array($context ?: new RequestContext()))
+            ->setMethods(array('redirect'))
+            ->getMock();
+    }
+}
+
+class TestDumpedRedirectableUrlMatcher extends UrlMatcher implements RedirectableUrlMatcherInterface
+{
+    public function redirect($path, $route, $scheme = null)
+    {
+        return array();
+    }
+}
diff --git a/src/Symfony/Component/Routing/Tests/Matcher/DumpedUrlMatcherTest.php b/src/Symfony/Component/Routing/Tests/Matcher/DumpedUrlMatcherTest.php
@@ -0,0 +1,39 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\Routing\Tests\Matcher;
+
+use Symfony\Component\Routing\Matcher\Dumper\PhpMatcherDumper;
+use Symfony\Component\Routing\RouteCollection;
+use Symfony\Component\Routing\RequestContext;
+
+class DumpedUrlMatcherTest extends UrlMatcherTest
+{
+    /**
+     * @expectedException \LogicException
+     * @expectedExceptionMessage The "schemes" requirement is only supported for URL matchers that implement RedirectableUrlMatcherInterface.
+     */
+    public function testSchemeRequirement()
+    {
+        parent::testSchemeRequirement();
+    }
+
+    protected function getUrlMatcher(RouteCollection $routes, RequestContext $context = null)
+    {
+        static $i = 0;
+
+        $class = 'DumpedUrlMatcher'.++$i;
+        $dumper = new PhpMatcherDumper($routes);
+        $dumpedRoutes = eval('?>'.$dumper->dump(array('class' => $class)));
+
+        return new $class($context ?: new RequestContext());
+    }
+}
diff --git a/src/Symfony/Component/Routing/Tests/Matcher/RedirectableUrlMatcherTest.php b/src/Symfony/Component/Routing/Tests/Matcher/RedirectableUrlMatcherTest.php
@@ -11,20 +11,19 @@
 
 namespace Symfony\Component\Routing\Tests\Matcher;
 
-use PHPUnit\Framework\TestCase;
 use Symfony\Component\Routing\Route;
 use Symfony\Component\Routing\RouteCollection;
 use Symfony\Component\Routing\RequestContext;
 
-class RedirectableUrlMatcherTest extends TestCase
+class RedirectableUrlMatcherTest extends UrlMatcherTest
 {
     public function testRedirectWhenNoSlash()
     {
         $coll = new RouteCollection();
         $coll->add('foo', new Route('/foo/'));
 
-        $matcher = $this->getMockForAbstractClass('Symfony\Component\Routing\Matcher\RedirectableUrlMatcher', array($coll, new RequestContext()));
-        $matcher->expects($this->once())->method('redirect');
+        $matcher = $this->getUrlMatcher($coll);
+        $matcher->expects($this->once())->method('redirect')->will($this->returnValue(array()));
         $matcher->match('/foo');
     }
 
@@ -38,7 +37,7 @@ public function testRedirectWhenNoSlashForNonSafeMethod()
 
         $context = new RequestContext();
         $context->setMethod('POST');
-        $matcher = $this->getMockForAbstractClass('Symfony\Component\Routing\Matcher\RedirectableUrlMatcher', array($coll, $context));
+        $matcher = $this->getUrlMatcher($coll, $context);
         $matcher->match('/foo');
     }
 
@@ -47,7 +46,7 @@ public function testSchemeRedirectRedirectsToFirstScheme()
         $coll = new RouteCollection();
         $coll->add('foo', new Route('/foo', array(), array(), array(), '', array('FTP', 'HTTPS')));
 
-        $matcher = $this->getMockForAbstractClass('Symfony\Component\Routing\Matcher\RedirectableUrlMatcher', array($coll, new RequestContext()));
+        $matcher = $this->getUrlMatcher($coll);
         $matcher
             ->expects($this->once())
             ->method('redirect')
@@ -62,11 +61,10 @@ public function testNoSchemaRedirectIfOnOfMultipleSchemesMatches()
         $coll = new RouteCollection();
         $coll->add('foo', new Route('/foo', array(), array(), array(), '', array('https', 'http')));
 
-        $matcher = $this->getMockForAbstractClass('Symfony\Component\Routing\Matcher\RedirectableUrlMatcher', array($coll, new RequestContext()));
+        $matcher = $this->getUrlMatcher($coll);
         $matcher
             ->expects($this->never())
-            ->method('redirect')
-        ;
+            ->method('redirect');
         $matcher->match('/foo');
     }
 
@@ -75,8 +73,22 @@ public function testRedirectPreservesUrlEncoding()
         $coll = new RouteCollection();
         $coll->add('foo', new Route('/foo:bar/'));
 
-        $matcher = $this->getMockForAbstractClass('Symfony\Component\Routing\Matcher\RedirectableUrlMatcher', array($coll, new RequestContext()));
-        $matcher->expects($this->once())->method('redirect')->with('/foo%3Abar/');
+        $matcher = $this->getUrlMatcher($coll);
+        $matcher->expects($this->once())->method('redirect')->with('/foo%3Abar/')->willReturn(array());
         $matcher->match('/foo%3Abar');
     }
+
+    public function testSchemeRequirement()
+    {
+        $coll = new RouteCollection();
+        $coll->add('foo', new Route('/foo', array(), array(), array(), '', array('https')));
+        $matcher = $this->getUrlMatcher($coll, new RequestContext());
+        $matcher->expects($this->once())->method('redirect')->with('/foo', 'foo', 'https')->willReturn(array('_route' => 'foo'));
+        $this->assertSame(array('_route' => 'foo'), $matcher->match('/foo'));
+    }
+
+    protected function getUrlMatcher(RouteCollection $routes, RequestContext $context = null)
+    {
+        return $this->getMockForAbstractClass('Symfony\Component\Routing\Matcher\RedirectableUrlMatcher', array($routes, $context ?: new RequestContext()));
+    }
 }