Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[Console] AnnotateAsCommand attribute as@final#60066
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
GromNaN left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 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 theAsCommand class allows inheritance, but the we don't scan the children classes. An alternative fix could be to makeAsCommand final.
But I see a use-cases for extending theAsCommand 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Of course, the example should have been clearer.
Given the arguments and concerns you raised, I think that makingAsCommand final seems like a better approach. It will remove the unexpected/undocumented behaviour.
Of course, it's a breaking change, butcursory search of GitHub reveals just one usage.
If you feel okay with changing of the attribute tofinal, I can edit this PR or make a new one, your 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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I also prefer the@final annotation onAsCommand. 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.