we don't need the logic to merge numeric keys, as we don't have them. I could also improve the genrated code by PhpMatcherDumper a little by saving a function call.
This PR was merged into the master branch.
Commits
-------
c7a8f7a [Routing] fixed possible parameters conflict in apache url matcher
Discussion
----------
[Routing] fixed possible parameters conflict in apache url matcher
Bug fix: yes
Feature addition: no
Backwards compatibility break: no (as long as rewrite rules are generated after upgrading)
Symfony2 tests pass: yes
- This fixes a conflict in route parameters:
The rewrite rules currently pass route informations through environment variables:
`_ROUTING_DEFAULT_x`: passes the default value of parameter x
`_ROUTING__allow_x`: passes the information that method x was allowed for this route
`_ROUTING_x`: passes the value of parameter x
The problem is that naming a route parameter `DEFAULT_*` or `_allow_*` would not behave as expected.
I fixed this by namespacing all environment variables; e.g. parameters are in `_ROUTING_param_*`, defaults in `_ROUTING_default_*`, etc.
- The PR fixes a second issue: sometimes the variables are prefixed with multiple REDIRECT_. This PR handles this case by ignoring them all.
- This also improves performance a little:
Matching a route with two parameters and two default parameters 100K times: (`$_SERVER` was copied from a real request, so with many non `_ROUTING_` variables)
master: 6.6s
this branch: 4.7s
---------------------------------------------------------------------------
by fabpot at 2012-10-27T13:37:24Z
Any news on this PR? Is it mergeable?
---------------------------------------------------------------------------
by arnaud-lb at 2012-10-27T14:50:08Z
There is an issue with default parameter values, I can't find how to fix that in a simple way. Before this PR, default values are never used (if a parameter is an optional not present in the url, the parameter's value is the empty string); after this PR, when a parameter is present and empty (e.g. a requirement like `.*`), its value is set to its default value.
---------------------------------------------------------------------------
by Tobion at 2012-10-29T01:36:08Z
The problem is, it's not consistent with the default php matcher. So one cannot safely exchange it with the apache matcher because it behaves differently under some (special) circumstances.
---------------------------------------------------------------------------
by fabpot at 2012-11-05T08:05:54Z
We need to move forward as I want to merge the hostname support in the routing ASAP to have plenty of time for feedback before the 2.2 release.
Does it sound reasonable to merge this PR as is an open a ticket about the remaining issue (which should not occur that often anyways)?
---------------------------------------------------------------------------
by arnaud-lb at 2012-11-05T09:22:02Z
@fabpot it sounds reasonable to me. Also, I've the hostname support branch is currently rebased so that it can be merged without this one.
---------------------------------------------------------------------------
by Tobion at 2012-11-11T21:50:20Z
Btw, does the ApacheMatcherDumper handle the _scheme requirement? It doesn't look like it. This would be another bug.
Anyway, we can probably merge this PR and open new issues for the remaining bugs.
- 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
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.
* 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'));
}
}
The _scheme requirement can be used to force routes to always match one given scheme
and to always be generated with the given scheme.
So, if _scheme is set to https, URL generation will force an absolute URL if the
current scheme is http. And if you request the URL with http, you will be redirected
to the https URL.
