-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Console] Add broader support for command "help" definition #59473
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
src/Symfony/Component/Console/DependencyInjection/AddConsoleCommandPass.php
Show resolved
Hide resolved
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'm worried that these texts will be unnecessarily duplicated when the container is compiled.
bb48d97
to
92d279b
Compare
It's already the same case for |
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.
That was on my list, thanks!
92d279b
to
d939cf3
Compare
Just rebased and conflicts solved. |
This wasn't on my radar but good call! |
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.
Like it 🖖
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'm worried that these texts will be unnecessarily duplicated when the container is compiled.
It's already the same case for name and description when configured via tag; I don't see a problem here.
The help text can be large, the duplication in the dumped container is only for commands using the attribute to declare it (twice in the XML and once in the PHP).
src/Symfony/Component/Console/DependencyInjection/AddConsoleCommandPass.php
Show resolved
Hide resolved
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php
Show resolved
Hide resolved
@@ -100,6 +100,10 @@ public function __construct(?string $name = null) | |||
$this->setDescription(static::getDefaultDescription() ?? ''); | |||
} | |||
|
|||
if ('' === $this->help && $attributes = (new \ReflectionClass(static::class))->getAttributes(AsCommand::class)) { | |||
$this->setHelp($attributes[0]->newInstance()->help ?? ''); | |||
} |
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.
we should either add addDefaultHelp or deprecate getDefaultName/Description to be consistent
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.
The name and description static methods remain useful for lazy commands, but this approach is not necessary for help
. Additionally, introducing a new getDefaultHelp()
method seems redundant since it's already possible to override getHelp()
when customization is needed.
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.
would you prefer adding a private static method for getDefaultHelp()
purely for the sake of consistency?
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.
The name and description static methods remain useful for lazy commands
Are they really? I miss a case where it cannot be found in the tag and #[AsCommand]
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.
Hmm, I think I missed the fact that the AsCommand
attribute is always auto-configured, so yes! they can now be deprecated. I'll make the changes in another PR, as it's not directly related to the feature proposed here.
src/Symfony/Component/Console/DependencyInjection/AddConsoleCommandPass.php
Show resolved
Hide resolved
src/Symfony/Component/Console/DependencyInjection/AddConsoleCommandPass.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Console/DependencyInjection/AddConsoleCommandPass.php
Outdated
Show resolved
Hide resolved
ccf8c15
to
4a51788
Compare
4a51788
to
bbd806b
Compare
rebased and conflicts solved. |
PR welcome to solve #59473 (comment) (deprecations) 🙏 |
bbd806b
to
e9a6b0a
Compare
Thank you @yceruto. |
…faultDescription methods (yceruto) This PR was merged into the 7.3 branch. Discussion ---------- [Console] Deprecating Command getDefaultName and getDefaultDescription methods | Q | A | ------------- | --- | Branch? | 7.3 | Bug fix? | no | New feature? | no | Deprecations? | yes | Issues | - | License | MIT related discussion #59473 (comment) Commits ------- b529726 Deprecating command getDefault* methods
Follow up #59340
Invokable and regular commands can now define the command
help
content via the#[AsCommand]
attribute.This is particularly useful for invokable commands, as it avoids the need to extend the
Command
class.Cheers!