[Validator] Pass validation groups on Collection validate. #23480

Aliance · 2017-07-11T16:49:12Z

Q	A
Branch?	2.7
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#23479
License	MIT

Complete description can be found in related issue.

nicolas-grekas · 2017-07-12T11:51:12Z

For some reason, Travis did not start. Please add tests also. The "BC break" flag means you should explain what's the precise behavior change, why you think we should still do it, explain why this cannot be done as a new feature on 3.4, with proper BC layer, etc.

Aliance · 2017-07-12T11:58:26Z

For some reason, Travis did not start.

Because I created a pr into master, and then changed to 2.8. Can you restart them manually?

Please add tests also.

Ok, I will try to find out where they should be.

you should explain what's the precise behavior change

All description in linked issue.

xabbuh · 2017-07-12T17:47:04Z

Does this not happen on Symfony 2.7?

Aliance · 2017-07-12T19:27:00Z

@xabbuh I'm using LTS version 2.8, but I can check 2.7 a bit later.

Aliance · 2017-07-12T22:10:15Z

So, I check all tests, after my fix I have to fix AbstractConstraintValidatorTest:: expectValidateValueAt: now by default the Default validation group is passed to the mock method (see b31795c). Has not added any new tests yet, any ideas the best place for them?

Have no ideas why appveyor build fails, but seems there are some global problems, not because of my code.

While fixing the tests, found another place with the same bug: AllValidator. Fix it too.

Aliance · 2017-07-13T08:18:22Z

Does this not happen on Symfony 2.7?

I have checked it, the answer is yes, this bug is also actual in Symfony 2.7. I think it will reproduce on 2.5+ (see linked pr in issue)

nicolas-grekas · 2017-07-13T13:41:37Z

Can you rebase on 2.7 please?

Aliance · 2017-07-13T14:03:55Z

@nicolas-grekas done!

So, does it BC break or not? I was not sure, so I mark pr as BC break. But may be it not 🤔

src/Symfony/Component/Validator/Tests/Constraints/AbstractConstraintValidatorTest.php

nicolas-grekas · 2017-07-28T09:02:44Z

@xabbuh WDYT?

nicolas-grekas · 2017-07-28T10:08:02Z

Just noticed that this looks a lot like #17696, maybe the discussion there would help move forward?

Aliance · 2017-07-31T05:14:18Z

I can't understand, why it pasts about 3 weeks since I've report a bug and make a PR that solves it, but it is still not merged. What should I do to speed up merging it?

trilobit · 2017-07-31T09:14:18Z

Hi! I have the same problem with validation groups. Let's merge this pull request.

trilobit · 2017-08-02T09:49:59Z

@nicolas-grekas @xabbuh what do you think?

xabbuh · 2017-09-20T11:16:19Z

The changes look valid to me. But can you add some test that would fail without them?

Aliance · 2017-09-24T18:38:23Z

@xabbuh I added some test (was taken from related pr), they fails in 2.7 without my fixes:

Aliance · 2017-09-24T18:39:33Z

@fabpot @nicolas-grekas @webmozart please look at my bugfix again and merge it please.

fabpot · 2017-09-24T18:54:16Z

You first need to rebase to get rid of the merge commit.

…

On 24 Sep 2017, at 20:39, Lesnykh Ilia ***@***.***> wrote: @fabpot @nicolas-grekas @webmozart please look at my bugfix again and merge it please. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

Aliance · 2017-09-24T18:55:47Z

@fabpot I've already rebased, mb you see the cached commits?

Aliance · 2017-09-28T10:53:20Z

@fabpot @xabbuh @nicolas-grekas is there something wrong with this pr, why you don't reply to me? :)
Also, please, remove BC break label, this is not BC.

javiereguiluz · 2017-09-28T11:11:12Z

@Aliance I've removed the BC Break. About the lack of response, please be more patient. At the moment we're focused on the Symfony 3.4 and Symfony 4.0 feature freeze which ends this weekend. Since your pull request is about a bug fix, we can work on it even during the feature freeze period. Thanks!

