-
Notifications
You must be signed in to change notification settings - Fork 729
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
Make ResultSet iterator current() not return a mixed value #1506
Changes from all commits
f0b6c64
16f041c
2f2ed17
465b14a
5014111
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -236,15 +236,11 @@ public function countSuggests() | |
/** | ||
* Returns the current object of the set. | ||
* | ||
* @return \Elastica\Result|bool Set object or false if not valid (no more entries) | ||
* @return \Elastica\Result Set object | ||
*/ | ||
public function current() | ||
{ | ||
if ($this->valid()) { | ||
return $this->_results[$this->key()]; | ||
} | ||
|
||
return false; | ||
return $this->_results[$this->key()]; | ||
} | ||
|
||
/** | ||
|
@@ -253,8 +249,6 @@ public function current() | |
public function next() | ||
{ | ||
++$this->_position; | ||
|
||
return $this->current(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. next() is not meant to return something. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
} | ||
|
||
/** | ||
|
@@ -308,7 +302,7 @@ public function offsetExists($offset) | |
* | ||
* @throws Exception\InvalidException If offset doesn't exist | ||
* | ||
* @return Result|null | ||
* @return Result | ||
*/ | ||
public function offsetGet($offset) | ||
{ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should treat this as a bug or breaking change ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It reminds me of #1374 Let's ping people in there on what they think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks indeed like a case of xkcd workflow
![xkcd workflow](https://camo.githubusercontent.com/e21057f0d29bb85706d9dfc78e135d4d06b4556637b69513682149863882f2d5/68747470733a2f2f696d67732e786b63642e636f6d2f636f6d6963732f776f726b666c6f772e706e67)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ruflin done. This is ready from my point of view.