Add priority to debug router #36110
Add priority to debug router #36110
Conversation
| @@ -209,6 +209,7 @@ protected function getRouteData(Route $route): array | |||
| 'hostRegex' => '' !== $route->getHost() ? $route->compile()->getHostRegex() : '', | |||
| 'scheme' => $route->getSchemes() ? implode('|', $route->getSchemes()) : 'ANY', | |||
| 'method' => $route->getMethods() ? implode('|', $route->getMethods()) : 'ANY', | |||
| 'priority' => $route->getPriority() ? $route->getPriority() : '', | |||
There was a problem hiding this comment.
why sometimes you compare 0 !== $route->getPriority() and sometimes only $route->getPriority()?
maybe using the same pattern is better
| * | ||
| * @return integer The priority | ||
| */ | ||
| public function getPriority(): ?int |
There was a problem hiding this comment.
As you can see in the implementation, routes don't have priorities on their own right now.
The command is not enough a reason to introduce this concept on routes IMHO.
I looked for alternatives and I propose to add a RouteCollection::getPriorities() method, which would just return $this->priorities. That should allow building the command without introducing any new concepts (since "priority" is already a concept of RouteCollection)
There was a problem hiding this comment.
Thank you @MrYamous for working on this feature. I've tested it with a custom SF app, looks good but it requires some adjustments related to the followings:
- as @nicolas-grekas already suggested, probably it would be better to keep this concept under the
RouteCollection - regarding the failing unit tests, I think, the source of the issue is
trim_trailing_whitespace = trueinside of.editorconfigfor both cases (Text and Markdown). I guess your IDE automatically removed some spaces in the fixtures files after save.
Let me know if you need further clarification.
Furthermore, could you please add the names of modified components to the title of the PR?
Cheers
Status: Needs Work
|
Friendly ping @MrYamous |
|
Hello, thank you for your feedback and sorry for the wait. |
Following #35608 and suggestion in this comment
Considering new priority parameter in route annotations, this PR add this parameter in the output of debug:router command
Sorry for this incomplete PR, i'm still having an issue with a TextDescriptorTest, i didn't find a good way to resolve it :/
I've also noticed some tests fails about MarkdownDescriptor that didn't seem to be related to this RP, I didn't know if it was appropriate to modify them at the same time.