8000 [Routing] Implement bug fixes and enhancements (from @Tobion) by vicb · Pull Request #3876 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Routing] Implement bug fixes and enhancements (from @Tobion) #3876

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Apr 11, 2012
Merged

[Routing] Implement bug fixes and enhancements (from @Tobion) #3876

merged 1 commit into from
Apr 11, 2012

Conversation

vicb
Copy link
Contributor
@vicb vicb commented Apr 11, 2012

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 [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 RouteCollectio 8000 ns 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.

@vicb vicb mentioned this pull request Apr 11, 2012
Closed
fabpot added a commit that referenced this pull request Apr 11, 2012
@fabpot
merged branch vicb/routing_improvements (PR #3876)
0c57db1 
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.
@fabpot fabpot merged commit 9307f5b into symfony:master Apr 11, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.1
Development

Successfully merging this pull request may close these issues.
4 participants
@vicb @mpdude @fabpot @Tobion
0