NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

Handle 'hidden' param from AsCommand attribute#41547

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

Jump to bottom

Closed

yoannrenard wants to merge2 commits intosymfony:5.4fromyoannrenard:handle_hidden_param_from_AsCommand_attribute

Closed

Handle 'hidden' param from AsCommand attribute#41547

yoannrenard wants to merge2 commits intosymfony:5.4fromyoannrenard:handle_hidden_param_from_AsCommand_attribute

Conversation

Copy link

Contributor

yoannrenard commentedJun 4, 2021•
edited
Loading

Q	A
Branch?	5.4
Bug fix?	yes
New feature?	no
Deprecations?	no
Tickets
License	MIT
Doc PR

The recently introducedAsCommand attribute works like a charm (as much as the whole 5.3 release 👌). Thanks a lot for that!

Though, it seems that thehidden property has been forgotten, whether one set it or not the command only checks the$this->hidden value set from theconfigure() method.

For example:

#[AsCommand(    name:'app:do:something',    description:'Will do something very cool.',    aliases: [],    hidden:true)]class DoSomethingCommandextends Command

When I runbash bin/console list app I get

Available commandsfor the"app" namespace:  app:do:something  Do something very cool.

I implemented a fix inspired by the$name/$description properties.

Handle 'hidden' param from AsCommand attribute

c234b9d

yoannrenard requested a review fromchalasr as acode owner

June 4, 2021 19:04

carsonbot added Status: Needs Review Bug labels

Jun 4, 2021

fixup! Handle 'hidden' param from AsCommand attribute

d310b79

yoannrenard changed the title~~Handle 'hidden' param from AsCommand attribute~~[Console] Handle 'hidden' param from AsCommand attribute

Jun 4, 2021

OskarStark approved these changes

Jun 5, 2021

View reviewed changes

Copy link

Contributor

OskarStark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Looks good to me except my comment

carsonbot added Status: Reviewed and removed Status: Needs Review labels

Jun 5, 2021

carsonbot changed the title~~[Console] Handle 'hidden' param from AsCommand attribute~~Handle 'hidden' param from AsCommand attribute

Jun 5, 2021

OskarStark reviewed

Jun 5, 2021

View reviewed changes

src/Symfony/Component/Console/Attribute/AsCommand.phpShow resolvedHide resolved

Copy link

Member

nicolas-grekas commentedJun 5, 2021

This patch doesn't fix the root issue. If you inspect the dumped container for example, you'll see that the command is correctly registered as hidden.
Yet, for some reason I didn't spot yet, thelist command doesn't see this flag as set.
This needs to be investigated.

nicolas-grekas reviewed

Jun 5, 2021

View reviewed changes

src/Symfony/Component/Console/Command/Command.php

		$isHidden =$attribute[0]->newInstance()->hidden;
		}

		return$this->hidden \|\|$isHidden;

Copy link

Member

nicolas-grekasJun 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

For reviewers (and for the author): this patch cannot be correct, because it makes the attribute a mandatory piece of configuration that cannot be ignored. This is against the root principles of attributes, which should be purely declarative.

nicolas-grekas added this to the5.3 milestone

Jun 5, 2021

Copy link

Member

nicolas-grekas commentedJun 5, 2021•
edited
Loading

Note that past some issue with registering commands, there is another one: when theCommand class is registered without the DI component (without theAddConsoleCommandPass), then the aliases and hidden flags are ignored, and the name is not parsed (explode('|')). I don't know how common it is to use Command without DI. A fix would involve parsing the name and using it when hidden+aliases are undefined.

Copy link

ContributorAuthor

yoannrenard commentedJun 5, 2021

As said@nicolas-grekas without DI, the command name is not parsed when thehidden param is set in the AsCommand attribute.
I've created a quick bug reproducerhere to show you.

Copy link

ContributorAuthor

yoannrenard commentedJun 6, 2021

After more tests this morning, I've discovered that AsCommand attribute works fine with CI. I just didn't know that this attribute needed to clear the cache to work. My bad.

I'll close this PR as it is wrong and push my fix for the usage without CI in another one.

yoannrenard closed this

Jun 6, 2021

yoannrenard mentioned this pull request

Jun 6, 2021

[Console] Fix AsCommand usage without DI#41564

Closed

Copy link

Member

stof commentedJun 7, 2021

@yoannrenard the need to clear cache when changing attributes is caused by#39988

Copy link

ContributorAuthor

yoannrenard commentedJun 7, 2021

Thanks for the information@stof

nicolas-grekas mentioned this pull request

Jun 12, 2021

[Console] Fix using #[AsCommand] without DI#41686

Merged

nicolas-grekas added a commit that referenced this pull request

Jun 12, 2021

bug#41686[Console] Fix using #[AsCommand] without DI (nicolas-grekas)

f640e6c

This PR was merged into the 5.3 branch.Discussion----------[Console] Fix using #[AsCommand] without DI| Q             | A| ------------- | ---| Branch?       | 5.3| Bug fix?      | yes| New feature?  | no| Deprecations? | no| Tickets       |Fix#41547| License       | MIT| Doc PR        | -Commits-------c4357ea [Console] Fix using #[AsCommand] without DI

yoannrenard deleted the handle_hidden_param_from_AsCommand_attribute branch

June 13, 2021 05:59