merged branch vicb/routing_dumpers (PR symfony#3858)

Commits ------- 77185e0 [Routing] Allow spaces in the script name for the apache dumper 6465a69 [Routing] Fixes to handle spaces in route pattern Discussion ---------- [Routing] Handling of space characters in the dumpers The compiler was using the 'x' modifier in order to ignore extra spaces and line feeds but the code was flawed: - it was actually ignoring all the spaces, not only the extra ones added by the compiler, - all the spaces were stripped in the php and apache matchers. The proposed fix: - do not use the 'x' modifier any more (and then do no add extra spaces / line feeds), - do not strip the spaces in the matchers, - escapes the spaces (both in regexs and script name) for the apache matcher. It also include [a small optimization](https://github.com/vicb/symfony/pull/new#L9L89) when the only token of a route is an optional variable token - the idea is to make the regex easier to read. --------------------------------------------------------------------------- by vicb at 2012-04-10T13:59:45Z @Baachi fixed now. Thanks. --------------------------------------------------------------------------- by Tobion at 2012-04-10T16:01:31Z +1, I saw no reason for pretty printing the regex in the first place (just for debugging I guess). @vicb since you want to make the regex easier to read, I propose the remove the `P` from the variable regex `?P<bar>`, which is not needed anymore in PHP 5.3 (and we only support PHP 5.3+ anyway). --------------------------------------------------------------------------- by vicb at 2012-04-10T16:08:36Z @Tobion could you make a PR to this branch for the named parameters ? --------------------------------------------------------------------------- by Tobion at 2012-04-10T16:12:34Z I can include it in symfony#3754 because I'm about the add 2 more fixes to it anyway. But when I proposed to apply these fixes to 2.0 Fabien rejected it. So not sure what branch you want me to apply this. --------------------------------------------------------------------------- by vicb at 2012-04-10T16:25:38Z May be the best is to put it on hold while I am reviewing your PRs. There are already enough changes, we'll make an other PR after all have been sorted out. What's the difference between 3754 and 3810 ? (3810 + 3763 = 3754 ?) --------------------------------------------------------------------------- by Tobion at 2012-04-10T16:39:32Z Lol you forget to link the PR numbers. At first sight I thought it's some sort of mathematical riddle. Haha symfony#3810 is for 2.0 = symfony#3763 (already merged) + symfony#3754 for master --------------------------------------------------------------------------- by vicb at 2012-04-10T16:52:18Z I didn't link on purpose... the question is if '=' means strictly or loosely equal (any diffs - beside master vs 2.0) ? --------------------------------------------------------------------------- by Tobion at 2012-04-10T17:06:04Z It just applies my changes to 2.0. Nothing more. So master still differs from 2.0 by the addional features that were already implemented (e.g. `RouteCollection->addCollection` with optional requirements and options). But since my changes are bug fixes (except the performance improvement in symfony#3763 but that doesn't break anything and makes 2.0 easier to maintain) I thought they should go into 2.0 as well. --------------------------------------------------------------------------- by vicb at 2012-04-10T17:14:27Z @Tobion only bug fixes mean "only bug fixes". You should re-open a PR for 2.0 with "only bug fixes", you might want to wait for me to review 3754. --------------------------------------------------------------------------- by Tobion at 2012-04-10T17:21:00Z Without symfony#3763 it's much harder to apply the bug fixes. And now that I found 2 more bugs which requiresome rewriting of the PhpMatcherDumper, I don't want to apply all the commits by hand again for 2.0...
lsmith77 · Apr 11, 2012 · 8835357 · 8835357
2 parents 57990cc + 77185e0
commit 8835357
Show file tree

Hide file tree

Showing 10 changed files with 192 additions and 70 deletions.
diff --git a/src/Symfony/Component/Routing/Matcher/Dumper/ApacheMatcherDumper.php b/src/Symfony/Component/Routing/Matcher/Dumper/ApacheMatcherDumper.php
@@ -31,6 +31,8 @@ class ApacheMatcherDumper extends MatcherDumper
      * @param array $options An array of options
      *
      * @return string A string to be used as Apache rewrite rules
+     *
+     * @throws \LogicException When the route regex is invalid
      */
     public function dump(array $options = array())
     {
@@ -39,15 +41,23 @@ public function dump(array $options = array())
             'base_uri'    => '',
         ), $options);
 
+        $options['script_name'] = self::escape($options['script_name'], ' ', '\\');
+
         $rules = array("# skip \"real\" requests\nRewriteCond %{REQUEST_FILENAME} -f\nRewriteRule .* - [QSA,L]");
         $methodVars = array();
 
         foreach ($this->getRoutes()->all() as $name => $route) {
             $compiledRoute = $route->compile();
 
             // prepare the apache regex
-            $regex = preg_replace('/\?P<.+?>/', '', substr(str_replace(array("\n", ' '), '', $compiledRoute->getRegex()), 1, -3));
-            $regex = '^'.preg_quote($options['base_uri']).substr($regex, 1);
+            $regex = $compiledRoute->getRegex();
+            $delimiter = $regex[0];
+            $regexPatternEnd = strrpos($regex, $delimiter);
+            if (strlen($regex) < 2 || 0 === $regexPatternEnd) {
+                throw new \LogicException('The "%s" route regex "%s" is invalid', $name, $regex);
+            }
+            $regex = preg_replace('/\?P<.+?>/', '', substr($regex, 1, $regexPatternEnd - 1));
+            $regex = '^'.self::escape(preg_quote($options['base_uri']).substr($regex, 1), ' ', '\\');
 
             $hasTrailingSlash = '/$' == substr($regex, -2) && '^/$' != $regex;
 
@@ -56,7 +66,6 @@ public function dump(array $options = array())
                 $variables[] = 'E=_ROUTING_'.$variable.':%'.($i + 1);
             }
             foreach ($route->getDefaults() as $key => $value) {
-                // todo: a more legit way to escape the value?
                 $variables[] = 'E=_ROUTING_'.$key.':'.strtr($value, array(
                     ':'  => '\\:',
                     '='  => '\\=',
@@ -112,4 +121,36 @@ public function dump(array $options = array())
 
         return implode("\n\n", $rules)."\n";
     }
+
+    /**
+     * Escapes a string.
+     *
+     * @param string $string The string to be escaped
+     * @param string $char   The character to be escaped
+     * @param string $with   The character to be used for escaping
+     *
+     * @return string The escaped string
+     */
+    static private function escape($string, $char, $with)
+    {
+        $escaped = false;
+        $output = '';
+        foreach(str_split($string) as $symbol) {
+            if ($escaped) {
+                $output .= $symbol;
+                $escaped = false;
+                continue;
+            }
+            if ($symbol === $char) {
+                $output .= $with.$char;
+                continue;
+            }
+            if ($symbol === $with) {
+                $escaped = true;
+            }
+            $output .= $symbol;
+        }
+
+        return $output;
+    }
 }
diff --git a/src/Symfony/Component/Routing/Matcher/Dumper/PhpMatcherDumper.php b/src/Symfony/Component/Routing/Matcher/Dumper/PhpMatcherDumper.php
@@ -148,7 +148,7 @@ private function compileRoute(Route $route, $name, $supportsRedirections, $paren
         $conditions = array();
         $hasTrailingSlash = false;
         $matches = false;
-        if (!count($compiledRoute->getVariables()) && false !== preg_match('#^(.)\^(?P<url>.*?)\$\1#', str_replace(array("\n", ' '), '', $compiledRoute->getRegex()), $m)) {
+        if (!count($compiledRoute->getVariables()) && false !== preg_match('#^(.)\^(?P<url>.*?)\$\1#', $compiledRoute->getRegex(), $m)) {
             if ($supportsRedirections && substr($m['url'], -1) === '/') {
                 $conditions[] = sprintf("rtrim(\$pathinfo, '/') === %s", var_export(rtrim(str_replace('\\', '', $m['url']), '/'), true));
                 $hasTrailingSlash = true;
@@ -160,7 +160,7 @@ private function compileRoute(Route $route, $name, $supportsRedirections, $paren
                 $conditions[] = sprintf("0 === strpos(\$pathinfo, %s)", var_export($compiledRoute->getStaticPrefix(), true));
             }
 
-            $regex = str_replace(array("\n", ' '), '', $compiledRoute->getRegex());
+            $regex = $compiledRoute->getRegex();
             if ($supportsRedirections && $pos = strpos($regex, '/$')) {
                 $regex = substr($regex, 0, $pos).'/?$'.substr($regex, $pos + 2);
                 $hasTrailingSlash = true;

diff --git a/src/Symfony/Component/Routing/RouteCompiler.php b/src/Symfony/Component/Routing/RouteCompiler.php
@@ -66,44 +66,62 @@ public function compile(Route $route)
         // find the first optional token
         $firstOptional = INF;
         for ($i = count($tokens) - 1; $i >= 0; $i--) {
-            if ('variable' === $tokens[$i][0] && $route->hasDefault($tokens[$i][3])) {
+            $token = $tokens[$i];
+            if ('variable' === $token[0] && $route->hasDefault($token[3])) {
                 $firstOptional = $i;
             } else {
                 break;
             }
         }
 
         // compute the matching regexp
-        $regex = '';
-        $indent = 1;
-        if (1 === count($tokens) && 0 === $firstOptional) {
-            $token = $tokens[0];
-            ++$indent;
-            $regex .= str_repeat(' ', $indent * 4).sprintf("%s(?:\n", preg_quote($token[1], '#'));
-            $regex .= str_repeat(' ', $indent * 4).sprintf("(?P<%s>%s)\n", $token[3], $token[2]);
-        } else {
-            foreach ($tokens as $i => $token) {
-                if ('text' === $token[0]) {
-                    $regex .= str_repeat(' ', $indent * 4).preg_quote($token[1], '#')."\n";
-                } else {
-                    if ($i >= $firstOptional) {
-                        $regex .= str_repeat(' ', $indent * 4)."(?:\n";
-                        ++$indent;
-                    }
-                    $regex .= str_repeat(' ', $indent * 4).sprintf("%s(?P<%s>%s)\n", preg_quote($token[1], '#'), $token[3], $token[2]);
-                }
-            }
-        }
-        while (--$indent) {
-            $regex .= str_repeat(' ', $indent * 4).")?\n";
+        $regexp = '';
+        for ($i = 0, $nbToken = count($tokens); $i < $nbToken; $i++) {
+            $regexp .= $this->computeRegexp($tokens, $i, $firstOptional);
         }
 
         return new CompiledRoute(
             $route,
             'text' === $tokens[0][0] ? $tokens[0][1] : '',
-            sprintf("#^\n%s$#xs", $regex),
+            sprintf("#^%s$#s", $regexp),
             array_reverse($tokens),
             $variables
         );
     }
+
+    /**
+     * Computes the regexp used to match the token.
+     *
+     * @param array   $tokens        The route tokens
+     * @param integer $index         The index of the current token
+     * @param integer $firstOptional The index of the first optional token
+     *
+     * @return string The regexp
+     */
+    private function computeRegexp(array $tokens, $index, $firstOptional)
+    {
+        $token = $tokens[$index];
+        if('text' === $token[0]) {
+            // Text tokens
+            return preg_quote($token[1], '#');
+        } else {
+            // Variable tokens
+            if (0 === $index && 0 === $firstOptional && 1 == count($tokens)) {
+                // When the only token is an optional variable token, the separator is required
+                return sprintf('%s(?P<%s>%s)?', preg_quote($token[1], '#'), $token[3], $token[2]);
+            } else {
+                $nbTokens = count($tokens);
+                $regexp = sprintf('%s(?P<%s>%s)', preg_quote($token[1], '#'), $token[3], $token[2]);
+                if ($index >= $firstOptional) {
+                    // Enclose each optional tokens in a subpattern to make it optional
+                    $regexp = "(?:$regexp";
+                    if ($nbTokens - 1 == $index) {
+                        // Close the optional subpatterns
+                        $regexp .= str_repeat(")?", $nbTokens - $firstOptional);
+                    }
+                }
+                return $regexp;
+            }
+        }
+    }
 }
diff --git a/tests/Symfony/Tests/Component/Routing/Fixtures/dumper/url_matcher1.apache b/tests/Symfony/Tests/Component/Routing/Fixtures/dumper/url_matcher1.apache
@@ -53,6 +53,10 @@ RewriteRule .* app.php [QSA,L,E=_ROUTING__route:baz5,E=_ROUTING_foo:%1]
 RewriteCond %{REQUEST_URI} ^/test/baz$
 RewriteRule .* app.php [QSA,L,E=_ROUTING__route:baz6,E=_ROUTING_foo:bar\ baz]
 
+# baz7
+RewriteCond %{REQUEST_URI} ^/te\ st/baz$
+RewriteRule .* app.php [QSA,L,E=_ROUTING__route:baz7]
+
 # 405 Method Not Allowed
 RewriteCond %{_ROUTING__allow_GET} !-z [OR]
 RewriteCond %{_ROUTING__allow_HEAD} !-z [OR]

diff --git a/tests/Symfony/Tests/Component/Routing/Fixtures/dumper/url_matcher1.php b/tests/Symfony/Tests/Component/Routing/Fixtures/dumper/url_matcher1.php
@@ -26,12 +26,12 @@ public function match($pathinfo)
         $pathinfo = urldecode($pathinfo);
 
         // foo
-        if (0 === strpos($pathinfo, '/foo') && preg_match('#^/foo/(?P<bar>baz|symfony)$#xs', $pathinfo, $matches)) {
+        if (0 === strpos($pathinfo, '/foo') && preg_match('#^/foo/(?P<bar>baz|symfony)$#s', $pathinfo, $matches)) {
             return array_merge($this->mergeDefaults($matches, array (  'def' => 'test',)), array('_route' => 'foo'));
         }
 
         // bar
-        if (0 === strpos($pathinfo, '/bar') && preg_match('#^/bar/(?P<foo>[^/]+?)$#xs', $pathinfo, $matches)) {
+        if (0 === strpos($pathinfo, '/bar') && preg_match('#^/bar/(?P<foo>[^/]+?)$#s', $pathinfo, $matches)) {
             if (!in_array($this->context->getMethod(), array('GET', 'HEAD'))) {
                 $allow = array_merge($allow, array('GET', 'HEAD'));
                 goto not_bar;
@@ -42,7 +42,7 @@ public function match($pathinfo)
         not_bar:
 
         // barhead
-        if (0 === strpos($pathinfo, '/barhead') && preg_match('#^/barhead/(?P<foo>[^/]+?)$#xs', $pathinfo, $matches)) {
+        if (0 === strpos($pathinfo, '/barhead') && preg_match('#^/barhead/(?P<foo>[^/]+?)$#s', $pathinfo, $matches)) {
             if (!in_array($this->context->getMethod(), array('GET', 'HEAD'))) {
                 $allow = array_merge($allow, array('GET', 'HEAD'));
                 goto not_barhead;
@@ -68,13 +68,13 @@ public function match($pathinfo)
         }
 
         // baz4
-        if (0 === strpos($pathinfo, '/test') && preg_match('#^/test/(?P<foo>[^/]+?)/$#xs', $pathinfo, $matches)) {
+        if (0 === strpos($pathinfo, '/test') && preg_match('#^/test/(?P<foo>[^/]+?)/$#s', $pathinfo, $matches)) {
             $matches['_route'] = 'baz4';
             return $matches;
         }
 
         // baz5
-        if (0 === strpos($pathinfo, '/test') && preg_match('#^/test/(?P<foo>[^/]+?)/$#xs', $pathinfo, $matches)) {
+        if (0 === strpos($pathinfo, '/test') && preg_match('#^/test/(?P<foo>[^/]+?)/$#s', $pathinfo, $matches)) {
             if ($this->context->getMethod() != 'POST') {
                 $allow[] = 'POST';
                 goto not_baz5;
@@ -85,7 +85,7 @@ public function match($pathinfo)
         not_baz5:
 
         // baz.baz6
-        if (0 === strpos($pathinfo, '/test') && preg_match('#^/test/(?P<foo>[^/]+?)/$#xs', $pathinfo, $matches)) {
+        if (0 === strpos($pathinfo, '/test') && preg_match('#^/test/(?P<foo>[^/]+?)/$#s', $pathinfo, $matches)) {
             if ($this->context->getMethod() != 'PUT') {
                 $allow[] = 'PUT';
                 goto not_bazbaz6;
@@ -101,33 +101,38 @@ public function match($pathinfo)
         }
 
         // quoter
-        if (preg_match('#^/(?P<quoter>[\']+)$#xs', $pathinfo, $matches)) {
+        if (preg_match('#^/(?P<quoter>[\']+)$#s', $pathinfo, $matches)) {
             $matches['_route'] = 'quoter';
             return $matches;
         }
 
+        // space
+        if ($pathinfo === '/spa ce') {
+            return array('_route' => 'space');
+        }
+
         if (0 === strpos($pathinfo, '/a')) {
             if (0 === strpos($pathinfo, '/a/b\'b')) {
                 // foo1
-                if (preg_match('#^/a/b\'b/(?P<foo>[^/]+?)$#xs', $pathinfo, $matches)) {
+                if (preg_match('#^/a/b\'b/(?P<foo>[^/]+?)$#s', $pathinfo, $matches)) {
                     $matches['_route'] = 'foo1';
                     return $matches;
                 }
 
                 // bar1
-                if (preg_match('#^/a/b\'b/(?P<bar>[^/]+?)$#xs', $pathinfo, $matches)) {
+                if (preg_match('#^/a/b\'b/(?P<bar>[^/]+?)$#s', $pathinfo, $matches)) {
                     $matches['_route'] = 'bar1';
                     return $matches;
                 }
 
                 // foo2
-                if (preg_match('#^/a/b\'b/(?P<foo1>[^/]+?)$#xs', $pathinfo, $matches)) {
+                if (preg_match('#^/a/b\'b/(?P<foo1>[^/]+?)$#s', $pathinfo, $matches)) {
                     $matches['_route'] = 'foo2';
                     return $matches;
                 }
 
                 // bar2
-                if (preg_match('#^/a/b\'b/(?P<bar1>[^/]+?)$#xs', $pathinfo, $matches)) {
+                if (preg_match('#^/a/b\'b/(?P<bar1>[^/]+?)$#s', $pathinfo, $matches)) {
                     $matches['_route'] = 'bar2';
                     return $matches;
                 }
@@ -145,21 +150,21 @@ public function match($pathinfo)
             }
 
             // foo4
-            if (preg_match('#^/aba/(?P<foo>[^/]+?)$#xs', $pathinfo, $matches)) {
+            if (preg_match('#^/aba/(?P<foo>[^/]+?)$#s', $pathinfo, $matches)) {
                 $matches['_route'] = 'foo4';
                 return $matches;
             }
 
         }
 
         // foo3
-        if (preg_match('#^/(?P<_locale>[^/]+?)/b/(?P<foo>[^/]+?)$#xs', $pathinfo, $matches)) {
+        if (preg_match('#^/(?P<_locale>[^/]+?)/b/(?P<foo>[^/]+?)$#s', $pathinfo, $matches)) {
             $matches['_route'] = 'foo3';
             return $matches;
         }
 
         // bar3
-        if (preg_match('#^/(?P<_locale>[^/]+?)/b/(?P<bar>[^/]+?)$#xs', $pathinfo, $matches)) {
+        if (preg_match('#^/(?P<_locale>[^/]+?)/b/(?P<bar>[^/]+?)$#s', $pathinfo, $matches)) {
             $matches['_route'] = 'bar3';
             return $matches;
         }

diff --git a/tests/Symfony/Tests/Component/Routing/Fixtures/dumper/url_matcher2.apache b/tests/Symfony/Tests/Component/Routing/Fixtures/dumper/url_matcher2.apache
@@ -0,0 +1,7 @@
+# skip "real" requests
+RewriteCond %{REQUEST_FILENAME} -f
+RewriteRule .* - [QSA,L]
+
+# foo
+RewriteCond %{REQUEST_URI} ^/foo$
+RewriteRule .* ap\ p_d\ ev.php [QSA,L,E=_ROUTING__route:foo]