WIP: Fix 330, return NULL if current() is not countable #334
Conversation
|
Adding @bartmcleod to review this, since I changed the previous implementation 8276cf6 |
| @@ -208,7 +203,7 @@ public function current() | |||
| if (is_array($this->buffer)) { | |||
| $this->buffer[$this->position] = $data; | |||
| } | |||
| return is_array($data) ? $data : null; | |||
| return (is_array($data) || $data instanceof Countable) ? $data : null; | |||
There was a problem hiding this comment.
Why is Countable a valid type, but (for example) Traversable not? What's the rationale behind the change?
There was a problem hiding this comment.
@ezimuel Since this apparently fixes #330 because of a bug I might have introduced earlier, it is not needed to cover every single interface that could be returned by ->current(). If it fixes #330 and does not break anything, then it is ok, but we should see a unit test that confirms #330 and thus explains the need for the fix.
There was a problem hiding this comment.
@bartmcleod The code that you introduced here just returns the current value of dataSource that can be a different type, depending on the adapter (e.g. for Mysqli is false, for PDO is null, etc). Maybe, I should align all the default value of the adapter to be the same but I'm not sure this will introduce BC breaks in some existing project. That's why I focused my fix on the AbstractResultSet.
@Ocramius I'm trying to accomodate different return type for Resultset::current() coming from different adapter, it's seems there is inconsistency with different adapters (see above). Anyway, I'm still testing all the use cases, it's quite difficult without type hinting...
There was a problem hiding this comment.
@ezimuel would it be okay to check with empty() and to always return null in that case? So that false and null are treated equally.
| // datasource was an array when the resultset was initialized | ||
| return $this->dataSource->current(); | ||
| } | ||
|
|
There was a problem hiding this comment.
I had my doubts about this part, but at the time, it was needed, so I am surprised the Travis build succeeds without these lines. If it does, it is probably ok.
| @@ -208,7 +203,7 @@ public function current() | |||
| if (is_array($this->buffer)) { | |||
| $this->buffer[$this->position] = $data; | |||
| } | |||
| return is_array($data) ? $data : null; | |||
| return (is_array($data) || $data instanceof Countable) ? $data : null; | |||
There was a problem hiding this comment.
@ezimuel Since this apparently fixes #330 because of a bug I might have introduced earlier, it is not needed to cover every single interface that could be returned by ->current(). If it fixes #330 and does not break anything, then it is ok, but we should see a unit test that confirms #330 and thus explains the need for the fix.
ezimuel
changed the title
|
Closing in favor of #337 |
This PR fixes #330 returning NULL if current() result is not countable. It removes and changes part of the
Zend\Db\ResultSet\AbstractResultSet::current()implementation modified with 8276cf6.