Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[FrameworkBundle] Fail properly on unregistrable command#48547
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
base:7.4
Are you sure you want to change the base?
Conversation
carsonbot commentedDec 8, 2022
Hey! To help keep things organized, we don't allow "Draft" pull requests. Could you please click the "ready for review" button or close this PR and open a new one when you are done? Note that a pull request does not have to be "perfect" or "ready for merge" when you first open it. We just want it to be ready for a first review. Cheers! Carsonbot |
| $this->renderRegistrationErrors($input, $output); | ||
| } | ||
| $this->setDispatcher($this->kernel->getContainer()->get('event_dispatcher')); | ||
| return parent::doRun($input, $output); | ||
| returnmax($statusCode,parent::doRun($input, $output)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think we should not change the status code of all existing commands.
Instead, I propose that we change the status code of the "list" command only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Hm, but why? Is there any reason any command should succeed when there are registration errors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Same question the other way around, I'm against breaking the whole console when only one command is down.
Uh oh!
There was an error while loading.Please reload this page.
When commands are not possible to be registered, the console should return non-zero status code and not fail silently
Real-case scenario: SncRedisBundle v4.4 broke the rest of my commands because its commands could not be registeredsnc/SncRedisBundle#683.
I run
bin/consolein CI to see if something is broken.But even though in CI there were tonz of warnings, it returned Success at the end and the broken application slipped into the production and broke it.