8000 [DX][Console] With many commands, registration warnings are not visible · Issue #26203 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[DX][Console] With many commands, registration warnings are not visible #26203

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
Closed
mhujer opened this issue Feb 17, 2018 · 6 comments
Closed

[DX][Console] With many commands, registration warnings are not visible #26203

mhujer opened this issue Feb 17, 2018 · 6 comments
Labels
Console DX DX = Developer eXperience (anything that improves the experience of using Symfony) Feature

Comments

@mhujer
Copy link
Contributor
mhujer commented Feb 17, 2018
Q A
Bug report? no
Feature request? yes
BC Break report? no
RFC? no
Symfony version 4.0.4

Steps to reproduce

  1. create a command e.g. system:clear-logs
  2. forget to call parent::__construct();
  3. run bin/console
  4. command is not listed in the available commands
  5. no warning is visible (but it is reported correctly, read on)

Output:
image

The warning is printed correctly, but is like two screens of scrolling above. And it looked more like a Command registration issue, so there was no reason to scroll up. I discovered it by accident, after checking the DI configuration, trying manually tagging the command etc.

image

Idea 1: Move the command registration warnings bellow the command output. (At least for the "default" command)

Idea 2: Change the warning to exception. I think it is kind of similar to missing scalar constructor parameter, which throws an exception.

What do you think?
@javiereguiluz javiereguiluz added Feature Console DX DX = Developer eXperience (anything that improves the experience of using Symfony) labels Feb 17, 2018
@chalasr
Copy link
Member
chalasr commented Feb 18, 2018

The idea is to not prevent running other commands when only some of them could not be registered, so throwing would be doing one step back.
👍 for displaying registration errors after running the command.

@nicolas-grekas
Copy link
Member

I'm not sure this would be better actually. The command being run is more important than the others at this time.

@mhujer
Copy link
Contributor Author
mhujer commented Feb 18, 2018

I agree that when a specific command is ran, the warning does not matter that much, so it should not mess with the output of the command.

What about this:

  • when a specific command is ran, display the warning before the command output (current behaviour)
  • when a default - list - command is ran, display the warning after the list of commands, as it tends to be longer than a screen (change)

@Simperfit
Copy link
Contributor

👍.
@mhujer do you want to work on it ?

@mhujer
Copy link
Contributor Author
mhujer commented Feb 23, 2018

@Simperfit I probably won't get around this until next week, so if you want to fix it, you are welcome 😃

@Simperfit
Copy link
Contributor

Yeah, I'm going to do it ;)

@Simperfit Simperfit mentioned this issue Feb 23, 2018
Merged
@fabpot fabpot closed this as completed Mar 15, 2018
fabpot added a commit that referenced this issue Mar 15, 2018
@fabpot
feature #26288 [FrameworkBundle] show the unregistered command warnin…
bf120d0 
…g at the end of the list command (Simperfit)

This PR was merged into the 4.1-dev branch.

Discussion
----------

[FrameworkBundle] show the unregistered command warning at the end of the list command

| Q             | A
| ------------- | ---
| Branch?       | 4.1
| Bug fix?      | no
| New feature?  | yes   <!-- don't forget to update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no   <!-- don't forget to update UPGRADE-*.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #26203  <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | I don't think we need one.

This PR modify the warning error when you have unregistered command on the list command, it shows it at the end.

Commits
-------

99b104a [FrameworkBundle] show the unregistered command warning at the end of the list command
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Console DX DX = Developer eXperience (anything that improves the experience of using Symfony) Feature
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@fabpot @javiereguiluz @nicolas-grekas @mhujer @Simperfit @chalasr
0