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 squashed before being merged into the master branch (closes#5904).
Commits
-------
84adcb1 [2.2][Routing] Added support for default attributes with default values of method params
Discussion
----------
[2.2][Routing] Added support for default attributes with default values of method params
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
With this patch, you can configure your default values likes this:
``` php
/**
* @Route("/hi/{name}", name="hi")
*/
public function hiAction($name = "Bob")
{
return new Response($name);
}
```
---------------------------------------------------------------------------
by Tobion at 2012-11-03T23:15:32Z
I'm unsure. How does one know if that param defines a default value or a requirement? It's too vague.
---------------------------------------------------------------------------
by lyrixx at 2012-11-03T23:35:27Z
It's only a default value, not a requirement.
It's just a shortcut to avoid `defaults={"name"="bob"}`
---------------------------------------------------------------------------
by Tobion at 2012-11-03T23:43:51Z
Yes, but its not clear. It could also be a shortcut to `requirements={"name"="bob"}`, which has totally different meaning. So it's not self-explanatory.
-1 for me.
---------------------------------------------------------------------------
by lyrixx at 2012-11-03T23:48:21Z
it is the default php behavior. It's a default value for a variable...
---------------------------------------------------------------------------
by stof at 2012-11-04T00:22:58Z
@Tobion using the default value of the method to set a requirement does not make any sense. I don't see why someone would expect this behavior
---------------------------------------------------------------------------
by fabpot at 2012-11-06T10:12:05Z
@lyrixx Can you add some unit tests?
---------------------------------------------------------------------------
by Tobion at 2012-11-06T10:28:42Z
Oh I misunderstood the PR. I thought this makes the `name` param default to `hi`. `@Route("/hi/{name}", name="hi")`. But it's just the name of the route. Your example was easy to misinterpret as you used `name` everywhere.
---------------------------------------------------------------------------
by fabpot at 2012-11-10T08:33:13Z
@lyrixx Can you finish this PR?
---------------------------------------------------------------------------
by lyrixx at 2012-11-10T13:16:34Z
@fabpot Yes i will as soon as possible.
---------------------------------------------------------------------------
by lyrixx at 2012-11-10T18:34:07Z
I rebase and amend my commit. (I changed doc in commit message to be less confusing)
I will try to add tests.
But for now, `AnnotationClassLoader::load` is not really tested, and `AnnotationClassLoader::addRoute` is absolutely not tested. So I think I should add tests for these methods ? And then add tests for my patch.
I will try tomorrow.
---------------------------------------------------------------------------
by lyrixx at 2012-11-11T18:23:41Z
@fabpot I added new tests. I tried to made very atomic commits.
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.
* 2.1:
[ClassLoader] fixed unbracketed namespaces (closes#5747)
slight refactoring in UrlMatcher
[Form] Created test for DoctrineOrmTypeGuesser see #5790
[Form] Fixed DoctrineOrmTypeGuesser to guess the "required" option for to-one associations
This PR was merged into the master branch.
Commits
-------
e54d749 [Routing] Simplified php matcher dumper (and optimized generated matcher)
Discussion
----------
[Routing] Simplified php matcher dumper (and optimized generated matcher)
Bug fix: no
Feature addition: no
Related: #3378
Backwards compatibility break: no
Symfony2 tests pass: yes
This simplifies the php matcher dumper by allowing the dumper to re-organize routes in the dumper's own structure.
As a result, dumping is made a little simpler. This is also helps much for my hostname-based routes PR #3378.
Reorganizing routes also allows to find more optimization opportunities:
Currently the dumper wraps some collections of routes in a `if (0 === strpos($pathinfo, '/someprefix')` if the collection has user-defined prefix, and if it contains more than one direct child Route. This can miss many optimization opportunities.
The PR changes this by building a prefix tree of routes based on the static prefix extracted from routes' patterns. Then every leave having a prefix and more than one child (route or collection) will be wrapped in a `if` statement.
Example:
```
// No explicit prefix is specified
@Route("/cafe")
@Route("/cacao")
@Route("/coca")
```
is compiled like this:
```
if (url starts with /c) {
if (url starts with /ca) {
// test route "/cafe"
// test route "/cacao"
}
// test route "/coca"
}
```
Some tests have many white space changes, much more easier to review [here](https://github.com/symfony/symfony/pull/5734/files?w=1)
---------------------------------------------------------------------------
by Tobion at 2012-10-13T02:27:54Z
I'm not sure if adding these specific classes just for dumping is the best implementation because they duplicate some logic and this optimization should also work out-of-the-box with the standard RouteCollection etc.
What I have in mind is a new method in RouteCollection like `RouteCollection::optimizeTree` that returns a new RouteCollection with the Routes that includes these optimization you do here. So there would probably be no need for the new classes.
It think it requires some changes in RouteCollection like the handling of prefix that must start with a slash currently, which is too restrictive. But it should be possible.
---------------------------------------------------------------------------
by arnaud-lb at 2012-10-13T12:52:32Z
@Tobion
> I'm not sure if adding these specific classes just for dumping is the best implementation because they duplicate some logic and this optimization should also work out-of-the-box with the standard RouteCollection etc.
I think RouteCollection and DumperCollection do not share the same concerns; and RouteCollection does things that don't allow to reorganize routes freely. For instance when adding a collection to a RouteCollection this changes all the child routes' prefix, requirements, options, etc. When setting a collection's prefix, this prepends the prefix to every child route's pattern, etc.
---------------------------------------------------------------------------
by arnaud-lb at 2012-10-15T08:50:23Z
squashed the CS commits
---------------------------------------------------------------------------
by arnaud-lb at 2012-10-15T13:50:16Z
@fabpot @Tobion this PR is ready to be merged if everyone agrees
---------------------------------------------------------------------------
by Tobion at 2012-10-16T18:10:36Z
When the above is fixed, I think it's good to be merged.
---------------------------------------------------------------------------
by arnaud-lb at 2012-10-17T08:40:20Z
Fixed; thanks @Tobion @stof for your reviews
---------------------------------------------------------------------------
by Tobion at 2012-10-19T03:30:10Z
@arnaud-lb could you please test whether your PR fixes#5780 as a side-effect?
I can image that it's fixed because you use `$route->compile()->getStaticPrefix();` for prefix optimization.
If it's fixed please add a test case. If not, that's fine, and we need to fix it in another PR.
* 2.1: (28 commits)
Delete use of CreationExeption
[Form] Fixed error message in PropertyPath to not advice to use a non-existing feature
[Form] Fixed creation of multiple money fields with different currencies
[Form] Fixed setting the "data" option to an object in "choice" and "entity" type
Fixed Serbian plural translations.
Fixed IPv6 Check in RequestMatcher
Fix typo
change what I think is a typo
[Console] Fix error when mode is not in PATH
[WebProfilerBundle] fixed macro usage (to be forward compatible with Twig 2.x)
Change monolog require-dev to use the branch alias instead of dev-master
[FrameworkBundle] partially reverted previous merge
[2.1] Added missing error return codes in commands
Made the router lazy when setting the context
[WebProfilerBundle] fixed typos
Fix incorrect variable in FileProfilerStorage
UnitTest fix
UnitTest fix
added a unit test
fixed#5384
...
Initializing the matcher and the generator to set the context does not make
sense as it is set anyway when building them. This avoids initializing
them in the RouterListener if you never actually use them (for instance
because you use the apache matcher).
* 2.1:
fixed CS
added doc comments
added doc comments
[Validator] Updated swedish translation
Update src/Symfony/Component/Validator/Resources/translations/validators.de.xlf
[2.1] Exclude tests from zips via gitattributes
[HttpKernel][Translator] Fixed type-hints
Updated lithuanian validation translation
[DomCrawler] Allows using multiselect through Form::setValues().
[Translation] forced the catalogue to be regenerated when a resource is added (closes symfony/Translation#1)
Unit test for patched method OptionsResolver::validateOptionValues().
validateOptionValues throw a notice if an allowed value is set and the corresponding option isn't.
[Form] Hardened code of ViolationMapper against errors
[HttpFoundation] Fixed#5611 - Request::splitHttpAcceptHeader incorrect result order.
[Form] Fixed negative index access in PropertyPathBuilder
Update src/Symfony/Component/Validator/Resources/translations/validators.ro.xlf
Conflicts:
src/Symfony/Component/DomCrawler/Form.php
src/Symfony/Component/Process/Process.php
