Fix: Return static instead of self

[localheinz]

This PR

• updates the @return annotations of interfaces to return static instead of self

See zendframework/zend-diactoros#37, especially

• Fix: Return static instead of self zendframework/zend-diactoros#37 (comment)
• Fix: Return static instead of self zendframework/zend-diactoros#37 (comment)

[weierophinney]

I agree with this change, and think we should create a clarification in the php-fig/fig-standards repo against PSR-7.

For those that don't want to follow the links that @localheinz has provided, here's the situation:

• When we were developing PSR-7, we were uncertain whether to return self, the interface name, or static. We reached out to Mike van Riel, and, likely due to us not communicating the context well, he indicated self was correct.
• However, the current PSR-5 proposal indicates that static should be used in interfaces and traits when returning a new instance; this also will ensure that it bubbles up the inheritance chain properly.

Since the intent of PSR-7 is to indicate that a new instance is returned, static is the correct verbiage for the return values of the with*() methods.

[evert]

I'm not sure if this is correct. static implies that for this case:

class X {
   /**
    * @return static
    */
   function foo();
}

class Y extends X {
}

$y = new Y();
$y->foo();

When I call foo on Y, with the annotation @return static, the implication is that the returned value is an instance of Y. I think you can only guarantee an instance of X to be returned.

[Maks3w]

note we are using static from an interface so in fact static type never should be used in interfaces docblock.

Said this, since static does not have sense in interfaces then should fallback to self meaning.

Finally, notice changes in interfaces should be duplicated to PSR7 text

[weierophinney]

The relevant section of PSR-5 seems to be the Keyword section, where self, static, and $this are defined. In our case, $this is definitely incorrect. In my reading, it appears that static could help disambiguate the situation for interfaces/traits, as they cannot return their own instances, but I'm unclear if that's part of the intent.

@mvriel — could you weigh in?

[evert]

I think the easiest, and clearest is that the object should always return instances implementing Psr\Http\Message\RequestInterface etc. This is the intent. If self is incorrect, lets fix it.

If there's a phpdoc alias that implies that, use it... but I think static has additional implications.

[liuggio]

+1

[localheinz]

@evert

Taking up your example right here:

class X 
{
   /**
    * @return static
    */
   public function foo();
}

class Y extends X 
{
}

$y = new Y();
$y->foo();

When I call foo on Y, with the annotation @return static, the implication is that the returned value is an instance of Y. I think you can only guarantee an instance of X to be returned.

If you extend a class you implement the interface it provides. Given that an interface suggests that you need to return static from a method, and when you're not doing it, then you're doing it wrong.

[mvriel]

I must admit that in the context of interfaces that the static keyword is ambiguous since PHP itself does not permit this keyword to be used as a typehint.

There are two scenarios possible:

1. When you use static then we assume, since something can only return an instance, that static refers to the direct class of the instance (given the following chain: 'SuperInterface' -> 'SubInterface' -> 'SuperClass' -> 'InstantiatedClass' this would be the 'InstantiatedClass') per the original rules for Late Static Bindings (http://php.net/manual/en/language.oop5.late-static-bindings.php)
2. If you consider Classes, Interfaces and Traits to each be their own binding scope then static could refer to the direct interface of the class of the instance that is returned (given the following chain: 'SuperInterface' -> 'SubInterface' -> 'SuperClass' -> 'InstantiatedClass' this would be the 'SubInterface')

I would consider the first to be the most logical version since static implies the most specific Type (hence the most direct class) while self implies the interface or class where the element (function or property) is declared and not called.

Does this help?

[simensen]

I ran into this today and I think it was broken. phpStorm was unable to determine what I wanted to do with the following section of code:

public function __invoke(
    ServerRequestInterface $request,
    ResponseInterface $response,
    callable $next
) {
    $request = $request
        ->withHeader(static::USER_ID_HEADER, (string) $user->userId)
        ->withHeader(static::USERNAME_HEADER, (string) $user->username)
        ;

    $now = $request->getServerParams()['REQUEST_TIME'];
    ...

Since withHeader is documented to return self, my $request instance is treated as if it were an instance of MessageInterface after the first time I called ->withHeader which is not the intention.

phpDoc interprets static in weird ways sometimes (not always perfect) but in this particular case, if withHeader had been documented to return static as per the PSR-5's currently proposed rules it would have continued to consider $request and instance of ServerRequestInterface.

I think this is a bug/clarification. As such, I would like to think we should be able to update the spec. Otherwise this is going to lead to years of annoying inability to do proper code hinting/code intel on anything PSR-7 related.

[weierophinney]

@simensen I agree that it's a clarification; the intention was always to indicate that it returns a new instance of the originating object type. I just was unable to get anybody to give me a definitive answer on the proper annotation to use when we were finalizing the spec.

[simensen]

@weierophinney Is this something that can be merged, then? Or do we need to find other people to weigh in on that? This would require v1.0.1 of psr/http-message to be tagged. To my knowledge we haven't done something like that before.

[weierophinney]

@simensen definitely mergeable, though we'd likely need to update the spec as well, and, as you note, tag a new version. Those are all reasonable and within scope. The question is: who will merge and tag? (I don't have commit rights.)

[localheinz]

Any news on this one?

[simensen]

@michaelcullum is this something that you can help us navigate?

[michaelcullum]

@simensen Of course. Are you able to compile all the necessary changes into one PR?

[weierophinney]

@php-fig/secretaries Ready to merge; I've also opened php-fig/fig-standards#793 to mirror the change.

[michaelcullum]

See, because self and static in theory are different things I think this verges slightly towards fix (towards what you actually meant) as opposed to a clarification; which complicates whether this needs a vote.

[weierophinney]

@michaelcullum I'm confused then, as it seemed from your earlier comment that you supported this as a clarification. Should I take this to the list to get more feedback?

It's also relevant to PSR-13, where we recently made the same change, for the same reasons. Granted, that proposal is not yet accepted…

[michaelcullum]

Indeed, sorry for that, I'd not really reviewed the discussion fully if I'm honest when I made that comment. I was more making the comment in that I could help in that I could merge the PR as opposed to whether or not the PR was mergeable.

[weierophinney]

@michaelcullum No worries; I've asked for opinions on the list. Hopefully they'll skew towards "fix" and not "clarification". 😄

[michaelcullum]

@localheinz Please could you resolve conflicts? Then we can merge this.

[localheinz]

@michaelcullum

Would love to, but I'm currently on vacation and don't have my computer with me. Can this wait until August 23?

[michaelcullum]

No worries. I'll rebase it myself. 😄

[michaelcullum]

Thanks all for your comments and @localheinz for the patch