-
Notifications
You must be signed in to change notification settings - Fork 190
Fix: Return static instead of self #50
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
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:
Since the intent of PSR-7 is to indicate that a new instance is returned, |
I'm not sure if this is correct. class X {
/**
* @return static
*/
function foo();
}
class Y extends X {
}
$y = new Y();
$y->foo();
When I call |
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 |
The relevant section of PSR-5 seems to be the Keyword section, where @mvriel — could you weigh in? |
I think the easiest, and clearest is that the object should always return instances implementing If there's a phpdoc alias that implies that, use it... but I think |
+1 |
Taking up your example right here: class X
{
/**
* @return static
*/
public function foo();
}
class Y extends X
{
}
$y = new Y();
$y->foo();
If you extend a class you implement the interface it provides. Given that an interface suggests that you need to return |
I must admit that in the context of interfaces that the There are two scenarios possible:
I would consider the first to be the most logical version since Does this help? |
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 phpDoc interprets 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. |
@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. |
@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. |
@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.) |
Any news on this one? |
@michaelcullum is this something that you can help us navigate? |
@simensen Of course. Are you able to compile all the necessary changes into one PR? |
This patch updates PSR-7 to always use `@return static` instead of `@return self` within method annotations, as done in php-fig/http-message#50. Per the discussion on that issue, `static` is interpreted by phpDoc and most IDEs in such a way that it always resolves to the class against which it was invoked, and not the defining class, unlike `self`, which always resolves to the class in which it was originally defined. Additionally, `static` has the *connotation* that a new instance, not the same instance, will be returned — which is the intention as outlined already in each method providing such a return value.
@php-fig/secretaries Ready to merge; I've also opened php-fig/fig-standards#793 to mirror the change. |
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. |
@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… |
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. |
@michaelcullum No worries; I've asked for opinions on the list. Hopefully they'll skew towards "fix" and not "clarification". 😄 |
This patch updates PSR-7 to always use `@return static` instead of `@return self` within method annotations, as done in php-fig/http-message#50. Per the discussion on that issue, `static` is interpreted by phpDoc and most IDEs in such a way that it always resolves to the class against which it was invoked, and not the defining class, unlike `self`, which always resolves to the class in which it was originally defined. Additionally, `static` has the *connotation* that a new instance, not the same instance, will be returned — which is the intention as outlined already in each method providing such a return value.
@localheinz Please could you resolve conflicts? Then we can merge this. |
Would love to, but I'm currently on vacation and don't have my computer with me. Can this wait until August 23? |
No worries. I'll rebase it myself. 😄 |
Fix: Return static instead of self * localheinz/fix/return: Fix: Return static instead of self
Thanks all for your comments and @localheinz for the patch |
This PR
@return
annotations of interfaces to returnstatic
instead ofself
See zendframework/zend-diactoros#37, especially