Skip to content

[FrameworkBundle] Only show relevant columns in debug:router call #59780

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 13 commits into
base: 7.3
Choose a base branch
from
Open

[FrameworkBundle] Only show relevant columns in debug:router call #59780

wants to merge 13 commits into from
+220 −31

Conversation

mamazu
Copy link
Contributor

@mamazu mamazu commented Feb 14, 2025 •
edited
Loading

Q A
Branch? 7.3
Bug fix? no
New feature? yes
Deprecations? yes
Issues Fix #59767
License MIT

What I did

  • Adding more tests for the descriptor (empty route collection)
  • Always showing the controllers in the list view (we but not removing the --show-controllers so it's not a BC break)
  • Only show the "Host" and "Schema" columns when the values are != "ANY"
  • Adding colors to the HTTP methods (it's the same colors Laravel uses)

Why

  • Color coding the HTTP methods gives you more info at a quick glance
  • Hiding columns that don't provide values reduces the noise of the table (for more info there is still detail view)

Screenshots

Before:
image

After:
image

@carsonbot carsonbot added this to the 7.3 milestone Feb 14, 2025
@carsonbot

This comment was marked as outdated.

Sign in to view
@mamazu mamazu changed the title Only show relevant columns in debug:router Only show relevant columns in debug:router call Feb 14, 2025
@mamazu mamazu force-pushed the console_clean_ups branch 2 times, most recently from 821481d to 44577d0 Compare February 14, 2025 18:34
@mamazu
Copy link
Contributor Author

mamazu commented Feb 14, 2025

Oddly enough the tests work locally. Any idea why? Or is there a way to generate the expected output from the input?

@fabpot
Copy link
Member

fabpot commented Feb 15, 2025

Can you share some before/after screenshots?

@mamazu
Copy link
Contributor Author

mamazu commented Feb 15, 2025

Added screenshots oddly enough it does not show the deprecation notice I added.

@carsonbot carsonbot changed the title Only show relevant columns in debug:router call [FrameworkBundle] Only show relevant columns in debug:router call Feb 15, 2025
@mamazu
Copy link
Contributor Author

mamazu commented Feb 17, 2025

Okay the last remaining error I presume is because the CI tests do not use a true color terminal. Question: should I set the CI colors to use COLORTERM=truecolor or should I update the test output to expect the smaller color set?

@ro0NL ro0NL mentioned this pull request Mar 25, 2025
Open
@javiereguiluz
Copy link
Member

@mamazu I like this a lot. Thanks!

Two quick comments:

(1): I'd use the HTTP method colors from the OpenAPI standard (and keep ANY without color). See e.g. Swagger:

http-colors

(2): I'm divided about outputting the controller. It's useful, but it will break the table design for most people.

See your screenshot. Even if you use very short controllers (most folks will display long FQCN) the resulting table is pretty wide. In practice, each row could be displayed in two rows instead. So, why not display each route in two lines since the beginning to avoid breaking the design?

@mamazu
Copy link
Contributor Author

mamazu commented Mar 27, 2025

@javiereguiluz Thanks for the feedback.

  1. Updated. There wasn't much of a difference.

  2. For me the main reason to use the debug:router is to see which controller belongs to which route. But I didn't want to remove the route name name. And the problem with multiple lines per entry destroys the command's grepability.

@mamazu mamazu force-pushed the console_clean_ups branch from 6534fa7 to 78b95e8 Compare April 8, 2025 17:22
@mamazu mamazu force-pushed the console_clean_ups branch from af774f0 to fe882b3 Compare April 9, 2025 07:02
@OskarStark
Copy link
Contributor

OskarStark commented Apr 9, 2025 •
edited
Loading

Because you mentioned "grepability", another approach can also be to use --format=json and then use jq binary

@mamazu
Copy link
Contributor Author

mamazu commented Apr 9, 2025

You are completely correct, the text representation contains the least amount of infos anyways, so any other format for searching would also work.

nicolas-grekas
nicolas-grekas reviewed Apr 16, 2025
View reviewed changes
src/Symfony/Bundle/FrameworkBundle/Command/RouterDebugCommand.php Outdated
@@ -84,6 +84,9 @@ protected function execute(InputInterface $input, OutputInterface $output): int
if ($this->fileLinkFormatter) {
$container = fn () => $this->getContainerBuilder($this->getApplication()->getKernel());
}
if ($input->getOption('show-controllers')) {
trigger_deprecation('symfony/console', '7.3', 'Using --show-controllers is deprecated. They are shown by default now.');
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I see the point of this change. The arguments in the PR description look like personal preference to me. I'd prefer not changing this.

Copy link
Contributor Author

@mamazu mamazu Apr 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

In general yes, it's personal preference but there is also a technical argument to it.

The TextDescriptor in List mode is the only thing that supports this flag. If you provide an argument for getting the route details or if you use a different formatter none of them use this flag. So this would also remove inconsistency in my opinion.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think that's sound enough to warrant a deprecation. Consistency is not the main aspect here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough. I'll remove it.

nicolas-grekas
nicolas-grekas reviewed Apr 16, 2025
View reviewed changes
src/Symfony/Bundle/FrameworkBundle/Console/Descriptor/TextDescriptor.php Outdated
$methods = ['ANY'];
}

return implode('|', array_map(fn (string $method): string => '<fg='.self::VERB_COLORS[$method].'>'.$method.'</>', $methods));
Copy link
Member

Choose a reason for hiding this comment

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

It's possible that $method is not defined in VERB_COLORS (eg TRACE, PURGE, or any custom verb)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should fall back to default now.

@mamazu mamazu force-pushed the console_clean_ups branch from d5d400c to b6f285a Compare April 16, 2025 17:25
@mamazu
Copy link
Contributor Author

mamazu commented Apr 22, 2025

The test failures aren't related to the feature.

@mamazu
Copy link
Contributor Author

mamazu commented May 1, 2025

@nicolas-grekas So I've removed the deprecation. (I don't know if I can remove the deprecation tag from the PR) Secondly I don't know how the pipeline failures are related to the feature. Could you please restart it or is there anything else I could do?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas left review comments

@MrYamous MrYamous MrYamous left review comments

Assignees
No one assigned
Labels
Deprecation Feature FrameworkBundle Status: Needs Review
Projects
None yet
Milestone
7.3
Development

Successfully merging this pull request may close these issues.

Improved output of debug route collection
7 participants
@mamazu @carsonbot @fabpot @javiereguiluz @OskarStark @nicolas-grekas @MrYamous