[Validator] Propagate embedded groups for collection validator #17696

blazarecki · 2016-02-04T22:17:11Z

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

HeahDude · 2016-02-04T22:25:53Z

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

@@ -60,7 +60,7 @@ public function validate($value, Constraint $constraint)
                        $context->getValidator()
                            ->inContext($context)
                            ->atPath('['.$field.']')
-                            ->validate($value[$field], $fieldConstraint->constraints);
+                            ->validate($value[$field], $fieldConstraint->constraints, $context->getGroup());
                    } else {
                        // 2.4 API


It's not the scope of this PR but shouldn't this be removed in 2.7 ?

Why removed ? I check on the 2.8 and 3.0 branch and the code is the same.

blazarecki · 2016-02-04T23:41:02Z

The issue is fixed for the current API.
But it fail on the 2.4 API.

For the 2.4 API I've try to use the group from context and from the constraint and both don't work.

Any ideas ?

Here the phpunit errors: https://travis-ci.org/symfony/symfony/jobs/107124774#L2515-L2525

There were 2 failures:

1) Symfony\Component\Validator\Tests\LegacyValidatorTest::testValidateMultipleGroupsForCollectionConstraint
Failed asserting that actual size 4 matches expected size 2.

/Users/benjamin.lazarecki/Github/symfony/src/Symfony/Component/Validator/Tests/Validator/AbstractValidatorTest.php:1108

2) Symfony\Component\Validator\Tests\LegacyValidatorTest::testValidateMultipleGroupsForAllConstraint
Failed asserting that actual size 4 matches expected size 2.

/Users/benjamin.lazarecki/Github/symfony/src/Symfony/Component/Validator/Tests/Validator/AbstractValidatorTest.php:1132

backbone87 · 2016-02-06T10:31:55Z

There are also problems with GroupSequence.

    public function testValidateGroupSequenceForCollectionConstraint()
    {
        $entity = new Entity();
        $entity->firstName = array('baz' => 2);

        $sequence = new GroupSequence(array('bar', 'foo'));
        $this->metadata->setGroupSequence($sequence);

        $this->metadata->addPropertyConstraint('firstName', new Collection(
            array(
                'fields' => array(
                    'baz' => array(
                        new Range(array('min' => 3, 'minMessage' => 'Group foo', 'groups' => 'foo')),
                        new Range(array('min' => 5, 'minMessage' => 'Group bar', 'groups' => 'bar')),
                    ),
                ),
            )
        ));

        $violations = $this->validate($entity, null, 'Default');

        $this->assertCount(1, $violations);
        $this->assertSame('Group bar', $violations[0]->getMessage());
    }

This test should fail, but I did not actually run it.

blazarecki · 2016-02-06T13:31:38Z

@backbone87 I add the test for group sequence.

backbone87 · 2016-02-08T13:08:20Z

@blazarecki what should c1311b8 fix?

blazarecki · 2016-02-08T14:34:44Z

Hi @backbone87,

With this fix, now the current group is propagate to the embedded constraints.
Then the executeConstraintValidators method, filters only the constraint that match the given group.

dunglas · 2016-02-09T10:13:38Z

👍

GroupSequence can be fixed in another PR.

ping @symfony/deciders

dunglas · 2016-02-11T13:58:59Z

Status: reviewed

fabpot · 2016-02-12T16:28:23Z

👍

xabbuh · 2016-02-15T20:37:17Z

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?

Hi, @xabbuh

I try to use the context but the test fail, it's the only way I find to make it work:

1) Symfony\Component\Validator\Tests\Validator\LegacyValidator2Dot5ApiTest::testValidateMultipleGroupsForAllConstraint Failed asserting that actual size 1 matches expected size 2. symfony/src/Symfony/Component/Validator/Tests/Validator/AbstractValidatorTest.php:1129 2) Symfony\Component\Validator\Tests\Validator\LegacyValidatorLegacyApiTest::testValidateMultipleGroupsForAllConstraint Failed asserting that actual size 1 matches expected size 2. symfony/src/Symfony/Component/Validator/Tests/Validator/AbstractValidatorTest.php:1129 3) Symfony\Component\Validator\Tests\Validator\RecursiveValidator2Dot5ApiTest::testValidateMultipleGroupsForAllConstraint Failed asserting that actual size 1 matches expected size 2.

Do you have another idea ?

blazarecki · 2016-02-18T15:16:23Z

@webmozart what do you think of this PR ?

blazarecki · 2016-03-09T09:36:22Z

@javiereguiluz any new about this PR ? Is there something missing ? Thanks.

webmozart · 2016-03-10T07:40:35Z

