merged branch Seldaek/router_esc (PR #1471)

Commits
-------

418d6a0 [Routing] Fix syntax error when dumping routes with single quotes in the requirements or pattern
2b5e22d [Routing] Fix ApacheDumper when a space appears in a default value
6c7f484 [Routing] Fix dumper so it doesn't print trailing whitespace
761724a [Routing] Adjust urlescaping rules, fixes #752

Discussion
----------

[Router] Bunch o' Fixes

The first commit changes the escaping rule to fix issues I had previously, and #752 as well, here's from the full commit message:

    Only + and % are now encoded in generated routes, since they are the only characters that, if not encoded, could cause problems/conflicts when decoded. Namely + turns into a space, and % followed by numbers could do funky things.

    The matcher decodes everything which works since nothing will have %NN without being escaped, and + are escaped as well.

Second commit is just a test fix for the first

Third and fourth are simply dumper escaping issues, nothing to argue about.

Note that all changes have had test cases added, and I spent a few hours torturing/testing all this stuff with both Apache and PHP dumpers, in many browsers, and with URLs as wonky as `/%01%02%03%04%05%06%07%08%0E%0F%10%11%12%13%14%15%16%17%18%19%1A%1B%1C%1D%1E%1F%20!%22$%25&%27%28%29*+,-./0123456789:;%3C=%3E@ABCDEFGHIJKLMNOPQRSTUVWXYZ%5B%5D%5E_%60abcdefghijklmnopqrstuvwxyz%7B|%7D~/baz` which essentially represent the 1-255 char range minus ? and #.

The only issues I really encountered after all the patches were applied is that Apache refuses to match `%22` (= `"`) and `*` in a url. I guess it's just because they're not allowed chars in windows paths, but | and < > works fine though. Anyway this works with the PHP dumper, and it didn't work either without my patches so it's not like I broke it, I'm just saying for the record.

src/Symfony/Component/Routing/Generator/UrlGenerator.php

@@ -125,8 +125,7 @@ class UrlGenerator implements UrlGeneratorInterface
                     }
 
                     if (!$isEmpty || !$optional) {
-                        // %2F is not valid in a URL, so we don't encode it (which is fine as the requirements explicitly allowed it)
-                        $url = $token[1].str_replace('%2F', '/', rawurlencode($tparams[$token[3]])).$url;
+                        $url = $token[1].strtr($tparams[$token[3]], array('%'=>'%25', '+'=>'%2B')).$url;
                     }
 
                     $optional = false;

src/Symfony/Component/Routing/Matcher/Dumper/ApacheMatcherDumper.php

@@ -62,6 +62,7 @@ class ApacheMatcherDumper extends MatcherDumper
                     ':'  => '\\:',
                     '='  => '\\=',
                     '\\' => '\\\\',
+                    ' '  => '\\ ',
                 ));
             }
             $variables = implode(',', $variables);

src/Symfony/Component/Routing/Matcher/Dumper/PhpMatcherDumper.php

@@ -61,6 +61,7 @@ class PhpMatcherDumper extends MatcherDumper
     public function match(\$pathinfo)
     {
         \$allow = array();
+        \$pathinfo = urldecode(\$pathinfo);
 
 $code
         throw 0 < count(\$allow) ? new MethodNotAllowedException(array_unique(\$allow)) : new ResourceNotFoundException();
@@ -113,14 +114,18 @@ EOF;
                     }
 
                     if ($prefix !== $parentPrefix) {
-                        $code[] = sprintf("        if (0 === strpos(\$pathinfo, '%s')) {", $prefix);
+                        $code[] = sprintf("        if (0 === strpos(\$pathinfo, %s)) {", var_export($prefix, true));
                         $indent = '    ';
                     }
                 }
 
                 foreach ($this->compileRoutes($route, $supportsRedirections, $prefix) as $line) {
                     foreach (explode("\n", $line) as $l) {
-                        $code[] = $indent.$l;
+                        if ($l) {
+                            $code[] = $indent.$l;
+                        } else {
+                            $code[] = $l;
+                        }
                     }
                 }
 
@@ -146,14 +151,14 @@ EOF;
         $matches = false;
         if (!count($compiledRoute->getVariables()) && false !== preg_match('#^(.)\^(?P<url>.*?)\$\1#', str_replace(array("\n", ' '), '', $compiledRoute->getRegex()), $m)) {
             if ($supportsRedirections && substr($m['url'], -1) === '/') {
-                $conditions[] = sprintf("rtrim(\$pathinfo, '/') === '%s'", rtrim(str_replace('\\', '', $m['url']), '/'));
+                $conditions[] = sprintf("rtrim(\$pathinfo, '/') === %s", var_export(rtrim(str_replace('\\', '', $m['url']), '/'), true));
                 $hasTrailingSlash = true;
             } else {
-                $conditions[] = sprintf("\$pathinfo === '%s'", str_replace('\\', '', $m['url']));
+                $conditions[] = sprintf("\$pathinfo === %s", var_export(str_replace('\\', '', $m['url']), true));
             }
         } else {
             if ($compiledRoute->getStaticPrefix() && $compiledRoute->getStaticPrefix() != $parentPrefix) {
-                $conditions[] = sprintf("0 === strpos(\$pathinfo, '%s')", $compiledRoute->getStaticPrefix());
+                $conditions[] = sprintf("0 === strpos(\$pathinfo, %s)", var_export($compiledRoute->getStaticPrefix(), true));
             }
 
             $regex = str_replace(array("\n", ' '), '', $compiledRoute->getRegex());
@@ -161,7 +166,7 @@ EOF;
                 $regex = substr($regex, 0, $pos).'/?$'.substr($regex, $pos + 2);
                 $hasTrailingSlash = true;
             }
-            $conditions[] = sprintf("preg_match('%s', \$pathinfo, \$matches)", $regex);
+            $conditions[] = sprintf("preg_match(%s, \$pathinfo, \$matches)", var_export($regex, true));
 
             $matches = true;
         }

src/Symfony/Component/Routing/Matcher/UrlMatcher.php

