[Console] Annotate AsCommand attribute as @final
#60066
Conversation
There was a problem hiding this comment.
The example is not very accurate: if you add a property to the child class you would have to read the attribute on your own. Creating a dedicated attribute class is better.
There is actually an issue as the AsCommand class allows inheritance, but the we don't scan the children classes. An alternative fix could be to make AsCommand final.
But I see a use-cases for extending the AsCommand class: set one of its properties. Example: automatically add a static prefix to the command name.
| @@ -61,7 +61,7 @@ public static function getDefaultName(): ?string | |||
| { | |||
| trigger_deprecation('symfony/console', '7.3', 'Method "%s()" is deprecated and will be removed in Symfony 8.0, use the #[AsCommand] attribute instead.', __METHOD__); | |||
|
|
|||
| if ($attribute = (new \ReflectionClass(static::class))->getAttributes(AsCommand::class)) { | |||
| if ($attribute = (new \ReflectionClass(static::class))->getAttributes(AsCommand::class, \ReflectionAttribute::IS_INSTANCEOF)) { | |||
There was a problem hiding this comment.
Keep in mind that by using this option, PHP tries to load all the classes when reading the attributes.
Not a big deal for Symfony Framework, but that could impact standalone usage.
There was a problem hiding this comment.
Of course, the example should have been clearer.
Given the arguments and concerns you raised, I think that making AsCommand final seems like a better approach. It will remove the unexpected/undocumented behaviour.
Of course, it's a breaking change, but cursory search of GitHub reveals just one usage.
If you feel okay with changing of the attribute to final, I can edit this PR or make a new one, your call.
There was a problem hiding this comment.
In 7.3, we need to tag it as @final in phpdoc instead of making it actually final, so that we trigger a deprecation warning for classes extending it.
There was a problem hiding this comment.
I also prefer the @final annotation on AsCommand. Can you update your PR?
AsCommand attributeAsCommand attribute as @final
…mrlik, GromNaN) This PR was merged into the 7.3 branch. Discussion ---------- [Console] Mark `AsCommand` attribute as ``@final`` | Q | A | ------------- | --- | Branch? | 7.3 | Bug fix? | yes | New feature? | no | Deprecations? | yes | License | MIT `AsCommand` attribute doesn't work as expected when extended and probably should be declared as final. In order to not break BC and trigger a deprecation warning, I marked the attribute as ``@final`` as per suggestions in previous PR. Superseeds #60066 as I closed it accidentally, sorry for confusion 😕 Commits ------- 1db80a5 [Console] Mark `AsCommand` attribute as ``@final``
AsCommandattribute is not final and thus can be extended, but its inheritors don't work as expected when used.The constructor of
CommandexpectsAsCommandattribute to be its base class and does not handle cases whereAsCommandmight be extended. This can be solved/fixed/improved by using\ReflectionAttribute::IS_INSTANCEOFwhen calling\ReflectionClass::getAttributes().These extensions might provide useful in cases such as
or
where metaprogramming enabled by attributes could be useful.
You could of course implement another attribute, but since
AsCommandis not final, one would expect its behaviour to be consistent when extended.I welcome suggestions for more tests.
Don't know if this is a feature worth documenting in docs.