fixed dumping of a root collection with prefix

symfony · Tobion · Apr 1, 2012 · Apr 4, 2012 · Apr 5, 2012 · Apr 10, 2012
commit 46434e0528c561f89f182e164b8689e5431bae44
diff --git a/src/Symfony/Component/Routing/Matcher/Dumper/PhpMatcherDumper.php b/src/Symfony/Component/Routing/Matcher/Dumper/PhpMatcherDumper.php
@@ -74,6 +74,13 @@ public function __construct(RequestContext \$context)
 EOF;
     }
 
+    /**
+     * Generates the code for the match method implementing UrlMatcherInterface.
+     *
+     * @param Boolean $supportsRedirections Whether redirections are supported by the base class
+     *
+     * @return string Match method as PHP code
+     */
     private function generateMatchMethod($supportsRedirections)
     {
         $code = rtrim($this->compileRoutes($this->getRoutes(), $supportsRedirections), "\n");
@@ -91,38 +98,79 @@ public function match(\$pathinfo)
 EOF;
     }
 
+    /**
+     * Counts the number of routes as direct child of the RouteCollection.
+     *
+     * @param RouteCollection $routes A RouteCollection instance
+     *
+     * @return integer Number of Routes
+     */
+    private function countDirectChildRoutes(RouteCollection $routes)
+    {
+        $count = 0;
+        foreach ($routes as $route) {
+            if ($route instanceof Route) {
+                $count++;
+            }
+        }
+
+        return $count;
+    }
+
+    /**
+     * Generates PHP code recursively to match a RouteCollection with all child routes and child collections.
+     *
+     * @param RouteCollection $routes               A RouteCollection instance
+     * @param Boolean         $supportsRedirections Whether redirections are supported by the base class
+     * @param string|null     $parentPrefix         The prefix of the parent collection used to optimize the code
+     *
+     * @return string PHP code
+     */
     private function compileRoutes(RouteCollection $routes, $supportsRedirections, $parentPrefix = null)
     {
         $code = '';
 
+        $prefix = $routes->getPrefix();
+        $countDirectChildRoutes = $this->countDirectChildRoutes($routes);
+        $countAllChildRoutes = count($routes->all());
+        $optimizable = '' !== $prefix && $prefix !== $parentPrefix && $countDirectChildRoutes > 0 && $countAllChildRoutes > 1 && false === strpos($prefix, '{');
+        // whether the matching of routes within the collection can be optimized by wrapping it with the prefix condition
+        // - no need to optimize if current prefix is the same as the parent prefix
+        // - if $countDirectChildRoutes === 0, the sub-collections can do their own optimizations (in case there are any)
+        // - it's not worth wrapping a single child route
+        // - prefixes with variables cannot be optimized because routes within the collection might have different requirements for the same variable
+        if ($optimizable) {
+            $code .= sprintf("    if (0 === strpos(\$pathinfo, %s)) {\n", var_export($prefix, true));
+        }
+
         foreach ($routes as $name => $route) {
-            if ($route instanceof RouteCollection) {
-                $prefix = $route->getPrefix();
-                $countWorth = count($route->all()) > 1;
-                // whether it's optimizable
-                if ('' !== $prefix && $prefix !== $parentPrefix && $countWorth && false === strpos($prefix, '{')) {
-                    $code .= sprintf("        if (0 === strpos(\$pathinfo, %s)) {\n", var_export($prefix, true));
-
-                    foreach (explode("\n", $this->compileRoutes($route, $supportsRedirections, $prefix)) as $line) {
-                        if ('' !== $line) {
-                            $code .= '    '; // apply extra indention
-                        }
-                        $code .= $line."\n";
-                    }
-
-                    $code = substr($code, 0, -2); // remove redundant last two line breaks
-                    $code .= "        }\n\n";
-                } else {
-                    $code .= $this->compileRoutes($route, $supportsRedirections, $countWorth ? $prefix : null);
-                }
-            } else {
-                $code .= $this->compileRoute($route, $name, $supportsRedirections, $parentPrefix)."\n";
+            if ($route instanceof Route) {
+                $code .= $this->compileRoute($route, $name, $supportsRedirections, 1 === $countAllChildRoutes ? null : $prefix)."\n";
+                // a single route in a sub-collection is not wrapped so it should do its own optimization in ->compileRoute with $parentPrefix = null
+            } elseif ($countAllChildRoutes - $countDirectChildRoutes > 0) { // we can stop iterating recursively if we already know there are no more routes
+                $code .= $this->compileRoutes($route, $supportsRedirections, $prefix);
             }
         }
 
+        if ($optimizable) {
+            $code .= "    }\n\n";
+            $code = preg_replace('/^.{2,}$/m', '    $0', $code); // apply extra indention at each line (except empty ones)
+        }
+
         return $code;
     }
 
+
+    /**
+     * Compiles a single Route to PHP code used to match it against the path info.
+     *
+     * @param Route       $routes               A Route instance
+     * @param string      $name                 The name of the Route
+     * @param Boolean     $supportsRedirections Whether redirections are supported by the base class
+     * @param string|null $parentPrefix         The prefix of the parent collection used to optimize the code
+     *
+     * @return string PHP code
+     */
     private function compileRoute(Route $route, $name, $supportsRedirections, $parentPrefix = null)
     {
         $code = '';

diff --git a/src/Symfony/Component/Routing/RouteCollection.php b/src/Symfony/Component/Routing/RouteCollection.php
@@ -215,6 +215,10 @@ public function addPrefix($prefix, $defaults = array(), $requirements = array(),
         // a prefix must not end with a slash
         $prefix = rtrim($prefix, '/');
 
+        if ('' === $prefix && empty($defaults) && empty($requirements) && empty($options)) {
            return;
+        }
+
         // a prefix must start with a slash
         if ('' !== $prefix && '/' !== $prefix[0]) {
             $prefix = '/'.$prefix;

diff --git a/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher1.php b/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher1.php
@@ -124,6 +124,7 @@ public function match($pathinfo)
                     $matches['_route'] = 'bar1';
                     return $matches;
                 }
+
             }
 
             // overriden
@@ -144,7 +145,9 @@ public function match($pathinfo)
                     $matches['_route'] = 'bar2';
                     return $matches;
                 }
+
             }
+
         }
 
         if (0 === strpos($pathinfo, '/multi')) {
@@ -162,6 +165,7 @@ public function match($pathinfo)
             if ($pathinfo === '/multi/hey/') {
                 return array('_route' => 'hey');
             }
+
         }
 
         // foo3
@@ -205,7 +209,9 @@ public function match($pathinfo)
                     $matches['_route'] = 'c';
                     return $matches;
                 }
+
             }
+
         }
 
         throw 0 < count($allow) ? new MethodNotAllowedException(array_unique($allow)) : new ResourceNotFoundException();

diff --git a/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher2.php b/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher2.php
@@ -130,6 +130,7 @@ public function match($pathinfo)
                     $matches['_route'] = 'bar1';
                     return $matches;
                 }
+
             }
 
             // overriden
@@ -150,7 +151,9 @@ public function match($pathinfo)
                     $matches['_route'] = 'bar2';
                     return $matches;
                 }
+
             }
+
         }
 
         if (0 === strpos($pathinfo, '/multi')) {
@@ -171,6 +174,7 @@ public function match($pathinfo)
                 }
                 return array('_route' => 'hey');
             }
+
         }
 
         // foo3
@@ -214,7 +218,9 @@ public function match($pathinfo)
                     $matches['_route'] = 'c';
                     return $matches;
                 }
+
             }
+
         }
 
         // secure

