Fix @return statements to use $this or static when relevant #21054

fabpot · 2016-12-26T07:56:39Z

Q	A
Branch?	2.7
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#20290
License	MIT
Doc PR	n/a

see #20290

ro0NL · 2016-12-26T08:04:31Z

With #21047 i also spotted some cases where @return FQCN|$this is used. (FQCN from the current class file, ie. the same ;-))

I have no clear ref on this, but PhpStorm really doesnt seem to like it. Or at least not when used inconsistently so it seems 😕 (1 class mixing $this, FQCN and/or FQCN|$this[|...]).

Perhaps worth looking at.

SpacePossum · 2016-12-26T09:06:27Z

src/Symfony/Bridge/Twig/NodeVisitor/Scope.php

@@ -42,7 +42,7 @@ public function __construct(Scope $parent = null)
    /**
     * Opens a new child scope.
     *
-     * @return Scope
+     * @return $this


this should be @return self right?
this - return exactly the same instance
self - return new instance of exactly same type
static - return new instance of same type or any child of type

fabpot · 2016-12-26T10:47:33Z

I would appreciate some careful review before merging as re-reading the PR several times allowed me to fix some issues I introduced. I know, this is quite painful to do, but let's try to do it well.

ro0NL · 2016-12-26T11:01:52Z

src/Symfony/Component/Config/Definition/Builder/NormalizationBuilder.php

@@ -38,7 +38,7 @@ public function __construct(NodeDefinition $node)
     * @param string $key    The key to remap
     * @param string $plural The plural of the key in case of irregular plural
     *
-     * @return NormalizationBuilder
+     * @return $this
     */
    public function remap($key, $plural = null)


before() (below) can be $this|ExprBuilder

ro0NL · 2016-12-26T11:04:55Z

src/Symfony/Component/Config/Loader/Loader.php

@@ -57,7 +57,7 @@ public function import($resource, $type = null)
     * @param mixed       $resource A resource
     * @param string|null $type     The resource type or null if unknown
     *
-     * @return LoaderInterface A LoaderInterface instance
+     * @return $this|self


Should be $this|LoaderInterface

ro0NL · 2016-12-26T11:07:21Z

src/Symfony/Component/Console/Helper/Helper.php

