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

Bugfix/array should be valid datasource for resultset #255

Closed
wants to merge 6 commits into
base: develop
from

Conversation

Projects
None yet
4 participants
@bartmcleod
Contributor

bartmcleod commented Jul 5, 2017

When I upgraded from an older version of Zend\Db I found that certain unit tests in the client project were failing. When a datasource provided to a ResultSet is an array, the ResultSet should work out of the box. It didn't, because current() did not account for a datasource which has entered the system as just an array, while the buffer is flagged -1, when the ResultSet was initialized with such a datasource. So that flag on the buffer can be used to early out current() in such a situation.

@bartmcleod bartmcleod changed the base branch from master to develop Jul 5, 2017

@waltertamboer

Couple of small things, other than that I don't see any problems.

Show outdated Hide outdated test/ResultSet/ReturnType.php Outdated
Show outdated Hide outdated test/ResultSet/ReturnType.php Outdated
@bartmcleod

This comment has been minimized.

Show comment
Hide comment
@bartmcleod

bartmcleod Jul 5, 2017

Contributor

Thanks @waltertamboer for the prompt review. I have updated the ReturnType test class.

Contributor

bartmcleod commented Jul 5, 2017

Thanks @waltertamboer for the prompt review. I have updated the ReturnType test class.

samsonasik added some commits Jul 5, 2017

Merge pull request #1 from samsonasik/bugfix/array-should-be-valid-da…
…tasource-for-resultset

use $returnType as ArrayObject for arrayobject prototype
@bartmcleod

This comment has been minimized.

Show comment
Hide comment
@bartmcleod

bartmcleod Jul 5, 2017

Contributor

@samsonasik thanks for your contribution

Contributor

bartmcleod commented Jul 5, 2017

@samsonasik thanks for your contribution

@ezimuel

This comment has been minimized.

Show comment
Hide comment
@ezimuel

ezimuel Nov 28, 2017

Member

@bartmcleod if this is a bugfix why did you send to develop branch?

Member

ezimuel commented Nov 28, 2017

@bartmcleod if this is a bugfix why did you send to develop branch?

@bartmcleod

This comment has been minimized.

Show comment
Hide comment
@bartmcleod

bartmcleod Nov 28, 2017

Contributor

@ezimuel I haven't contributed in a while. Where should it go? master? Happy to change the target.

Contributor

bartmcleod commented Nov 28, 2017

@ezimuel I haven't contributed in a while. Where should it go? master? Happy to change the target.

@ezimuel

This comment has been minimized.

Show comment
Hide comment
@ezimuel

ezimuel Nov 28, 2017

Member

@bartmcleod I can do that, no problem. I was just checking if this was a bugfix. Thanks!

Member

ezimuel commented Nov 28, 2017

@bartmcleod I can do that, no problem. I was just checking if this was a bugfix. Thanks!

@bartmcleod

This comment has been minimized.

Show comment
Hide comment
@bartmcleod

bartmcleod Nov 28, 2017

Contributor

@ezimuel Ok, thanks. It's a bugfix.

Contributor

bartmcleod commented Nov 28, 2017

@ezimuel Ok, thanks. It's a bugfix.

@ezimuel

This comment has been minimized.

Show comment
Hide comment
@ezimuel

ezimuel Nov 28, 2017

Member

@bartmcleod I tried to merge in master but it seems there are some issues. Can you try to send the PR against master? Thanks.

Member

ezimuel commented Nov 28, 2017

@bartmcleod I tried to merge in master but it seems there are some issues. Can you try to send the PR against master? Thanks.

@bartmcleod

This comment has been minimized.

Show comment
Hide comment
@bartmcleod

bartmcleod Nov 28, 2017

Contributor

@ezimuel I will set a reminder for myself

Contributor

bartmcleod commented Nov 28, 2017

@ezimuel I will set a reminder for myself

@ezimuel ezimuel added this to the 2.9.0 milestone Nov 28, 2017

@ezimuel

This comment has been minimized.

Show comment
Hide comment
@ezimuel

ezimuel Nov 30, 2017

Member

@bartmcleod I finally rebased the PR and merged in master (and develop). I'm closing the PR. Thanks again for your contribution!

Member

ezimuel commented Nov 30, 2017

@bartmcleod I finally rebased the PR and merged in master (and develop). I'm closing the PR. Thanks again for your contribution!

@ezimuel ezimuel closed this Nov 30, 2017

ezimuel added a commit that referenced this pull request Nov 30, 2017

@bartmcleod

This comment has been minimized.

Show comment
Hide comment
@bartmcleod

bartmcleod Nov 30, 2017

Contributor

@ezimuel welcome, and thanks for the merge. I had the reminder set for tomorrow morning, can clear that now :-)

Contributor

bartmcleod commented Nov 30, 2017

@ezimuel welcome, and thanks for the merge. I had the reminder set for tomorrow morning, can clear that now :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment