Skip to content
This repository has been archived by the owner on Jan 29, 2020. It is now read-only.

Fix: Return static instead of self #37

Merged
merged 1 commit into from Jun 12, 2015

Conversation

localheinz
Copy link
Member

This PR

  • updates docblocks of MessageTrait and ResponseTrait to return static instead of self

❓ Seems to make more sense to me, how about you?

@Maks3w
Copy link
Member

Maks3w commented May 31, 2015

  1. Return types must match interface return types.
  2. PSR-5 (draft) clarify when use self or $this

@Maks3w Maks3w closed this May 31, 2015
@localheinz localheinz deleted the fix/self-vs-static branch May 31, 2015 14:08
@localheinz
Copy link
Member Author

@Maks3w

Thank you for pointing me to the draft, had a look, but unsure - maybe I don't understand the document right.

@Maks3w
Copy link
Member

Maks3w commented May 31, 2015

If you think return type is wrong please open an issue in the https://github.com/php-fig/http-message project first.

@localheinz
Copy link
Member Author

@Maks3w

😉 Hehe, no worries! Asking @mvriel what he says.

@mvriel
Copy link

mvriel commented May 31, 2015

Since $this is cloned the methods will return a different instance of the defining class or one that inherited the defining class; so static is accurate in this case.

There are three options possible:

  1. @return $this – returns the current instance of the class (usually denoting a fluent interface)
  2. @return self – returns an instance of the class where the return type is defined
  3. @return static – returns an instance of the current class, this means the class where the associated method is called on (if a class inherits another class and the method is defined on the superclass but called on an object of the subclass then the expected object is an instance of the subclass)

@localheinz localheinz restored the fix/self-vs-static branch May 31, 2015 14:37
@localheinz
Copy link
Member Author

@mvriel

Thanks a lot for clarifying!

@weierophinney
Copy link
Member

We can update in here, and later I'll update the http-message library and
send a clarification for PSR-7. Interestingly, @simensen and I asked about
this a couple months back, but I think we didn't make the intentions clear
at the time. :-)
On May 31, 2015 9:38 AM, "Andreas Möller" notifications@github.com wrote:

@mvriel https://github.com/mvriel

Thanks a lot for clarifying!


Reply to this email directly or view it on GitHub
#37 (comment)
.

@Maks3w
Copy link
Member

Maks3w commented May 31, 2015

@weierophinney Unfortunatelly it's not clear for me that Interface will change the type since require a change in the PSR-7.

@mvriel
Copy link

mvriel commented May 31, 2015

@weierophinney I must have missed that; I have had a rough couple of months so things might easily have been missed

@Maks3w
Copy link
Member

Maks3w commented May 31, 2015

Anyway I'm not sure if the debate is about how PSR-5 should understand the return type for interface methods.

Because the return type is defined in an interface then self is an illegal return type when defined in interfaces. So as workaround we could raise an issue to PSR-5 so self have the same meaning of static when is used as return type in an interface method.

@weierophinney
Copy link
Member

It's not a change in the PSR.

The intent is exactly the same: return a different instance. We were
unaware of the difference between self and static, however, and used self,
while stipulating in the docblock that a new instance must be returned. I'm
simply proposing we clarify the PSR to use the static verbiage, which will
simplify the annotation and make it consistent with the documented intent.
On May 31, 2015 9:54 AM, "Maks3w" notifications@github.com wrote:

@weierophinney https://github.com/weierophinney Unfortunatelly it's not
clear for me that Interface will change the type since require a change in
the PSR-7.


Reply to this email directly or view it on GitHub
#37 (comment)
.

@localheinz
Copy link
Member Author

@weierophinney

👍

@localheinz localheinz deleted the fix/self-vs-static branch June 3, 2015 16:48
@localheinz localheinz restored the fix/self-vs-static branch June 3, 2015 17:41
@localheinz
Copy link
Member Author

@Maks3w @weierophinney

Can we reopen the PR, then?

I've opened a PR upstream, see php-fig/http-message#50.

