Skip to content

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

Already on GitHub? Sign in to your account

Closed

Conversation

l-vo
Copy link
Contributor

@l-vo l-vo commented Nov 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):

capture d ecran 2018-11-25 a 17 53 04

This PR fixes this behavior.

@l-vo l-vo force-pushed the remove_command_placeholder_from_sypnosis branch from 0c959e9 to ae5150a Compare November 25, 2018 17:02
@chalasr chalasr added this to the 2.8 milestone Nov 25, 2018
@l-vo l-vo force-pushed the remove_command_placeholder_from_sypnosis branch from ae5150a to 1456d19 Compare November 25, 2018 17:18
@ro0NL
Copy link
Contributor

ro0NL commented Nov 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

@ro0NL
Copy link
Contributor

ro0NL commented Nov 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.

@nicolas-grekas
Copy link
Member

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

@nicolas-grekas nicolas-grekas removed this from the 2.8 milestone Nov 26, 2018
("<command>" is displayed but should not be displayed)
@l-vo l-vo force-pushed the remove_command_placeholder_from_sypnosis branch from 1456d19 to 5ee9451 Compare November 26, 2018 12:15
@l-vo l-vo requested review from dunglas and lyrixx as code owners November 26, 2018 12:15
@l-vo l-vo changed the base branch from 2.8 to 3.4 November 26, 2018 12:17
@l-vo
Copy link
Contributor Author

l-vo commented Nov 26, 2018

@nicolas-grekas rebased on 3.4. thanks.

@l-vo
Copy link
Contributor Author

l-vo commented Nov 26, 2018

@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. In getSynopsis method, the task name is hardcoded. So I remove the task name placeholder from the displayed arguments. It's nothing more.

@ro0NL
Copy link
Contributor

ro0NL commented Nov 26, 2018

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 command

Usage:
  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) :)

@l-vo
Copy link
Contributor Author

l-vo commented Nov 26, 2018

@ro0NL thanks for your clarifications, I was wrong, so it's not a composer bug.

@l-vo
Copy link
Contributor Author

l-vo commented Nov 26, 2018

Status: Needs Work

@ro0NL
Copy link
Contributor

ro0NL commented Nov 26, 2018

If you like we can re-try #20054 on master. That time i was pretty convinced about it :)

@l-vo
Copy link
Contributor Author

l-vo commented Nov 26, 2018

@ro0NL I'm not sure to understand why you closed it ?

@ro0NL
Copy link
Contributor

ro0NL commented Nov 26, 2018

no feedback :) felt like nobody dares to touch it / get involved :P

@l-vo
Copy link
Contributor Author

l-vo commented Nov 29, 2018

@ro0NL

If you like we can re-try #20054 on master. That time i was pretty convinced about it :)

Yes please, I tested it on 3.4, it works fine and the approach looks good to me :)

@l-vo
Copy link
Contributor Author

l-vo commented Nov 29, 2018

Closed in favor of #20054

@l-vo l-vo closed this Nov 29, 2018
@l-vo l-vo deleted the remove_command_placeholder_from_sypnosis branch November 29, 2018 21:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants