[Routing] fixed regression (/ should not be matched by /{foo} when foo has no default value)

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

@@ -105,12 +105,14 @@ class UrlGenerator implements UrlGeneratorInterface
         foreach ($tokens as $token) {
             if ('variable' === $token[0]) {
                 if (false === $optional || !isset($defaults[$token[3]]) || (isset($parameters[$token[3]]) && $parameters[$token[3]] != $defaults[$token[3]])) {
-                    // check requirement
-                    if (!preg_match('#^'.$token[2].'$#', $tparams[$token[3]])) {
-                        throw new \InvalidArgumentException(sprintf('Parameter "%s" for route "%s" must match "%s" ("%s" given).', $token[3], $name, $token[2], $tparams[$token[3]]));
+                    if (!$isEmpty = in_array($tparams[$token[3]], array(null, '', false), true)) {
+                        // check requirement
+                        if ($tparams[$token[3]] && !preg_match('#^'.$token[2].'$#', $tparams[$token[3]])) {
+                            throw new \InvalidArgumentException(sprintf('Parameter "%s" for route "%s" must match "%s" ("%s" given).', $token[3], $name, $token[2], $tparams[$token[3]]));
+                        }
                     }
 
-                    if (!in_array($tparams[$token[3]], array(null, '', false), true) || !$optional) {
+                    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', '/', urlencode($tparams[$token[3]])).$url;
                     }

src/Symfony/Component/Routing/RouteCompiler.php

@@ -47,7 +47,7 @@ class RouteCompiler implements RouteCompilerInterface
                 if ($pos !== $len) {
                     $seps[] = $pattern[$pos];
                 }
-                $regexp = sprintf('[^%s]*?', preg_quote(implode('', array_unique($seps)), '#'));
+                $regexp = sprintf('[^%s]+?', preg_quote(implode('', array_unique($seps)), '#'));
             }
 
             $tokens[] = array('variable', $match[0][0][0], $regexp, $var);

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

@@ -7,10 +7,10 @@ RewriteCond %{REQUEST_URI} ^/foo/(baz|symfony)$
 RewriteRule .* app.php [QSA,L,E=_ROUTING__route:foo,E=_ROUTING_bar:%1,E=_ROUTING_def:test]
 
 # bar
-RewriteCond %{REQUEST_URI} ^/bar/([^/]*?)$
+RewriteCond %{REQUEST_URI} ^/bar/([^/]+?)$
 RewriteCond %{REQUEST_METHOD} !^(get|head)$ [NC]
 RewriteRule .* - [S=1,E=_ROUTING__allow_get:1,E=_ROUTING__allow_head:1]
-RewriteCond %{REQUEST_URI} ^/bar/([^/]*?)$
+RewriteCond %{REQUEST_URI} ^/bar/([^/]+?)$
 RewriteRule .* app.php [QSA,L,E=_ROUTING__route:bar,E=_ROUTING_foo:%1]
 
 # baz
@@ -28,18 +28,18 @@ RewriteCond %{REQUEST_URI} ^/test/baz3/$
 RewriteRule .* app.php [QSA,L,E=_ROUTING__route:baz3]
 
 # baz4
-RewriteCond %{REQUEST_URI} ^/test/([^/]*?)$
+RewriteCond %{REQUEST_URI} ^/test/([^/]+?)$
 RewriteRule .* $0/ [QSA,L,R=301]
-RewriteCond %{REQUEST_URI} ^/test/([^/]*?)/$
+RewriteCond %{REQUEST_URI} ^/test/([^/]+?)/$
 RewriteRule .* app.php [QSA,L,E=_ROUTING__route:baz4,E=_ROUTING_foo:%1]
 
 # baz5
-RewriteCond %{REQUEST_URI} ^/test/([^/]*?)/$
+RewriteCond %{REQUEST_URI} ^/test/([^/]+?)/$
 RewriteCond %{REQUEST_METHOD} !^(post)$ [NC]
 RewriteRule .* - [S=2,E=_ROUTING__allow_post:1]
-RewriteCond %{REQUEST_URI} ^/test/([^/]*?)$
+RewriteCond %{REQUEST_URI} ^/test/([^/]+?)$
 RewriteRule .* $0/ [QSA,L,R=301]
-RewriteCond %{REQUEST_URI} ^/test/([^/]*?)/$
+RewriteCond %{REQUEST_URI} ^/test/([^/]+?)/$
 RewriteRule .* app.php [QSA,L,E=_ROUTING__route:baz5,E=_ROUTING_foo:%1]
 
 # 405 Method Not Allowed

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

@@ -30,7 +30,7 @@ class ProjectUrlMatcher extends Symfony\Component\Routing\Matcher\UrlMatcher
         }
 
         // bar
-        if (0 === strpos($pathinfo, '/bar') && preg_match('#^/bar/(?P<foo>[^/]*?)$#x', $pathinfo, $matches)) {
+        if (0 === strpos($pathinfo, '/bar') && preg_match('#^/bar/(?P<foo>[^/]+?)$#x', $pathinfo, $matches)) {
             if (!in_array($this->context->getMethod(), array('get', 'head'))) {
                 $allow = array_merge($allow, array('get', 'head'));
                 goto not_bar;
@@ -56,13 +56,13 @@ class ProjectUrlMatcher extends Symfony\Component\Routing\Matcher\UrlMatcher
         }
 
         // baz4
-        if (0 === strpos($pathinfo, '/test') && preg_match('#^/test/(?P<foo>[^/]*?)/$#x', $pathinfo, $matches)) {
+        if (0 === strpos($pathinfo, '/test') && preg_match('#^/test/(?P<foo>[^/]+?)/$#x', $pathinfo, $matches)) {
             $matches['_route'] = 'baz4';
             return $matches;
         }
 
         // baz5
-        if (0 === strpos($pathinfo, '/test') && preg_match('#^/test/(?P<foo>[^/]*?)/$#x', $pathinfo, $matches)) {
+        if (0 === strpos($pathinfo, '/test') && preg_match('#^/test/(?P<foo>[^/]+?)/$#x', $pathinfo, $matches)) {
             if ($this->context->getMethod() != 'post') {
                 $allow[] = 'post';
                 goto not_baz5;
@@ -73,7 +73,7 @@ class ProjectUrlMatcher extends Symfony\Component\Routing\Matcher\UrlMatcher
         not_baz5:
 
         // baz.baz6
-        if (0 === strpos($pathinfo, '/test') && preg_match('#^/test/(?P<foo>[^/]*?)/$#x', $pathinfo, $matches)) {
+        if (0 === strpos($pathinfo, '/test') && preg_match('#^/test/(?P<foo>[^/]+?)/$#x', $pathinfo, $matches)) {
             if ($this->context->getMethod() != 'put') {
                 $allow[] = 'put';
                 goto not_bazbaz6;

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

@@ -30,7 +30,7 @@ class ProjectUrlMatcher extends Symfony\Tests\Component\Routing\Fixtures\Redirec
         }
 
         // bar
-        if (0 === strpos($pathinfo, '/bar') && preg_match('#^/bar/(?P<foo>[^/]*?)$#x', $pathinfo, $matches)) {
+        if (0 === strpos($pathinfo, '/bar') && preg_match('#^/bar/(?P<foo>[^/]+?)$#x', $pathinfo, $matches)) {
             if (!in_array($this->context->getMethod(), array('get', 'head'))) {
                 $allow = array_merge($allow, array('get', 'head'));
                 goto not_bar;
@@ -59,7 +59,7 @@ class ProjectUrlMatcher extends Symfony\Tests\Component\Routing\Fixtures\Redirec
         }
 
         // baz4
-        if (0 === strpos($pathinfo, '/test') && preg_match('#^/test/(?P<foo>[^/]*?)/?$#x', $pathinfo, $matches)) {
+        if (0 === strpos($pathinfo, '/test') && preg_match('#^/test/(?P<foo>[^/]+?)/?$#x', $pathinfo, $matches)) {
             if (substr($pathinfo, -1) !== '/') {
                 return $this->redirect($pathinfo.'/', 'baz4');
             }
@@ -68,7 +68,7 @@ class ProjectUrlMatcher extends Symfony\Tests\Component\Routing\Fixtures\Redirec
         }
 
         // baz5
-        if (0 === strpos($pathinfo, '/test') && preg_match('#^/test/(?P<foo>[^/]*?)/?$#x', $pathinfo, $matches)) {
+        if (0 === strpos($pathinfo, '/test') && preg_match('#^/test/(?P<foo>[^/]+?)/?$#x', $pathinfo, $matches)) {
             if ($this->context->getMethod() != 'post') {
                 $allow[] = 'post';
                 goto not_baz5;
@@ -82,7 +82,7 @@ class ProjectUrlMatcher extends Symfony\Tests\Component\Routing\Fixtures\Redirec
         not_baz5:
 
         // baz.baz6
-        if (0 === strpos($pathinfo, '/test') && preg_match('#^/test/(?P<foo>[^/]*?)/?$#x', $pathinfo, $matches)) {
+        if (0 === strpos($pathinfo, '/test') && preg_match('#^/test/(?P<foo>[^/]+?)/?$#x', $pathinfo, $matches)) {
             if ($this->context->getMethod() != 'put') {
                 $allow[] = 'put';
                 goto not_bazbaz6;

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

@@ -119,5 +119,14 @@ class UrlMatcherTest extends \PHPUnit_Framework_TestCase
 
         $matcher = new UrlMatcher($coll, new RequestContext());
         $this->assertEquals(array('foo' => 'bar', '_route' => 'bar'), $matcher->match('/foo/bar/bar'));
+
+        $collection = new RouteCollection();
+        $collection->add('foo', new Route('/{bar}'));
+        $matcher = new UrlMatcher($collection, new RequestContext(), array());
+        try {
+            $matcher->match('/');
+            $this->fail();
+        } catch (NotFoundException $e) {
+        }
     }
 }

tests/Symfony/Tests/Component/Routing/RouteCompilerTest.php

@@ -43,51 +43,51 @@ class RouteCompilerTest extends \PHPUnit_Framework_TestCase
             array(
                 'Route with a variable',
                 array('/foo/{bar}'),
-                '/foo', '#^/foo/(?P<bar>[^/]*?)$#x', array('bar'), array(
-                    array('variable', '/', '[^/]*?', 'bar'),
+                '/foo', '#^/foo/(?P<bar>[^/]+?)$#x', array('bar'), array(
+                    array('variable', '/', '[^/]+?', 'bar'),
                     array('text', '/foo'),
                 )),
 
             array(
                 'Route with a variable that has a default value',
                 array('/foo/{bar}', array('bar' => 'bar')),
-                '/foo', '#^/foo(?:/(?P<bar>[^/]*?))?$#x', array('bar'), array(
-                    array('variable', '/', '[^/]*?', 'bar'),
+                '/foo', '#^/foo(?:/(?P<bar>[^/]+?))?$#x', array('bar'), array(
+                    array('variable', '/', '[^/]+?', 'bar'),
                     array('text', '/foo'),
                 )),
 
             array(
                 'Route with several variables',
                 array('/foo/{bar}/{foobar}'),
-                '/foo', '#^/foo/(?P<bar>[^/]*?)/(?P<foobar>[^/]*?)$#x', array('bar', 'foobar'), array(
-                    array('variable', '/', '[^/]*?', 'foobar'),
-                    array('variable', '/', '[^/]*?', 'bar'),
+                '/foo', '#^/foo/(?P<bar>[^/]+?)/(?P<foobar>[^/]+?)$#x', array('bar', 'foobar'), array(
+                    array('variable', '/', '[^/]+?', 'foobar'),
+                    array('variable', '/', '[^/]+?', 'bar'),
                     array('text', '/foo'),
                 )),
 
             array(
                 'Route with several variables that have default values',
                 array('/foo/{bar}/{foobar}', array('bar' => 'bar', 'foobar' => '')),
-                '/foo', '#^/foo(?:/(?P<bar>[^/]*?)(?:/(?P<foobar>[^/]*?))?)?$#x', array('bar', 'foobar'), array(
-                    array('variable', '/', '[^/]*?', 'foobar'),
-                    array('variable', '/', '[^/]*?', 'bar'),
+                '/foo', '#^/foo(?:/(?P<bar>[^/]+?)(?:/(?P<foobar>[^/]+?))?)?$#x', array('bar', 'foobar'), array(
+                    array('variable', '/', '[^/]+?', 'foobar'),
+                    array('variable', '/', '[^/]+?', 'bar'),
                     array('text', '/foo'),
                 )),
 
             array(
                 'Route with several variables but some of them have no default values',
                 array('/foo/{bar}/{foobar}', array('bar' => 'bar')),
-                '/foo', '#^/foo/(?P<bar>[^/]*?)/(?P<foobar>[^/]*?)$#x', array('bar', 'foobar'), array(
-                    array('variable', '/', '[^/]*?', 'foobar'),
-                    array('variable', '/', '[^/]*?', 'bar'),
+                '/foo', '#^/foo/(?P<bar>[^/]+?)/(?P<foobar>[^/]+?)$#x', array('bar', 'foobar'), array(
+                    array('variable', '/', '[^/]+?', 'foobar'),
+                    array('variable', '/', '[^/]+?', 'bar'),
                     array('text', '/foo'),
                 )),
 
             array(
                 'Route with an optional variable as the first segment',
                 array('/{bar}', array('bar' => 'bar')),
-                '', '#^/(?:(?P<bar>[^/]*?))?$#x', array('bar'), array(
-                    array('variable', '/', '[^/]*?', 'bar'),
+                '', '#^/(?:(?P<bar>[^/]+?))?$#x', array('bar'), array(
+                    array('variable', '/', '[^/]+?', 'bar'),
                 )),
 
             array(