[FrameworkBundle] [Routing] Added --sort to debug:router #36579

vjandrea · 2020-04-25T13:17:28Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
Deprecations?	no
Tickets
License	MIT
Doc PR	symfony/symfony-docs#13591

This PR adds a --sort functionality to console debug:router. It allows to sort by name, path or priority (default).

nicolas-grekas

LGTM, just some nitpicking.

src/Symfony/Bundle/FrameworkBundle/Command/RouterDebugCommand.php

src/Symfony/Bundle/FrameworkBundle/Tests/Command/RouterDebugCommandTest.php

Tobion · 2020-04-29T21:34:30Z

I think this PR gives the wrong impression. First of all routes are not only sorted by priority but also by the order your define them in by default. So the description is not correct.
And sorting by name and path can be done using something like sort -k 5 which is what I used so far.
I think it's probably still ok to add those options for convenience but it needs a big fat warning that when sorting by name or path, the shown routes do not reflect the matching order anymore.

vjandrea · 2020-05-03T05:58:04Z

@Tobion Yes, probably "priority" is not the best name because by default debug:router outputs the list in the evaluation order. There's also the new priority annotation to consider: keeping priority, it might sound ambiguous if the priority annotation value will be shown in the routes table.
I agree to show a red warning when sorting by name or path. I'll fix the conflicts for now and I'll see in a couple of days if there are other opinions about the points you suggested.

fabpot · 2020-06-03T08:29:48Z

Something went wrong here.

vjandrea · 2020-06-03T11:43:25Z

Sorry @fabpot, I was trying to fix the failing tests without noticing that the main build was failing too. I will try to cleanup the git mess otherwise I'll drop this PR and submit a new one.

src/Symfony/Bundle/FrameworkBundle/Command/RouterDebugCommand.php

vjandrea · 2020-07-11T14:32:25Z

@Tobion, I added a warning when the sorting result might be misleading: 49029ae

vjandrea · 2020-07-24T17:31:54Z

@sroze, @xabbuh, do you mind reviewing this PR please? thanks!

Tobion · 2020-08-10T13:20:28Z

src/Symfony/Bundle/FrameworkBundle/Command/RouterDebugCommand.php

+        if ('name' === $propertyName) {
+            ksort($sortedRoutes);
+        } elseif ('path' === $propertyName) {
+            uasort($sortedRoutes, function ($a, $b) { return $a->getPath() <=> $b->getPath(); });


please add the type for $a and $b

if routes have the same path but different prio, this can result is routes getting mixed up. sorting will only be stable in php 8: https://wiki.php.net/rfc/stable_sorting
So we would need to implement the stable part ourselves prio to php 8.

Ok, I'll refactor the test to verify the stability

Tobion · 2020-08-10T13:21:29Z

src/Symfony/Bundle/FrameworkBundle/Command/RouterDebugCommand.php

+    private function sortRoutes(RouteCollection $routes, string $propertyName): RouteCollection
+    {
+        $sortedRoutes = $routes->all();
+        if ('name' === $propertyName) {


I think a switch statement would be a better fit here

Ok I'm going to change this

src/Symfony/Bundle/FrameworkBundle/Command/RouterDebugCommand.php

Tobion · 2020-08-10T13:37:22Z

src/Symfony/Bundle/FrameworkBundle/Tests/Command/RouterDebugCommandTest.php

+        $tester->execute(['--sort' => null], ['decorated' => false]);
+    }
+
+    public function testSortingByPriorityWithDuplicatePath()


i think this test is useless as it doesn't cover anything new

Tobion · 2020-08-10T13:37:32Z

src/Symfony/Bundle/FrameworkBundle/Tests/Command/RouterDebugCommandTest.php

+        $this->assertStringNotContainsString('! [CAUTION] The routes list is not sorted in the parsing order.', $tester->getDisplay(true));
+    }
+
+    public function testSortingByNameWithDuplicatePath()


i think this test is useless as it doesn't cover anything new

Tobion · 2020-08-10T13:38:42Z

src/Symfony/Bundle/FrameworkBundle/Tests/Command/RouterDebugCommandTest.php

+        $this->assertStringContainsString('! [CAUTION] The routes list is not sorted in the parsing order.', $tester->getDisplay(true));
+    }
+
+    public function testSortingByPathWithDuplicatePath()


this test should actually also make sure the routes with the same path stay in the correct order as before (check routes names) as I mention in the stable sorting comment

fabpot · 2020-08-17T06:07:11Z

@vjandrea Can you take the latest comments into account?

@Tobion

c/o @Tobion Co-authored-by: Tobias Schultze <webmaster@tubo-world.de>

@Tobion

c/o @Tobion Co-authored-by: Tobias Schultze <webmaster@tubo-world.de>

fabpot · 2020-09-05T14:42:34Z