Aliance · 2017-11-25T09:39:18Z

May be someone can take a look at this bugfix now?

xabbuh · 2017-11-28T12:56:36Z

src/Symfony/Component/Validator/Constraints/AllValidator.php

@@ -44,12 +44,12 @@ public function validate($value, Constraint $constraint)
            $validator = $context->getValidator()->inContext($context);

            foreach ($value as $key => $element) {
-                $validator->atPath('['.$key.']')->validate($element, $constraint->constraints);
+                $validator->atPath('['.$key.']')->validate($element, $constraint->constraints, $constraint->groups);


I asked the same in #17696 too: Don't we have to use $context->getGroup() here to not accidentally widen the validation scope?

I think getters is better than properties, but there is $constraint->constraints not $constraint->getConstraints(). So I wrote the same way.
Should I change only groups, change both or no one?

No, what I mean is fetching the groups from the context but not from the constraint? There may be more groups configured on the constraint than what groups are actually used to validate.

Nope, It's not the same. Please see my original issue: #23479
I have attached xdebug screenshots there.
Also I have post a link to my symfony standart fork with easy code, which is broken with current version of Symfony, but it works with my fixes. You can clone it and make sure that $context->getGroup will have only a Default string, while $constraint->constraints will have an array of needed groups: ["info", "Default", "Category] for the Category entity as it was set in validation.yml

You are right that there is still a problem. But just passing the constraints here does not work either. For example, if you take your example project and modify the controller to only run validation in the Default and modify group, the Length constraint will still be evaluated which is only part of the info group.

nicolas-grekas · 2018-01-21T17:47:53Z

@Aliance how could we move forward here?

Aliance · 2018-01-23T07:27:44Z

@nicolas-grekas I have no idea, but 1 year bug in Symfony makes me sad 😢
Does someone has any ideas?

xabbuh · 2018-01-23T09:27:50Z

see also #25888 which tries to fix the issue too (currently with the same approach)

artursvonda · 2018-08-02T10:49:51Z

Need any help with finishing this PR? Seems easy enough. Is changing $constraint->groups to fetching group from context only change required here?

xabbuh · 2018-12-07T14:52:57Z

Thank you all for your input and working on this. This is finally going to be fixed in #29499.

@stof

This PR was merged into the 3.4 branch. Discussion ---------- [Validator] Fixed grouped composite constraints | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #17675, #25888, #23480 | License | MIT | Doc PR | ~ From Lisbon :). Thanks @stof, @xabbuh for your help to finally fix this old issue. Commits ------- b53d911 [Validator] Fixed grouped composite constraints

carsonbot added Status: Needs Review Bug BC Break labels Jul 11, 2017

Aliance changed the base branch from master to 2.8 July 11, 2017 16:50

Aliance changed the title ~~[Validation] Pass validation groups on Collection validate.~~ [Validator] Pass validation groups on Collection validate. Jul 13, 2017

nicolas-grekas added this to the 2.7 milestone Jul 13, 2017

Aliance force-pushed the pass-groups-on-collection-validate branch from 9687609 to 7a8cc0d Compare July 13, 2017 14:03

Aliance changed the base branch from 2.8 to 2.7 July 13, 2017 14:03

nicolas-grekas reviewed Jul 28, 2017

View reviewed changes

src/Symfony/Component/Validator/Tests/Constraints/AbstractConstraintValidatorTest.php Outdated Show resolved Hide resolved

Aliance force-pushed the pass-groups-on-collection-validate branch from 89f25c2 to 343c8ce Compare July 31, 2017 05:01

Aliance added 4 commits September 24, 2017 21:35

Pass validation groups on Collection validate.

a585590

Pass validation groups to All validator.

9d342a3

Expect Default validation group for validator mock.

7217c65

Add collection validator group tests.

5c3e710

Aliance force-pushed the pass-groups-on-collection-validate branch from 185b55c to 5c3e710 Compare September 24, 2017 18:36

javiereguiluz removed the BC Break label Sep 28, 2017

xabbuh reviewed Nov 28, 2017

View reviewed changes

xabbuh mentioned this pull request Jan 23, 2018

[Validator] Propagate embedded groups for collection validator #25888

Closed

xabbuh added Status: Needs Work Validator and removed Status: Needs Review labels Jan 23, 2018

fabpot modified the milestones: 2.7, 2.8 May 28, 2018

nicolas-grekas modified the milestones: 2.8, 3.4 Nov 26, 2018

HeahDude mentioned this pull request Dec 7, 2018

[Validator] Fixed grouped composite constraints #29499

Merged

xabbuh closed this Dec 7, 2018

Aliance deleted the pass-groups-on-collection-validate branch December 8, 2018 16:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Validator] Pass validation groups on Collection validate. #23480

[Validator] Pass validation groups on Collection validate. #23480

Aliance commented Jul 11, 2017 •

edited

nicolas-grekas commented Jul 12, 2017

Aliance commented Jul 12, 2017

xabbuh commented Jul 12, 2017

Aliance commented Jul 12, 2017 •

edited

Aliance commented Jul 12, 2017

Aliance commented Jul 13, 2017

nicolas-grekas commented Jul 13, 2017

Aliance commented Jul 13, 2017 •

edited

nicolas-grekas commented Jul 28, 2017

nicolas-grekas commented Jul 28, 2017

Aliance commented Jul 31, 2017

trilobit commented Jul 31, 2017

trilobit commented Aug 2, 2017

xabbuh commented Sep 20, 2017

Aliance commented Sep 24, 2017

Aliance commented Sep 24, 2017

fabpot commented Sep 24, 2017 via email

Aliance commented Sep 24, 2017

Aliance commented Sep 28, 2017

javiereguiluz commented Sep 28, 2017

Aliance commented Nov 25, 2017

xabbuh Nov 28, 2017

Aliance Nov 28, 2017

xabbuh Nov 28, 2017

Aliance Dec 4, 2017

xabbuh Dec 4, 2017

nicolas-grekas commented Jan 21, 2018

Aliance commented Jan 23, 2018

xabbuh commented Jan 23, 2018

artursvonda commented Aug 2, 2018

xabbuh commented Dec 7, 2018

[Validator] Pass validation groups on Collection validate. #23480

[Validator] Pass validation groups on Collection validate. #23480

Conversation

Aliance commented Jul 11, 2017 • edited

nicolas-grekas commented Jul 12, 2017

Aliance commented Jul 12, 2017

xabbuh commented Jul 12, 2017

Aliance commented Jul 12, 2017 • edited

Aliance commented Jul 12, 2017

Aliance commented Jul 13, 2017

nicolas-grekas commented Jul 13, 2017

Aliance commented Jul 13, 2017 • edited

nicolas-grekas commented Jul 28, 2017

nicolas-grekas commented Jul 28, 2017

Aliance commented Jul 31, 2017

trilobit commented Jul 31, 2017

trilobit commented Aug 2, 2017

xabbuh commented Sep 20, 2017

Aliance commented Sep 24, 2017

Aliance commented Sep 24, 2017

fabpot commented Sep 24, 2017 via email

Aliance commented Sep 24, 2017

Aliance commented Sep 28, 2017

javiereguiluz commented Sep 28, 2017

Aliance commented Nov 25, 2017

xabbuh Nov 28, 2017

Choose a reason for hiding this comment

Aliance Nov 28, 2017

Choose a reason for hiding this comment

xabbuh Nov 28, 2017

Choose a reason for hiding this comment

Aliance Dec 4, 2017

Choose a reason for hiding this comment

xabbuh Dec 4, 2017

Choose a reason for hiding this comment

nicolas-grekas commented Jan 21, 2018

Aliance commented Jan 23, 2018

xabbuh commented Jan 23, 2018

artursvonda commented Aug 2, 2018

xabbuh commented Dec 7, 2018

Aliance commented Jul 11, 2017 •

edited

Aliance commented Jul 12, 2017 •

edited

Aliance commented Jul 13, 2017 •

edited