Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| * unique. | ||
| */ | ||
| if (0 ===count($result) || (1 ===count($result) &&$entity === ($resultinstanceof \Iterator ?$result->current() :current($result)))) { | ||
| if (null ===$result ||0 ===count($result) || (1 ===count($result) &&$entity === ($resultinstanceof \Iterator ?$result->current() :current($result)))) { |
There was a problem hiding this comment.
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 useis_countable via the polyfill?
There was a problem hiding this comment.
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 left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
(for 2.7)
stof commentedApr 6, 2018
Well, if your method returns The UniqueEntityValidator expects a results corresponding to |
gpenverne commentedApr 6, 2018
@stof indeed. Another way to fix is to ensure item is an array. What do you think about? |
xabbuh commentedApr 9, 2018
I think we should throw an exception then. |
gpenverne commentedApr 9, 2018
@xabbuh With php7.1, no trouble with this. Throw an exception will be a breaking change. |
xabbuh commentedApr 9, 2018
What result would you expect here if you do not return a countable collection? |
nicolas-grekas commentedApr 20, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 left a comment
There was a problem hiding this 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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 commentedApr 25, 2018
Thank you@gpenverne. |
…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 | MITPhp7.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 constraintFor 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
Uh oh!
There was an error while loading.Please reload this page.
Php7.2 will throw a warning on count(null)http://php.net/manual/en/migration72.incompatible.php
Error:
when no result returned on validating unique constraint
For example, on an entity with annotation uniqueEntity:
And in repository, a method
findMemberWithPasswordFromEmailwhich return null if no entity found (getOneOrNullResult)