Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add priority to debug router #36110

Open
wants to merge 4 commits into
base: master
from

Conversation

@MrYamous
Copy link

MrYamous commented Mar 17, 2020

Q A
Branch? master
Bug fix? no
New feature? yes
Deprecations? no
Tickets /
License MIT
Doc PR On my way

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.

@@ -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() : '',

This comment has been minimized.

Copy link
@noniagriconomie

noniagriconomie Mar 18, 2020

Contributor

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

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Mar 18, 2020

Member

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)

@nicolas-grekas nicolas-grekas added this to the next milestone Mar 18, 2020
Copy link
Contributor

gyszucs left a 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 = true inside of .editorconfig for 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

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Mar 31, 2020

Friendly ping @MrYamous

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.