src/Symfony/Component/Validator/ExecutionContext.php

        foreach ($constraints as $constraint) {
+            if (in_array($this->getGroup(), $constraint->groups)) {


This looks weird to me. Why is this necessary?

It's necessary because for embedded constraint with multiple validation groups, the validator is execute for each validation groups.

With this example:

$entity = new Entity(); $entity->firstName = array('baz' => 2); $this->metadata->addPropertyConstraint('firstName', new Collection( array( 'fields' => array( 'baz' => array( new Range(array('min' => 3, 'groups' => array('foo', 'bar'))), ), ), ) )); $violations = $this->validate($entity, null, array('foo', 'bar'));

We have two violations but we want only one.

Is there another way to fix that ?

webmozart · 2016-03-10T07:46:44Z

Thank you for working on this PR @blazarecki! I think that in general, this functionality is nice. This PR needs some work however to ensure that we don't introduce bugs with this functionality.

Status: Needs work

blazarecki · 2016-03-11T10:36:59Z

Thanks for your feedback, I'll work on it.

fabpot · 2016-07-01T05:46:37Z

@blazarecki What's the status of this PR? Can you answer @webmozart's question?

blazarecki · 2016-07-01T07:11:34Z

@webmozart what kind of work is needed for this PR to ensure that we don't introduce bugs with this functionality?

This reverts commit 4850301.

nicolas-grekas · 2018-01-21T17:37:03Z

@blazarecki rebase needed please. Can you please check again and see what's missing? Anyone else to help review?

blazarecki · 2018-01-22T15:37:51Z

Gitub cannot find the repository and branch so I create a new PR #25888

carsonbot added the Status: Needs Review label Feb 4, 2016

blazarecki mentioned this pull request Feb 4, 2016

[Validator] fix embedded groups are not propaged for collection validator #17695

Closed

HeahDude reviewed Feb 4, 2016
View reviewed changes

xabbuh added Status: Needs Work and removed Status: Needs Review labels Feb 8, 2016

blazarecki changed the title ~~[Validator] Propage embedded groups for collection validator~~ [Validator] Propagate embedded groups for collection validator Feb 9, 2016

blazarecki mentioned this pull request Feb 10, 2016

[Validator] All validation groups are not used on embedded constraints in a collection constraint #17675

Closed

carsonbot added Status: Reviewed and removed Status: Needs Work labels Feb 11, 2016

xabbuh reviewed Feb 15, 2016
View reviewed changes

javiereguiluz added Validator Bug labels Mar 3, 2016

webmozart reviewed Mar 10, 2016
View reviewed changes

carsonbot added Status: Needs Work and removed Status: Reviewed labels Mar 10, 2016

blazarecki mentioned this pull request Oct 20, 2016

[Validator] Allow validating multiple groups in one GroupSequence step #19982

Merged

Benjamin Lazarecki added 3 commits October 20, 2016 16:37

[Validator] Propage embedded groups for collection validator

ddd32c6

Add failling test for group sequence

20c0157

Revert "Add failling test for group sequence"

7a81029

This reverts commit 4850301.

nicolas-grekas added this to the 2.7 milestone Dec 6, 2016

nicolas-grekas mentioned this pull request Jul 28, 2017

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

Closed

blazarecki mentioned this pull request Jan 22, 2018

[Validator] Propagate embedded groups for collection validator #25888

Closed

blazarecki closed this Jan 22, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Validator] Propagate embedded groups for collection validator #17696

[Validator] Propagate embedded groups for collection validator #17696

blazarecki commented Feb 4, 2016

HeahDude Feb 4, 2016

blazarecki Feb 5, 2016

blazarecki commented Feb 4, 2016

backbone87 commented Feb 6, 2016

blazarecki commented Feb 6, 2016

backbone87 commented Feb 8, 2016

blazarecki commented Feb 8, 2016

dunglas commented Feb 9, 2016

dunglas commented Feb 11, 2016

fabpot commented Feb 12, 2016

xabbuh Feb 15, 2016

blazarecki Feb 16, 2016

blazarecki commented Feb 18, 2016

blazarecki commented Mar 9, 2016

webmozart Mar 10, 2016

blazarecki Jul 1, 2016

webmozart commented Mar 10, 2016

blazarecki commented Mar 11, 2016

fabpot commented Jul 1, 2016

blazarecki commented Jul 1, 2016

nicolas-grekas commented Jan 21, 2018

blazarecki commented Jan 22, 2018

		foreach ($constraints as $constraint) {
		if (in_array($this->getGroup(), $constraint->groups)) {

[Validator] Propagate embedded groups for collection validator #17696

[Validator] Propagate embedded groups for collection validator #17696

Conversation

blazarecki commented Feb 4, 2016

HeahDude Feb 4, 2016

Choose a reason for hiding this comment

blazarecki Feb 5, 2016

Choose a reason for hiding this comment

blazarecki commented Feb 4, 2016

backbone87 commented Feb 6, 2016

blazarecki commented Feb 6, 2016

backbone87 commented Feb 8, 2016

blazarecki commented Feb 8, 2016

dunglas commented Feb 9, 2016

dunglas commented Feb 11, 2016

fabpot commented Feb 12, 2016

xabbuh Feb 15, 2016

Choose a reason for hiding this comment

blazarecki Feb 16, 2016

Choose a reason for hiding this comment

blazarecki commented Feb 18, 2016

blazarecki commented Mar 9, 2016

webmozart Mar 10, 2016

Choose a reason for hiding this comment

blazarecki Jul 1, 2016

Choose a reason for hiding this comment

webmozart commented Mar 10, 2016

blazarecki commented Mar 11, 2016

fabpot commented Jul 1, 2016

blazarecki commented Jul 1, 2016

nicolas-grekas commented Jan 21, 2018

blazarecki commented Jan 22, 2018