merged branch arnaud-lb/apache-dumper (PR #5792)

This PR was merged into the master branch. Commits ------- c7a8f7a [Routing] fixed possible parameters conflict in apache url matcher Discussion ---------- [Routing] fixed possible parameters conflict in apache url matcher Bug fix: yes Feature addition: no Backwards compatibility break: no (as long as rewrite rules are generated after upgrading) Symfony2 tests pass: yes - This fixes a conflict in route parameters: The rewrite rules currently pass route informations through environment variables: `_ROUTING_DEFAULT_x`: passes the default value of parameter x `_ROUTING__allow_x`: passes the information that method x was allowed for this route `_ROUTING_x`: passes the value of parameter x The problem is that naming a route parameter `DEFAULT_*` or `_allow_*` would not behave as expected. I fixed this by namespacing all environment variables; e.g. parameters are in `_ROUTING_param_*`, defaults in `_ROUTING_default_*`, etc. - The PR fixes a second issue: sometimes the variables are prefixed with multiple REDIRECT_. This PR handles this case by ignoring them all. - This also improves performance a little: Matching a route with two parameters and two default parameters 100K times: (`$_SERVER` was copied from a real request, so with many non `_ROUTING_` variables) master: 6.6s this branch: 4.7s --------------------------------------------------------------------------- by fabpot at 2012-10-27T13:37:24Z Any news on this PR? Is it mergeable? --------------------------------------------------------------------------- by arnaud-lb at 2012-10-27T14:50:08Z There is an issue with default parameter values, I can't find how to fix that in a simple way. Before this PR, default values are never used (if a parameter is an optional not present in the url, the parameter's value is the empty string); after this PR, when a parameter is present and empty (e.g. a requirement like `.*`), its value is set to its default value. --------------------------------------------------------------------------- by Tobion at 2012-10-29T01:36:08Z The problem is, it's not consistent with the default php matcher. So one cannot safely exchange it with the apache matcher because it behaves differently under some (special) circumstances. --------------------------------------------------------------------------- by fabpot at 2012-11-05T08:05:54Z We need to move forward as I want to merge the hostname support in the routing ASAP to have plenty of time for feedback before the 2.2 release. Does it sound reasonable to merge this PR as is an open a ticket about the remaining issue (which should not occur that often anyways)? --------------------------------------------------------------------------- by arnaud-lb at 2012-11-05T09:22:02Z @fabpot it sounds reasonable to me. Also, I've the hostname support branch is currently rebased so that it can be merged without this one. --------------------------------------------------------------------------- by Tobion at 2012-11-11T21:50:20Z Btw, does the ApacheMatcherDumper handle the _scheme requirement? It doesn't look like it. This would be another bug. Anyway, we can probably merge this PR and open new issues for the remaining bugs.
symfony · Nov 12, 2012 · a088722 · a088722
2 parents 0876a19 + c7a8f7a
commit a088722
Show file tree

Hide file tree

Showing 5 changed files with 181 additions and 120 deletions.
diff --git a/src/Symfony/Component/Routing/Matcher/ApacheUrlMatcher.php b/src/Symfony/Component/Routing/Matcher/ApacheUrlMatcher.php
@@ -17,6 +17,7 @@
  * ApacheUrlMatcher matches URL based on Apache mod_rewrite matching (see ApacheMatcherDumper).
  *
  * @author Fabien Potencier <fabien@symfony.com>
