[Routing][WIP] Add priorities to routing component #26132

magnusnordlander · 2018-02-10T13:15:24Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets
License	MIT
Doc PR	None yet

When using the Route annotation, route ordering is unpredictable, and if you need a route in one controller to be first/last (e.g. as a catch-all route), you can't reliably achieve this. Instead you have to resort to using one of the other configuration formats.

In order to remedy this, I'd like to propose the addition of priority to the routing system. I suppose that makes the route collection a priority ordered dictionary, instead of just an ordered dictionary. When adding a route to a route collection, you can optionally specify a priority (the default is 0). Routes with higher priority are sorted before routes with lower priority. Routes with the same priority are still sorted by when they were added to the collection.

Adding a collection to your route collection respects priority, and if the added collection contains a route with the same name, but a higher/lower priority, the order will be updated accordingly (same priority still means they are added last).

This PR is not done, I still need to add priority to loader(s), I do however want feedback on this early.

magnusnordlander · 2018-02-10T13:15:52Z

Status: Needs work

nicolas-grekas · 2018-02-11T10:07:48Z

Could be useful :) Maybe also display the priorities on the debug:router command (but on when there are some?)

Tobion · 2018-02-11T18:52:29Z

Something like his has been proposed before in #11753

When using the Route annotation, route ordering is unpredictable, and if you need a route in one controller to be first/last (e.g. as a catch-all route), you can't reliably achieve this.

This is not true. It creates the route in the order the methods are defined. See http://php.net/manual/de/reflectionclass.getmethods.php#115197
So by reordering your controller actions, you can also controll routing priority.
I'm not saying it's the best solution. But I also don't think priorities for routes are much better.
In an ideal scenario, the routing component would either warn you about unreachable routes or even reorder routes automatically when neccessary like in #24748

javiereguiluz · 2018-02-11T18:56:06Z

@Tobion you are right: reordering methods in a single PHP class is simple. However, the problem I sometimes have faced is that I need the routes of some controller to be defined before some other controller. However, with this default config:

controllers:
    resource: ../../src/Controller/
    type: annotation

Symfony loads controllers alphabetically, so you can't control this in any way ... except importing controllers manually from routes.yaml ... and that's really boring. But I don't know if allowing to define priorities would be a better solution.

nicolas-grekas · 2018-02-23T15:21:41Z

If we could prevent this from being happening at all, that'd be best IMHO, as this will add complexity.
Can't we say that if you need special ordering like @javiereguiluz says, then you need to switch out from annotations for these routes? That'd be more maintainable on the long run IMHO, both for teaching/doc and core code.

fabpot · 2018-02-24T11:02:14Z

I agree with @nicolas-grekas

Taluu · 2018-02-24T12:34:18Z

src/Symfony/Component/Routing/RouteCollection.php

+     * @param string $name  The route name
+     * @param Route  $route A Route instance
+     */
+    public function addWithPriority($name, Route $route, int $priority = 0)


Instead of this, I would have used the deprecation mechanism with args and merged this with addRoute (only one entrypoint to add a route would then be used)

magnusnordlander · 2018-03-09T14:44:41Z

