Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Fix @return statements to use $this or static when relevant#21054
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
ro0NL commentedDec 26, 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.
With#21047 i also spotted some cases where 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 Perhaps worth looking at. |
| * Opens a new child scope. | ||
| * | ||
| * @returnScope | ||
| * @return$this |
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.
this should be@return self right?this - return exactly the same instanceself - return new instance of exactly same typestatic - return new instance of same type or any child of type
21c14d3 to78f726aComparec66a477 to683eaf2Comparefabpot commentedDec 26, 2016
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. |
| * @returnNormalizationBuilder | ||
| * @return$this | ||
| */ | ||
| publicfunctionremap($key,$plural =null) |
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.
before() (below) can be$this|ExprBuilder
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.
fixed
| * @param string|null $type The resource type or null if unknown | ||
| * | ||
| * @returnLoaderInterface A LoaderInterface instance | ||
| * @return$this|self |
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|LoaderInterface
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.
fixed
| * Gets the helper set associated with this helper. | ||
| * | ||
| * @return HelperSet A HelperSet instance | ||
| * @return HelperSet |
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 beHelperSet|null
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.
fixed
| * Returns the EventDispatcher that dispatches this Event. | ||
| * | ||
| * @returnEventDispatcherInterface | ||
| * @returnEventDispatcher |
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.
EventDispatcherInterface was right? The@var is wrong..
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.
fixed
| * @param string $expr | ||
| * | ||
| * @returnExpression | ||
| * @returnstatic |
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.
self
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.
fixed
| * Opens a new child scope. | ||
| * | ||
| * @returnScope | ||
| * @returnself |
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.
This seems to be inconsistent with a lot of changes below.. where it goes tostatic fornew self.
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.
Ifnew self is used, it should beself anyway.
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.
Agree.
| * @param string $expr | ||
| * | ||
| * @returnRegex | ||
| * @returnstatic |
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.
self. Im stopping here for now.
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.
fixed
| * Closes current scope and returns parent one. | ||
| * | ||
| * @returnScope|null | ||
| * @returnself|null |
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'm not sure we should prefer usingself over the class/interface name. IMHO, we should only distinguish$this,static andclassname usages, butself does not bring anything more, so it isn't worth it.
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'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.
ogizanagiDec 26, 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.
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.
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).
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.
Avoid duplicating terms. But you're right, if not done consistently it's not really worth it anyway.
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 will keepself as in the code, that's also what we are using.
| * @param string $name The style name | ||
| * | ||
| * @returnTableStyle A TableStyle instance | ||
| * @returnstatic |
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.
This change looks wrong to me.self::$styles is an array ofTableStyle.
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.
fixed
| * @param Token $foundToken | ||
| * | ||
| * @returnSyntaxErrorException | ||
| * @returnstatic |
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 beself (same below)
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.
fixed
| * @param array $options The options | ||
| * | ||
| * @returnFormBuilderInterface The created builder | ||
| * @return$this |
ogizanagiDec 26, 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.
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.
This change looks wrong to me. It doesn't return the current instance, but creates another one, for a child for 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.
fixed
| * @param string $name The name of the child | ||
| * | ||
| * @returnFormBuilderInterface The builder for the child | ||
| * @return$this |
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. It returns a child. Not the same 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.
fixed
| * @param bool $inheritData Whether the form should inherit its parent's data | ||
| * | ||
| * @returnFormConfigBuilder The configuration object | ||
| * @return$this |
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.
It doesn't return anything actually.
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.
fixed
| * Alias of {@link getInheritData()}. | ||
| * | ||
| * @returnFormConfigBuilder The configuration object | ||
| * @return$this |
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.
Hmmm. It returns a boolean actually.
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 :)
ogizanagi left a comment• 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.
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'm done 😅
Sorry, I've left a lot of comment related to what seems misleading usages ofstatic 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!
| * Returns the parent form. | ||
| * | ||
| * @returnFormInterface|null The parent form or null if there is none | ||
| * @return$this|null The parent form or null if there is none |
ogizanagiDec 26, 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.
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.
Wrong change. The returned instance is the form parent.
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 beself|null then
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.
fixed
| * Returns all children in this group. | ||
| * | ||
| * @return FormInterface[] An array of FormInterface instances | ||
| * @return FormInterface[] |
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.
Why notself[] then? (Related to#21054 (comment))
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.
fixed
| * @param Guess[] $guesses An array of guesses | ||
| * | ||
| * @returnGuess|null The guess with the highest confidence | ||
| * @returnstatic|null |
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 keepGuess|null
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.
Orself|null
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.
fixed
| * @param string $headerValue | ||
| * | ||
| * @returnAcceptHeader | ||
| * @returnstatic |
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.
self
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.
fixed
| * @param string $itemValue | ||
| * | ||
| * @returnAcceptHeaderItem | ||
| * @returnstatic |
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.
self ?
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.
fixed
| * described in the ICU DecimalFormat or ICU RuleBasedNumberFormat documentation | ||
| * | ||
| * @returnNumberFormatter | ||
| * @returnstatic |
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.
self?
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.
fixed
| * Creates a property accessor with the default configuration. | ||
| * | ||
| * @returnPropertyAccessor The new property accessor | ||
| * @returnstatic |
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.
Looks wrong. (same for other changes in this class)
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.
fixed
| * @param object $domainObject | ||
| * | ||
| * @returnObjectIdentity | ||
| * @returnstatic |
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.
self?
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.
fixed
| * @param UserInterface $user | ||
| * | ||
| * @returnUserSecurityIdentity | ||
| * @returnstatic |
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.
self?
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.
fixed
| * @returnUserSecurityIdentity | ||
| * @returnstatic | ||
| */ | ||
| publicstaticfunctionfromToken(TokenInterface$token) |
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.
self?
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.
fixed
wouterj left a comment
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.
Did just a quick review, these are the glitches I found.
| * @param Command|null $parent Parent command | ||
| * | ||
| * @returnCommand New Command instance | ||
| * @returnstatic |
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.
self
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.
fixed
| * @param string $label The unique label | ||
| * | ||
| * @returnCommand The current Command instance | ||
| * @returnself |
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 beself|string, according toadd().
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.
fixed
| * Returns parent command (if any). | ||
| * | ||
| * @returnCommand Parent command | ||
| * @returnself |
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.
self|null
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 don't think so as it throws an exception is parent isnull.
| * @param string $label | ||
| * | ||
| * @returnCommand The labeled command | ||
| * @returnself |
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.
self|string
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.
fixed
| * Returns the parent form. | ||
| * | ||
| * @returnFormInterface|null The parent form or null if there is none | ||
| * @return$this|null The parent form or null if there is none |
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 beself|null then
| * @param string $name The name of the child to remove | ||
| * | ||
| * @returnFormInterface The form instance | ||
| * @returnself |
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.
It's actually$this.
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.
fixed
| * @param Guess[] $guesses An array of guesses | ||
| * | ||
| * @returnGuess|null The guess with the highest confidence | ||
| * @returnstatic|null |
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.
Orself|null
| * Finds children profilers. | ||
| * | ||
| * @return Profile[] An array of Profile | ||
| * @return Profile[] |
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.
self[] ?
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.
fixed
fabpot commentedDec 26, 2016
@wouterj@ogizanagi@ro0NL Thank you very much for your reviews. I think we're good now. |
ro0NL commentedDec 26, 2016
@fabpot are you planning to do all branches? |
…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/asee#20290Commits-------3c0693d fixed@return when returning this or static
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/aFollow-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`
see#20290