8000 [RFC][Routing] Improve the current matching process by vicb · Pull Request #3678 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[RFC][Routing] Improve the current matching process #3678

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

[RFC][Routing] Improve the current matching process #3678

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fcff74d
[Routing] Fix separators & optimize regexp
vicb Mar 20, 2012
cb6afd7
[Routing] Update tests according to the previous commit
vicb Mar 20, 2012
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 1 addition & 5 deletions src/Symfony/Component/Routing/RouteCompiler.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,17 +37,13 @@ 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 {
if ($pos !== $len) {
$seps[] = $pattern[$pos];
}
$regexp = sprintf('[^%s]+?', preg_quote(implode('', array_unique($seps)), '#'));
$regexp = $pos === $len ? '.+' : sprintf('[^%s]+', preg_quote($pattern[$pos], '#'));
}

$tokens[] = array('variable', $match[0][0][0], $regexp, $var);
Expand Down
22 changes: 11 additions & 11 deletions src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher1.apache
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,17 +7,17 @@ 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]

# baragain
RewriteCond %{REQUEST_URI} ^/baragain/([^/]+? 8000 )$
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]
RewriteCond %{REQUEST_URI} ^/baragain/([^/]+?)$
RewriteCond %{REQUEST_URI} ^/baragain/(.+)$
RewriteRule .* app.php [QSA,L,E=_ROUTING__route:baragain,E=_ROUTING_foo:%1]

# baz
Expand All @@ -35,25 +35,25 @@ 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} !^(GET|HEAD)$ [NC]
RewriteRule .* - [S=2,E=_ROUTING__allow_GET:1,E=_ROUTING__allow_HEAD: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]

# baz5unsafe
RewriteCond %{REQUEST_URI} ^/testunsafe/([^/]+?)/$
RewriteCond %{REQUEST_URI} ^/testunsafe/([^/]+)/$
RewriteCond %{REQUEST_METHOD} !^(POST)$ [NC]
RewriteRule .* - [S=1,E=_ROUTING__allow_POST:1]
RewriteCond %{REQUEST_URI} ^/testunsafe/([^/]+?)/$
RewriteCond %{REQUEST_URI} ^/testunsafe/([^/]+)/$
RewriteRule .* app.php [QSA,L,E=_ROUTING__route:baz5unsafe,E=_ROUTING_foo:%1]

# baz6
Expand Down
24 changes: 12 additions & 12 deletions src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher1.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ public function match($pathinfo)
}

// bar
if (0 === strpos($pathinfo, '/bar') && preg_match('#^/bar/(?P<foo>[^/]+?)$#xs', $pathinfo, $matches)) {
if (0 === strpos($pathinfo, '/bar') && preg_match('#^/bar/(?P<foo>.+)$#xs', $pathinfo, $matches)) {
if (!in_array($this->context->getMethod(), array('GET', 'HEAD'))) {
$allow = array_merge($allow, array('GET', 'HEAD'));
goto not_bar;
Expand All @@ -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>.+)$#xs', $pathinfo, $matches)) {
if (!in_array($this->context->getMethod(), array('GET', 'HEAD'))) {
$allow = array_merge($allow, array('GET', 'HEAD'));
goto not_barhead;
Expand All @@ -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>[^/]+)/$#xs', $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>[^/]+)/$#xs', $pathinfo, $matches)) {
if ($this->context->getMethod() != 'POST') {
$allow[] = 'POST';
goto not_baz5;
Expand All @@ -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>[^/]+)/$#xs', $pathinfo, $matches)) {
if ($this->context->getMethod() != 'PUT') {
$allow[] = 'PUT';
goto not_bazbaz6;
Expand All @@ -109,25 +109,25 @@ public function match($pathinfo)
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>.+)$#xs', $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>.+)$#xs', $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>.+)$#xs', $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>.+)$#xs', $pathinfo, $matches)) {
$matches['_route'] = 'bar2';
return $matches;
}
Expand All @@ -145,21 +145,21 @@ public function match($pathinfo)
}

// foo4
if (preg_match('#^/aba/(?P<foo>[^/]+?)$#xs', $pathinfo, $matches)) {
if (preg_match('#^/aba/(?P<foo>.+)$#xs', $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>.+)$#xs', $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>.+)$#xs', $pathinfo, $matches)) {
$matches['_route'] = 'bar3';
return $matches;
}
Expand Down
24 changes: 12 additions & 12 deletions src/Symfony/Component/Routing/Tests/Fixtures/dumper/url_matcher2.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ public function match($pathinfo)
}

// bar
if (0 === strpos($pathinfo, '/bar') && preg_match('#^/bar/(?P<foo>[^/]+?)$#xs', $pathinfo, $matches)) {
if (0 === strpos($pathinfo, '/bar') && preg_match('#^/bar/(?P<foo>.+)$#xs', $pathinfo, $matches)) {
if (!in_array($this->context->getMethod(), array('GET', 'HEAD'))) {
$allow = array_merge($allow, array('GET', 'HEAD'));
goto not_bar;
Expand All @@ -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>.+)$#xs', $pathinfo, $matches)) {
if (!in_array($this->context->getMethod(), array('GET', 'HEAD'))) {
$allow = array_merge($allow, array('GET', 'HEAD'));
goto not_barhead;
Expand Down Expand Up @@ -71,7 +71,7 @@ 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>[^/]+)/?$#xs', $pathinfo, $matches)) {
if (substr($pathinfo, -1) !== '/') {
return $this->redirect($pathinfo.'/', 'baz4');
}
Expand All @@ -80,7 +80,7 @@ public function match($pathinfo)
}

// baz5
if (0 === strpos($pathinfo, '/test') && preg_match('#^/test/(?P<foo>[^/]+?)/$#xs', $pathinfo, $matches)) {
if (0 === strpos($pathinfo, '/test') && preg_match('#^/test/(?P<foo>[^/]+)/$#xs', $pathinfo, $matches)) {
if ($this->context->getMethod() != 'POST') {
$allow[] = 'POST';
goto not_baz5;
Expand All @@ -91,7 +91,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>[^/]+)/$#xs', $pathinfo, $matches)) {
if ($this->context->getMethod() != 'PUT') {
$allow[] = 'PUT';
goto not_bazbaz6;
Expand All @@ -115,25 +115,25 @@ public function match($pathinfo)
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>.+)$#xs', $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>.+)$#xs', $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>.+)$#xs', $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>.+)$#xs', $pathinfo, $matches)) {
$matches['_route'] = 'bar2';
return $matches;
}
Expand All @@ -151,21 +151,21 @@ public function match($pathinfo)
}

// foo4
if (preg_match('#^/aba/(?P<foo>[^/]+?)$#xs', $pathinfo, $matches)) {
if (preg_match('#^/aba/(?P<foo>.+)$#xs', $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>.+)$#xs', $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>.+)$#xs', $pathinfo, $matches)) {
$matches['_route'] = 'bar3';
return $matches;
}
Expand Down
15 changes: 13 additions & 2 deletions src/Symfony/Component/Routing/Tests/Matcher/UrlMatcherTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,14 +187,14 @@ public function testMatchOverridenRoute()
$this->assertEquals(array(), $matcher->match('/foo'));
}

public function testMatchRegression()
public function testMatchWhenNoSepatorSpecified()
{
$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'));
$this->assertEquals(array('foo' => 'bar/bar', '_route' => 'foo'), $matcher->match('/foo/bar/bar'));

$collection = new RouteCollection();
$collection->add('foo', new Route('/{bar}'));
Expand All @@ -216,4 +216,15 @@ public function testSchemeRequirement()
$matcher = new UrlMatcher($coll, new RequestContext());
$matcher->match('/foo');
}

public function testSeparators()
{
$coll = new RouteCollection();
$coll->add('foo', new Route('/foo-{bar}-{baz}'));
$matcher = new UrlMatcher($coll, new RequestContext());
$this->assertEquals(
array('bar' => 'b/ar', 'baz' => 'b-az', '_route' => 'foo'),
$matcher->match('/foo-b/ar-b-az')
);
}
}
39 changes: 24 additions & 15 deletions src/Symfony/Component/Routing/Tests/RouteCompilerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,51 +43,51 @@ public function provideCompileData()
array(
'Route with a variable',
array('/foo/{bar}'),
'/foo', '#^/foo/(?P<bar>[^/]+?)$#xs', array('bar'), array(
array('variable', '/', '[^/]+?', 'bar'),
'/foo', '#^/foo/(?P<bar>.+)$#xs', 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>[^/]+?))?$#xs', array('bar'), array(
array('variable', '/', '[^/]+?', 'bar'),
'/foo', '#^/foo(?:/(?P<bar>.+))?$#xs', array('bar'), array(
array('variable', '/', '.+', 'bar'),
array('text', '/foo'),
)),

array(
'Route with several variables',
array('/foo/{bar}/{foobar}'),
'/foo', '#^/foo/(?P<bar>[^/]+?)/(?P<foobar>[^/]+?)$#xs', array('bar', 'foobar'), array(
array('variable', '/', '[^/]+?', 'foobar'),
array('variable', '/', '[^/]+?', 'bar'),
'/foo', '#^/foo/(?P<bar>[^/]+)/(?P<foobar>.+)$#xs', 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>[^/]+?))?)?$#xs', array('bar', 'foobar'), array(
array('variable', '/', '[^/]+?', 'foobar'),
array('variable', '/', '[^/]+?', 'bar'),
'/foo', '#^/foo(?:/(?P<bar>[^/]+)(?:/(?P<foobar>.+))?)?$#xs', 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>[^/]+?)$#xs', array('bar', 'foobar'), array(
array('variable', '/', '[^/]+?', 'foobar'),
array('variable', '/', '[^/]+?', 'bar'),
'/foo', '#^/foo/(?P<bar>[^/]+)/(?P<foobar>.+)$#xs', 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>[^/]+?))?$#xs', array('bar'), array(
array('variable', '/', '[^/]+?', 'bar'),
'', '#^/(?:(?P<bar>.+))?$#xs', array('bar'), array(
array('variable', '/', '.+', 'bar'),
)),

array(
Expand All @@ -96,6 +96,15 @@ public function provideCompileData()
'', '#^/(?:(?P<bar>(foo|bar)))?$#xs', array('bar'), array(
array('variable', '/', '(foo|bar)', 'bar'),
)),

array(
'Route with different separators',
array('/foo-{bar}-{baz}'),
'/foo', '#^/foo\-(?P<bar>[^\-]+)\-(?P<baz>.+)$#xs', array('bar', 'baz'), array(
array('variable', '-', '.+', 'baz'),
array('variable', '-', '[^\-]+', 'bar'),
array('text', '/foo'),
)),
);
}

Expand Down
0