merged branch lmcd/master (PR #1290)
Commits -------49dd558
Couple more CS fixes5a986bf
Add $keysCount & minor CS fix91f4097
[Routing] Better nesting for RouteCollections in dumped URL matcher classes With this change, a route prefixed with '/blogger' will be nested inside '/blog' (for example) Discussion ---------- [Routing] Better nesting for RouteCollections in dumped URL matcher classes Consider the following routing file: _blog: resource: "@AcmeDemoBundle/Resources/config/routing/BlogController.yml" prefix: /blog _blogger: resource: "@AcmeDemoBundle/Resources/config/routing/BloggerController.yml" prefix: /blogger _bloggeroo: resource: "@AcmeDemoBundle/Resources/config/routing/BloggerooController.yml" prefix: /bloggeroo _blogtest: pattern: /blogtest defaults: { _controller: AcmeDemoBundle:Blog:test } login: pattern: /login defaults: { _controller: AcmeDemoBundle:Security:login } Note: Each imported file contains two simple blog/blogger/bloggeroo routes (e.g. blog_index & blog_view) With this change, the cached URL matcher looks something like this: ```php <?php class appprodUrlMatcher { public function match($pathinfo) { $allow = array(); if (0 === strpos($pathinfo, '/blog')) { // blog_index if (preg_match('#^/blog/(?P<slug>[^/]+?)$#x', $pathinfo, $matches)) {...} // blog_view if (preg_match('#^/blog/(?P<year>\d{4})/(?P<month>\d{2})/(?P<slug>[^/]+?)$#x', $pathinfo, $matches)) {...} if (0 === strpos($pathinfo, '/blogger')) { // blogger_index if (preg_match('#^/blogger/(?P<slug>[^/]+?)$#x', $pathinfo, $matches)) {...} // blogger_view if (preg_match('#^/blogger/(?P<year>\d{4})/(?P<month>\d{2})/(?P<slug>[^/]+?)$#x', $pathinfo, $matches)) {...} if (0 === strpos($pathinfo, '/bloggeroo')) { // bloggeroo_index if (preg_match('#^/bloggeroo/(?P<slug>[^/]+?)$#x', $pathinfo, $matches)) {...} // bloggeroo_view if (preg_match('#^/bloggeroo/(?P<year>\d{4})/(?P<month>\d{2})/(?P<slug>[^/]+?)$#x', $pathinfo, $matches)) {...} throw 0 < count($allow) ? new MethodNotAllowedException(array_unique($allow)) : new ResourceNotFoundException(); } throw 0 < count($allow) ? new MethodNotAllowedException(array_unique($allow)) : new ResourceNotFoundException(); } // _blogtest if ($pathinfo === '/blogtest') {...} throw 0 < count($allow) ? new MethodNotAllowedException(array_unique($allow)) : new ResourceNotFoundException(); } // login if ($pathinfo === '/login') {...} throw 0 < count($allow) ? new MethodNotAllowedException(array_unique($allow)) : new ResourceNotFoundException(); } } ``` As far as I can see, you can throw the 404 earlier (as I've done), as by nesting all these possibilities, there is no chance another route will be matched outside the parent if statement. --------------------------------------------------------------------------- by stloyd at 2011/06/11 08:37:15 -0700 You should follow Symfony [CS rules] (http://symfony.com/doc/current/contributing/code/standards.html), also you should modify tests. --------------------------------------------------------------------------- by lmcd at 2011/06/11 08:46:32 -0700 Besides the inline `continue` statement, I can't see spot any other CS violations. I'm on to the updated tests. --------------------------------------------------------------------------- by fabpot at 2011/06/13 02:19:14 -0700 What if you have this: _blog: resource: "@AcmeDemoBundle/Resources/config/routing/BlogController.yml" prefix: /blog login: pattern: /login defaults: { _controller: AcmeDemoBundle:Security:login } _blogger: resource: "@AcmeDemoBundle/Resources/config/routing/BloggerController.yml" prefix: /blogger You cannot send 404 early because the same `blog` prefix is used in two different places. I can see many things that will break with this patch. I'm pretty sure we can enhance the performance of the existing code but let's do that after 2.0. --------------------------------------------------------------------------- by lmcd at 2011/06/13 05:04:03 -0700 @fabpot: The code was designed so it would work in these kind of circumstances. I ran the dumper against the routing file you provided and this is the output below. I see no reason why you can't throw the 404 earlier, as by nesting everything with similar prefixes, we're eliminating the possibility that a route will ever be matched outside of that if statement. ```php public function match($pathinfo) { $allow = array(); if (0 === strpos($pathinfo, '/blog')) { // blog_index if (preg_match('#^/blog/(?P<slug>[^/]+?)$#x', $pathinfo, $matches)) { ... } // blog_view if (preg_match('#^/blog/(?P<year>\d{4})/(?P<month>\d{2})/(?P<slug>[^/]+?)$#x', $pathinfo, $matches)) { ... } if (0 === strpos($pathinfo, '/blogger')) { // blog_index if (preg_match('#^/blogger/(?P<slug>[^/]+?)$#x', $pathinfo, $matches)) { ... } // blog_view if (preg_match('#^/blogger/(?P<year>\d{4})/(?P<month>\d{2})/(?P<slug>[^/]+?)$#x', $pathinfo, $matches)) { ... } throw 0 < count($allow) ? new MethodNotAllowedException(array_unique($allow)) : new ResourceNotFoundException(); } throw 0 < count($allow) ? new MethodNotAllowedException(array_unique($allow)) : new ResourceNotFoundException(); } // login if ($pathinfo === '/login') { ... } throw 0 < count($allow) ? new MethodNotAllowedException(array_unique($allow)) : new ResourceNotFoundException(); } ``` --------------------------------------------------------------------------- by fabpot at 2011/06/13 07:48:49 -0700 Can you add some more tests to ensure that everything works fine and that we won't have regressions later on? Thanks.
This commit is contained in:
commit
c8df0ccf79
@ -72,12 +72,45 @@ EOF;
|
|||||||
private function compileRoutes(RouteCollection $routes, $supportsRedirections, $parentPrefix = null)
|
private function compileRoutes(RouteCollection $routes, $supportsRedirections, $parentPrefix = null)
|
||||||
{
|
{
|
||||||
$code = array();
|
$code = array();
|
||||||
foreach ($routes as $name => $route) {
|
|
||||||
|
$routeIterator = $routes->getIterator();
|
||||||
|
$keys = array_keys($routeIterator->getArrayCopy());
|
||||||
|
$keysCount = count($keys);
|
||||||
|
|
||||||
|
$i = 0;
|
||||||
|
|
||||||
|
foreach ($routeIterator as $name => $route) {
|
||||||
|
$i++;
|
||||||
|
|
||||||
if ($route instanceof RouteCollection) {
|
if ($route instanceof RouteCollection) {
|
||||||
$prefix = $route->getPrefix();
|
$prefix = $route->getPrefix();
|
||||||
$optimizable = $prefix && count($route->all()) > 1 && false === strpos($route->getPrefix(), '{');
|
$optimizable = $prefix && count($route->all()) > 1 && false === strpos($route->getPrefix(), '{');
|
||||||
$indent = '';
|
$indent = '';
|
||||||
if ($optimizable) {
|
if ($optimizable) {
|
||||||
|
for ($j = $i; $j < $keysCount; $j++) {
|
||||||
|
if ($keys[$j] === null) {
|
||||||
|
continue;
|
||||||
|
}
|
||||||
|
|
||||||
|
$testRoute = $routeIterator->offsetGet($keys[$j]);
|
||||||
|
$isCollection = ($testRoute instanceof RouteCollection);
|
||||||
|
|
||||||
|
$testPrefix = $isCollection ? $testRoute->getPrefix() : $testRoute->getPattern();
|
||||||
|
|
||||||
|
if (0 === strpos($testPrefix, $prefix)) {
|
||||||
|
$routeIterator->offsetUnset($keys[$j]);
|
||||||
|
|
||||||
|
if ($isCollection) {
|
||||||
|
$route->addCollection($testRoute);
|
||||||
|
} else {
|
||||||
|
$route->add($keys[$j], $testRoute);
|
||||||
|
}
|
||||||
|
|
||||||
|
$i++;
|
||||||
|
$keys[$j] = null;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
$code[] = sprintf(" if (0 === strpos(\$pathinfo, '%s')) {", $prefix);
|
$code[] = sprintf(" if (0 === strpos(\$pathinfo, '%s')) {", $prefix);
|
||||||
$indent = ' ';
|
$indent = ' ';
|
||||||
}
|
}
|
||||||
@ -89,6 +122,7 @@ EOF;
|
|||||||
}
|
}
|
||||||
|
|
||||||
if ($optimizable) {
|
if ($optimizable) {
|
||||||
|
$code[] = " throw 0 < count(\$allow) ? new MethodNotAllowedException(array_unique(\$allow)) : new ResourceNotFoundException();";
|
||||||
$code[] = " }\n";
|
$code[] = " }\n";
|
||||||
}
|
}
|
||||||
} else {
|
} else {
|
||||||
|
Reference in New Issue
Block a user