Skip to content

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

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

Merged
merged 1 commit into from
Apr 25, 2018
Merged

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

merged 1 commit into from
Apr 25, 2018

Conversation

gpenverne
Copy link
Contributor

@gpenverne gpenverne 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)))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

(for 2.7)

@nicolas-grekas nicolas-grekas added this to the 2.7 milestone Apr 6, 2018
@stof
Copy link
Member

stof 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
Copy link
Contributor Author

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

@xabbuh
Copy link
Member

xabbuh commented Apr 9, 2018

I think we should throw an exception then.

@gpenverne
Copy link
Contributor Author

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

@xabbuh
Copy link
Member

xabbuh commented Apr 9, 2018

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

@nicolas-grekas
Copy link
Member

nicolas-grekas 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 April 25, 2018 12:04
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

(changes applied, rebased on 2.7)

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

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

@nicolas-grekas
Copy link
Member

Thank you @gpenverne.

@nicolas-grekas nicolas-grekas merged commit 715373f into symfony:2.7 Apr 25, 2018
nicolas-grekas added a commit that referenced this pull request Apr 25, 2018
…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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants