Skip to content

Commit

Permalink
[Routing] fixed regression in Routing matching algorithm
Browse files Browse the repository at this point in the history
  • Loading branch information
@fabpot
fabpot committed Apr 26, 2011
1 parent ee89163 commit 035afc1
Show file tree
Hide file tree
Showing 6 changed files with 29 additions and 15 deletions.
6 changes: 5 additions & 1 deletion src/Symfony/Component/Routing/RouteCompiler.php
View file
Expand Up @@ -37,13 +37,17 @@ public function compile(Route $route)
if ($text = substr($pattern, $pos, $match[0][1] - $pos)) {
$tokens[] = array('text', $text);
}
$seps = array($pattern[$pos]);
$pos = $match[0][1] + strlen($match[0][0]);
$var = $match[1][0];

if ($req = $route->getRequirement($var)) {
$regexp = $req;
} else {
$regexp = $pos !== $len ? sprintf('[^%s]*?', $pattern[$pos]) : '.*?';
if ($pos !== $len) {
$seps[] = $pattern[$pos];
}
$regexp = sprintf('[^%s]*?', preg_quote(implode('', array_unique($seps)), '#'));
}

$tokens[] = array('variable', $match[0][0][0], $regexp, $var);
Expand Down
4 changes: 2 additions & 2 deletions tests/Symfony/Tests/Component/Routing/Fixtures/dumper/url_matcher1.apache
View file
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion tests/Symfony/Tests/Component/Routing/Fixtures/dumper/url_matcher1.php
View file
Expand Up @@ -30,7 +30,7 @@ public function match($pathinfo)
}

// 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;
Expand Down
2 changes: 1 addition & 1 deletion tests/Symfony/Tests/Component/Routing/Fixtures/dumper/url_matcher2.php
View file
Expand Up @@ -30,7 +30,7 @@ public function match($pathinfo)
}

// 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;
Expand Down
10 changes: 10 additions & 0 deletions tests/Symfony/Tests/Component/Routing/Matcher/UrlMatcherTest.php
View file
Expand Up @@ -97,4 +97,14 @@ public function testMatch()
$matcher = new UrlMatcher($collection, new RequestContext('', 'head'), array());
$this->assertInternalType('array', $matcher->match('/foo'));
}

public function testMatchRegression()
{
$coll = new RouteCollection();
$coll->add('foo', new Route('/foo/{foo}'));
$coll->add('bar', new Route('/foo/bar/{foo}'));

$matcher = new UrlMatcher($coll, new RequestContext());
$this->assertEquals(array('foo' => 'bar', '_route' => 'bar'), $matcher->match('/foo/bar/bar'));
}
}
20 changes: 10 additions & 10 deletions tests/Symfony/Tests/Component/Routing/RouteCompilerTest.php
View file
Expand Up @@ -43,42 +43,42 @@ public function provideCompileData()
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'),
'/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'),
'/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'),
'/foo', '#^/foo/(?P<bar>[^/]*?)/(?P<foobar>[^/]*?)$#x', array('bar', 'foobar'), array(
array('variable', '/', '[^/]*?', 'foobar'),
array('variable', '/', '[^/]*?', 'bar'),
array('text', '/foo'),
)),
Expand Down

0 comments on commit 035afc1

Please sign in to comment.