Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Console] Allow to catch CommandNotFoundException#22181
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| private$handled =false; | ||
| publicfunction__construct(Command$command,InputInterface$input,OutputInterface$output,$error,$exitCode) | ||
| publicfunction__construct(Command$command =null,InputInterface$input,OutputInterface$output,$error,$exitCode) |
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.
It's always weird to have optional argument before required one. Could we do something for that ?
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.
Agreed, arguments have been reordered
2337015 to0ff0123Comparechalasr commentedMar 29, 2017
ping@wouterj |
| $exception =$exitCode; | ||
| $exitCode =$tmpExitCode; | ||
| @trigger_error(sprintf('Passing an instance of %s as first argument of %s() is deprecated since version 3.3. Pass it as 5th argument instead.', Command::class,__METHOD__),E_USER_DEPRECATED); |
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.
This deprecation is useless imo. People creating the class as($command, $input, $output) already get a deprecation, as the class shouldn't be initialized (ConsoleErrorEvent should be initialized instead).
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.
Indeed, deprecation removed.
fc50aef toca50cf9Compare| private$exitCode; | ||
| publicfunction__construct(Command$command,InputInterface$input,OutputInterface$output,\Exception$exception,$exitCode,$deprecation =true) | ||
| publicfunction__construct($input,$output,$exception,$exitCode,$command =null,$deprecation =true) |
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.
Given that the class is deprecated anyway I would just allow$command to benull instead of shifting constructor arguments.
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.
Thanks, I wasn't sure. Fixed.
ca50cf9 tob21ce85Comparechalasr commentedMar 30, 2017
Ready, build failure unrelated. |
chalasr commentedApr 5, 2017
Bump, it will be harder to make this change after 3.3 (would involve deprecations). |
fabpot commentedApr 5, 2017
Thank you@chalasr. |
…lasr)This PR was merged into the 3.3-dev branch.Discussion----------[Console] Allow to catch CommandNotFoundException| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#22144 (comment)| License | MIT| Doc PR | n/aBasically reverts#22144, making the command argument optional in console.error event instead, so that `CommandNotFoundException` can be handled as any other console error.Commits-------b21ce85 [Console] Allow to catch CommandNotFoundException
Basically reverts#22144, making the command argument optional in console.error event instead, so that
CommandNotFoundExceptioncan be handled as any other console error.