@@ -35,7 +35,7 @@ public function setHelperSet(HelperSet $helperSet = null)
    /**
     * Gets the helper set associated with this helper.
     *
-     * @return HelperSet A HelperSet instance
+     * @return HelperSet


Should be HelperSet|null

ro0NL · 2016-12-26T11:15:28Z

src/Symfony/Component/EventDispatcher/Event.php

@@ -81,7 +81,7 @@ public function setDispatcher(EventDispatcherInterface $dispatcher)
    /**
     * Returns the EventDispatcher that dispatches this Event.
     *
-     * @return EventDispatcherInterface
+     * @return EventDispatcher


EventDispatcherInterface was right? The @var is wrong..

ro0NL · 2016-12-26T11:16:29Z

src/Symfony/Component/Finder/Expression/Expression.php

@@ -27,7 +27,7 @@ class Expression implements ValueInterface
    /**
     * @param string $expr
     *
-     * @return Expression
+     * @return static


ro0NL · 2016-12-26T11:18:52Z

src/Symfony/Bridge/Twig/NodeVisitor/Scope.php

@@ -42,7 +42,7 @@ public function __construct(Scope $parent = null)
    /**
     * Opens a new child scope.
     *
-     * @return Scope
+     * @return self


This seems to be inconsistent with a lot of changes below.. where it goes to static for new self.

If new self is used, it should be self anyway.

ro0NL · 2016-12-26T11:20:04Z

src/Symfony/Component/Finder/Expression/Regex.php

@@ -55,7 +55,7 @@ class Regex implements ValueInterface
    /**
     * @param string $expr
     *
-     * @return Regex
+     * @return static


self. Im stopping here for now.

ogizanagi · 2016-12-26T12:06:25Z

src/Symfony/Bridge/Twig/NodeVisitor/Scope.php

@@ -52,7 +52,7 @@ public function enter()
    /**
     * Closes current scope and returns parent one.
     *
-     * @return Scope|null
+     * @return self|null


I'm not sure we should prefer using self over the class/interface name. IMHO, we should only distinguish $this, static and classname usages, but self does not bring anything more, so it isn't worth it.

I'm not sure we should prefer using self over the class/interface name.

Personally, i prefer it... so while we're at it 👍 for doing this as well.

Personally, i prefer it...

I don't 😆

I just don't understand the motivations behind, and there are many places where this was not applied (FormFactoryBuilderInterface::getFormFactory() for instance).

Avoid duplicating terms. But you're right, if not done consistently it's not really worth it anyway.

I will keep self as in the code, that's also what we are using.

ogizanagi · 2016-12-26T12:14:13Z

src/Symfony/Component/Console/Helper/Table.php

@@ -93,7 +93,7 @@ public static function setStyleDefinition($name, TableStyle $style)
     *
     * @param string $name The style name
     *
-     * @return TableStyle A TableStyle instance
+     * @return static


This change looks wrong to me. self::$styles is an array of TableStyle.

ogizanagi · 2016-12-26T12:16:18Z

src/Symfony/Component/CssSelector/Exception/SyntaxErrorException.php

@@ -27,7 +27,7 @@ class SyntaxErrorException extends ParseException
     * @param string $expectedValue
     * @param Token  $foundToken
     *
-     * @return SyntaxErrorException
+     * @return static


Should be self (same below)

ogizanagi · 2016-12-26T12:34:15Z

src/Symfony/Component/Form/FormBuilderInterface.php

@@ -38,7 +38,7 @@ public function add($child, $type = null, array $options = array());
     * @param string|FormTypeInterface $type    The type of the form or null if name is a property
     * @param array                    $options The options
     *
-     * @return FormBuilderInterface The created builder
+     * @return $this


This change looks wrong to me. It doesn't return the current instance, but creates another one, for a child for instance.

ogizanagi · 2016-12-26T12:34:52Z

src/Symfony/Component/Form/FormBuilderInterface.php

@@ -47,7 +47,7 @@ public function create($name, $type = null, array $options = array());
     *
     * @param string $name The name of the child
     *
-     * @return FormBuilderInterface The builder for the child
+     * @return $this


Same. It returns a child. Not the same instance.

ogizanagi · 2016-12-26T12:37:13Z

src/Symfony/Component/Form/FormConfigBuilder.php

@@ -715,7 +715,7 @@ public function setInheritData($inheritData)
     *
     * @param bool $inheritData Whether the form should inherit its parent's data
     *
-     * @return FormConfigBuilder The configuration object
+     * @return $this


It doesn't return anything actually.

ogizanagi · 2016-12-26T12:38:09Z

src/Symfony/Component/Form/FormConfigBuilder.php

@@ -349,7 +349,7 @@ public function getInheritData()
    /**
     * Alias of {@link getInheritData()}.
     *
-     * @return FormConfigBuilder The configuration object
+     * @return $this


Hmmm. It returns a boolean actually.

ogizanagi

I'm done 😅
Sorry, I've left a lot of comment related to what seems misleading usages of static to me (see #21054 (comment)), in order to keep track of it. If you don't agree, I'll remove them.

Anyway, what a colossal job you've achieved here!

ogizanagi · 2016-12-26T12:45:06Z

src/Symfony/Component/Form/FormInterface.php

@@ -36,7 +36,7 @@ public function setParent(FormInterface $parent = null);
    /**
     * Returns the parent form.
     *
-     * @return FormInterface|null The parent form or null if there is none
+     * @return $this|null The parent form or null if there is none


Wrong change. The returned instance is the form parent.

Should be self|null then

ogizanagi · 2016-12-26T12:46:30Z

src/Symfony/Component/Form/FormInterface.php

@@ -89,7 +89,7 @@ public function remove($name);
    /**
     * Returns all children in this group.
     *
-     * @return FormInterface[] An array of FormInterface instances
+     * @return FormInterface[]


Why not self[] then? (Related to #21054 (comment))

ogizanagi · 2016-12-26T12:50:00Z

src/Symfony/Component/Form/Guess/Guess.php

@@ -70,7 +70,7 @@
     *
     * @param Guess[] $guesses An array of guesses
     *
-     * @return Guess|null The guess with the highest confidence
+     * @return static|null


Should keep Guess|null

Or self|null

ogizanagi · 2016-12-26T12:50:24Z

src/Symfony/Component/HttpFoundation/AcceptHeader.php

@@ -48,7 +48,7 @@ public function __construct(array $items)
     *
     * @param string $headerValue
     *
-     * @return AcceptHeader
+     * @return static


ogizanagi · 2016-12-26T12:50:49Z

src/Symfony/Component/HttpFoundation/AcceptHeaderItem.php

@@ -57,7 +57,7 @@ public function __construct($value, array $attributes = array())
     *
     * @param string $itemValue
     *
-     * @return AcceptHeaderItem
+     * @return static


ogizanagi · 2016-12-26T13:01:58Z

src/Symfony/Component/Intl/NumberFormatter/NumberFormatter.php

@@ -302,7 +302,7 @@ public function __construct($locale = 'en', $style = null, $pattern = null)
     *                        NumberFormat::PATTERN_RULEBASED. It must conform to  the syntax
     *                        described in the ICU DecimalFormat or ICU RuleBasedNumberFormat documentation
     *
-     * @return NumberFormatter
+     * @return static


ogizanagi · 2016-12-26T13:04:02Z

src/Symfony/Component/PropertyAccess/PropertyAccess.php

@@ -21,7 +21,7 @@
    /**
     * Creates a property accessor with the default configuration.
     *
-     * @return PropertyAccessor The new property accessor
+     * @return static


Looks wrong. (same for other changes in this class)

ogizanagi · 2016-12-26T13:05:08Z

src/Symfony/Component/Security/Acl/Domain/ObjectIdentity.php

@@ -52,7 +52,7 @@ public function __construct($identifier, $type)
     *
     * @param object $domainObject
     *
-     * @return ObjectIdentity
+     * @return static


ogizanagi · 2016-12-26T13:05:21Z

src/Symfony/Component/Security/Acl/Domain/UserSecurityIdentity.php

@@ -52,7 +52,7 @@ public function __construct($username, $class)
     *
     * @param UserInterface $user
     *
-     * @return UserSecurityIdentity
+     * @return static


ogizanagi · 2016-12-26T13:05:28Z

src/Symfony/Component/Security/Acl/Domain/UserSecurityIdentity.php

@@ -64,7 +64,7 @@ public static function fromAccount(UserInterface $user)
     *
     * @param TokenInterface $token
     *
-     * @return UserSecurityIdentity
+     * @return static
     */
    public static function fromToken(TokenInterface $token)


