[Console] Application update PHPDoc of add and register methods #19389

SpacePossum · 2016-07-19T16:09:37Z

Q	A
Branch?	2.7
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
License	MIT

The PHPDoc states the method will always return a command, but it doesn't. Since Application::register returns the result of add directly is also doesn't always return the command (as its PHPDoc states).

GuilhemN · 2016-07-19T22:57:45Z

Why do we even return the command ? IMO we should instead return a boolean specifiing whether or not the command was added to the application.

unkind · 2016-07-19T23:02:33Z

IMO we should instead return a boolean specifiing whether or not the command was added to the application.

What do you suggest to do with false result? Try to add command again?

GuilhemN · 2016-07-19T23:17:02Z

Nothing in the core but it might be useful in third party libraries. At least more that returning one of the parameter passed.

SpacePossum · 2016-07-20T05:14:48Z

it is done so you chain things up like;

$app->add(new MyCommand())->myCommandMethod('called after construction and adding');

this only works if you always get an object back.
So it might make more sense to either throw an exception or allow adding a disabled command so it can be enabled later on and still be used. (and updating the PHPDoc and test would be nice as well ;) )

linaori · 2016-07-20T06:06:02Z

I think the behavior was correct, just need to update the docblock.

SpacePossum · 2016-07-20T08:09:38Z

Updated the doc and the return value.
SF switched to explicit null return to support the new return void 'type' right?
(i.e. fabbot is wrong on this one)

fabpot · 2016-07-20T14:10:31Z

src/Symfony/Component/Console/Application.php

@@ -360,7 +360,7 @@ public function add(Command $command)
        if (!$command->isEnabled()) {
            $command->setApplication(null);

-            return;
+            return null;


should be reverted, that's the default anyway in PHP.

then the docs are off;
https://github.com/symfony/symfony-docs/pull/6614/files#diff-42d49998b5b27b6669bad0e79210dee0R94

after;
#17201

ok, I don't agree, but fair enough.

unkind · 2016-07-20T20:57:01Z

Nothing in the core but it might be useful in third party libraries.

I think that @return bool is poor replacement for exceptions when it is supposed to describe whether command (in general meaning of this term, not CLI one) succeeded.

SpacePossum · 2016-07-20T21:04:42Z

@unkind I agree, I would expect that the only returned value is the Command added and if it wasn't an exception was thrown. But I think the PR is a nice compromise, having a correct PHPDoc is a nice little improvement, I don't want the PR to grow bigger than needed :)

javiereguiluz · 2016-07-22T13:57:42Z

👍

Status: reviewed

stof · 2016-07-22T14:02:24Z

src/Symfony/Component/Console/Application.php

@@ -351,7 +351,7 @@ public function addCommands(array $commands)
     *
     * @param Command $command A Command object
     *
-     * @return Command The registered command
+     * @return Command|null The registered command


you should document that it returns null when the command is not enabled.

I've updated the docs, please take a look again :)

xabbuh · 2016-07-22T20:33:34Z

src/Symfony/Component/Console/Application.php

+     * @param string $string
+     * 
+     * @return int
+     */


👎 on this, we usually don't add this to private methods

Symfony version (count is src folder only)
3.0 (a84ea7a):
Number of private methods: 1005, with PHPDoc 589, with comment 0
2.5 (5c83d12):
Number of private methods: 873, with PHPDoc 546, with comment 0

(but maybe my script is off?)

I agree with @xabbuh , this provides no value

happy to remove, but it does add value as it provides the type info, but fair enough :)

nicolas-grekas · 2016-07-25T16:31:57Z

src/Symfony/Component/Console/Application.php

