Skip to content
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

[Bridge/Doctrine] count(): Parameter must be an array or an object that implements Countable #26831

Merged
merged 1 commit into from Apr 25, 2018

Conversation

Projects
None yet
6 participants
@gpenverne
Copy link
Contributor

commented Apr 6, 2018

Q A
Branch? master
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
License MIT

Php7.2 will throw a warning on count(null) http://php.net/manual/en/migration72.incompatible.php

Error:

count(): Parameter must be an array or an object that implements Countable

when no result returned on validating unique constraint

For example, on an entity with annotation uniqueEntity:

 @UniqueEntity(
     fields={"email"},
     repositoryMethod="findMemberWithPasswordFromEmail",
 )

And in repository, a method findMemberWithPasswordFromEmail which return null if no entity found (getOneOrNullResult)

@@ -155,7 +155,7 @@ public function validate($entity, Constraint $constraint)
* which is the same as the entity being validated, the criteria is
* unique.
*/
if (0 === count($result) || (1 === count($result) && $entity === ($result instanceof \Iterator ? $result->current() : current($result)))) {
if (null === $result || 0 === count($result) || (1 === count($result) && $entity === ($result instanceof \Iterator ? $result->current() : current($result)))) {

This comment has been minimized.

Copy link
@linaori

linaori Apr 6, 2018

Contributor

Would it be an option to use is_countable via the polyfill?

This comment has been minimized.

Copy link
@linaori

linaori Apr 6, 2018

Contributor

Ah it seems like it's not available (yet): https://github.com/symfony/polyfill-php73 /cc @nicolas-grekas

@nicolas-grekas nicolas-grekas changed the title [Bridge/Doctrine] count(): Parameter must be an array or an object th… [Bridge/Doctrine] count(): Parameter must be an array or an object that implements Countable Apr 6, 2018

@nicolas-grekas
Copy link
Member

left a comment

(for 2.7)

@nicolas-grekas nicolas-grekas added this to the 2.7 milestone Apr 6, 2018

@stof

This comment has been minimized.

Copy link
Member

commented Apr 6, 2018

Well, if your method returns object|null rather than returning object[], the following code is broken as well, in case the current object is found.

The UniqueEntityValidator expects a results corresponding to getResults, i.e. an array.

@gpenverne

This comment has been minimized.

Copy link
Contributor Author

commented Apr 6, 2018

@stof indeed. Another way to fix is to ensure item is an array. What do you think about?

@xabbuh

This comment has been minimized.

Copy link
Member

commented Apr 9, 2018

I think we should throw an exception then.

@gpenverne

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2018

@xabbuh With php7.1, no trouble with this. Throw an exception will be a breaking change.

@xabbuh

This comment has been minimized.

Copy link
Member

commented Apr 9, 2018

What result would you expect here if you do not return a countable collection?

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Apr 20, 2018

Actually, even the "instanceof Iterator" case is broken, because not all iterators are countable.

Here is an alternative proposal that should do it:

--- a/src/Symfony/Bridge/Doctrine/Validator/Constraints/UniqueEntityValidator.php
+++ b/src/Symfony/Bridge/Doctrine/Validator/Constraints/UniqueEntityValidator.php
@@ -132,16 +132,26 @@ class UniqueEntityValidator extends ConstraintValidator
          * iterator implementation.
          */
         if ($result instanceof \Iterator) {
             $result->rewind();
+            $result = $result instanceof \Countable && 1 < count($result) ? array($result->current(), $result->current()) : (array) $result->current();
         } elseif (is_array($result)) {
             reset($result);
+        } else {
+            $result = (array) $result;
         }
 
         /* If no entity matched the query criteria or a single entity matched,
          * which is the same as the entity being validated, the criteria is
          * unique.
          */
-        if (0 === count($result) || (1 === count($result) && $entity === ($result instanceof \Iterator ? $result->current() : current($result)))) {
+        if (!$result || (1 === \count($result) && current($result) === $entity)) {
             return;
         }

@nicolas-grekas nicolas-grekas changed the base branch from master to 2.7 Apr 25, 2018

@nicolas-grekas
Copy link
Member

left a comment

(changes applied, rebased on 2.7)

$result = array($result->current(), $result->current());
} else {
$result = $result->current();
$result = null === $result ? array() : array($result);

This comment has been minimized.

Copy link
@xabbuh

xabbuh Apr 25, 2018

Member

Should we also account for iterators that have more than one result?

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Apr 25, 2018

Member

I fear this could consume the iterator, could be an issue with non-rewindable ones

This comment has been minimized.

Copy link
@xabbuh

xabbuh Apr 25, 2018

Member

hm indeed, we should rather not break that, might be a good idea to deprecate some of the edge cases we currently try to handle

@xabbuh

xabbuh approved these changes Apr 25, 2018

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Apr 25, 2018

Thank you @gpenverne.

@nicolas-grekas nicolas-grekas merged commit 715373f into symfony:2.7 Apr 25, 2018

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request Apr 25, 2018

bug #26831 [Bridge/Doctrine] count(): Parameter must be an array or a…
…n object that implements Countable (gpenverne)

This PR was merged into the 2.7 branch.

Discussion
----------

[Bridge/Doctrine] count(): Parameter must be an array or an object that implements Countable

| Q             | A
| ------------- | ---
| Branch?       | master |
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| License       | MIT

Php7.2 will throw a warning on count(null) [http://php.net/manual/en/migration72.incompatible.php](http://php.net/manual/en/migration72.incompatible.php)

Error:
```
count(): Parameter must be an array or an object that implements Countable
```
when no result returned on validating unique constraint

For example, on an entity with annotation uniqueEntity:
```
 @UniqueEntity(
     fields={"email"},
     repositoryMethod="findMemberWithPasswordFromEmail",
 )
```

And in repository, a method ``findMemberWithPasswordFromEmail`` which return null if no entity found (``getOneOrNullResult``)

Commits
-------

715373f [Bridge/Doctrine] fix count() notice on PHP 7.2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.