@vjandrea It looks like you did some work, but I still see some feedback that are not taken into account. Do you plan to finish this PR soon?

fabpot · 2020-09-19T07:16:31Z

@vjandrea Still willing to finish this PR?

vjandrea · 2020-09-19T08:29:26Z

Yes Fabien, still wishing to complete it but I'm really busy with a tight deadline in this moment, I think I'll have time at the beginning of October. Thanks Il sab 19 set 2020, 09:16 Fabien Potencier <notifications@github.com> ha scritto:

…

@vjandrea <https://github.com/vjandrea> Still willing to finish this PR? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#36579 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAMQKTK2WFKUY3P4UWI5L5LSGRLFZANCNFSM4MQX2ZFQ> .

fabpot · 2020-09-24T15:29:18Z

Re-reading the whole thing, I think that having a --sort flag is wrong for the reasons explained by @Tobion. As the output of a command can be piped through any other command, I think we don't need to support this in core. Let's close.

carsonbot added Status: Needs Review FrameworkBundle Routing Feature labels Apr 25, 2020

vjandrea mentioned this pull request Apr 25, 2020

[FrameworkBundle] [Routing] Added --sort to debug:router symfony/symfony-docs#13591

Closed

chalasr added this to the next milestone Apr 25, 2020

nicolas-grekas reviewed Apr 26, 2020

View reviewed changes

vjandrea requested review from sroze and xabbuh as code owners May 3, 2020 15:05

vjandrea added 3 commits May 3, 2020 19:56

Added debug:router --sort option

f663aa0

Updated CHANGELOG.md

afba921

PR 36579 corrections via @nicolas-grekas

ced94da

vjandrea force-pushed the debug-router-sort branch from 460477f to 2b9e044 Compare May 3, 2020 18:01

vjandrea force-pushed the debug-router-sort branch 2 times, most recently from 6c67ce1 to ced94da Compare June 7, 2020 06:37

nicolas-grekas reviewed Jun 30, 2020

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/Command/RouterDebugCommand.php Outdated Show resolved Hide resolved

Added caution alert when sorting criterion != priority

49029ae

vjandrea force-pushed the debug-router-sort branch from 7af952f to 49029ae Compare July 11, 2020 09:13

Tobion suggested changes Aug 10, 2020

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Aug 10, 2020

vjandrea and others added 2 commits August 20, 2020 09:25

Update src/Symfony/Bundle/FrameworkBundle/Command/RouterDebugCommand.php

6f743c1

c/o @Tobion Co-authored-by: Tobias Schultze <webmaster@tubo-world.de>

Update src/Symfony/Bundle/FrameworkBundle/Command/RouterDebugCommand.php

4a034ac

c/o @Tobion Co-authored-by: Tobias Schultze <webmaster@tubo-world.de>

fabpot closed this Sep 24, 2020

nicolas-grekas modified the milestones: next, 5.2 Oct 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FrameworkBundle] [Routing] Added --sort to debug:router #36579

[FrameworkBundle] [Routing] Added --sort to debug:router #36579

vjandrea commented Apr 25, 2020 •

edited

Loading

nicolas-grekas left a comment

Tobion commented Apr 29, 2020 •

edited

Loading

vjandrea commented May 3, 2020 •

edited

Loading

fabpot commented Jun 3, 2020

vjandrea commented Jun 3, 2020

vjandrea commented Jul 11, 2020

vjandrea commented Jul 24, 2020

Tobion Aug 10, 2020

Tobion Aug 10, 2020

vjandrea Aug 20, 2020

Tobion Aug 10, 2020

vjandrea Aug 20, 2020

Tobion Aug 10, 2020

Tobion Aug 10, 2020

Tobion Aug 10, 2020

fabpot commented Aug 17, 2020

fabpot commented Sep 5, 2020

fabpot commented Sep 19, 2020

vjandrea commented Sep 19, 2020 via email

fabpot commented Sep 24, 2020

[FrameworkBundle] [Routing] Added --sort to debug:router #36579

[FrameworkBundle] [Routing] Added --sort to debug:router #36579

Conversation

vjandrea commented Apr 25, 2020 • edited Loading

nicolas-grekas left a comment

Choose a reason for hiding this comment

Tobion commented Apr 29, 2020 • edited Loading

vjandrea commented May 3, 2020 • edited Loading

fabpot commented Jun 3, 2020

vjandrea commented Jun 3, 2020

vjandrea commented Jul 11, 2020

vjandrea commented Jul 24, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Aug 17, 2020

fabpot commented Sep 5, 2020

fabpot commented Sep 19, 2020

vjandrea commented Sep 19, 2020 via email

fabpot commented Sep 24, 2020

vjandrea commented Apr 25, 2020 •

edited

Loading

Tobion commented Apr 29, 2020 •

edited

Loading

vjandrea commented May 3, 2020 •

edited

Loading