Returning null instead of false on failure for first/last/current/next #362
Conversation
| @@ -88,15 +88,15 @@ public function toArray(); | |||
| * Sets the internal iterator to the first element in the collection and returns this element. | |||
| * | |||
| * @return mixed | |||
| * @psalm-return T|false | |||
| * @psalm-return T|null | |||
There was a problem hiding this comment.
This is a breaking change: consumers would need to change their code after upgrading to null checks instead of false checks. Implementors would need to change their code to return null instead of `false.
Breaking changes should go on 3.x, but that's not all: there should be an upgrade path for both consumers and implementors, meaning there should be a way to be compatible with both 2.x and 3.x.
For consumers, that would mean checking for false AND for null. For implementors (for instance PersistentCollection in doctrine/orm), it would mean returning null or false depending on whether 2.x or 3.x is installed. That's kind of a breaking change, although it's unlikely people are use PersistentCollection as a type declaration. They are probably referring to ArrayCollection instead, which in turns means they should have a dependency on doctrine/collection in their composer.json.
There was a problem hiding this comment.
Yes indeed, it is fine to be in 3.x, I agree, it is a big change.
I'm willing to make the documentation for this BC, please point me to some previous documentation that cover some similar change to get inspiration from.
Or I can work on a 2.x implementation that will live into the current interface, adding methods like: firstOrNull, lastOrNull, currentOrNull and nextOrNull but it feels that it does not belong here.
There was a problem hiding this comment.
Adding methods to an interface is a breaking change, not for consumers but for implementors, so that's out of the question anyway.
Breaking changes are documented at https://github.com/doctrine/collections/blob/2.1.x/UPGRADE.md
There was a problem hiding this comment.
(...) but that's not all: there should be an upgrade path for both consumers and implementors, meaning there should be a way to be compatible with both 2.x and 3.x.
Reading this piece makes me feel that this PR is too big of a change? The underlying idea was to allow the consumer to benefit from the nullsafe operator but if in the meantime they must check for either false or null all the benefit is lost.
Am I missing something?
There was a problem hiding this comment.
I understand you motivation, but indeed I wouldn't change the semantics of those methods. It's not worth it.
If we really wanted to go that way, we would need to introduce an entirely new contract (== interface) with different methods and phase out the old one.
There was a problem hiding this comment.
Not a problem, let's close this PR. Still I guess we should raise those reflections back to the original issue #190, so it is not lost?
ReadableCollection::last returns null if no item found ReadableCollection::current returns null if no item found ReadableCollection::next returns null if no item found # Conflicts: # src/ReadableCollection.php
jeromegxj
force-pushed
the
nullable-methods
branch
from
96ae179
to
91c2092
Compare
|
Closing as discussed. |
This PR references #190
The idea is to have the
ReadableCollectioninterface andArrayCollectionimplementation of the methodfirst,last,currentandnextreturnsnullinstead offalse.This way we can use PHP nullsafe operator like this:
Let me know what are the next steps