wouterj

Did just a quick review, these are the glitches I found.

wouterj · 2016-12-26T14:28:58Z

src/Symfony/Component/Finder/Shell/Command.php

@@ -61,7 +61,7 @@ public function __toString()
     *
     * @param Command|null $parent Parent command
     *
-     * @return Command New Command instance
+     * @return static


wouterj · 2016-12-26T14:31:10Z

src/Symfony/Component/Finder/Shell/Command.php

@@ -157,7 +157,7 @@ public function cmd($esc)
     *
     * @param string $label The unique label
     *
-     * @return Command The current Command instance
+     * @return self


Should be self|string, according to add().

wouterj · 2016-12-26T14:31:43Z

src/Symfony/Component/Finder/Shell/Command.php

@@ -194,7 +194,7 @@ public function get($label)
    /**
     * Returns parent command (if any).
     *
-     * @return Command Parent command
+     * @return self


self|null

I don't think so as it throws an exception is parent is null.

wouterj · 2016-12-26T14:31:52Z

src/Symfony/Component/Finder/Shell/Command.php

@@ -178,7 +178,7 @@ public function ins($label)
     *
     * @param string $label
     *
-     * @return Command The labeled command
+     * @return self


self|string

wouterj · 2016-12-26T14:35:49Z

src/Symfony/Component/Form/FormInterface.php

@@ -36,7 +36,7 @@ public function setParent(FormInterface $parent = null);
    /**
     * Returns the parent form.
     *
-     * @return FormInterface|null The parent form or null if there is none
+     * @return $this|null The parent form or null if there is none


Should be self|null then

wouterj · 2016-12-26T14:36:45Z

src/Symfony/Component/Form/FormInterface.php

@@ -80,7 +80,7 @@ public function has($name);
     *
     * @param string $name The name of the child to remove
     *
-     * @return FormInterface The form instance
+     * @return self


It's actually $this.

wouterj · 2016-12-26T14:37:31Z

src/Symfony/Component/Form/Guess/Guess.php

@@ -70,7 +70,7 @@
     *
     * @param Guess[] $guesses An array of guesses
     *
-     * @return Guess|null The guess with the highest confidence
+     * @return static|null


Or self|null

wouterj · 2016-12-26T14:39:24Z

src/Symfony/Component/HttpKernel/Profiler/Profile.php

@@ -191,7 +191,7 @@ public function getStatusCode()
    /**
     * Finds children profilers.
     *
-     * @return Profile[] An array of Profile
+     * @return Profile[]


fabpot · 2016-12-26T18:31:53Z

@wouterj @ogizanagi @ro0NL Thank you very much for your reviews. I think we're good now.

ro0NL · 2016-12-26T18:43:12Z

@fabpot are you planning to do all branches?

@return

…ant (fabpot) This PR was merged into the 2.7 branch. Discussion ---------- Fix @return statements to use $this or static when relevant | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #20290 | License | MIT | Doc PR | n/a see #20290 Commits ------- 3c0693d fixed @return when returning this or static

@return

This PR was merged into the 2.8 branch. Discussion ---------- fixed @return when returning this or static | Q | A | ------------- | --- | Branch? | 2.8 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | n/a Follow-up of #21054 for the 2.8 branch. Commits ------- 7808b67 fixed @return when returning this or static

…terface (wouterj) This PR was merged into the 2.7 branch. Discussion ---------- Fixed `@return self` with `$this` in FormConfigBuilderInterface | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - Fixes a tiny bug introduced by #21054 . `FormConfigBuilder` methods actually return `$this` and not `self`. This is important, as the `FormBuilder` (extending `FormConfigBuilder`) methods can also be called (e.g. `$builder->setDataMapper(...)->add()` is perfectly possible). Commits ------- 505e84d Fixed `@return self` with `$this`

carsonbot added the Status: Needs Review label Dec 26, 2016

fabpot mentioned this pull request Dec 26, 2016

Fixed the @return value of Response::setStatusCode() #20290

Closed

ro0NL mentioned this pull request Dec 26, 2016

[Config] Improve PHPdoc / IDE autocomplete #21047

Merged

fabpot force-pushed the return-stmt branch from 51d3474 to 38bc592 Compare December 26, 2016 08:16

SpacePossum reviewed Dec 26, 2016

View reviewed changes

fabpot force-pushed the return-stmt branch 6 times, most recently from 21c14d3 to 78f726a Compare December 26, 2016 09:47

fabpot changed the base branch from master to 2.7 December 26, 2016 09:49

fabpot force-pushed the return-stmt branch 2 times, most recently from c66a477 to 683eaf2 Compare December 26, 2016 10:46

ro0NL reviewed Dec 26, 2016

View reviewed changes

ogizanagi reviewed Dec 26, 2016

View reviewed changes

wouterj reviewed Dec 26, 2016

View reviewed changes

nicolas-grekas added this to the 2.7 milestone Dec 26, 2016

fabpot force-pushed the return-stmt branch from 683eaf2 to 95c3b85 Compare December 26, 2016 18:31

fixed @return when returning this or static

3c0693d

fabpot force-pushed the return-stmt branch from 95c3b85 to 3c0693d Compare December 27, 2016 07:23

fabpot merged commit 3c0693d into symfony:2.7 Dec 27, 2016

fabpot mentioned this pull request Dec 27, 2016

fixed @return when returning this or static #21059

Merged

wouterj mentioned this pull request Jan 2, 2017

Fixed @return self with $this in FormConfigBuilderInterface #21137

Merged

fabpot deleted the return-stmt branch January 7, 2017 00:34

javiereguiluz mentioned this pull request Jan 16, 2017

Little PHPDocs correction #19821

Closed

Fix @return statements to use $this or static when relevant #21054

Fix @return statements to use $this or static when relevant #21054

Conversation

fabpot commented Dec 26, 2016

ro0NL commented Dec 26, 2016 • edited Loading

Choose a reason for hiding this comment

fabpot commented Dec 26, 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

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

ro0NL Dec 26, 2016 • edited Loading

Choose a reason for hiding this comment

ogizanagi Dec 26, 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

Choose a reason for hiding this comment

ogizanagi Dec 26, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogizanagi left a comment • edited Loading

Choose a reason for hiding this comment

ogizanagi Dec 26, 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

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

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

wouterj left a 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

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

ro0NL commented Dec 26, 2016 •

edited

Loading

ro0NL Dec 26, 2016 •

edited

Loading

ogizanagi Dec 26, 2016 •

edited

Loading

ogizanagi Dec 26, 2016 •

edited

Loading

ogizanagi left a comment •

edited

Loading

ogizanagi Dec 26, 2016 •

edited

Loading