[Routing] fixed several bugs and applied improvements #3754
Conversation
|
it's ready now |
| $this->remove(array_keys($collection->all())); | ||
|
|
||
| $collection->setParent($this); | ||
| // the sub-collection must have the prefix of the parent (current instance) prepended because it it does not |
|
This PR conflicts with master according to github. You need to rebase your branch |
|
Rebased and fixed CS. |
| if ($routes instanceof RouteCollection) { | ||
| $routes->remove($name); | ||
| if (is_array($name)) { | ||
| foreach ($name as $n) { |
There was a problem hiding this comment.
could be simplified foreach((array) $name) -> no need for the if
There was a problem hiding this comment.
I would not break BC here but add a foreach in the caller
There was a problem hiding this comment.
I did it intentionally so that the root only needs to be retrieved once for all routes-to-be-deleted.
|
@Tobion I will finish the review tomorrow, please make one fix w/ associated tests by commit the next time to ease the review (It would actually be great if you can re-arrange the PR this way, otherwise I'll deal with it). |
| */ | ||
| public function setParent(RouteCollection $parent) | ||
| public function getRoot() |
There was a problem hiding this comment.
does this need to be public ?
There was a problem hiding this comment.
it's quite useful. most of the time you need to root collection to work with it (like dumping the matcher) and might not have a reference to it anymore.
|
Ok I fixed CS mentioned by @vicb so far. |
|
@Tobion you will need to rebase your branch. It conflicts according to github |
|
Added another fix. See 10) above |
| @@ -364,9 +364,11 @@ To get the diff between two versions, go to https://github.com/symfony/symfony/c | |||
| * added a TraceableUrlMatcher | |||
| * added the possibility to define options, default values and requirements for placeholders in prefix, including imported routes | |||
| * added RouterInterface::getRouteCollection | |||
| * [BC BREAK] the UrlMatcher urldecodes the route parameters only once, they were | |||
| decoded twice before. Note that the `urldecode()` calls have been change for a | |||
There was a problem hiding this comment.
fixed typo there: changed
@stof I rebased. Btw, how do I rebase and resolve conflict without using the --force option when pushing? Without it, it gets rejected becaues of non-fast-forward updates.
There was a problem hiding this comment.
@Tobion when you rebase, you need to use --force when pushing as rebasing rewrites the history.
There was a problem hiding this comment.
I see. The problem is github puts all commits at the end after that in Tobion added some commits section.
So conversation history is not that easy to follow.
There was a problem hiding this comment.
For reference there is also another syntax git push tobion +branch_name which also forces the write.
|
We optimize regular expressions with static prefixes. But from reading the documentation of PCRE, the prefix optimization doesn't seem to increase performance that much because PCRE anchors the regex and process them from left to right. Static suffixes are also pretty common, e.g. |
| * | ||
| * @return integer Number of Routes | ||
| */ | ||
| private function countDirectChildRoutes(RouteCollection $routes) |
There was a problem hiding this comment.
Do you really need an arg here ?
There was a problem hiding this comment.
what do you mean? how else should it work?
There was a problem hiding this comment.
My mistake (think we were in RouteCollection here)
Commits ------- 77185e0 [Routing] Allow spaces in the script name for the apache dumper 6465a69 [Routing] Fixes to handle spaces in route pattern Discussion ---------- [Routing] Handling of space characters in the dumpers The compiler was using the 'x' modifier in order to ignore extra spaces and line feeds but the code was flawed: - it was actually ignoring all the spaces, not only the extra ones added by the compiler, - all the spaces were stripped in the php and apache matchers. The proposed fix: - do not use the 'x' modifier any more (and then do no add extra spaces / line feeds), - do not strip the spaces in the matchers, - escapes the spaces (both in regexs and script name) for the apache matcher. It also include [a small optimization](https://github.com/vicb/symfony/pull/new#L9L89) when the only token of a route is an optional variable token - the idea is to make the regex easier to read. --------------------------------------------------------------------------- by vicb at 2012-04-10T13:59:45Z @Baachi fixed now. Thanks. --------------------------------------------------------------------------- by Tobion at 2012-04-10T16:01:31Z +1, I saw no reason for pretty printing the regex in the first place (just for debugging I guess). @vicb since you want to make the regex easier to read, I propose the remove the `P` from the variable regex `?P<bar>`, which is not needed anymore in PHP 5.3 (and we only support PHP 5.3+ anyway). --------------------------------------------------------------------------- by vicb at 2012-04-10T16:08:36Z @Tobion could you make a PR to this branch for the named parameters ? --------------------------------------------------------------------------- by Tobion at 2012-04-10T16:12:34Z I can include it in #3754 because I'm about the add 2 more fixes to it anyway. But when I proposed to apply these fixes to 2.0 Fabien rejected it. So not sure what branch you want me to apply this. --------------------------------------------------------------------------- by vicb at 2012-04-10T16:25:38Z May be the best is to put it on hold while I am reviewing your PRs. There are already enough changes, we'll make an other PR after all have been sorted out. What's the difference between 3754 and 3810 ? (3810 + 3763 = 3754 ?) --------------------------------------------------------------------------- by Tobion at 2012-04-10T16:39:32Z Lol you forget to link the PR numbers. At first sight I thought it's some sort of mathematical riddle. Haha #3810 is for 2.0 = #3763 (already merged) + #3754 for master --------------------------------------------------------------------------- by vicb at 2012-04-10T16:52:18Z I didn't link on purpose... the question is if '=' means strictly or loosely equal (any diffs - beside master vs 2.0) ? --------------------------------------------------------------------------- by Tobion at 2012-04-10T17:06:04Z It just applies my changes to 2.0. Nothing more. So master still differs from 2.0 by the addional features that were already implemented (e.g. `RouteCollection->addCollection` with optional requirements and options). But since my changes are bug fixes (except the performance improvement in #3763 but that doesn't break anything and makes 2.0 easier to maintain) I thought they should go into 2.0 as well. --------------------------------------------------------------------------- by vicb at 2012-04-10T17:14:27Z @Tobion only bug fixes mean "only bug fixes". You should re-open a PR for 2.0 with "only bug fixes", you might want to wait for me to review 3754. --------------------------------------------------------------------------- by Tobion at 2012-04-10T17:21:00Z Without #3763 it's much harder to apply the bug fixes. And now that I found 2 more bugs which requiresome rewriting of the PhpMatcherDumper, I don't want to apply all the commits by hand again for 2.0...
|
@Tobion your branch needs to be rebased |
|
done |
|
closed in favor of #3876 |
Commits ------- 9307f5b [Routing] Implement bug fixes and enhancements Discussion ---------- [Routing] Implement bug fixes and enhancements (from @Tobion) This is mainly #3754 with some minor formatting changes. Original PR message from @Tobion: Here a list of what is fixed. Tests pass. 1. The `RouteCollection` states > it overrides existing routes with the same name defined in the instance or its children and parents. But this is not true for `->addCollection()` but only for `->add()`. addCollection does not remove routes with the same name in its parents (only in its children). I don't think this is on purpose. So I fixed it by making sure there can only be one route per name in all connected collections. This way we can also simplify `->get()` and `->remove()` and improve performance since we can stop iterating recursively when we found the first and only route with a given name. See `testUniqueRouteWithGivenName` that fails in the old code but works now. 2. There was an bug with `$collection->addPrefix('0');` that didn't apply the starting slash. Fixed and test case added. 3. There is an issue with `->get()` that I also think is not intended. Currently it allows to access a sub-RouteCollection by specifing $name as array integer index. But according to the PHPdoc you should only be allowed to receive a Route instance and not a RouteCollection. See `testGet` that has a failing test case. I fixed this behavior. 4. Then I recognized that `->addCollection` depended on the order of applying them. So $collection1->addCollection($collection2, '/b'); $collection2->addCollection($collection3, '/c'); $rootCollection->addCollection($collection1, '/a'); had a different pattern result from $collection2->addCollection($collection3, '/c'); $collection1->addCollection($collection2, '/b'); $rootCollection->addCollection($collection1, '/a'); Fixed and test case added. See `testPatternDoesNotChangeWhenDefinitionOrderChanges`. 5. PHP could have ended in an infinite loop when one tried to add an existing RouteCollection to the tree. Fixed by throwing an exception when this situation is detected. See tests `testCannotSelfJoinCollection` and `testCannotAddExistingCollectionToTree`. 6. I made `setParent()` private because its not useful outside the class itself. And `remove()` also removes the route from its parents. Added public `getRoot()` method. 7. The `Route` class throwed a PHP warning when trying to set an empty requirement. 8. Fixed issue #3777. See discussion there for more info. I fixed it by removing the over-optimization that was introduced in 91f4097 but didn't work properly. One cannot reorder the route definitions, as is was done, because then the wrong route might me matched before the correct one. If one really wanted to do that, it would require to calculate the intersection of two regular expressions to determine if they can be grouped together (a tool that would also be useful to check whether a route is unreachable, see discussion in #3678) We can only safely optimize routes with a static prefix within a RouteCollection, not across multiple RouteCollections with different parents. This is especially true when using variables and regular expressions requirements. I could however apply an optimization that was missing yet: Collections with a single route were missing the static prefix optimization with `0 === strpos()`. 9. Fixed an issue where the `PhpMatcherDumper` would not apply the optimization if the root collection to be dumped has a prefix itself. For this I had to rewrite `compileRoutes`. It is also much easier to understand now. Addionally I added many comments and PHPdoc because complex recursive methods like this are still hard to grasp. I added a test case for this (`url_matcher3.php`). 10. Fix that `Route::compile` needs to recompile a route once it is modified. Otherwise we have a wrong result. Test case added.
Ok this PR was hard because stuff was tricky to understand and there was much room for improvement. Here a list of what is fixed. Tests pass.
The
RouteCollectionstatesBut this is not true for
->addCollection()but only for->add(). addCollection does not remove routes with the same name in its parents (only in its children). I don't think this is on purpose.So I fixed it by making sure there can only be one route per name in all connected collections. This way we can also simplify
->get()and->remove()and improve performance since we can stop iterating recursively when we found the first and only route with a given name.See
testUniqueRouteWithGivenNamethat fails in the old code but works now.There was an bug with
$collection->addPrefix('0');that didn't apply the starting slash. Fixed and test case added.There is an issue with
->get()that I also think is not intended. Currently it allows to access a sub-RouteCollection by specifing $name as array integer index. But according to the PHPdoc you should only be allowed to receive a Route instance and not a RouteCollection.See
testGetthat has a failing test case. I fixed this behavior.Then I recognized that
->addCollectiondepended on the order of applying them. Sohad a different pattern result from
Fixed and test case added. See
testDefinitionOrderDoesNotMatter.PHP could have ended in an infinite loop when one tried to add an existing RouteCollection to the tree. Fixed by throwing an exception when this situation is detected. See tests
testCannotSelfJoinCollectionandtestCannotAddExistingCollectionToTree.I made
setParent()private because its not useful outside the class itself. Andremove()also removes the route from its parents. Added publicgetRoot()method.The
Routeclass throwed a PHP warning when trying to set an empty requirement.Fixed issue [Routing] optimization in PhpMatcherDumper produces wrong result #3777. See discussion there for more info. I fixed it by removing the over-optimization that was introduced in 91f4097 but didn't work properly. One cannot reorder the route definitions, as is was done, because then the wrong route might me matched before the correct one. If one really wanted to do that, it would require to calculate the intersection of two regular expressions to determine if they can be grouped together (a tool that would also be useful to check whether a route is unreachable, see discussion in [RFC][Routing] Improve the current matching process #3678) We can only safely optimize routes with a static prefix within a RouteCollection, not across multiple RouteCollections with different parents. This i 8000 s especially true when using variables and regular expressions requirements.
I could however apply an optimization that was missing yet: Collections with a single route were missing the static prefix optimization with
0 === strpos().Fixed an issue where the
PhpMatcherDumperwould not apply the optimization if the root collection to be dumped has a prefix itself. For this I had to rewritecompileRoutes. It is also much easier to understand now. Addionally I added many comments and PHPdoc because complex recursive methods like this are still hard to grasp.I added a test case for this (
url_matcher3.php).Fix that
Route::compileneeds to recompile a route once it is modified. Otherwise we have a wrong result. Test case added.