@@ -335,6 +335,8 @@ public function register($name)
    /**
     * Adds an array of command objects.
     *
+     * @see Application::add()


we don't use @see (we removed all of them recently)

What about inline?

like {@inheritdoc} ? not sure, last time I played around with Sami it would not result in the docs. you prop. want, but happy to put it in there

SpacePossum · 2016-07-25T17:57:59Z

cleaned up to the minimal of changes to get the type hint correct on end points

nicolas-grekas · 2016-07-26T04:44:19Z

Thank you @SpacePossum.

…methods (SpacePossum) This PR was squashed before being merged into the 2.7 branch (closes #19389). Discussion ---------- [Console] Application update PHPDoc of add and register methods | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | License | MIT The [PHPDoc](https://github.com/symfony/symfony/blob/2.7/src/Symfony/Component/Console/Application.php#L354) states the method will always return a command, but it doesn't. Since [Application::register](https://github.com/symfony/symfony/blob/2.7/src/Symfony/Component/Console/Application.php#L328) returns the result of `add` directly is also doesn't always return the command (as its PHPDoc states). Commits ------- 6f0474f [Console] Application update PHPDoc of add and register methods

Update Application.php

0bdaa8e

carsonbot added Status: Needs Review Console Bug labels Jul 19, 2016

SpacePossum mentioned this pull request Jul 19, 2016

[Console] Improve code coverage #19328

Closed

Update Application.php

d8ff9ca

fabpot reviewed Jul 20, 2016
View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Jul 22, 2016

stof reviewed Jul 22, 2016
View reviewed changes

Update Application.php

30cbb43

xabbuh reviewed Jul 22, 2016
View reviewed changes

SpacePossum changed the title ~~[Console] Application incorrect return null by add and register method~~ [Console] Application update PHPDoc of add and register methods Jul 23, 2016

Update Application.php

fc706e9

nicolas-grekas reviewed Jul 25, 2016
View reviewed changes

Update Application.php

95e3cd5

nicolas-grekas closed this Jul 26, 2016

SpacePossum deleted the patch-2 branch July 26, 2016 05:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Console] Application update PHPDoc of add and register methods #19389

[Console] Application update PHPDoc of add and register methods #19389

SpacePossum commented Jul 19, 2016

GuilhemN commented Jul 19, 2016

unkind commented Jul 19, 2016

GuilhemN commented Jul 19, 2016

SpacePossum commented Jul 20, 2016

linaori commented Jul 20, 2016

SpacePossum commented Jul 20, 2016 •

edited

fabpot Jul 20, 2016

SpacePossum Jul 20, 2016 •

edited

fabpot Jul 20, 2016

unkind commented Jul 20, 2016 •

edited

SpacePossum commented Jul 20, 2016 •

edited

javiereguiluz commented Jul 22, 2016

stof Jul 22, 2016

SpacePossum Jul 22, 2016

xabbuh Jul 22, 2016

SpacePossum Jul 24, 2016

nicolas-grekas Jul 25, 2016

SpacePossum Jul 25, 2016

nicolas-grekas Jul 25, 2016

linaori Jul 25, 2016

SpacePossum Jul 25, 2016

SpacePossum commented Jul 25, 2016

nicolas-grekas commented Jul 26, 2016

[Console] Application update PHPDoc of add and register methods #19389

[Console] Application update PHPDoc of add and register methods #19389

Conversation

SpacePossum commented Jul 19, 2016

GuilhemN commented Jul 19, 2016

unkind commented Jul 19, 2016

GuilhemN commented Jul 19, 2016

SpacePossum commented Jul 20, 2016

linaori commented Jul 20, 2016

SpacePossum commented Jul 20, 2016 • edited

Choose a reason for hiding this comment

SpacePossum Jul 20, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unkind commented Jul 20, 2016 • edited

SpacePossum commented Jul 20, 2016 • edited

javiereguiluz commented Jul 22, 2016

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SpacePossum commented Jul 25, 2016

nicolas-grekas commented Jul 26, 2016

SpacePossum commented Jul 20, 2016 •

edited

SpacePossum Jul 20, 2016 •

edited

unkind commented Jul 20, 2016 •

edited

SpacePossum commented Jul 20, 2016 •

edited