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

Xano’s picture

Issue summary: View changes
jhodgdon’s picture

And 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.

damiankloip’s picture

Well, 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.

  • @return static: You have a static create() factory method on a class, that returns something like 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 self: You are in the context of the instance of a class, something like 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.

neclimdul’s picture

Testing 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.

dawehner’s picture

damiankloip’s picture

I would be fine with $this too, as it makes the semantic difference between them.

Xano’s picture

$this seems a useful and distinct type hint. It works in PhpStorm 7.

amateescu’s picture

My reason for "introducing" static was:

class A {
  /**
   * @return self
   */
  public function setStuffInA();
}

class B extends A {
  /**
   * @return self
   */
  public function setStuffInB();
}

$some_var = new B();
$some_var->setStuffInA()->[ide_autocomplete]

With the code above, [ide_autocomplete] only shows setStuffInA() and we need it to also show setStuffInB(). 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 like static :)

damiankloip’s picture

I like static, don't get me wrong. I just also like the distinction between the two :/

Xano’s picture

Issue tags: +Stalking Crell

Adding Crell's tag™, as he is Drupal's FIG representative. Maybe he can give some more advice.

dawehner’s picture

I don't even get why we need more discussion on this issue. $this is part of PSR-5 and already supported by IDEs.

Xano’s picture

$this or PSR-5 are not part of the Drupal coding standards, and that is what this issue is about.

jhodgdon’s picture

The 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?

neclimdul’s picture

I'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]:

When provided, it MUST contain a "Type" (See Appendix A) to indicate what is returned...

From Appendix A [2] (emphasis mine):

...
keyword = "array" / "bool" / "boolean" / "callable" / "double" / "false" / "float" / "int" / "integer"
keyword =/ "mixed" / "null" / "object" / "resource" / "self" / "static" / "string" / "true" / "void"
keyword =/ "$this"
...

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.

jhodgdon’s picture

Thanks 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?

tim.plunkett’s picture

Title: [policy, no patch] Use self and static as code comment typehints » [policy, no patch] Use $this and static as code comment typehints

So, when you are returning the same object, use @return $this.
When creating a new instance of the same class, use @return static.

jhodgdon’s picture

Title: [policy, no patch] Use $this and static as code comment typehints » [policy, no patch] Use $this and $static in documentation for @return types
Issue summary: View changes
Status: Active » Needs review

Oh, 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.

amateescu’s picture

+1 from me as well.

jhodgdon’s picture

Title: [policy, no patch] Use $this and $static in documentation for @return types » [policy, no patch] Use "$this" and "static" in documentation for @return types

whoops, I messed up the title.

damiankloip’s picture

So, when you are returning the same object, use @return $this.
When creating a new instance of the same class, use @return static.

+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

neclimdul’s picture

Yeah, 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? ;)

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Stalking Crell +Coding standards

I 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).

yched’s picture

if anyone has any "+1" comments, please go ahead

+1 then...

Xano’s picture

Aye!

kaare’s picture

Issue tags: +DX

+1

Tagging as DX enhancement (for the better).

tim.plunkett’s picture

Issue tags: -DX +DX (Developer Experience)
Crell’s picture

Given 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...

sun’s picture

@tim.plunkett's short version in #16 read much more clear to me:

When you are returning the same object, use @return $this
When creating a new instance of the same class, use @return static

I'd recommend to add that to the proposal in the issue summary. Even better, replace it with something short and fancy like that ;)

jhodgdon’s picture

Ummm... 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.

sun’s picture

Yeah, 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.

damiankloip’s picture

Yes, how Tim phrased this in #20 is ideal. Sums up more concisely what i was trying to say in #3 :)

sun’s picture

By 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:

class MyClass {
  /**
   * Performs foo.
   *
   * Only use this to do foo. For other use-cases:
   *
   * @see \Drupal\mymodule\MyClass::bar()
   * @see \Drupal\mymodule\MyClass::beer()
   */
  public function foo() {
  }
}

If $this and static is properly interpreted by IDEs and phpDoc parsers, then we should be able to leverage the same idea to do this:

class MyClass {
  /**
   * Performs foo.
   *
   * Only use this to do foo. For other use-cases:
   *
   * @see static::bar()
   * @see static::beer()
   */
  public function foo() {
  }
}

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.

Xano’s picture

Can'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.

Crell’s picture

Hopefully 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.

Xano’s picture

Right. 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.

neclimdul’s picture

lets 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.

jhodgdon’s picture

Issue summary: View changes

Sheesh. 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.

sun’s picture

Cool, 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? :)

jhodgdon’s picture

It'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.

jhodgdon’s picture

Status: Reviewed & tested by the community » Fixed

This 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

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

fago’s picture

The 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

dww’s picture

p.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