merged branch Tobion/path-slash (PR #5683)

This PR was merged into the 2.1 branch.

Commits
-------

1566f9f [Routing] fix handling of whitespace and synch between collection prefix and route pattern
90145d2 [Routing] fix handling of two starting slashes in the pattern

Discussion
----------

[Routing] fix handling of slashes and whitespace in pattern/prefix

BC break: no
feature addition: no

The first commit fixes the handling of two starting slashes in the pattern. It would be confused with a network path e.g. `//domain/path` when generating a path, so should be prevented.

The second commit fixes the handling of whitespace in RouteCollection::addPrefix. It wasn't trimmed there but it is trimmed in Route::setPattern. So it can be out-of-synch between RouteCollection::getPrefix <-> Route::getPattern.

src/Symfony/Component/Routing/Route.php

@@ -95,13 +95,9 @@ public function getPattern()
      */
     public function setPattern($pattern)
     {
-        $this->pattern = trim($pattern);
-
-        // a route must start with a slash
-        if ('' === $this->pattern || '/' !== $this->pattern[0]) {
-            $this->pattern = '/'.$this->pattern;
-        }
-
+        // A pattern must start with a slash and must not have multiple slashes at the beginning because the
+        // generated path for this route would be confused with a network path, e.g. '//domain.com/path'.
+        $this->pattern = '/' . ltrim(trim($pattern), '/');
         $this->compiled = null;
 
         return $this;

src/Symfony/Component/Routing/RouteCollection.php

@@ -223,25 +223,25 @@ public function addCollection(RouteCollection $collection, $prefix = '', $defaul
      */
     public function addPrefix($prefix, $defaults = array(), $requirements = array(), $options = array())
     {
-        // a prefix must not end with a slash
-        $prefix = rtrim($prefix, '/');
+        $prefix = trim(trim($prefix), '/');
 
         if ('' === $prefix && empty($defaults) && empty($requirements) && empty($options)) {
             return;
         }
 
-        // a prefix must start with a slash
-        if ('' !== $prefix && '/' !== $prefix[0]) {
-            $prefix = '/'.$prefix;
+        // a prefix must start with a single slash and must not end with a slash
+        if ('' !== $prefix) {
+            $this->prefix = '/' . $prefix . $this->prefix;
         }
 
-        $this->prefix = $prefix.$this->prefix;
-
         foreach ($this->routes as $route) {
             if ($route instanceof RouteCollection) {
-                $route->addPrefix($prefix, $defaults, $requirements, $options);
+                // we add the slashes so the prefix is not lost by trimming in the sub-collection
+                $route->addPrefix('/' . $prefix . '/', $defaults, $requirements, $options);
             } else {
-                $route->setPattern($prefix.$route->getPattern());
+                if ('' !== $prefix) {
+                    $route->setPattern('/' . $prefix . $route->getPattern());
+                }
                 $route->addDefaults($defaults);
                 $route->addRequirements($requirements);
                 $route->addOptions($options);

src/Symfony/Component/Routing/Tests/Generator/UrlGeneratorTest.php

@@ -214,6 +214,14 @@ public function testSchemeRequirementForcesAbsoluteUrl()
         $routes = $this->getRoutes('test', new Route('/', array(), array('_scheme' => 'http')));
         $this->assertEquals('http://localhost/app.php/', $this->getGenerator($routes, array('scheme' => 'https'))->generate('test'));
     }
+
+    public function testPathWithTwoStartingSlashes()
+    {
+        $routes = $this->getRoutes('test', new Route('//path-and-not-domain'));
+
+        // this must not generate '//path-and-not-domain' because that would be a network path
+        $this->assertSame('/path-and-not-domain', $this->getGenerator($routes, array('BaseUrl' => ''))->generate('test'));
+    }
 
     public function testNoTrailingSlashForMultipleOptionalParameters()
     {

src/Symfony/Component/Routing/Tests/RouteCollectionTest.php

@@ -140,14 +140,19 @@ public function testAddPrefix()
     {
         $collection = new RouteCollection();
         $collection->add('foo', $foo = new Route('/foo'));
-        $collection->add('bar', $bar = new Route('/bar'));
+        $collection2 = new RouteCollection();
+        $collection2->add('bar', $bar = new Route('/bar'));
+        $collection->addCollection($collection2);
+        $this->assertSame('', $collection->getPrefix(), '->getPrefix() is initialized with ""');
+        $collection->addPrefix(' / ');
+        $this->assertSame('', $collection->getPrefix(), '->addPrefix() trims the prefix and a single slash has no effect');
         $collection->addPrefix('/{admin}', array('admin' => 'admin'), array('admin' => '\d+'), array('foo' => 'bar'));
         $this->assertEquals('/{admin}/foo', $collection->get('foo')->getPattern(), '->addPrefix() adds a prefix to all routes');
         $this->assertEquals('/{admin}/bar', $collection->get('bar')->getPattern(), '->addPrefix() adds a prefix to all routes');
-        $this->assertEquals(array('admin' => 'admin'), $collection->get('foo')->getDefaults(), '->addPrefix() adds a prefix to all routes');
-        $this->assertEquals(array('admin' => 'admin'), $collection->get('bar')->getDefaults(), '->addPrefix() adds a prefix to all routes');
-        $this->assertEquals(array('admin' => '\d+'), $collection->get('foo')->getRequirements(), '->addPrefix() adds a prefix to all routes');
-        $this->assertEquals(array('admin' => '\d+'), $collection->get('bar')->getRequirements(), '->addPrefix() adds a prefix to all routes');
+        $this->assertEquals(array('admin' => 'admin'), $collection->get('foo')->getDefaults(), '->addPrefix() adds defaults to all routes');
+        $this->assertEquals(array('admin' => 'admin'), $collection->get('bar')->getDefaults(), '->addPrefix() adds defaults to all routes');
+        $this->assertEquals(array('admin' => '\d+'), $collection->get('foo')->getRequirements(), '->addPrefix() adds requirements to all routes');
+        $this->assertEquals(array('admin' => '\d+'), $collection->get('bar')->getRequirements(), '->addPrefix() adds requirements to all routes');
         $this->assertEquals(
             array('foo' => 'bar', 'compiler_class' => 'Symfony\\Component\\Routing\\RouteCompiler'),
             $collection->get('foo')->getOptions(), '->addPrefix() adds an option to all routes'
@@ -158,6 +163,10 @@ public function testAddPrefix()
         );
         $collection->addPrefix('0');
         $this->assertEquals('/0/{admin}', $collection->getPrefix(), '->addPrefix() ensures a prefix must start with a slash and must not end with a slash');
+        $collection->addPrefix('/ /');
+        $this->assertSame('/ /0/{admin}', $collection->getPrefix(), '->addPrefix() can handle spaces if desired');
+        $this->assertSame('/ /0/{admin}/foo', $collection->get('foo')->getPattern(), 'the route pattern is in synch with the collection prefix');
+        $this->assertSame('/ /0/{admin}/bar', $collection->get('bar')->getPattern(), 'the route pattern in a sub-collection is in synch with the collection prefix');
     }
 
     public function testAddPrefixOverridesDefaultsAndRequirements()

src/Symfony/Component/Routing/Tests/RouteTest.php

@@ -34,6 +34,8 @@ public function testPattern()
         $route->setPattern('bar');
         $this->assertEquals('/bar', $route->getPattern(), '->setPattern() adds a / at the beginning of the pattern if needed');
         $this->assertEquals($route, $route->setPattern(''), '->setPattern() implements a fluent interface');
+        $route->setPattern('//path');
+        $this->assertEquals('/path', $route->getPattern(), '->setPattern() does not allow two slahes "//" at the beginning of the pattern as it would be confused with a network path when generating the path from the route');
     }
 
     public function testOptions()