Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

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.

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

Already on GitHub?Sign in to your account

Conversation

magnusnordlander
Copy link
Contributor

QA
Branch?master
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets
LicenseMIT
Doc PRNone 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 ofpriority 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.

zmitic, apfelbox, jvasseur, hason, grachevko, maxhelias, freema, Pictor13, and UBERPHP reacted with thumbs up emojiKoc reacted with hooray emoji
@magnusnordlander
Copy link
ContributorAuthor

Status: Needs work

@magnusnordlandermagnusnordlander changed the titleWIP: Add priorities to routing component[Routing][WIP] Add priorities to routing componentFeb 10, 2018
@nicolas-grekasnicolas-grekas added this to the4.1 milestoneFeb 11, 2018
@nicolas-grekas
Copy link
Member

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

@Tobion
Copy link
Contributor

Tobion commentedFeb 11, 2018
edited
Loading

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. Seehttp://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
Copy link
Member

@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 fromroutes.yaml ... and that's really boring. But I don't know if allowing to define priorities would be a better solution.

Koc, jvasseur, hason, and deguif reacted with thumbs up emoji

@nicolas-grekas
Copy link
Member

nicolas-grekas commentedFeb 23, 2018
edited
Loading

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
Copy link
Member

I agree with@nicolas-grekas

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I agree

@magnusnordlander
Copy link
ContributorAuthor

@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
Copy link
Member

Possible solutions:

  1. Configure each controller individually in theconfig/routes/annotations.yaml file, in the expected order
  2. 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.yamlfirst_controllers:    resource: ../../src/Controller/VeryFirstController.php    type: annotationall_controllers:    # Loads everything in alphabetical order:  LowerPriorityController, VeryFirstController    resource: ../../src/Controller/    type: annotation

Unfortunately, this solution is not working because the routes defined in theVeryFirstController 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-grekasnicolas-grekas modified the milestones:4.1,nextApr 20, 2018
@nicolas-grekas
Copy link
Member

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 oneAnnotationDirectoryLoader?

@magnusnordlander
Copy link
ContributorAuthor

@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.

Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

routesByPriority?

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

should be removed

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

routePriorities?

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

@magnusnordlander
Copy link
ContributorAuthor

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

@fabpot
Copy link
Member

@magnusnordlander friendly ping ;)

Copy link
Member

@fabpotfabpot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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?

Tobion reacted with thumbs up emoji
* @param string $name The route name
* @param Route $route A Route instance
*/
public function addWithPriority($name, Route $route, int $priority = 0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I agree

@nicolas-grekas
Copy link
Member

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

fabpot added a commit that referenced this pull requestFeb 5, 2020
…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
@nicolas-grekasnicolas-grekas modified the milestones:next,5.1May 4, 2020
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@TaluuTaluuTaluu left review comments

@nicolas-grekasnicolas-grekasnicolas-grekas left review comments

@fabpotfabpotfabpot requested changes

Assignees
No one assigned
Projects
None yet
Milestone
5.1
Development

Successfully merging this pull request may close these issues.

8 participants
@magnusnordlander@nicolas-grekas@Tobion@javiereguiluz@fabpot@GromNaN@Taluu@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp