[Routing] optimization in PhpMatcherDumper produces wrong result #3777
Comments
|
The optimization is a good and needed feature to reduce regular matching but it is flawed. |
|
Does #3754 fix this issue? |
|
No, #3754 only removes the bugs in RouteCollection, meaning there can really only be one route with a given name. |
|
I think I have an idea how to fix it and even allow more optimizations to be applied. One needs to regroup the RouteCollection and its routes and find routes with the same prefix. Then something like could be recognized as having the same static prefix and thus be optimized. This isn't done currently. It could be expanded to prefixes with variables that aren't optimized yet, too. |
|
Ok #3754 includes the fix now. |
fabpot
added a commit
that referenced
this issue
Apr 11, 2012
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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
The optimization in line 93 of https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Routing/Matcher/Dumper/PhpMatcherDumper.php#L93 does not work correctly and produces a matcher that can match the wrong route.
This is because it aggregates routes with the same prefix but does not consider there might be routes in between these routes that have higher priority and thus should be matched earlier. Hard to explain. Here is a test case that shows it:
This creates
As you can see the route
/prefix/static3would now match route namedstatic3but should rather match routevarwhich has higher priority but comes later in the code generation.The text was updated successfully, but these errors were encountered: