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 1 commit into
base: 7.4
Choose a base branch
from
Open

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

wants to merge 1 commit into from

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 Swagger 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 2 times, most recently 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
@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?

@fabpot fabpot modified the milestones: 7.3, 7.4 May 26, 2025
@mamazu mamazu force-pushed the console_clean_ups branch 3 times, most recently from 2cf3244 to 9fad15b Compare July 6, 2025 11:55
@mamazu mamazu force-pushed the console_clean_ups branch from 9fad15b to bd7a1d2 Compare July 6, 2025 11:55
stloyd
stloyd reviewed Jul 6, 2025
View reviewed changes
src/Symfony/Bundle/FrameworkBundle/Console/Descriptor/TextDescriptor.php
Comment on lines +619 to +625
if (count($methods) === 0) {
$methods = ['ANY'];
}

return implode('|', array_map(
fn (string $method): string => sprintf('<fg=%s>%s</>', self::VERB_COLORS[$method] ?? 'default', $method),
$methods
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (count($methods) === 0) {
$methods = ['ANY'];
}
return implode('|', array_map(
fn (string $method): string => sprintf('<fg=%s>%s</>', self::VERB_COLORS[$method] ?? 'default', $method),
$methods
return implode('|', array_map(
fn (string $method): string => sprintf('<fg=%s>%s</>', self::VERB_COLORS[$method] ?? 'default', $method),
$methods ?: ['ANY']

src/Symfony/Bundle/FrameworkBundle/Console/Descriptor/TextDescriptor.php
'' !== $route->getHost() ? $route->getHost() : 'ANY',
$this->formatControllerLink($controller, $route->getPath(), $options['container'] ?? null),
'Name' => $name,
'Methods' => $this->formatMethods($route->getMethods()),
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no way to disable colors which is BC break if someone depends on the output, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, currently there is no BC layer planned. As discussed in the thread we don't expect anyone using this output for automation (like scripting) and only for visual output.

@mamazu
Copy link
Contributor Author

mamazu commented Jul 6, 2025

Rebased to 7.4 and fixed the conflicts.

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

@stloyd stloyd stloyd left review comments

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

Successfully merging this pull request may close these issues.

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