@nicolas-grekas: I mean, sure we could treat this as a documentation issue (and I would be willing to work on a doc PR for that if that's the decision), but if the problem is "ordering routes is impractical when using annotations", that's pretty much a "won't fix". That's fine of course if I'm fairly alone in having it, but I'm not sure that's the case.

I might do some kind of informal poll to see if people think this is a real issue that should be solved, or if they don't mind switching to a different route format for routes where order matters.

GromNaN · 2018-03-10T00:21:51Z

Possible solutions:

Configure each controller individually in the config/routes/annotations.yaml file, in the expected order
Rename Controller classes to get alphabetical order matching expected order

Moreover, a 3nd solution could be to specify only the highest priority controller in the config before loading the rest:

# config/routes/annotations.yaml
first_controllers:
    resource: ../../src/Controller/VeryFirstController.php
    type: annotation

all_controllers:
    # Loads everything in alphabetical order:  LowerPriorityController, VeryFirstController
    resource: ../../src/Controller/
    type: annotation

Unfortunately, this solution is not working because the routes defined in the VeryFirstController are loaded twice, and the later remove every route with the same name. When loading a directory, would it be possible to ignore files that have already been loaded?

nicolas-grekas · 2018-06-19T14:19:19Z

Instead of adding priorities here, can we do it at the annotations loader one? Could be a new parameter on annotations to control the order in one AnnotationDirectoryLoader?

magnusnordlander · 2018-06-20T08:59:00Z

@nicolas-grekas: It would help, but I don't think that would solve the issue completely. It would be good to be able to order annotation routes in relation to routes defined in yaml or xml, not just among other annotation routes.

nicolas-grekas

Let's unlock this one :)
Still interested in finishing it?
Annotations and loaders should be updated also.

nicolas-grekas · 2019-04-03T14:29:57Z

src/Symfony/Component/Routing/RouteCollection.php

     */
-    private $routes = array();
+    private $prioritizedRoutes = array();


routesByPriority?

nicolas-grekas · 2019-04-03T14:31:12Z

src/Symfony/Component/Routing/RouteCollection.php

@@ -48,12 +55,13 @@ public function __clone()
     * It implements \IteratorAggregate.
     *
     * @see all()
+     * @todo Change return type to Iterator|Route[] in 5.0


should be removed

nicolas-grekas · 2019-04-03T14:31:55Z

src/Symfony/Component/Routing/RouteCollection.php

+    /**
+     * @var int[]
+     */
+    private $priorities = array();


routePriorities?

nicolas-grekas · 2019-04-03T14:33:31Z

src/Symfony/Component/Routing/RouteCollection.php

+     * @param string $name  The route name
+     * @param Route  $route A Route instance
+     */
+    public function addWithPriority($name, Route $route, int $priority = 0)


should be removed and a third arg added to add(), using the common technique we use elsewhere (docblock @param + commented argument in /* ...*/ + func_num/get_arg(s)

nicolas-grekas · 2019-04-03T14:38:22Z

src/Symfony/Component/Routing/RouteCollection.php

-        foreach ($collection->all() as $name => $route) {
-            unset($this->routes[$name]);
-            $this->routes[$name] = $route;
+        foreach ($collection->prioritizedRoutes as $priority => $routes) {


this breaks classes extending RouteCollection - we might need a new method to get routes by priorities

magnusnordlander · 2019-04-04T14:54:51Z

@nicolas-grekas Sure, I'll take a fresh look at it this weekend.

fabpot · 2019-07-08T09:30:37Z

@magnusnordlander friendly ping ;)

fabpot

If the use case is to manage priorities for annotations (I don't think that managing priorities for other loaders is useful), then, what about managing priorities directly in the annotation directory loader?

fabpot · 2020-02-03T15:05:35Z

src/Symfony/Component/Routing/RouteCollection.php

+     * @param string $name  The route name
+     * @param Route  $route A Route instance
+     */
+    public function addWithPriority($name, Route $route, int $priority = 0)


nicolas-grekas · 2020-02-05T17:58:47Z

Closing in favor of #35608
Thanks for pushing this forward!

…olas-grekas) This PR was merged into the 5.1-dev branch. Discussion ---------- [Routing] add priority option to annotated routes | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | Deprecations? | yes | Tickets | Fix #26132 | License | MIT | Doc PR | - This PR allows defining the priority of routes using annotations: `@Route(name="foo", priority=10)` As requested [in this comment](#26132 (review)), priority is reserved to using annotations. For other formats, the order works fine. Commits ------- 8522a83 [Routing] add priority option to annotated routes

Added priorities to RouteCollection

d88517c

carsonbot added Status: Needs Review Feature labels Feb 10, 2018

carsonbot added Status: Needs Work and removed Status: Needs Review labels Feb 10, 2018

magnusnordlander changed the title ~~WIP: Add priorities to routing component~~ [Routing][WIP] Add priorities to routing component Feb 10, 2018

nicolas-grekas mentioned this pull request Feb 10, 2018

[Routing] Match 77.7x faster by compiling routes in one regexp #26059

Merged

13 tasks

nicolas-grekas added this to the 4.1 milestone Feb 11, 2018

javiereguiluz added the Routing label Feb 11, 2018

mimol91 mentioned this pull request Feb 23, 2018

[Routing] Overriding route puts route on end of collection. #24748

Closed

Taluu reviewed Feb 24, 2018

View reviewed changes

nicolas-grekas modified the milestones: 4.1, next Apr 20, 2018

gabiudrescu mentioned this pull request Jun 30, 2018

How to mess up EasyAdmin routing & FOS UserBundle and a catch all route EasyCorp/EasyAdminBundle#2278

Closed

nicolas-grekas reviewed Apr 3, 2019

View reviewed changes

fabpot requested changes Feb 3, 2020

View reviewed changes

nicolas-grekas mentioned this pull request Feb 5, 2020

[Routing] add priority option to annotated routes #35608

Merged

nicolas-grekas closed this Feb 5, 2020

MrYamous mentioned this pull request Mar 17, 2020

Add priority to debug router #36110

Closed

nicolas-grekas modified the milestones: next, 5.1 May 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Routing][WIP] Add priorities to routing component #26132

[Routing][WIP] Add priorities to routing component #26132

magnusnordlander commented Feb 10, 2018

magnusnordlander commented Feb 10, 2018

nicolas-grekas commented Feb 11, 2018

Tobion commented Feb 11, 2018 •

edited

Loading

javiereguiluz commented Feb 11, 2018

nicolas-grekas commented Feb 23, 2018 •

edited

Loading

fabpot commented Feb 24, 2018

Taluu Feb 24, 2018

fabpot Feb 3, 2020

magnusnordlander commented Mar 9, 2018

GromNaN commented Mar 10, 2018

nicolas-grekas commented Jun 19, 2018

magnusnordlander commented Jun 20, 2018

nicolas-grekas left a comment

nicolas-grekas Apr 3, 2019

nicolas-grekas Apr 3, 2019

nicolas-grekas Apr 3, 2019

nicolas-grekas Apr 3, 2019

nicolas-grekas Apr 3, 2019

magnusnordlander commented Apr 4, 2019

fabpot commented Jul 8, 2019

fabpot left a comment

fabpot Feb 3, 2020

nicolas-grekas commented Feb 5, 2020

[Routing][WIP] Add priorities to routing component #26132

[Routing][WIP] Add priorities to routing component #26132

Conversation

magnusnordlander commented Feb 10, 2018

magnusnordlander commented Feb 10, 2018

nicolas-grekas commented Feb 11, 2018

Tobion commented Feb 11, 2018 • edited Loading

javiereguiluz commented Feb 11, 2018

nicolas-grekas commented Feb 23, 2018 • edited Loading

fabpot commented Feb 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magnusnordlander commented Mar 9, 2018

GromNaN commented Mar 10, 2018

nicolas-grekas commented Jun 19, 2018

magnusnordlander commented Jun 20, 2018

nicolas-grekas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magnusnordlander commented Apr 4, 2019

fabpot commented Jul 8, 2019

fabpot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Feb 5, 2020

Tobion commented Feb 11, 2018 •

edited

Loading

nicolas-grekas commented Feb 23, 2018 •

edited

Loading