+ * @author Arnaud Le Blanc <arnaud.lb@gmail.com>
  */
 class ApacheUrlMatcher extends UrlMatcher
 {
@@ -36,36 +37,52 @@ public function match($pathinfo)
         $parameters = array();
         $defaults = array();
         $allow = array();
-        $match = false;
+        $route = null;
 
         foreach ($_SERVER as $key => $value) {
             $name = $key;
 
-            if (0 === strpos($name, 'REDIRECT_')) {
-                $name = substr($name, 9);
+            // skip non-routing variables
+            // this improves performance when $_SERVER contains many usual
+            // variables like HTTP_*, DOCUMENT_ROOT, REQUEST_URI, ...
+            if (false === strpos($name, '_ROUTING_')) {
+                continue;
             }
 
-            if (0 === strpos($name, '_ROUTING_DEFAULTS_')) {
-                $name = substr($name, 18);
-                $defaults[$name] = $value;
-            } elseif (0 === strpos($name, '_ROUTING_')) {
+            while (0 === strpos($name, 'REDIRECT_')) {
                 $name = substr($name, 9);
-                if ('_route' == $name) {
-                    $match = true;
-                    $parameters[$name] = $value;
-                } elseif (0 === strpos($name, '_allow_')) {
-                    $allow[] = substr($name, 7);
-                } else {
+            }
+
+            // expect _ROUTING_<type>_<name>
+            // or _ROUTING_<type>
+
+            if (0 !== strpos($name, '_ROUTING_')) {
+                continue;
+            }
+            if (false !== $pos = strpos($name, '_', 9)) {
+                $type = substr($name, 9, $pos-9);
+                $name = substr($name, $pos+1);
+            } else {
+                $type = substr($name, 9);
+            }
+
+            if ('param' === $type) {
+                if ('' !== $value) {
                     $parameters[$name] = $value;
                 }
-            } else {
-                continue;
+            } elseif ('default' === $type) {
+                $defaults[$name] = $value;
+            } elseif ('route' === $type) {
+                $route = $value;
+            } elseif ('allow' === $type) {
+                $allow[] = $name;
             }
 
             unset($_SERVER[$key]);
         }
 
-        if ($match) {
+        if (null !== $route) {
+            $parameters['_route'] = $route;
             return $this->mergeDefaults($parameters, $defaults);
         } elseif (0 < count($allow)) {
             throw new MethodNotAllowedException($allow);

diff --git a/src/Symfony/Component/Routing/Matcher/Dumper/ApacheMatcherDumper.php b/src/Symfony/Component/Routing/Matcher/Dumper/ApacheMatcherDumper.php
@@ -11,6 +11,8 @@
 
 namespace Symfony\Component\Routing\Matcher\Dumper;
 
+use Symfony\Component\Routing\Route;
+
 /**
  * Dumps a set of Apache mod_rewrite rules.
  *
@@ -46,88 +48,116 @@ public function dump(array $options = array())
         $methodVars = array();
 
         foreach ($this->getRoutes()->all() as $name => $route) {
-            $compiledRoute = $route->compile();
-
-            // prepare the apache regex
-            $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('/\?<.+?>/', '', substr($regex, 1, $regexPatternEnd - 1));
-            $regex = '^'.self::escape(preg_quote($options['base_uri']).substr($regex, 1), ' ', '\\');
-
-            $methods = array();
-            if ($req = $route->getRequirement('_method')) {
-                $methods = explode('|', strtoupper($req));
-                // GET and HEAD are equivalent
-                if (in_array('GET', $methods) && !in_array('HEAD', $methods)) {
-                    $methods[] = 'HEAD';
-                }
+            $rules[] = $this->dumpRoute($name, $route, $options);
+            $methodVars = array_merge($methodVars, $this->getRouteMethods($route));
+        }
+
+        if (0 < count($methodVars)) {
+            $rule = array('# 405 Method Not Allowed');
+            $methodVars = array_values(array_unique($methodVars));
+            foreach ($methodVars as $i => $methodVar) {
+                $rule[] = sprintf('RewriteCond %%{_ROUTING_allow_%s} !-z%s', $methodVar, isset($methodVars[$i + 1]) ? ' [OR]' : '');
             }
+            $rule[] = sprintf('RewriteRule .* %s [QSA,L]', $options['script_name']);
 
-            $hasTrailingSlash = (!$methods || in_array('HEAD', $methods)) && '/$' === substr($regex, -2) && '^/$' !== $regex;
+            $rules[] = implode("\n", $rule);
+        }
 
-            $variables = array('E=_ROUTING__route:'.$name);
-            foreach ($compiledRoute->getVariables() as $i => $variable) {
-                $variables[] = 'E=_ROUTING_'.$variable.':%'.($i + 1);
-            }
-            foreach ($route->getDefaults() as $key => $value) {
-                $variables[] = 'E=_ROUTING_DEFAULTS_'.$key.':'.strtr($value, array(
-                    ':'  => '\\:',
-                    '='  => '\\=',
-                    '\\' => '\\\\',
-                    ' '  => '\\ ',
-                ));
-            }
-            $variables = implode(',', $variables);
-
-            $rule = array("# $name");
-
-            // method mismatch
-            if ($req = $route->getRequirement('_method')) {
-                $methods = explode('|', strtoupper($req));
-                // GET and HEAD are equivalent
-                if (in_array('GET', $methods) && !in_array('HEAD', $methods)) {
-                    $methods[] = 'HEAD';
-                }
-                $allow = array();
-                foreach ($methods as $method) {
-                    $methodVars[] = $method;
-                    $allow[] = 'E=_ROUTING__allow_'.$method.':1';
-                }
-
-                $rule[] = "RewriteCond %{REQUEST_URI} $regex";
-                $rule[] = sprintf("RewriteCond %%{REQUEST_METHOD} !^(%s)$ [NC]", implode('|', $methods));
-                $rule[] = sprintf('RewriteRule .* - [S=%d,%s]', $hasTrailingSlash ? 2 : 1, implode(',', $allow));
-            }
+        return implode("\n\n", $rules)."\n";
+    }
+
+    private function dumpRoute($name, $route, array $options)
+    {
+        $compiledRoute = $route->compile();
+
+        // prepare the apache regex
+        $regex = $this->regexToApacheRegex($compiledRoute->getRegex());
+        $regex = '^'.self::escape(preg_quote($options['base_uri']).substr($regex, 1), ' ', '\\');
+
+        $methods = $this->getRouteMethods($route);
+
+        $hasTrailingSlash = (!$methods || in_array('HEAD', $methods)) && '/$' === substr($regex, -2) && '^/$' !== $regex;
+
+        $variables = array('E=_ROUTING_route:'.$name);
+        foreach ($compiledRoute->getVariables() as $i => $variable) {
+            $variables[] = 'E=_ROUTING_param_'.$variable.':%'.($i + 1);
+        }
+        foreach ($route->getDefaults() as $key => $value) {
+            $variables[] = 'E=_ROUTING_default_'.$key.':'.strtr($value, array(
+                ':'  => '\\:',
+                '='  => '\\=',
+                '\\' => '\\\\',
+                ' '  => '\\ ',
+            ));
+        }
+        $variables = implode(',', $variables);
+
+        $rule = array("# $name");
 
-            // redirect with trailing slash appended
-            if ($hasTrailingSlash) {
-                $rule[] = 'RewriteCond %{REQUEST_URI} '.substr($regex, 0, -2).'$';
-                $rule[] = 'RewriteRule .* $0/ [QSA,L,R=301]';
+        // method mismatch
+        if (0 < count($methods)) {
+            $allow = array();
+            foreach ($methods as $method) {
+                $methodVars[] = $method;
+                $allow[] = 'E=_ROUTING_allow_'.$method.':1';
             }
 
-            // the main rule
             $rule[] = "RewriteCond %{REQUEST_URI} $regex";
-            $rule[] = "RewriteRule .* {$options['script_name']} [QSA,L,$variables]";
+            $rule[] = sprintf("RewriteCond %%{REQUEST_METHOD} !^(%s)$ [NC]", implode('|', $methods));
+            $rule[] = sprintf('RewriteRule .* - [S=%d,%s]', $hasTrailingSlash ? 2 : 1, implode(',', $allow));
+        }
 
-            $rules[] = implode("\n", $rule);
+        // redirect with trailing slash appended
+        if ($hasTrailingSlash) {
+            $rule[] = 'RewriteCond %{REQUEST_URI} '.substr($regex, 0, -2).'$';
+            $rule[] = 'RewriteRule .* $0/ [QSA,L,R=301]';
         }
 
-        if (0 < count($methodVars)) {
-            $rule = array('# 405 Method Not Allowed');
-            $methodVars = array_values(array_unique($methodVars));
-            foreach ($methodVars as $i => $methodVar) {
-                $rule[] = sprintf('RewriteCond %%{_ROUTING__allow_%s} !-z%s', $methodVar, isset($methodVars[$i + 1]) ? ' [OR]' : '');
+        // the main rule
+        $rule[] = "RewriteCond %{REQUEST_URI} $regex";
+        $rule[] = "RewriteRule .* {$options['script_name']} [QSA,L,$variables]";
+
+        return implode("\n", $rule);
+    }
+
+    /**
+     * Returns methods allowed for a route
+     *
+     * @param Route  $route The route
+     *
+     * @return array The methods
+     */
+    private function getRouteMethods(Route $route)
+    {
+        $methods = array();
+        if ($req = $route->getRequirement('_method')) {
+            $methods = explode('|', strtoupper($req));
+            // GET and HEAD are equivalent
+            if (in_array('GET', $methods) && !in_array('HEAD', $methods)) {
+                $methods[] = 'HEAD';
             }
-            $rule[] = sprintf('RewriteRule .* %s [QSA,L]', $options['script_name']);
+        }
 
-            $rules[] = implode("\n", $rule);
+        return $methods;
+    }
+
+    /**
+     * Converts a regex to make it suitable for mod_rewrite
+     *
+     * @param string  $regex The regex
+     *
+     * @return string The converted regex
+     */
+    private function regexToApacheRegex($regex)
+    {
+        $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('/\?<.+?>/', '', substr($regex, 1, $regexPatternEnd - 1));
 
-        return implode("\n\n", $rules)."\n";
+        return $regex;
     }
 
     /**

diff --git a/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher1.apache b/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher1.apache
@@ -4,72 +4,72 @@ RewriteRule .* - [QSA,L]
 
 # foo
 RewriteCond %{REQUEST_URI} ^/foo/(baz|symfony)$
-RewriteRule .* app.php [QSA,L,E=_ROUTING__route:foo,E=_ROUTING_bar:%1,E=_ROUTING_DEFAULTS_def:test]
+RewriteRule .* app.php [QSA,L,E=_ROUTING_route:foo,E=_ROUTING_param_bar:%1,E=_ROUTING_default_def:test]
 
 # foobar
 RewriteCond %{REQUEST_URI} ^/foo(?:/([^/]++))?$
-RewriteRule .* app.php [QSA,L,E=_ROUTING__route:foobar,E=_ROUTING_bar:%1,E=_ROUTING_DEFAULTS_bar:toto]
+RewriteRule .* app.php [QSA,L,E=_ROUTING_route:foobar,E=_ROUTING_param_bar:%1,E=_ROUTING_default_bar:toto]
 
 # bar
 RewriteCond %{REQUEST_URI} ^/bar/([^/]++)$
 RewriteCond %{REQUEST_METHOD} !^(GET|HEAD)$ [NC]
-RewriteRule .* - [S=1,E=_ROUTING__allow_GET:1,E=_ROUTING__allow_HEAD:1]
+RewriteRule .* - [S=1,E=_ROUTING_allow_GET:1,E=_ROUTING_allow_HEAD:1]
 RewriteCond %{REQUEST_URI} ^/bar/([^/]++)$
-RewriteRule .* app.php [QSA,L,E=_ROUTING__route:bar,E=_ROUTING_foo:%1]
+RewriteRule .* app.php [QSA,L,E=_ROUTING_route:bar,E=_ROUTING_param_foo:%1]
 
 # baragain
 RewriteCond %{REQUEST_URI} ^/baragain/([^/]++)$
 RewriteCond %{REQUEST_METHOD} !^(GET|POST|HEAD)$ [NC]
-RewriteRule .* - [S=1,E=_ROUTING__allow_GET:1,E=_ROUTING__allow_POST:1,E=_ROUTING__allow_HEAD:1]
+RewriteRule .* - [S=1,E=_ROUTING_allow_GET:1,E=_ROUTING_allow_POST:1,E=_ROUTING_allow_HEAD:1]
 RewriteCond %{REQUEST_URI} ^/baragain/([^/]++)$
-RewriteRule .* app.php [QSA,L,E=_ROUTING__route:baragain,E=_ROUTING_foo:%1]
+RewriteRule .* app.php [QSA,L,E=_ROUTING_route:baragain,E=_ROUTING_param_foo:%1]
 
 # baz
 RewriteCond %{REQUEST_URI} ^/test/baz$
-RewriteRule .* app.php [QSA,L,E=_ROUTING__route:baz]
+RewriteRule .* app.php [QSA,L,E=_ROUTING_route:baz]
 
 # baz2
 RewriteCond %{REQUEST_URI} ^/test/baz\.html$
-RewriteRule .* app.php [QSA,L,E=_ROUTING__route:baz2]
+RewriteRule .* app.php [QSA,L,E=_ROUTING_route:baz2]
 
 # baz3
 RewriteCond %{REQUEST_URI} ^/test/baz3$
 RewriteRule .* $0/ [QSA,L,R=301]
 RewriteCond %{REQUEST_URI} ^/test/baz3/$
-RewriteRule .* app.php [QSA,L,E=_ROUTING__route:baz3]
+RewriteRule .* app.php [QSA,L,E=_ROUTING_route:baz3]
 
 # baz4
 RewriteCond %{REQUEST_URI} ^/test/([^/]++)$
 RewriteRule .* $0/ [QSA,L,R=301]
 RewriteCond %{REQUEST_URI} ^/test/([^/]++)/$
-RewriteRule .* app.php [QSA,L,E=_ROUTING__route:baz4,E=_ROUTING_foo:%1]
+RewriteRule .* app.php [QSA,L,E=_ROUTING_route:baz4,E=_ROUTING_param_foo:%1]
 
 # baz5
 RewriteCond %{REQUEST_URI} ^/test/([^/]++)/$
 RewriteCond %{REQUEST_METHOD} !^(GET|HEAD)$ [NC]
-RewriteRule .* - [S=2,E=_ROUTING__allow_GET:1,E=_ROUTING__allow_HEAD:1]
+RewriteRule .* - [S=2,E=_ROUTING_allow_GET:1,E=_ROUTING_allow_HEAD:1]
 RewriteCond %{REQUEST_URI} ^/test/([^/]++)$
 RewriteRule .* $0/ [QSA,L,R=301]
 RewriteCond %{REQUEST_URI} ^/test/([^/]++)/$
-RewriteRule .* app.php [QSA,L,E=_ROUTING__route:baz5,E=_ROUTING_foo:%1]
+RewriteRule .* app.php [QSA,L,E=_ROUTING_route:baz5,E=_ROUTING_param_foo:%1]
 
 # baz5unsafe
 RewriteCond %{REQUEST_URI} ^/testunsafe/([^/]++)/$
 RewriteCond %{REQUEST_METHOD} !^(POST)$ [NC]
-RewriteRule .* - [S=1,E=_ROUTING__allow_POST:1]
+RewriteRule .* - [S=1,E=_ROUTING_allow_POST:1]
 RewriteCond %{REQUEST_URI} ^/testunsafe/([^/]++)/$
-RewriteRule .* app.php [QSA,L,E=_ROUTING__route:baz5unsafe,E=_ROUTING_foo:%1]
+RewriteRule .* app.php [QSA,L,E=_ROUTING_route:baz5unsafe,E=_ROUTING_param_foo:%1]
 
 # baz6
 RewriteCond %{REQUEST_URI} ^/test/baz$
-RewriteRule .* app.php [QSA,L,E=_ROUTING__route:baz6,E=_ROUTING_DEFAULTS_foo:bar\ baz]
+RewriteRule .* app.php [QSA,L,E=_ROUTING_route:baz6,E=_ROUTING_default_foo:bar\ baz]
 
 # baz7
 RewriteCond %{REQUEST_URI} ^/te\ st/baz$
-RewriteRule .* app.php [QSA,L,E=_ROUTING__route:baz7]
+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]
-RewriteCond %{_ROUTING__allow_POST} !-z
+RewriteCond %{_ROUTING_allow_GET} !-z [OR]
+RewriteCond %{_ROUTING_allow_HEAD} !-z [OR]
+RewriteCond %{_ROUTING_allow_POST} !-z
 RewriteRule .* app.php [QSA,L]
diff --git a/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher2.apache b/src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher2.apache
@@ -4,4 +4,4 @@ RewriteRule .* - [QSA,L]
 
 # foo
 RewriteCond %{REQUEST_URI} ^/foo$
-RewriteRule .* ap\ p_d\ ev.php [QSA,L,E=_ROUTING__route:foo]
+RewriteRule .* ap\ p_d\ ev.php [QSA,L,E=_ROUTING_route:foo]