@@ -93,6 +93,8 @@ class UrlMatcher implements UrlMatcherInterface
 
     protected function matchCollection($pathinfo, RouteCollection $routes)
     {
+        $pathinfo = urldecode($pathinfo);
+
         foreach ($routes as $name => $route) {
             if ($route instanceof RouteCollection) {
                 if (false === strpos($route->getPrefix(), '{') && $route->getPrefix() !== substr($pathinfo, 0, strlen($route->getPrefix()))) {

tests/Symfony/Tests/Component/Routing/Fixtures/dumper/url_matcher1.apache

@@ -49,6 +49,10 @@ RewriteRule .* $0/ [QSA,L,R=301]
 RewriteCond %{REQUEST_URI} ^/test/([^/]+?)/$
 RewriteRule .* app.php [QSA,L,E=_ROUTING__route:baz5,E=_ROUTING_foo:%1]
 
+# baz6
+RewriteCond %{REQUEST_URI} ^/test/baz$
+RewriteRule .* app.php [QSA,L,E=_ROUTING__route:baz6,E=_ROUTING_foo:bar\ baz]
+
 # 405 Method Not Allowed
 RewriteCond %{_ROUTING__allow_GET} !-z [OR]
 RewriteCond %{_ROUTING__allow_HEAD} !-z [OR]

tests/Symfony/Tests/Component/Routing/Fixtures/dumper/url_matcher1.php

@@ -23,6 +23,7 @@ class ProjectUrlMatcher extends Symfony\Component\Routing\Matcher\UrlMatcher
     public function match($pathinfo)
     {
         $allow = array();
+        $pathinfo = urldecode($pathinfo);
 
         // foo
         if (0 === strpos($pathinfo, '/foo') && preg_match('#^/foo/(?P<bar>baz|symfony)$#x', $pathinfo, $matches)) {
@@ -99,45 +100,51 @@ class ProjectUrlMatcher extends Symfony\Component\Routing\Matcher\UrlMatcher
             return array (  'def' => 'test',  '_route' => 'foofoo',);
         }
 
+        // quoter
+        if (preg_match('#^/(?P<quoter>[\']+)$#x', $pathinfo, $matches)) {
+            $matches['_route'] = 'quoter';
+            return $matches;
+        }
+
         if (0 === strpos($pathinfo, '/a')) {
-            if (0 === strpos($pathinfo, '/a/b')) {
+            if (0 === strpos($pathinfo, '/a/b\'b')) {
                 // foo
-                if (preg_match('#^/a/b/(?P<foo>[^/]+?)$#x', $pathinfo, $matches)) {
+                if (preg_match('#^/a/b\'b/(?P<foo>[^/]+?)$#x', $pathinfo, $matches)) {
                     $matches['_route'] = 'foo';
                     return $matches;
                 }
-        
+
                 // bar
-                if (preg_match('#^/a/b/(?P<bar>[^/]+?)$#x', $pathinfo, $matches)) {
+                if (preg_match('#^/a/b\'b/(?P<bar>[^/]+?)$#x', $pathinfo, $matches)) {
                     $matches['_route'] = 'bar';
                     return $matches;
                 }
-        
+
                 // foo1
-                if (preg_match('#^/a/b/(?P<foo1>[^/]+?)$#x', $pathinfo, $matches)) {
+                if (preg_match('#^/a/b\'b/(?P<foo1>[^/]+?)$#x', $pathinfo, $matches)) {
                     $matches['_route'] = 'foo1';
                     return $matches;
                 }
-        
+
                 // bar1
-                if (preg_match('#^/a/b/(?P<bar1>[^/]+?)$#x', $pathinfo, $matches)) {
+                if (preg_match('#^/a/b\'b/(?P<bar1>[^/]+?)$#x', $pathinfo, $matches)) {
                     $matches['_route'] = 'bar1';
                     return $matches;
                 }
-        
+
             }
-    
+
             // ababa
             if ($pathinfo === '/ababa') {
                 return array('_route' => 'ababa');
             }
-    
+
             // foo
             if (preg_match('#^/aba/(?P<foo>[^/]+?)$#x', $pathinfo, $matches)) {
                 $matches['_route'] = 'foo';
                 return $matches;
             }
-    
+
         }
 
         // foo

tests/Symfony/Tests/Component/Routing/Fixtures/dumper/url_matcher2.php

@@ -23,6 +23,7 @@ class ProjectUrlMatcher extends Symfony\Tests\Component\Routing\Fixtures\Redirec
     public function match($pathinfo)
     {
         $allow = array();
+        $pathinfo = urldecode($pathinfo);
 
         // foo
         if (0 === strpos($pathinfo, '/foo') && preg_match('#^/foo/(?P<bar>baz|symfony)$#x', $pathinfo, $matches)) {
@@ -111,45 +112,51 @@ class ProjectUrlMatcher extends Symfony\Tests\Component\Routing\Fixtures\Redirec
             return array (  'def' => 'test',  '_route' => 'foofoo',);
         }
 
+        // quoter
+        if (preg_match('#^/(?P<quoter>[\']+)$#x', $pathinfo, $matches)) {
+            $matches['_route'] = 'quoter';
+            return $matches;
+        }
+
         if (0 === strpos($pathinfo, '/a')) {
-            if (0 === strpos($pathinfo, '/a/b')) {
+            if (0 === strpos($pathinfo, '/a/b\'b')) {
                 // foo
-                if (preg_match('#^/a/b/(?P<foo>[^/]+?)$#x', $pathinfo, $matches)) {
+                if (preg_match('#^/a/b\'b/(?P<foo>[^/]+?)$#x', $pathinfo, $matches)) {
                     $matches['_route'] = 'foo';
                     return $matches;
                 }
-        
+
                 // bar
-                if (preg_match('#^/a/b/(?P<bar>[^/]+?)$#x', $pathinfo, $matches)) {
+                if (preg_match('#^/a/b\'b/(?P<bar>[^/]+?)$#x', $pathinfo, $matches)) {
                     $matches['_route'] = 'bar';
                     return $matches;
                 }
-        
+
                 // foo1
-                if (preg_match('#^/a/b/(?P<foo1>[^/]+?)$#x', $pathinfo, $matches)) {
+                if (preg_match('#^/a/b\'b/(?P<foo1>[^/]+?)$#x', $pathinfo, $matches)) {
                     $matches['_route'] = 'foo1';
                     return $matches;
                 }
-        
+
                 // bar1
-                if (preg_match('#^/a/b/(?P<bar1>[^/]+?)$#x', $pathinfo, $matches)) {
+                if (preg_match('#^/a/b\'b/(?P<bar1>[^/]+?)$#x', $pathinfo, $matches)) {
                     $matches['_route'] = 'bar1';
                     return $matches;
                 }
-        
+
             }
-    
+
             // ababa
             if ($pathinfo === '/ababa') {
                 return array('_route' => 'ababa');
             }
-    
+
             // foo
             if (preg_match('#^/aba/(?P<foo>[^/]+?)$#x', $pathinfo, $matches)) {
                 $matches['_route'] = 'foo';
                 return $matches;
             }
-    
+
         }
 
         // foo

tests/Symfony/Tests/Component/Routing/Matcher/Dumper/ApacheMatcherDumperTest.php

@@ -68,6 +68,11 @@ class ApacheMatcherDumperTest extends \PHPUnit_Framework_TestCase
             array(),
             array('_method' => 'post')
         ));
+        // complex
+        $collection->add('baz6', new Route(
+            '/test/baz',
+            array('foo' => 'bar baz')
+        ));
 
         $dumper = new ApacheMatcherDumper($collection);
 

tests/Symfony/Tests/Component/Routing/Matcher/Dumper/PhpMatcherDumperTest.php

@@ -73,17 +73,23 @@ class PhpMatcherDumperTest extends \PHPUnit_Framework_TestCase
             '/foofoo',
             array('def' => 'test')
         ));
+        // pattern with quotes
+        $collection->add('quoter', new Route(
+            '/{quoter}',
+            array(),
+            array('quoter' => '[\']+')
+        ));
 
         // prefixes
         $collection1 = new RouteCollection();
         $collection1->add('foo', new Route('/{foo}'));
         $collection1->add('bar', new Route('/{bar}'));
         $collection2 = new RouteCollection();
-        $collection2->addCollection($collection1, '/b');
+        $collection2->addCollection($collection1, '/b\'b');
         $collection1 = new RouteCollection();
         $collection1->add('foo1', new Route('/{foo1}'));
         $collection1->add('bar1', new Route('/{bar1}'));
-        $collection2->addCollection($collection1, '/b');
+        $collection2->addCollection($collection1, '/b\'b');
         $collection->addCollection($collection2, '/a');
 
         // "dynamic" prefix

tests/Symfony/Tests/Component/Routing/Matcher/UrlMatcherTest.php

@@ -150,6 +150,17 @@ class UrlMatcherTest extends \PHPUnit_Framework_TestCase
         $this->assertEquals(array('_locale' => 'fr', '_route' => 'foo', 'foo' => 'foo'), $matcher->match('/fr/b/foo'));
     }
 
+    public function testMatchNonAlpha()
+    {
+        $collection = new RouteCollection();
+        $chars = '!"$%éà &\'()*+,./:;<=>@ABCDEFGHIJKLMNOPQRSTUVWXYZ\\[]^_`abcdefghijklmnopqrstuvwxyz{|}~-';
+        $collection->add('foo', new Route('/{foo}/bar', array(), array('foo' => '['.preg_quote($chars).']+')));
+
+        $matcher = new UrlMatcher($collection, new RequestContext(), array());
+        $this->assertEquals(array('_route' => 'foo', 'foo' => $chars), $matcher->match('/'.urlencode($chars).'/bar'));
+        $this->assertEquals(array('_route' => 'foo', 'foo' => $chars), $matcher->match('/'.strtr($chars, array('%' => '%25', '+' => '%2B')).'/bar'));
+    }
+
     public function testMatchRegression()
     {
         $coll = new RouteCollection();