Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Hide commands from ApplicationDescriptor, but allow invoking#20029
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
| private$processTitle; | ||
| private$aliases =array(); | ||
| private$definition; | ||
| private$hidden; |
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.
Should be$this->hidden = false;
| * @param bool $hiddenBool To show this command or not | ||
| * | ||
| * @return Command The current instance | ||
| */ |
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.
Should be$hidden
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.
indeed. Thank you sir
| /** | ||
| * Sets if the command should be hidden from application inspection. | ||
| * |
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.
Derp.
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.
What aboutTo hide the command in application descriptions.
ogizanagi commentedSep 25, 2016
What about other descriptors ? |
ro0NL commentedSep 25, 2016 • 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.
@ogizanagi all formats use |
ogizanagi commentedSep 25, 2016
javiereguiluz commentedSep 29, 2016
Could we change the method names to follow the same notation as for services? Instead of |
ro0NL commentedSep 29, 2016
Maybe if we run a private command it should give you a heads up in verbose mode.. would that be useful? |
jwdeitch commentedSep 29, 2016
Good idea. Should we merge this first then address that? Or I can throw it into this PR |
ogizanagi commentedSep 29, 2016 • 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.
@jwdeitch : Could you update the test suite ( |
| /** | ||
| * To hide the command in application descriptions. | ||
| * |
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'd suggest something like:Whether the command should be publicly shown or not. and remove the above description (which brings nothing more IMHO).
| /** | ||
| * Returns if the command is public in application descriptions. | ||
| * |
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.
Same. I feel this repetition is useless.
| * @return Command The current instance | ||
| */ | ||
| publicfunctionsetPublic($public) | ||
| { |
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.
should be$this->public = (bool) $public;
| * @param bool $public Whether the command should be publicly shown or not. | ||
| * | ||
| * @return Command The current instance | ||
| */ |
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.
What aboutsetPrivate() without an argument? I don't see a need to switch from private to public.
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 suggested changing it tosetPublic(false) to mimic Symfony services ... where you must setpublic: false for private services. If we use opposing techniques to do the same (public: false for services,private: true for commands) we're increasing the learning curve.
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 could meet in the middle atsetPublicity as it's not favoring one way or another :)
fabpot commentedOct 5, 2016
Thank you@jwdeitch. |
…voking (jwdeitch, Jordan Deitch)This PR was merged into the 3.2-dev branch.Discussion----------Hide commands from ApplicationDescriptor, but allow invokingI would like to hide commands from cluttering the descriptors, but still allow their invocation from code or cron.| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | comma-separated list of tickets fixed by the PR, if any| License | MIT| Doc PR | reference to the documentation PR, if anyCommits-------746dab3 casting setPublic() arg to bool0a3c290 update docblocks and added test6d87837 Update ApplicationDescription.phpe969581 update hidden to public3efa874 Update Command.phpdfc1ac8 Update Command.phpcd77139 Update Command.php56a8b93 Update Command.phpfb1f30c Update Command.php1993196 Update Command.php1add2ad Update Command.phpb73f494 Update ApplicationDescription.php8d0262f Update Command.phpb423ab4 Add hidden field
wouterj commentedOct 20, 2016 • 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.
I think calling thisprivate will introduce the same confusion that was just solved with the services: I expect I cannot run private commands outside the app, but I still can. It's simply only hidden in the command listing. Calling thishidden does a better job in describing the feature imo. |
fabpot commentedOct 21, 2016
I tend to agree with@wouterj. A private command is indeed not really private in the same sense as for the container, it's just hidden from the list of commands and can still be executed like any other commands.@javiereguiluz WDYT? |
javiereguiluz commentedOct 21, 2016
I understand "private" as something hidden, secret, confidential, special. Those are precisely thesynonyms of the "private" word. If we talk about commands that cannot be executed outside the app, to me that would be: "banned", "secured", "disabled", "closed", etc. But if this is confusing to others, then "hidden" is also a very understandable name for this feature. |
stof commentedOct 21, 2016
@javiereguiluz the confusion is that it has a different behavior than
|
xabbuh commentedOct 21, 2016
…xabbuh)This PR was merged into the 3.2-dev branch.Discussion----------[Console] rename Command::private to Command::hidden| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#20029| License | MIT| Doc PR |Commits-------f900ef0 rename Command::private to Command::hidden
…eguiluz, wouterj)This PR was submitted for the master branch but it was merged into the 3.2 branch instead (closes#7052).Discussion----------Added an article about private console commandsThis documentssymfony/symfony#20029.Commits-------d1cb043 Reflect private to hidden renaming in the file name01f474f Reflect renaming from private to hidden9415643 Added an article about private console commands
Uh oh!
There was an error while loading.Please reload this page.
I would like to hide commands from cluttering the descriptors, but still allow their invocation from code or cron.