NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[Console] Fix synopsis when an error occurs#29320

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

Jump to bottom

Closed

l-vo wants to merge1 commit intosymfony:3.4froml-vo:remove_command_placeholder_from_sypnosis

Closed

[Console] Fix synopsis when an error occurs#29320

l-vo wants to merge1 commit intosymfony:3.4froml-vo:remove_command_placeholder_from_sypnosis

Conversation

Copy link

Contributor

l-vo commentedNov 25, 2018

Q	A
Branch?	2.8
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no->
Tests pass?	yes
Fixed tickets	#...
License	MIT
Doc PR	symfony/symfony-docs#...

The short help displayed when an error occurs (synopsis) displays the real command name but also a placeholder for this command name (debug:container and<command> are the same thing):

This PR fixes this behavior.

carsonbot added Status: Needs Review Console Bug labels

Nov 25, 2018

l-vo force-pushed theremove_command_placeholder_from_sypnosis branch from0c959e9 toae5150aCompare

November 25, 2018 17:02

chalasr added this to the2.8 milestone

Nov 25, 2018

l-vo force-pushed theremove_command_placeholder_from_sypnosis branch fromae5150a to1456d19Compare

November 25, 2018 17:18

Copy link

Contributor

ro0NL commentedNov 25, 2018

Im not sure about the fix. AFAIK the root-cause is a bit deeper, as we wrongly merge the application- and command definitions at some point.

It doesnt solve e.g.#17804

Copy link

Contributor

ro0NL commentedNov 25, 2018

The last time i took a stab it i ended up with#20054. If i remember well it solved the synopsis as well.

Copy link

Member

nicolas-grekas commentedNov 26, 2018

Should target 3.4 now. Or maybe master btw if this is considered as an improvement.

nicolas-grekas removed this from the2.8 milestone

Nov 26, 2018

[Console] Fix synopsis when an error occurs

5ee9451

("<command>" is displayed but should not be displayed)

l-vo force-pushed theremove_command_placeholder_from_sypnosis branch from1456d19 to5ee9451Compare

November 26, 2018 12:15

l-vo requested review fromdunglas andlyrixx ascode owners

November 26, 2018 12:15

l-vo changed the base branch from2.8 to3.4

November 26, 2018 12:17

Copy link

ContributorAuthor

l-vo commentedNov 26, 2018

@nicolas-grekas rebased on 3.4. thanks.

Copy link

ContributorAuthor

@ro0NL I think it is normal that this PR doesn't fix#17804. I don't reproduce this bug on Symfony commands. Maybe this bug was existing before in Symfony codebase but it seems to not be the case anymore (or perhaps with a specific behavior ?). I'm ok that this bug still exists in composer, but composer override widely the Application class. IMO it's a composer bug.

I read#20054, my PR has not the ambition to rethink the arguments merging between Application and Command. Its goal is more straightforward. IngetSynopsis method, the task name is hardcoded. So I remove the task name placeholder from the displayed arguments. It's nothing more.

Copy link

Contributor

ro0NL commentedNov 26, 2018•
edited
Loading

In getSynopsis method, the task name is hardcoded. So I remove the task name placeholder from the displayed arguments

but having this unexpected argument in the command definition smells already, right now we're kinda ducktaping it.

$ bin/console help   Description:  Displays help for a commandUsage:  help [options] [--] <command> [<command_name>]Arguments:  command               The command to execute           <==== THIS =====<  command_name          The command name [default: "help"]

I think the issue is still there (4.2) :)