Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[FrameworkBundle] Only show relevant columns indebug: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.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
mamazu wants to merge13 commits intosymfony:7.4
base:7.4
Choose a base branch
Loading
frommamazu:console_clean_ups

Conversation

mamazu
Copy link
Contributor

@mamazumamazu commentedFeb 14, 2025•
edited
Loading

QA
Branch?7.3
Bug fix?no
New feature?yes
Deprecations?yes
IssuesFix#59767
LicenseMIT

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

ro0NL, 94noni, GromNaN, MrYamous, and Spomky reacted with thumbs up emoji
@carsonbot

This comment was marked as outdated.

@mamazumamazu changed the titleOnly show relevant columns in debug:routerOnly show relevant columns indebug:router callFeb 14, 2025
@mamazumamazuforce-pushed theconsole_clean_ups branch 2 times, most recently from821481d to44577d0CompareFebruary 14, 2025 18:34
@mamazu
Copy link
ContributorAuthor

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

Can you share some before/after screenshots?

@mamazu
Copy link
ContributorAuthor

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

@carsonbotcarsonbot changed the titleOnly show relevant columns indebug:router call[FrameworkBundle] Only show relevant columns indebug:router callFeb 15, 2025
@mamazu
Copy link
ContributorAuthor

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 useCOLORTERM=truecolor or should I update the test output to expect the smaller color set?

@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 keepANY 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?

Kocal reacted with thumbs up emoji

@mamazu
Copy link
ContributorAuthor

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

@OskarStark
Copy link
Contributor

OskarStark commentedApr 9, 2025•
edited
Loading

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

@mamazu
Copy link
ContributorAuthor

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

OskarStark reacted with thumbs up emoji

@@ -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.');

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
ContributorAuthor

@mamazumamazuApr 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.

TheTextDescriptor 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.

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
ContributorAuthor

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.

$methods = ['ANY'];
}

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

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
ContributorAuthor

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
Copy link
ContributorAuthor

The test failures aren't related to the feature.

@mamazu
Copy link
ContributorAuthor

@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?

@fabpotfabpot modified the milestones:7.3,7.4May 26, 2025
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas left review comments

@MrYamousMrYamousMrYamous left review comments

Assignees
No one assigned
Projects
None yet
Milestone
7.4
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
[8]ページ先頭
©2009-2025 Movatter.jp