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

[Validator] Incorrect collection validation with different groups #23479

Closed
Aliance opened this Issue Jul 11, 2017 · 3 comments

Comments

Projects
None yet
4 participants
@Aliance
Contributor

Aliance commented Jul 11, 2017

Q A
Bug report? yes
Feature request? no
BC Break report? no
RFC? no
Symfony version 2.7.*

Hello everyone. I have the problem with yaml validation of collection with fields of different groups. I debugged a lot, and I came to the conclusion that there is a bug in validation of \Symfony\Component\Validator\Constraints\Collection with different groups.


Example description

For example, we had two equal entities: User and Category. Both have id, modifier and info (the array of some additional information with keys id, title and text).

– Root field id is required (I mean it should have the NotBlank constraint).
– Second field modifier is required only on update, so we add custom validation group called modify. We assume, on create we won't pass this group, but we will pass it on update.
– Third field info it is a large collection of different non-required parameters: in our case they are id (some other identification), title (length should be between 3 and 25 chars, only with group info) and text (length should be between 3 and 25 chars). I added additional validation group info for title because I wanted to check it not always, only when group is passed.


Problem reproduce

I prepared a little repository for tests to figure out the problem. The original successful code is located in validation branch of Aliance/SymfonyTest repository: https://github.com/Aliance/SymfonyTest/tree/validation
The failed code is located into validation-collection-with-different-groups branch, see pr: Aliance/SymfonyTest#1
If you execute phpunit tests you will find out that they fails after adding second group in other field on collection.

Note that tests fails only with yaml validation!


What I have already debugged

– First of all, code execution runs to \Symfony\Component\Validator\Validator\RecursiveContextualValidator::validate with my object as a $value and all three passed groups ['Default','modify','info'] as a $groups
2017-07-11 19 04 42
– Later it iterates on each groups on every field. Reaching the info type it will raise \Symfony\Component\Validator\Constraints\CollectionValidator::validate. It will repeat validation on every field without any groups passed:
2017-07-11 19 26 39
Looks strange, seems to me that CollectionValidator should pass $fieldConstraint->groups to validate method...
– But later somehow valid groups are came to validator: \Symfony\Component\Validator\Validator\RecursiveContextualValidator::validateEachObjectIn:
2017-07-11 19 16 03
– But in deeps of this method, checked constraint-group has already cached as validated, because cache key only consist of spl_object_hash($constraint), without validation group handling: see https://github.com/symfony/symfony/blob/v2.8.22/src/Symfony/Component/Validator/Validator/RecursiveContextualValidator.php#L866-L872


Can anyone approve this bug or tell me what I am doing wrong?

@sstok

This comment has been minimized.

Show comment
Hide comment
@sstok

sstok Jul 12, 2017

Contributor
Contributor

sstok commented Jul 12, 2017

@Aliance

This comment has been minimized.

Show comment
Hide comment
@Aliance

Aliance Jul 12, 2017

Contributor

Seems like #11505 broke groups handling at Symfony 2.5.

Contributor

Aliance commented Jul 12, 2017

Seems like #11505 broke groups handling at Symfony 2.5.

@Aliance Aliance changed the title from [Validation] Incorrect collection validation with different groups to [Validator] Incorrect collection validation with different groups Jul 13, 2017

@xabbuh

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh Jan 23, 2018

Member

just spotted that this is a duplicate of #17675

Member

xabbuh commented Jan 23, 2018

just spotted that this is a duplicate of #17675

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