Skip to content
This repository was archived by the owner on Jan 29, 2020. It is now read-only.

Fix #295: handle empty array as datasource at AbstractResultSet::initialize() at php 7.2 #303

Merged
merged 3 commits into from
Apr 3, 2018

Conversation

samsonasik
Copy link
Contributor

@samsonasik samsonasik commented Feb 1, 2018

Provide a narrative description of what you are trying to accomplish:

  • Are you fixing a bug?
    • Detail how the bug is invoked currently.
      is count() on datasource whenever datasource is empty array
    • Detail the original, incorrect behavior.
      when datasource is empty array, it got error: "count(): Parameter must be an array or an object that implements Countable"
    • Detail the new, expected behavior.
      check the current() method, when it return false, set the count itself as 0.
    • Base your feature on the master branch, and submit against that branch.
    • Add a regression test that demonstrates the bug, and proves the fix.
    • Add a CHANGELOG.md entry for the fix.

Fix #295

CHANGELOG.md Outdated
@@ -22,7 +22,7 @@ All notable changes to this project will be documented in this file, in reverse

### Fixed

- Nothing.
- [#303](https://github.com/zendframework/zend-db/pull/303) fix error when datasource passed to `AbstractResultSet::initialize()` is empty array at php 7.2 environment.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#303 is the PR but #295 is the bug report. You should use #295.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done ;), I've updated the reference link to use #295

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@froschdesign

#303 is the PR but #295 is the bug report. You should use #295.

Why?
All changelog use PR link not bug report.

Copy link

@mvoelker mvoelker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solves the problem with my code and unit tests with php 7.2.

@samsonasik
Copy link
Contributor Author

is there any chance to get it merged? Thank you.

@adear11
Copy link

adear11 commented Mar 22, 2018

Just wondering if this will be merged into a release any time soon. I have an Apigility API that is using zend-db. This is blocking me from upgrading it to php 7.2.

Copy link
Member

@michalbundyra michalbundyra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@samsonasik
Copy link
Contributor Author

rebased.

@ezimuel ezimuel merged commit 587789c into zendframework:master Apr 3, 2018
@samsonasik samsonasik deleted the fix-295 branch April 3, 2018 12:22
@jhse-labs
Copy link

It's good to use zend-db now with PHP 7.2!
Unfortunately this patch changed the return type behaviour of AbstractResultSet->current().
When the result set is empty (array has zero elements), the current() method returned null prior this bugfix. But now it returns an empty array (array() or []) in that case.

I noticed this as the persistence layer in my application stopped working in some cases. So iIf anyone else runs into similar problems, maybe this entry helps!

@samsonasik
Copy link
Contributor Author

@rzjack I created test at #313 and the AbstractResultSet::current() still return null ( correct ). I can't reproduce issue with the test at AbstractResultSet::current() with initialize with empty array. Please provide unit test to prove the changed behavior.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Initializing a result set with empty array produces warning on PHP 7.2
9 participants