[PSR-7] Clarify requirement for new instances

[weierophinney]

An "optimization" suggested on several occasions is to return $this; from with*() methods if the argument presented is equal to the current value in the object. However, the specification indicates a new instance MUST be returned.

This patch provides verbiage indicating why the behavior indicated in the specification was chosen (to remove ambiguity with regards to the return value).

Updated: This patch provides verbiage indicating that return $this is explicitly allowed if the instances are equivalent in value.

[weierophinney]

Ping @simensen for review, and @stof as he has brought it up in code reviews of implementations.

[simensen]

👍 @pmjones if you can merge this, awesome.

[mtdowling]

-1 from me. This seems like a pointless requirement.

The code paths that would result in return $this; (i.e., calling a with*() method with the same value as is already present) will typically be rare, making the practice a micro-optimization.

Why prohibit this if it is a micro-optimization? Where did you come up with the idea that it is an edge case? I feel like this claim cannot be backed up.

Cloning in PHP is a cheap operation, particularly with shallow object structures (as are present in the HTTP message interfaces), again suggesting that returning the same instance is a micro-optimization.

This is the same point as above, which I don't think is valid. What is the reasoning of preventing a micro-optimization?

Loosening the restriction leads to ambiguity in the interfaces, as consumers cannot expect the return value to fail an identity test against the original object (e.g., $new !== $old would not be consistent).

What's the use case here? I see no reason to compare with strict equality on these value objects. The aggregate of the values stored in the object is the value -- the value is not based on whether or not the result of spl_object_hash() changes after calling one of the with methods.

Differences between implementations as to whether or not they clone always or conditionally does not affect their usage of the interfaces. It's an implementation detail and shouldn't matter to the end user.

[weierophinney]

@mtdowling TBH, I could go either way. :)

The "uniformity of return value" is an important argument when designing interfaces, as it reduces ambiguity. On the flip side, modeling as a value object means that if two objects are equivalent, they're also interchangeable.

I'm open to changing the verbiage on this to allow return $this; — the current incarnation was written when we were originally considering this as an errata post-acceptance. Since we're in review mode, we have more flexibility.

[simensen]

@mtdowling This reasoning is being added to the spec to back up why the spec currently requires it. That was the original way the spec was written. If the spec stays the way it is now, we need to have this.

The alternative is to change the spec to remove the requirement of returning a new instance. I believe that is still on the table as well. Now that we are in review we can revisit that.

The big issue with the spec as it was is that it was written in such a way that people were implementing it counter to the spec (returning $this as a micro-optimization).

[mtdowling]

Gotcha. I see that the docblocks say that it always returns a clone. Perhaps it could be updated to say something along the lines of "returns a clone of the object or implementations may choose to return the same instance if the value is not modified as a result of calling the with method".

Any sort of language that states that values MUST be cloned could be a slippery slope.. For example, if a request MUST be cloned, then should the URI instance and stream owned by the request be cloned as well, or is it ok to use a reference here instead?

[simensen]

@mtdowling Yep, that was one of the things that has come up in the last week or so. Everyone saw it and was cool with it and then implemented it differently. :) I think that the point of the docblock was to convey the "spirit of immutability" by returning a new object. However, that is an implementation detail. Personally, I think we could just remove "and return a new instance that contains the changed state" and call it good.

This method MUST be implemented in such a way as to retain the immutability of the original message, and MUST return an instance that has the new protocol version.

I think this would allow for both returning a clone along with this, as long as the protocol version passed in was the protocol version that the object had originally (since the returned object would have the desired protocol version). But maybe it isn't explicit enough.

[pmjones]

Let me know what the final story is on this.

[weierophinney]

@mtdowling Here's a draft of a revised MessageInterface: https://gist.github.com/weierophinney/1b32277f70e68c1b8097

Essentially, it changes "a new instance" to "new instance", and "Creates a new instance" to "Returns an instance" throughout. This should allow for either return $this when no changes are necessary, or returning a clone when they are. Would this work? If so, I'll change this PR to reflect that throughout the various interfaces.

[mtdowling]

Awesome! I think that is the best way to address the issue.

[weierophinney]

@mtdowling and @simensen : I've updated the patch to explicitly allow return $this;. Please review!

[simensen]

👍 Would like to see input from both @pmjones and @Crell on this.

[mtdowling]

🚢

[pmjones]

Cannot "an instance" mean "this instance, even with changed values" ? I like the clarity of "a new instance" every time -- it nothing else it's consistent. I fail to see the true harm of that wording preventing a micro-optization.

[weierophinney]

@pmjones the text prior to "an instance" is "preserve immutability". You cannot change the current instance AND preserve immutability. However, if no change is made, you're still preserving immutability, even if the same instance is returned.

Also, I'd not considered this, but consumers such as ReactPHP and Guzzle are cases where the same values may be set again and again. In those cases, the proposed changes would have a performance impact, as they could potentially eliminate the needs for dozens or hundreds of additional instances.

[pmjones]

the text prior to "an instance" is "preserve immutability".

Ah so; I concentrated on the red/green. I am satisfied.

[ircmaxell]

At an interface level this may be dangerous, because of copy-on-write semantics. Especially if you're getting/setting array and object values. As they may appear equal (pass == or ===) but not be strictly equivalent due to references internally.

I'm not saying it's not worth doing, just pointing out that there's non-trivial edge-case complications associated with this edge-case.

[stof]

@ircmaxell the issue is that the way it was written previously would mean that returning the same instance when no change was needed would be invalid according to the spec. There is no reason for the spec to forbid it IMO, as long as the implementation respects immutability.

[weierophinney]

@ircmaxell and @stof : the salient points for allowing either case are:

• The only likely place that an identity check would happen are unit tests. In userland code, you will either be re-assigning, or passing the value on as a method argument or return value without further comparison. Since the interface specifies immutability, the userland assumption can be that the original instance will not change, which means returning the original instance has no side effects.
• The cases where return $this might be triggered are very few, and the primary place I'd expect it to happen are in situations where requests or responses are being treated inside a looping structure (e.g., making many requests to an API). As such, there would be associated performance impact due to spawning new instances. If the number of new instances is small, then return $this is a micro-optimization, but in the case of long-running processes such as you might have with ReactPHP or HHVM, it could be beneficial.

Essentially, we have to weigh the edge cases we want to address here. Based on playing with PSR-7 in applications already, I can note that comparison of two instances is rare, while spawning new instances is rampant, and, as such, I think it makes sense to address the latter.

[ircmaxell]

@weierophinney My point was more around detecting when a change happened via with* operation. It's non-trivial to detect "set" operations that don't change the value in some small way.

Perhaps better would be to simply specify the interface as immutable, and let implementations determine when to return $this while still maintaining immutability. (meaning: don't say "a new instance must always be returned", but instead say "any modification requires a new instance to be created").

[weierophinney]

@ircmaxell —

Perhaps better would be to simply specify the interface as immutable, and let implementations determine when to return $this while still maintaining immutability. (meaning: don't say "a new instance must always be returned", but instead say "any modification requires a new instance to be created").

That's actually quite similar to the language this patch proposes:

This method MUST be implemented in such a way as to retain the
immutability of the message, and MUST return an instance that has the
new and/or updated header and value.

The above is the general language in the various with*() statements as modified by this patch. Note that it now indicates "MUST return an instance" (emphasis mine), and no longer "a new instance".

[webmozart]

👍

[kelvinj]

👍 much clearer

[Tobion]

👍

[nyamsprod]

👍

[weierophinney]

@pmjones and/or @simensen — this one is ready for merge, based on the feedback received.