diff --git a/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher3.php b/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher3.php
@@ -0,0 +1,44 @@
+<?php
+
+use Symfony\Component\Routing\Exception\MethodNotAllowedException;
+use Symfony\Component\Routing\Exception\ResourceNotFoundException;
+use Symfony\Component\Routing\RequestContext;
+
+/**
+ * ProjectUrlMatcher
+ *
+ * This class has been auto-generated
+ * by the Symfony Routing Component.
+ */
+class ProjectUrlMatcher extends Symfony\Component\Routing\Matcher\UrlMatcher
+{
+    /**
+     * Constructor.
+     */
+    public function __construct(RequestContext $context)
+    {
+        $this->context = $context;
+    }
+
+    public function match($pathinfo)
+    {
+        $allow = array();
+        $pathinfo = urldecode($pathinfo);
+
+        if (0 === strpos($pathinfo, '/rootprefix')) {
+            // static
+            if ($pathinfo === '/rootprefix/test') {
+                return array('_route' => 'static');
+            }
+
+            // dynamic
+            if (preg_match('#^/rootprefix/(?P<var>[^/]+?)$#xs', $pathinfo, $matches)) {
+                $matches['_route'] = 'dynamic';
+                return $matches;
+            }
+
+        }
+
+        throw 0 < count($allow) ? new MethodNotAllowedException(array_unique($allow)) : new ResourceNotFoundException();
+    }
+}
diff --git a/src/Symfony/Component/Routing/Tests/Matcher/Dumper/PhpMatcherDumperTest.php b/src/Symfony/Component/Routing/Tests/Matcher/Dumper/PhpMatcherDumperTest.php
@@ -18,33 +18,6 @@
 
 class PhpMatcherDumperTest extends \PHPUnit_Framework_TestCase
 {
-    public function testDump()
-    {
-        $collection = $this->getRouteCollection();
-
-        $dumper = new PhpMatcherDumper($collection, new RequestContext());
-
-        $this->assertStringEqualsFile(__DIR__.'/../../Fixtures/dumper/url_matcher1.php', $dumper->dump(), '->dump() dumps basic routes to the correct PHP file.');
-
-        // force HTTPS redirection
-        $collection->add('secure', new Route(
-            '/secure',
-            array(),
-            array('_scheme' => 'https')
-        ));
-
-        // force HTTP redirection
-        $collection->add('nonsecure', new Route(
-            '/nonsecure',
-            array(),
-            array('_scheme' => 'http')
-        ));
-
-        $dumper = new PhpMatcherDumper($collection, new RequestContext());
-    
-        $this->assertStringEqualsFile(__DIR__.'/../../Fixtures/dumper/url_matcher2.php', $dumper->dump(array('base_class' => 'Symfony\Component\Routing\Tests\Fixtures\RedirectableUrlMatcher')), '->dump() dumps basic routes to the correct PHP file.');
-    }
-
     /**
      * @expectedException \LogicException
      */
@@ -60,8 +33,22 @@ public function testDumpWhenSchemeIsUsedWithoutAProperDumper()
         $dumper->dump();
     }
 
-    protected function getRouteCollection()
+    /**
+     * @dataProvider getRouteCollections
+     */
+    public function testDump(RouteCollection $collection, $fixture, $options = array())
+    {
+        $basePath = __DIR__.'/../../Fixtures/dumper/';
+
+        $dumper = new PhpMatcherDumper($collection, new RequestContext());
+
+        $this->assertStringEqualsFile($basePath.$fixture, $dumper->dump($options), '->dump() correctly dumps routes as optimized PHP code.');
+    }
+
+    public function getRouteCollections()
     {
+        /* test case 1 */
+
         $collection = new RouteCollection();
 
         $collection->add('overriden', new Route('/overriden'));
@@ -182,6 +169,38 @@ protected function getRouteCollection()
         $collection2->addCollection($collection3, '/c');
         $collection->addCollection($collection1, '/a');
 
-        return $collection;
+
+        /* test case 2 */
+
+        $redirectCollection = clone $collection;
+
+        // force HTTPS redirection
+        $redirectCollection->add('secure', new Route(
+            '/secure',
+            array(),
+            array('_scheme' => 'https')
+        ));
+
+        // force HTTP redirection
+        $redirectCollection->add('nonsecure', new Route(
+            '/nonsecure',
+            array(),
+            array('_scheme' => 'http')
+        ));
+
+
+        /* test case 3 */
+
+        $rootprefixCollection = new RouteCollection();
+        $rootprefixCollection->add('static', new Route('/test'));
+        $rootprefixCollection->add('dynamic', new Route('/{var}'));
+        $rootprefixCollection->addPrefix('rootprefix');
+
+
+        return array(
+           array($collection, 'url_matcher1.php', array()),
+           array($redirectCollection, 'url_matcher2.php', array('base_class' => 'Symfony\Component\Routing\Tests\Fixtures\RedirectableUrlMatcher')),
+           array($rootprefixCollection, 'url_matcher3.php', array())
+        );
     }
 }