@weierophinney weierophinney reopened this Jun 3, 2015
@@ -66,7 +66,7 @@ public function getProtocolVersion()
* new protocol version.
*
* @param string $version HTTP protocol version
* @return self
* @return static
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just joining the conversation: since the interface defines that a MessageInterface is returned, so should also the trait providing the concrete impl :-)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Ocramius No, it does not. The interface indicates uses @return self, and indicates a new instance is returned. Considering that MessageInterface is not expected to be directly implemented, but rather implemented via one of RequestInterface or ResponseInterface, the question is whether self or static should be specified in the return annotation to ensure that IDEs, PHPDocumentor, and implementors are following the spec properly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering that MessageInterface is not expected to be directly implemented

It's an interface. It will be implemented, and nobody says that it will necessarily be a RequestInterface or a ResponseInterface :-)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Ocramius The issue here is the inheritance tree, which includes interfaces, and, more specifically, traits. Per this comment from @mvriel, self indicates that it's an instance of the class where the docblock is defined, while static is defined to return an instance of the current class, following inheritance.

So, a change is required here for sure. The question is whether changes are needed on the PSR-7 interfaces. I'd argue they are for MessageInterface and RequestInterface, as those two both are inherited by other interfaces in the specification.

weierophinney added a commit that referenced this pull request Jun 12, 2015
Fix: Return static instead of self
@weierophinney weierophinney merged commit 72f3089 into zendframework:master Jun 12, 2015
weierophinney added a commit that referenced this pull request Jun 12, 2015
weierophinney added a commit that referenced this pull request Jun 12, 2015
@weierophinney weierophinney added this to the 1.0.4 milestone Jun 12, 2015
@weierophinney weierophinney self-assigned this Jun 12, 2015
@Maks3w
Copy link
Member

Maks3w commented Jun 12, 2015

I totally disagree with this. If there is an issue with the interface then should be fixed in the interface first.

@simensen
Copy link
Contributor

Has there been a PR sent against fig-standards or http-message? If not, we should do that right away. We should also get feedback from others on whether this should be an allowed change to PSR-7 now that it has been accepted. I think that the change from self to static should be fair since it fits with the spirit of the proposal (we simply didn't know at the time that the phpdoc might be interpreted differently between self and static) but we also don't want to mess anything up. As far as I understand it, PSR-7 is wrong as it is currently docblocked so I think changing it should happen sooner rather than later.

@Maks3w
Copy link
Member

Maks3w commented Jun 12, 2015

Where is the limit when you say your own side changes are compatible?

This is the first step for define your own standard.

@weierophinney
Copy link
Member

@Maks3w I am following the advice of the author/editor of psr-5, and will
also be applying this upstream. I am complying with standards, not
inventing new ones.
On Jun 12, 2015 1:28 PM, "Maks3w" notifications@github.com wrote:

Where is the limit when you say your own side changes are compatible?

This is the first step for define your own standard.


Reply to this email directly or view it on GitHub
#37 (comment)
.

@weierophinney
Copy link
Member

We are currently only applying it to the two traits, to clarify inheritance
within the component (so that return values are understood to resolve to
concrete implementations, and not the traits, which are typeless). This can
be done separately from psr-7, and even there, only needs to be done on the
message and request interfaces (the ones that are extended in the spec
itself).
On Jun 12, 2015 1:22 PM, "Maks3w" notifications@github.com wrote:

I totally disagree with this. If there is an issue with the interface then
should be fixed in the interface first.


Reply to this email directly or view it on GitHub
#37 (comment)
.

@weierophinney
Copy link
Member

@simensen I believe the author of this proposal has created patches already
for the spec and interface packages.
On Jun 12, 2015 1:26 PM, "Beau Simensen" notifications@github.com wrote:

Has there been a PR sent against fig-standards or http-message? If not, we
should do that right away. We should also get feedback from others on
whether this should be an allowed change to PSR-7 now that it has been
accepted. I think that the change from self to static should be fair
since it fits with the spirit of the proposal (we simply didn't know at the
time that the phpdoc might be interpreted differently between self and
static) but we also don't want to mess anything up. As far as I
understand it, PSR-7 is wrong as it is currently docblocked so I think
changing it should happen sooner rather than later.


Reply to this email directly or view it on GitHub
#37 (comment)
.

@localheinz localheinz deleted the fix/self-vs-static branch June 12, 2015 22:52
@localheinz
Copy link
Member Author

@simensen

For reference, here it is: php-fig/http-message#50.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants