[Validator] Propagate embedded groups for collection validator #25888

blazarecki · 2018-01-22T15:36:49Z

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#17675
License	MIT
Doc PR	no

This PR fix the issue describe in #17675
This PR is related to #17696

hhamon · 2018-01-23T09:07:40Z

/propage/propagate/ in your commit message :)

xabbuh · 2018-01-23T09:23:37Z

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);


Shouldn't this be $context->getGroup() too?

To give more context: This implementation suffers the same implications as discussed in #23480 (comment).

The Composite constraint is special, All and Collection inherit it.
The rules are explained here https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Validator/Constraints/Composite.php#L34-L48, we need to not break them.
So I tend to agree with @xabbuh that we should not just rely on the groups public property of nested constraints to resolve any issue.

xabbuh · 2018-01-23T09:23:47Z

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

@@ -60,10 +60,10 @@ public function validate($value, Constraint $constraint)
                        $context->getValidator()
                            ->inContext($context)
                            ->atPath('['.$field.']')
-                            ->validate($value[$field], $fieldConstraint->constraints);
+                            ->validate($value[$field], $fieldConstraint->constraints, $fieldConstraint->groups);


backbone87 · 2018-01-24T05:56:57Z

can we please close either #23480 or this one? both address the same problem and propose the same fixing strategy.

Using $contraint->groups is wrong. You need to use $context->getGroup(), because that is the group that is actually validated in the current call of the contraints validator.
The following produces a violation with this PR, but it should not:

$contraint = new Collection([
  'fields' => [
    'key' => new Length(['min' => 6, groups => ['b']]),
  ],
  'groups' => ['a', 'b'],
]);
$validator->validate([ 'key' => 'value' ], $contraint, 'a');

From my point of view we have a 2 possibilities:

Deprecate/disallow validation groups in nested contraints, because they dont work correctly and document the use case presented in [Validator] All validation groups are not used on embedded constraints in a collection constraint #17675 and [Validator] Incorrect collection validation with different groups #23479 to use multiple Collection and All constraints (something like here [Validator] All validation groups are not used on embedded constraints in a collection constraint #17675 (comment) but with groups moved to the composite contraint)
Disable caching for composite contraints (and optionally deprecate/disallow validation groups on composite contraints)
Change the caching strategy

HeahDude · 2018-01-24T08:00:32Z

I'm not sure there is really an issue here, just a misusage while not being aware of https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Validator/Constraints/Composite.php#L34-L48 (documentation issue).

Actually #17675 (comment), could be written:

    /**
     * @Collection(
     *     fields={
     *          "foo" = {
     *              @NotBlank(groups={"foo"}),
     *              @Length(min="2", groups={"bar"}),
     *              @Length(min="4", groups={"baz"}),
     *          }
     *     },
     *    groups={"foo", "bar", "baz"}
     * )
     */

And this should be the way to go IMHO.

backbone87 · 2018-01-24T17:48:34Z

This doesnt work with grp sequences

HeahDude · 2018-01-25T00:16:11Z

So this is what needs to be fixed :). Could you please update your tests here @blazarecki?

blazarecki · 2018-01-25T08:41:25Z

Yes I'll do that.

backbone87 · 2018-01-25T12:52:33Z

@blazarecki imho there is nothing wrong with grp sequences. the possible fix path are described in my comment above. Despite the comment in https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Validator/Constraints/Composite.php#L34 different grps for composite contraint and nested constraints just dont work.

If we dont want to change anything, then my proposed solution 1 is a good way.
If we want to support grps on nested contraints correctly, then contraints must be able to opt out of caching (my solution 2). Maybe with a marker interface or a contraint setting (a setting that is decided at dev time of the contraint). This also requires, that a contraint, that does not get cached, never does any validation of its own, but depends for the decision "pass" or "not pass" entirely and only on its nested contraints.

fabpot · 2018-05-28T06:06:07Z

@HeahDude As you started to work on this one (at least commenting on it) I need your help for the next steps (related to #23480 as well)?

artursvonda · 2018-08-02T12:27:03Z

Any help needed with this one? Comments seem to be addressed and tests passing.

blazarecki · 2018-08-02T12:50:05Z

@artursvonda tell me if I need to change something.

artursvonda · 2018-08-02T12:55:50Z

Just wanted to bump up the ticket since comments already seem to be addressed. @fabpot @HeahDude ?

HeahDude

As said in my previous comment, there is no issue to fix as is.

Tests must be adapted to check if group sequences fail in some way as @backbone87 claimed.
Otherwise we can close here.

HeahDude · 2018-08-02T18:02:40Z

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, array($context->getGroup()));


Only subsets are allowed on traversable node. This would be a broken feature not a bug fix.

HeahDude · 2018-08-02T18:04:37Z

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

+                         new Range(array('min' => 1, 'groups' => 'bar')),
+                     ),
+                 ),
+                 'groups' => 'foo',


This test is wrong and breaks BC. Groups should be array('foo, 'bar') here, and the test would be green already, without the changes introduced in this PR.

HeahDude · 2018-08-02T18:08:59Z

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

+            array(array('foo'), array('foo', 'bar'), array('foo', 'bar')),
+            array(array('foo'), array('bar'), array('foo', 'bar')),
+            array(array('foo'), array('foo'), array('foo', 'bar')),
+            array(array('foo'), array('foo'), array('foo')),


You're passing foo and bar for every case of collection groups above, but not here, while there is no bar group on any entry.
Are there some cases actually failing before your changes?

HeahDude · 2018-08-02T18:09:59Z

src/Symfony/Component/Validator/Tests/LegacyValidatorTest.php

+                    ),
+                ),
+            )
+        ));


If there is no group set on the collection contraint, group on entry should never be validated (since it won't ever be a subset).

HeahDude · 2018-08-02T18:10:21Z

src/Symfony/Component/Validator/Tests/LegacyValidatorTest.php

+                    new Range(array('min' => 5, 'minMessage' => 'Group bar', 'groups' => 'bar')),
+                ),
+            )
+        ));


Aliance · 2018-08-15T14:00:15Z

As said in my previous comment, there is no issue to fix as is.

@HeahDude so in my issue #23479 (and related pr #23480) there is no issue too, you think?

xabbuh · 2018-12-07T14:53:02Z

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 Validator Bug labels Jan 22, 2018

blazarecki mentioned this pull request Jan 22, 2018

[Validator] Propagate embedded groups for collection validator #17696

Closed

xabbuh added this to the 2.7 milestone Jan 23, 2018

xabbuh reviewed Jan 23, 2018

View reviewed changes

xabbuh mentioned this pull request Jan 23, 2018

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

Closed

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

[Validator] Propagate embedded groups for collection validator

e0ecd17

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

HeahDude suggested changes Aug 2, 2018

View reviewed changes

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Validator] Propagate embedded groups for collection validator #25888

[Validator] Propagate embedded groups for collection validator #25888

blazarecki commented Jan 22, 2018

hhamon commented Jan 23, 2018

xabbuh Jan 23, 2018

xabbuh Jan 23, 2018

HeahDude Jan 23, 2018

xabbuh Jan 23, 2018

backbone87 commented Jan 24, 2018

HeahDude commented Jan 24, 2018

backbone87 commented Jan 24, 2018

HeahDude commented Jan 25, 2018

blazarecki commented Jan 25, 2018

backbone87 commented Jan 25, 2018 •

edited

fabpot commented May 28, 2018

artursvonda commented Aug 2, 2018

blazarecki commented Aug 2, 2018

artursvonda commented Aug 2, 2018

HeahDude left a comment

HeahDude Aug 2, 2018

HeahDude Aug 2, 2018

HeahDude Aug 2, 2018

HeahDude Aug 2, 2018

HeahDude Aug 2, 2018

Aliance commented Aug 15, 2018

xabbuh commented Dec 7, 2018

[Validator] Propagate embedded groups for collection validator #25888

[Validator] Propagate embedded groups for collection validator #25888

Conversation

blazarecki commented Jan 22, 2018

hhamon commented Jan 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

backbone87 commented Jan 24, 2018

HeahDude commented Jan 24, 2018

backbone87 commented Jan 24, 2018

HeahDude commented Jan 25, 2018

blazarecki commented Jan 25, 2018

backbone87 commented Jan 25, 2018 • edited

fabpot commented May 28, 2018

artursvonda commented Aug 2, 2018

blazarecki commented Aug 2, 2018

artursvonda commented Aug 2, 2018

HeahDude left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aliance commented Aug 15, 2018

xabbuh commented Dec 7, 2018

backbone87 commented Jan 25, 2018 •

edited