[Console] Skip commands from ConsoleCommandEvent #9235
Conversation
| /** | ||
| * The return code for skipped commands, this will also be passed into the terminate event | ||
| */ | ||
| const RETURN_CODE_DISABLED = -1024; |
There was a problem hiding this comment.
This is not a valid exist code (http://www.tldp.org/LDP/abs/html/exitcodes.html).
Use case: We have different variations of the same application, for which only certain commands are allowed. Right now this is done in a custom Application class, but it would be much easier to just be able to skip commands from a listener, where you can disable commands via the Event object. This patch provides this feature and corresponding test cases. | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes - for Console tests | Fixed tickets | None | License | MIT | Doc PR | None
|
@fabpot I now use return code |
|
👍 Can you create a PR on the docs as well? |
| /** | ||
| * The return code for skipped commands, this will also be passed into the terminate event | ||
| */ | ||
| const RETURN_CODE_DISABLED = 113; |
There was a problem hiding this comment.
Why 113 ?
I think this constant should be defined in Application class because it is used in it and makes the return code of the application.
There was a problem hiding this comment.
@GromNaN http://www.tldp.org/LDP/abs/html/exitcodes.html @fabpot pointed me to this document, and there the author recommends the return codes within the range 64-113, and I am a kind of max guy, so I chose the upper range limit.
| Q | A | ------------- | --- | Doc fix? | no | New docs? | yes (symfony/symfony#9235) | Applies to | master | Fixed tickets | none
|
@fabpot Created PR on the docs :) |
| Q | A | ------------- | --- | Doc fix? | no | New docs? | yes (symfony/symfony#9235) | Applies to | master | Fixed tickets | none
| Q | A | ------------- | --- | Doc fix? | no | New docs? | yes (symfony/symfony#9235) | Applies to | master | Fixed tickets | none
| Q | A | ------------- | --- | Doc fix? | no | New docs? | yes (symfony/symfony#9235) | Applies to | master | Fixed tickets | none
|
Thank you @tPl0ch. |
This PR was merged into the 2.6-dev branch. Discussion ---------- [Console] Skip commands from ConsoleCommandEvent Use case: We have different variations of the same application, for which only certain commands are allowed. Right now this is done in a custom Application class, but it would be much easier to just be able to skip commands from a listener, where you can disable commands via the Event object. This patch provides this feature and corresponding test cases. | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes - for Console tests | Fixed tickets | None | License | MIT | Doc PR | symfony/symfony-docs#4058 Commits ------- acb1ae6 [Console] Skip commands from ConsoleCommandEvent
| * | ||
| * @return bool | ||
| */ | ||
| public function commandShouldRun() |
There was a problem hiding this comment.
should is the wrong term. if "should" says no, it doesn't mean it's not allowed to run. but that is what it is used for.
There was a problem hiding this comment.
Since the return code is named _DISABLED. maybe this method should be called commandIsDisabled()?
|
The disabled commands are still listed in the help. But when you call them, it won't work. So I dislike the implementation at this stage. 👎 |
|
@Tobion Well, commands are registered during compile time, and this feature is making it possible to disable a command during run time, i.e. because an important resource is locked or some other restrictions apply. The only thing I can see is making the failure more explicit by throwing sth. like a |
|
I'm saying disabled commands should either not be listed or mention that they are currently disabled. |
This PR was merged into the master branch. Discussion ---------- Skip console commands from event listeners | Q | A | ------------- | --- | Doc fix? | no | New docs? | yes (symfony/symfony#9235) | Applies to | master | Fixed tickets | none Commits ------- cb508f7 [WCM] Skip console commands from event listeners
Use case: We have different variations of the same application, for which
only certain commands are allowed. Right now this is done in a custom
Application class, but it would be much easier to just be able to skip
commands from a listener, where you can disable commands via the Event
object.
This patch provides this feature and corresponding test cases.