[Console] Add getter for the original command "code" object #61078
Conversation
|
Hey! Thanks for your PR. You are targeting branch "7.4" but it seems your PR description refers to branch "7.4 for features". Cheers! Carsonbot |
Can you elaborate on this? Why are they lost? Also: Please always provide tests that cover your change. |
|
@derrabus - I added a hyperlink to the OP explaining whats meant by "wrapping command". Let me know if its still unclear. If anyone can suggest an existing test to modify, I will do so. |
|
I don't understand why you need to access attributes of the wrapped invokable command from the actual |
|
Jérôme has a good point about manipulating the command at compile time instead of runtime. When accessing the service you get the real instance . |
It is. Your link is pointing to some line in our codebase, but that line does not explain your use-case. It neither tells me what you're trying to achieve, nor why you want to do that, nor how you tried to do that.
This is a new feature, which means we need new tests. You can have a look at the existing test suite for inspiration, but we won't write those tests for you. And we won't merge a new feature without any tests. |
|
Apologies for not including my use case originally. I'm no Symfony expert, so your expertise here is much appreciated. I'm the maintainer of Drush, the CLI for Drupal. Drush is a happy user of the standalone Console component. I am exploring using Invokable Commands in our next version. Symfony made this possible in May in #60394. Drush has no command services at this time, so the suggestion to read Attributes at compile time won't work. We need a runtime solution, and so will other standalone Console users. We could read the Attributes as we add commands to the Application, but then we would have to store them separate from the Command object. Or we would have to extend Command and add a method to get the callable with its Attributes. That feels awkward enough that we wouldn't support Invokable Commands which is a shame IMO - they are great. |
|
Thanks for the context. I see that Drush is built with |
|
We are working to remove the dependency on consolidation/annotated-command in favor native Symfony Console conventions. See drush-ops/drush#6135. This Console PR is part of the effort. Symfony Console supports a few Attributes like AsCommand and Argument and Option - we need a lot more in Drush. We can add declare Attribute classes and add them to our Invokable Commands but we can't cleanly read the Attributes - thus this PR. FYI, I removed the return type hint to an Internal class. |
|
I have added tests to the PR. I'm getting a failure because I removed the return type hint to an internal class. How can we best resolve this? So, do we think this is solvable for us and other standalone Console users? I hope so. |
src/Symfony/Component/Console/Tests/Command/InvokableCommandTest.php
Outdated
Show resolved
Hide resolved
|
I think I've addressed all feedback. The one test failure is unrelated to this PR. |
src/Symfony/Component/Console/Tests/Command/InvokableCommandTest.php
Outdated
Show resolved
Hide resolved
|
Changelog entry added. |
This name matches the existing |
|
Anything else needed here before merge? Thanks all. |
4161145 to
cc9aaa9
Compare
|
@symfony/mergers Could you review this PR please? |
src/Symfony/Component/Console/Tests/Command/InvokableCommandTest.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Console/Tests/Command/InvokableCommandTest.php
Outdated
Show resolved
Hide resolved
…code" object (weitzman) This PR was squashed before being merged into the 7.4 branch. Discussion ---------- [Console] Add getter for the original command "code" object | Q | A | ------------- | --- | Branch? | 7.4 | Bug fix? | no | New feature? | yes | Deprecations? | no | License | MIT I'm playing with Invokable Commands and am seeing a limitation which is bothersome. Attributes attached to the invokable class are lost by the time we have a `Command` added to the Application. Consider an invokable class like below. The #[CLI\FieldLabels] and #[CLI\DefaultFields] Attributes are not retrievable from the Command. That is, there is no easy way to access the actual InvokableCommand object instead of the [wrapping Command](https://github.com/symfony/symfony/blob/a384c231a0051c0ac10e89c2a0516343f9fd3187/src/Symfony/Component/Console/Application.php#L553). With this PR, Attributes become accessible via `$command->getCode()->getCallable()` ```php namespace Drush\Commands\core; #[AsCommand( name: 'twig:unused', description: 'Find potentially unused Twig templates.', aliases: ['twu'], )] #[CLI\FieldLabels(labels: ['template' => 'Template', 'compiled' => 'Compiled'])] #[CLI\DefaultFields(fields: ['template', 'compiled'])] final class TwigUnusedCommand { public function __invoke( #[Argument(description: 'A comma delimited list of paths to recursively search')] string $searchpaths, InputInterface $input, OutputInterface $output ): int { $data = $this->doExecute($searchpaths); $this->writeFormattedOutput($input, $output, $data); return Command::SUCCESS; } } ``` Commits ------- b57d275 [Console] Add getter for the original command "code" object
71a85eb to
b57d275
Compare
|
It took time, but here we go, this is in now. Thank you very much @weitzman. |
I'm playing with Invokable Commands and am seeing a limitation which is bothersome. Attributes attached to the invokable class are lost by the time we have a
Commandadded to the Application. Consider an invokable class like below. The #[CLI\FieldLabels] and #[CLI\DefaultFields] Attributes are not retrievable from the Command. That is, there is no easy way to access the actual InvokableCommand object instead of the wrapping Command.With this PR, Attributes become accessible via
$command->getCode()->getCallable()