[Console] Add support for SignalableCommandInterface with invokable commands
#60389
Conversation
b2deac6 to
9767999
Compare
9767999 to
ae770b7
Compare
|
I believe there’s another way to keep this working without implementing the interface: https://github.com/symfony/symfony-docs/pull/20932/files#diff-738612fa300cdcb8d044a240dcec7bc9db05a1671f30e3d1b987bbf200037b8c but I might be missing something, could you please confirm? |
|
@yceruto it should work indeed, but the proposed approach makes sense to me also. |
|
@yceruto I remember we discussed this at the time and you had some arguments against |
|
Yes, I'd prefer a single way to do this for invokable command, less variants to document and maintain. |
ae770b7 to
bb97a2a
Compare
|
@yceruto Just a heads up, |
|
I understand the intention of supporting all features used in normal commands, but in standalone usage, the invokable command approach doesn’t work naturally, people need to call explicitly setCode(), name, description, etc. In those cases, you’ll probably extend the |
|
My argument was more about why not to deprecate or remove the |
|
In those standalone cases without an |
|
If the goal is to simplify support for invokable commands, I suggest keeping this PR as-is with the |
|
@yceruto If you mean we should not support |
|
I’m fine with invokable commands not supporting all CLI features out of the box. If extending the Command class is the way to enable them, that’s totally acceptable to me, just like we already do with invokable controllers and AbstractController. I don’t mind extending a class if that’s all it takes to unlock the functionality I need. |
|
Sure, but why make it more complex than expected? I don't see your point sorry. |
|
Shortcut methods and unsupported features are different stories. Stop me if I'm wrong but there is no AbstractController feature that cannot be implemented the same way by controllers not extending it |
|
My goal is to make it simpler for developers, but I’m probably wrong in this case, sorry for the noise and confusion @nicolas-grekas. |
|
No worries, it's always useful to have a discussion. Here I don't see much added complexity (I see this as simpler actually, on the Command side) so I'm fine with the change. |
|
Thank you @HypeMC. |
|
@HypeMC @nicolas-grekas having I unfortunately implemented something with the same method name... Is it something covered by the BC promise (and hence a bug/breaking changes) or not? Edit: I'm not the only one it seems 😓 mac-cain13/daemonizable-command#60 |
…mmands when the component is used standalone (HypeMC) This PR was merged into the 7.4 branch. Discussion ---------- [Console][FrameworkBundle] Simplify using invokable commands when the component is used standalone | Q | A | ------------- | --- | Branch? | 7.4 | Bug fix? | no | New feature? | yes | Deprecations? | yes | Issues | - | License | MIT Inspired by #60389 (comment) . This PR enables using invokable command when the component is used standalone: ```php #[AsCommand(name: 'app:example')] class ExampleCommand implements SignalableCommandInterface { public function __invoke(InputInterface $input, OutputInterface $output): int { // ... return Command::SUCCESS; } public function getSubscribedSignals(): array { return [\SIGINT, \SIGTERM]; } public function handleSignal(int $signal, int|false $previousExitCode = 0): int|false { // handle signal return 0; } } $application = new Application(); $application->addCommand(new ExampleCommand()); $application->run(); ``` Commits ------- 1886c10 [Console] Simplify using invokable commands when the component is used standalone
Currently, it's not possible to use the
SignalableCommandInterfacewith invokable commands.This PR adds support for that.