Hide commands from ApplicationDescriptor, but allow invoking #20029

jwdeitch · 2016-09-22T20:13:30Z

I 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 any

ro0NL · 2016-09-25T20:06:57Z

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

@@ -34,6 +34,7 @@ class Command
    private $processTitle;
    private $aliases = array();
    private $definition;
+    private $hidden;


Should be $this->hidden = false;

ro0NL · 2016-09-25T20:07:08Z

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

+     * 
+     * @return Command The current instance
+     */
+    public function setHidden($hiddenBool)


Should be $hidden

indeed. Thank you sir

ro0NL · 2016-09-25T20:09:11Z

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

@@ -447,6 +448,30 @@ public function getName()
    }

    /**
+     * Sets if the command should be hidden from application inspection.
+     *
+     * @param bool $hiddenBool To show this command or not


What about To hide the command in application descriptions.

ogizanagi · 2016-09-25T20:18:28Z

What about other descriptors ?

ro0NL · 2016-09-25T20:22:39Z

@ogizanagi all formats use ApplicationDescription::getCommands. https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Console/Descriptor/JsonDescriptor.php#L67

ogizanagi · 2016-09-25T20:27:15Z

@ro0NL, @jwdeitch : Oh right. Sorry. PR title & description may be updated I guess then 😉 .

javiereguiluz · 2016-09-29T10:21:21Z

Could we change the method names to follow the same notation as for services? Instead of setHidden() and isHidden() -> setPublic(true / false) and isPublic() ?

ro0NL · 2016-09-29T18:45:48Z

Maybe if we run a private command it should give you a heads up in verbose mode.. would that be useful?

jwdeitch · 2016-09-29T18:55:18Z

Good idea. Should we merge this first then address that? Or I can throw it into this PR

ogizanagi · 2016-09-29T18:57:18Z

@jwdeitch : Could you update the test suite (AbstractDescriptorTest) by adding a new private command in DescriptorApplication2 ?

ogizanagi · 2016-09-29T19:01:37Z

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

@@ -447,6 +448,30 @@ public function getName()
    }

    /**
+     * To hide the command in application descriptions.
+     *
+     * @param bool $public To show this command or not


I'd suggest something like: Whether the command should be publicly shown or not. and remove the above description (which brings nothing more IMHO).

ogizanagi · 2016-09-29T19:02:35Z

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

+    /**
+     * Returns if the command is public in application descriptions.
+     *
+     * @return bool If the command is public or not


Same. I feel this repetition is useless.

fabpot · 2016-09-30T19:37:35Z

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

+     */
+    public function setPublic($public)
+    {
+        $this->public = $public;


should be $this->public = (bool) $public;

fabpot · 2016-09-30T19:38:05Z

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

+     * 
+     * @return Command The current instance
+     */
+    public function setPublic($public)


What about setPrivate() without an argument? I don't see a need to switch from private to public.

I suggested changing it to setPublic(false) to mimic Symfony services ... where you must set public: 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.

we could meet in the middle at setPublicity as it's not favoring one way or another :)

fabpot · 2016-10-05T02:53:19Z

Thank you @jwdeitch.

…voking (jwdeitch, Jordan Deitch) This PR was merged into the 3.2-dev branch. Discussion ---------- Hide commands from ApplicationDescriptor, but allow invoking I 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 any Commits ------- 746dab3 casting setPublic() arg to bool 0a3c290 update docblocks and added test 6d87837 Update ApplicationDescription.php e969581 update hidden to public 3efa874 Update Command.php dfc1ac8 Update Command.php cd77139 Update Command.php 56a8b93 Update Command.php fb1f30c Update Command.php 1993196 Update Command.php 1add2ad Update Command.php b73f494 Update ApplicationDescription.php 8d0262f Update Command.php b423ab4 Add hidden field

wouterj · 2016-10-20T22:35:43Z

I think calling this private 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 this hidden does a better job in describing the feature imo.

fabpot · 2016-10-21T13:48:22Z

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 · 2016-10-21T13:57:29Z

I understand "private" as something hidden, secret, confidential, special. Those are precisely the synonyms 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 · 2016-10-21T14:53:58Z

@javiereguiluz the confusion is that it has a different behavior than private in the DI component:

in DI, a private service can be injected into other services, but cannot be accessed from the outside of the container
in console, a private command can be called like any other command, as long as you know it exists.

xabbuh · 2016-10-21T19:28:29Z

I agree with @wouterj and @fabpot. I opened #20266 which would rename $public to $hidden.

…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 commands This documents symfony/symfony#20029. Commits ------- d1cb043 Reflect private to hidden renaming in the file name 01f474f Reflect renaming from private to hidden 9415643 Added an article about private console commands

jwdeitch added 3 commits September 22, 2016 15:52

Add hidden field

b423ab4

Update Command.php

8d0262f

Update ApplicationDescription.php

b73f494

carsonbot added Status: Needs Review Feature labels Sep 22, 2016

jwdeitch added 2 commits September 22, 2016 16:16

Update Command.php

1add2ad

Update Command.php

1993196

ro0NL reviewed Sep 25, 2016

View reviewed changes

Update Command.php

fb1f30c

ro0NL reviewed Sep 25, 2016

View reviewed changes

jwdeitch added 4 commits September 25, 2016 16:10

Update Command.php

56a8b93

Update Command.php

cd77139

Update Command.php

dfc1ac8

Update Command.php

3efa874

jwdeitch changed the title ~~Hide commands from TextDescriptor, but allow invoking~~ Hide commands from ApplicationDescriptor, but allow invoking Sep 25, 2016

jwdeitch added 2 commits September 29, 2016 14:31

update hidden to public

e969581

Update ApplicationDescription.php

6d87837

ogizanagi suggested changes Sep 29, 2016

View reviewed changes

update docblocks and added test

0a3c290

fabpot reviewed Sep 30, 2016

View reviewed changes

casting setPublic() arg to bool

746dab3

fabpot merged commit 746dab3 into symfony:master Oct 5, 2016

This was referenced Oct 10, 2016

Added an article about private console commands symfony/symfony-docs#7051

Closed

Added an article about private console commands symfony/symfony-docs#7052

Closed

xabbuh mentioned this pull request Oct 21, 2016

[Console] rename Command::private to Command::hidden #20266

Merged

fabpot mentioned this pull request Oct 27, 2016

Release v3.2.0-BETA1 #20317

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hide commands from ApplicationDescriptor, but allow invoking #20029

Hide commands from ApplicationDescriptor, but allow invoking #20029

jwdeitch commented Sep 22, 2016 •

edited

Loading

ro0NL Sep 25, 2016

ro0NL Sep 25, 2016

jwdeitch Sep 25, 2016

ro0NL Sep 25, 2016

ro0NL Sep 25, 2016

ogizanagi commented Sep 25, 2016

ro0NL commented Sep 25, 2016 •

edited

Loading

ogizanagi commented Sep 25, 2016

javiereguiluz commented Sep 29, 2016

ro0NL commented Sep 29, 2016

jwdeitch commented Sep 29, 2016

ogizanagi commented Sep 29, 2016 •

edited

Loading

ogizanagi Sep 29, 2016

ogizanagi Sep 29, 2016

fabpot Sep 30, 2016

fabpot Sep 30, 2016

javiereguiluz Sep 30, 2016

jwdeitch Sep 30, 2016

fabpot commented Oct 5, 2016

wouterj commented Oct 20, 2016 •

edited

Loading

fabpot commented Oct 21, 2016

javiereguiluz commented Oct 21, 2016

stof commented Oct 21, 2016

xabbuh commented Oct 21, 2016

Hide commands from ApplicationDescriptor, but allow invoking #20029

Hide commands from ApplicationDescriptor, but allow invoking #20029

Conversation

jwdeitch commented Sep 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogizanagi commented Sep 25, 2016

ro0NL commented Sep 25, 2016 • edited Loading

ogizanagi commented Sep 25, 2016

javiereguiluz commented Sep 29, 2016

ro0NL commented Sep 29, 2016

jwdeitch commented Sep 29, 2016

ogizanagi commented Sep 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Oct 5, 2016

wouterj commented Oct 20, 2016 • edited Loading

fabpot commented Oct 21, 2016

javiereguiluz commented Oct 21, 2016

stof commented Oct 21, 2016

xabbuh commented Oct 21, 2016

jwdeitch commented Sep 22, 2016 •

edited

Loading

ro0NL commented Sep 25, 2016 •

edited

Loading

ogizanagi commented Sep 29, 2016 •

edited

Loading

wouterj commented Oct 20, 2016 •

edited

Loading