Problem/Motivation
It would be nice to be able to use clear shorthand in @return documentation when a class method is returning the object itself or a new instance of the class. Our current standards require something like
* @return \Full\Name\Of\The\Class
* Some description goes here.
Proposed resolution
Allow the use of @return $this
when the method is returning the same object ($this), and @return static
when the method is returning a new instance of the same class (and not specifically the object $this). In both cases, you could omit the description.
Note: This agrees with the proposed FIG standards (PSR-5), and PHPStorm supports it. However, PHPDoc does *not* support either of these -- they have @return self
, which is less specific and just indicates that the return value is of the same class or a subclass. PSR-5 also allows @return self
.
Remaining tasks
Agree on the new standard and add it to the Documentation standards page section at https://drupal.org/coding-standards/docs#types
Specifically:
- In the Syntax Examples section, include "$this" and "static" in the examples. So it would look like:
int string|bool \Drupal\Core\Database\StatementInterface \Drupal\node\NodeInterface[] $this static
- In the Syntax Notes bullet list, add the following two bullet points:
- When you are returning the same object, use @return $this.
- When creating a new instance of the same class, use @return static.
User interface changes
None
API changes
None
Original report by @Xano
We have been using @return self
for a while, as it is supported by phpDoc and major IDEs, such as PhpStorm. We recently started using @return static
instead, as the return value more often than not is an instance of a child class, rather than of self (see PHP's late static bindings.
Example:
/**
* Foos the bar into the baz.
*
* @return static
*/
function foo() {
return $this;
}
However, these types are not part of the coding standards yet. This issue is about discussing whether to add them or not.
Comments
Comment #1
XanoComment #2
jhodgdonAnd by the way, our coding standards currently say that every @return needs a description as well as a type. So if you want to amend to say "@return self" or "@return static" does not need a description (which seems reasonable), then we should make a note of that as well.
Comment #3
damiankloip CreditAttribution: damiankloip commentedWell, I'm not sure late static binding is always applicable here. As that is a technique related to either a class name (which is static) or a static method. In my mind static and self are distinctly different. E.g.
static(params..)
. This is not in the context of a class instance but creating an instance. So yes, LSB makes sense here, as you could override a class but keep the static create() method.return $this
. LSB does not matter here, as you are dealing with the instance of a class you already have.afaik, IDE's like PHPStorm work with both self and static.
Comment #4
neclimdulTesting with DefaultPluginBag and the ImageEffectBag in ImageStyle::getEffects, static and self both work in phpStorm. I agree with damiankloip's semantic argument that self is correct when returning $this.
edit: added IDE I tested.
Comment #5
dawehnerIf you look at https://github.com/phpDocumentor/fig-standards/blob/master/proposed/phpd...
you will use @return $this
Comment #6
damiankloip CreditAttribution: damiankloip commentedI would be fine with $this too, as it makes the semantic difference between them.
Comment #7
Xano$this
seems a useful and distinct type hint. It works in PhpStorm 7.Comment #8
amateescu CreditAttribution: amateescu commentedMy reason for "introducing"
static
was:With the code above, [ide_autocomplete] only shows
setStuffInA()
and we need it to also showsetStuffInB()
. I just checked in PhpStorm 7.1 and@return $this
works as well.At https://github.com/phpDocumentor/fig-standards/blob/master/proposed/phpd..., I see that both static (14) and $this (15) are mentioned/supported, so I'm ok with switching to
$this
if people don't likestatic
:)Comment #9
damiankloip CreditAttribution: damiankloip commentedI like static, don't get me wrong. I just also like the distinction between the two :/
Comment #10
XanoAdding Crell's tag™, as he is Drupal's FIG representative. Maybe he can give some more advice.
Comment #11
dawehnerI don't even get why we need more discussion on this issue. $this is part of PSR-5 and already supported by IDEs.
Comment #12
Xano$this
or PSR-5 are not part of the Drupal coding standards, and that is what this issue is about.Comment #13
jhodgdonThe proposals I see in this issue are:
a) @return $this
b) @return self
c) @return static
The first one is apparently the industry standard. The second and third are currently being used in Core. I don't see agreement above on which we want to adopt.
In any case, none of this is in our current code/docs standards, and our standards currently say that all @returns need a description, which would be silly here. So we need to (1) adopt one or more of these options and (2) amend the standards.
Any votes on what to adopt?
Comment #14
neclimdulI'd kinda forgotten about PSR-5 when I got into this discussion in IRC. I spent the morning scouring the relevant bits and PSR-5 and it gives some surprisingly explicit explanations of this problem. I'd actually be interested discussing adopting it as a whole but that's definitely a different discussion. It how ever gives us a little direction on this particular issue because of the broadness of the issue. All the listed methods are legal, they just mean different things.
Importantly, all of the listed methods are legal under PSR-5. They just mean different things. I think in the context of the example of the summary returning $this is correct but I think the expanded definition in PSR-5 is more appropriate and should be allowed.
On legality
From @return docs [1]:
From Appendix A [2] (emphasis mine):
On Meaning
The important thing to understand is how they behave which is in the keyword documentation dawehner linked.
Summary:
a) @return $this
Refers to the object in the context. Could be any extending class but importantly it must be the same instance as where its called.
b) @return self
Refers specifically to the implementing class. This is probably generally not what we're meaning since especially in base classes. It would mean we're documenting returning the base classes, not any of the extending classes. This is the behavior documented in #8.
c) @return static
Almost exactly like $this. Its the documented class or any subclass. The difference is $this refers to the exact instance where static could technically return a new or different instance of the same class. Clone, factory, etc.
Comment #15
jhodgdonThanks for that!!!
So I think that 99% of the time, we want "@return $this", because the statement at the end of the method is "return $this", right? But we should go ahead and add all three of those options to node/1354, as summarized in the comment above, because we may have a few cases where self/static are appropriate (such as cloning or a factory).
So I propose to add exactly the "summary" section from the comment above to node/1354 in the "data types" section.
Any objections?
Comment #16
tim.plunkettSo, when you are returning the same object, use
@return $this
.When creating a new instance of the same class, use
@return static
.Comment #17
jhodgdonOh, that's even clearer. I'm putting that into the issue summary as the proposal, and setting status to "needs review". Please indicate whether you approve of the proposal that is in the Proposed Resolution in the summary.
I vote +1.
Comment #18
amateescu CreditAttribution: amateescu commented+1 from me as well.
Comment #19
jhodgdonwhoops, I messed up the title.
Comment #20
damiankloip CreditAttribution: damiankloip commented+1, That's not far from what I originally wanted with self/static. The main thing is that we differentiate between an actual instance, and creating instances. I.e. What I outlined in #3
Comment #21
neclimdulYeah, we don't need to explain the RFC. That's a much better plain English explanation. Do we RTBC policies or how does this work now? ;)
Comment #22
jhodgdonI will go ahead and set this to RTBC, because that is normally the only way to get the attention of Core developers (there are so many issues, a lot of people only watch the RTBC queue), and it looks like the people who've been watching this issue are mostly in agreement, and I doubt this is really very controversial.
Also adding the "coding standards" tag, which was omitted before.
Anyway, if anyone has any "+1" comments, please go ahead. And if anyone has any objections, feel free to change the status back to something else.
As far as process goes, what happens next is we wait for a week or so for comments, and then change the standards page and mark this Fixed (assuming everyone is still in agreement).
Comment #23
yched CreditAttribution: yched commented+1 then...
Comment #24
XanoAye!
Comment #25
kaare+1
Tagging as DX enhancement (for the better).
Comment #26
tim.plunkettComment #27
Crell CreditAttribution: Crell commentedGiven that this is part of the proposed PSR-5, I'm comfortable making a jump on it. Although I'm not sure it that's going to break for me in NetBeans at this point...
Comment #28
sun@tim.plunkett's short version in #16 read much more clear to me:
I'd recommend to add that to the proposal in the issue summary. Even better, replace it with something short and fancy like that ;)
Comment #29
jhodgdonUmmm... The issue summary is proposing:
----
Allow the use of @return $this when the method is returning $this, and @return static when the method is returning a new instance of the same class (and not specifically the object $this). In both cases, you could omit the description.
----
That is almost exactly what is in #28. And in any case, it will most likely have to be added to
https://drupal.org/coding-standards/docs#types
in slightly different format.
Comment #30
sunYeah, I've read the issue summary. :) But yeah, a sentence of "return $this when you return $this" does not actually explain what is being meant with $this, given that the information tries to make a differentiation between $this and something else. ;)
Thus, I (1) had to read all comments to understand what is being meant and (2) was surprised to see that the wording in the summary was chosen despite there being a shorter but yet more explanatory and much more clear one in the thread. ;)
Anyway, that was just my first time/sight experience, so I wanted to share feedback. Perhaps it's only me.
Comment #31
damiankloip CreditAttribution: damiankloip commentedYes, how Tim phrased this in #20 is ideal. Sums up more concisely what i was trying to say in #3 :)
Comment #32
sunBy the way, and perhaps more fundamentally:
If we're doing this for
@return
, then why only for@return
?The exact same issue exists for
@see
:If
$this
andstatic
is properly interpreted by IDEs and phpDoc parsers, then we should be able to leverage the same idea to do this:No? Am I mistaken?
Happy to create a separate issue for that — but it obviously depends on the final outcome here, so not doing that just yet.
Comment #33
XanoCan't we just have a types section that lists the allowed variable types, just like the PhpDoc/PSR-5 documentation? We can then say that
@see
,@var
,@return
and possibly others must be followed by any valid type.Comment #34
Crell CreditAttribution: Crell commentedHopefully we can eventually replace our docs docs with a link to PSR-5. Until then, we're largely biding our time and addressing specific issues as they come up.
Comment #35
XanoRight. I'm just saying that until that may or may not happen, just fix our current issue in a similar way. It's not an attempt to move towards PSR-5; I just think it's the easiest and best way to solve the problem @sun raised in #32.
Comment #36
neclimdullets discuss @var and @see separate or discuss PSR-5. We're creaping scope on this issue.
Also, I don't think the same rules are valid for @see under PSR-5 because it can take a type but static::bar() is not a type its a FQSEN and that is defined with different rules. Best I can tell from skimming it requires the full namespace so this isn't valid.
Comment #37
jhodgdonSheesh. Sorry for the confusion. Updated the issue summary to be more explicit about the exact change to be added to node/1354 (check the Remaining Tasks section).
Some other responses to recent comments:
- Please don't bring up stuff like "let's just replace node/1354 with a link to FIG on this issue, which is specifically about whether we should allow @return $this and @return static.
- @see is for making links. It is not the same as specifying a data type and is outside the scope of this issue.
- Take a look at https://drupal.org/coding-standards/docs#types -- it already says it is for @return and @var and @params.
Comment #38
sunCool, thanks. :)
I'd RTBC this, if it wasn't RTBC already. I've the impression that everyone is fine with this (especially because it appears to be adopting a small subset of PSR-5).
So let's move forward? :)
Comment #39
jhodgdonIt's normal to leave coding standards issues at RTBC for at least a week before making the coding standards change. This one was only tagged "coding standards" and marked RTBC on the 16th, so we should wait a few more days.
Comment #40
jhodgdonThis issue has now been at RTBC for 12 days with no dissenting opinions (aside from the exact wording of the policy).
So, I went ahead and edited https://drupal.org/coding-standards/docs#types
Comment #42
fagoThe docu-change misses the clarification that static may omit the description also. Created a follow up:
#2631930: Docs fail to mention that @return static may omit the descript just as @return $this
Comment #43
dwwp.s. If anyone is still following this, I ran into this and finally opened that follow-up for @see. ;)
#3112830: [policy, no patch] Allow static::methodName() and/or self::methodName() in @see comments when referring to the same class
Thanks,
-Derek