Make ResultSet iterator current() not return a mixed value #1506
Conversation
| @@ -253,8 +249,6 @@ public function current() | |||
| public function next() | |||
| { | |||
| ++$this->_position; | |||
|
|
|||
| return $this->current(); | |||
There was a problem hiding this comment.
next() is not meant to return something.
There was a problem hiding this comment.
also it's not documented. and it would also not be really useful because it might return false or null or whatever when the iterator reaches the end which is what happens as it happens before valid is called. so you cannot rely on chaining next()->getData() like a test does.
| return $this->valid() | ||
| ? $this->_bulkResponses[$this->key()] | ||
| : false; | ||
| return $this->_bulkResponses[$this->key()]; |
There was a problem hiding this comment.
I wonder if we should treat this as a bug or breaking change ...
There was a problem hiding this comment.
I hope nobody would be affected by this potential bc break as it means people would use the Iterator in a wrong way. As an alternative we could leave the implementation and only change the phpdoc. But that would also be confusing.
There was a problem hiding this comment.
It reminds me of #1374 Let's ping people in there on what they think.
There was a problem hiding this comment.
I'd take the pragmatic approach here - it could only break code that was already abusing a well-defined mechanism, and I can't think of a really practical example how it would hurt existing code, it would more probably fix it by accident. So I wouldn't call BC on this.
There was a problem hiding this comment.
Looks indeed like a case of xkcd workflow
There was a problem hiding this comment.
What would we do without xkcd?
Sounds like we are all agreement we should move forward with this.
@Tobion I suggest we still add an entry to the breaking changes for people to be aware of it.
There was a problem hiding this comment.
@ruflin done. This is ready from my point of view.
Returning potentially false from the iterator current() means each result set item could be `false` which makes it impractical to safely use it. When using phpstan, you will get errors like `Cannot call method getId() on bool|Elastica\Result.` when you just iterate over the result set. Also `current()` should not need to check `valid()` again because that is done automatically by php when using foreach. When valid returns false, the loop is ended. So that is part of the Iterator spec. So using current on a non-existing item should not happen unless you misuse the Iterator.
Tobion
force-pushed
the
no-bool-current-iterator
branch
from
d66d692 to
465b14a
Compare
Returning potentially false from the iterator current() means each result set item could be
falsewhich makes it impractical to safely use it.When using phpstan, you will get errors like
Cannot call method getId() on bool|Elastica\Result.when you just iterate over the result set.Also
current()should not need to checkvalid()again because that is done automatically by php when using foreach. When valid returns false, the loop is ended. So that is part of the Iterator spec. So using current on a non-existing item should not happen unless you misuse the Iterator.