- The route compiler does not add extra space or line-feed,
- The generated regex does not use the 'x' modified any more,
- The PHP and apache matchers do not need to strip any chars (vs space and line feed before),
- The space characters are escaped according to the apache format
Commits
-------
e6e9b5a [Routing] Return the _route parameter from ApacheUrlMatcher
Discussion
----------
[Routing] Return the _route parameter from ApacheUrlMatcher
---------------------------------------------------------------------------
by fabpot at 2012-02-22T23:13:49Z
Can you squash your commits before I merge? Thanks.
---------------------------------------------------------------------------
by arnaud-lb at 2012-02-23T09:12:45Z
sure, done
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.
Commits
-------
49dd558 Couple more CS fixes
5a986bf Add $keysCount & minor CS fix
91f4097 [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.
* lmcd/master:
$code referenced but not defined in compileRoute()
[Routing] Optimised the PHP URL matcher dumper The cached URL matcher classes contain some unneeded logic. Consider the following example:
The cached URL matcher classes contain some unneeded logic. Consider the following example:
if (0 === strpos($pathinfo, '/Blog')) {
// blog_index
if (0 === strpos($pathinfo, '/Blog') && preg_match('#^/Blog/(?P<slug>[^/]+?)$#x', $pathinfo, $matches)) {
return array_merge($this->mergeDefaults($matches, array ( '_action' => 'index',)), array('_route' => 'blog_index'));
}
}
The 2nd strpos is not required, as we have already satisfied this condition in the parent if statement.
My change will produce the following code for the same routing setup::
if (0 === strpos($pathinfo, '/Blog')) {
// blog_index
if (preg_match('#^/Blog/(?P<slug>[^/]+?)$#x', $pathinfo, $matches)) {
return array_merge($this->mergeDefaults($matches, array ( '_action' => 'index',)), array('_route' => 'blog_index'));
}
}