8000 [Validator] Propagate embedded groups for collection validator by blazarecki · Pull Request #17696 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Validator] Propagate embedded groups for collection validator #17696

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

[Validator] Propagate embedded groups for collection validator #17696

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ddd32c6
[Validator] Propage embedded groups for collection validator
Feb 4, 2016
20c0157
Add failling test for group sequence
Feb 6, 2016
7a81029
Revert "Add failling test for group sequence"
Feb 9, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/Symfony/Component/Validator/Constraints/AllValidator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

}
} else {
// 2.4 API
foreach ($value as $key => $element) {
$context->validateValue($element, $constraint->constraints, '['.$key.']');
$context->validateValue($element, $constraint->constraints, '['.$key.']', $context->getGroup());
}
}
}
Expand Down
4 changes: 2 additions & 2 deletions src/Symfony/Component/Validator/Constraints/CollectionValidator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
} else {
// 2.4 API
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

$context->validateValue($value[$field], $fieldConstraint->constraints, '['.$field.']');
$context->validateValue($value[$field], $fieldConstraint->constraints, '['.$field.']', $context->getGroup());
}
}
} elseif (!$fieldConstraint instanceof Optional && !$constraint->allowMissingFields) {
Expand Down
8 changes: 8 additions & 0 deletions src/Symfony/Component/Validator/ExecutionContext.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,15 @@ public function getMetadataFactory()
*/
private function executeConstraintValidators($value, array $constraints)
{
// Filter constraints with the context group.
$filteredConstraints = array();
foreach ($constraints as $constraint) {
if (in_array($this->getGroup(), $constraint->groups)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks weird to me. Why is this necessary?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

$filteredConstraints[] = $constraint;
}
}

foreach ($filteredConstraints as $constraint) {
$validator = $this->globalContext->getValidatorFactory()->getInstance($constraint);
$validator->initialize($this);
$validator->validate($value, $constraint);
Expand Down
2 changes: 1 addition & 1 deletion src/Symfony/Component/Validator/Tests/Constraints/AbstractConstraintValidatorTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,7 @@ protected function expectValidateAt($i, $propertyPath, $value, $group)
->with($value, $this->logicalOr(null, array()), $group);
}

protected function expectValidateValueAt($i, $propertyPath, $value, $constraints, $group = null)
protected function expectValidateValueAt($i, $propertyPath, $value, $constraints, $group = array(Constraint::DEFAULT_GROUP))
{
$contextualValidator = $this->context->getValidator()->inContext($this->context);
$contextualValidator->expects($this->at(2 * $i))
Expand Down
60 changes: 60 additions & 0 deletions src/Symfony/Component/Validator/Tests/Constraints/CollectionValidatorTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -386,4 +386,64 @@ public function testObjectShouldBeLeftUnchanged()
'foo' => 3,
), (array) $value);
}

/**
* @expectedException \Symfony\Component\Validator\Exception\ConstraintDefinitionException
*/
public function testGroupShouldBePassedToTheContainingConstraint()
{
$value = array('baz' => 2);

$constraint = new Collection(
array(
'fields' => array(
'baz' => array(
new Range(array('min' => 1, 'groups' => 'bar')),
),
),
'groups' => 'foo',
)
);

$data = $this->prepareTestData($value);
$this->validator->validate($data, $constraint);
}

/**
* @dataProvider multipleGroupsForCollectionProvider
*/
public function testValidateMultipleGroupsForCollectionConstraint($fooGroups, $barGroups, $collectionGroups, $expectedGroups)
{
$value = array('baz' => 2);

$constraint = new Collection(
array(
'fields' => array(
'baz' => array(
$fooConstraint = new Range(array('min' => 3, 'minMessage' => 'Group foo', 'groups' => $fooGroups)),
$barConstraint = new Range(array('min' => 5, 'minMessage' => 'Group bar', 'groups' => $barGroups)),
),
),
'groups' => $collectionGroups,
)
);

$data = $this->prepareTestData($value);

$this->expectValidateValueAt(0, '[baz]', $value['baz'], array($fooConstraint, $barConstraint), $expectedGroups);

$this->validator->validate($data, $constraint);
}

public static function multipleGroupsForCollectionProvider()
{
return array(
array(array('foo', 'bar'), array('foo', 'bar'), array('foo', 'bar'), array('foo', 'bar')),
array(array('foo', 'bar'), array('bar'), array('foo', 'bar'), array('foo', 'bar')),
array(array('foo'), array('foo', 'bar'), array('foo', 'bar'), array('foo', 'bar')),
array(array('foo'), array('bar'), array('foo', 'bar'), array('foo', 'bar')),
array(array('foo'), array('foo'), array('foo', 'bar'), array('foo')),
array(array('foo'), array('foo'), array('foo'), array('foo')),
);
}
}
47 changes: 47 additions & 0 deletions src/Symfony/Component/Validator/Tests/Validator/AbstractValidatorTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,11 @@

namespace Symfony\Component\Validator\Tests\Validator;

use Symfony\Component\Validator\Constraints\All;
use Symfony\Component\Validator\Constraints\Callback;
use Symfony\Component\Validator\Constraints\Collection;
use Symfony\Component\Validator\Constraints\GroupSequence;
use Symfony\Component\Validator\Constraints\Range;
use Symfony\Component\Validator\Constraints\Valid;
use Symfony\Component\Validator\ConstraintViolationInterface;
use Symfony\Component\Validator\ExecutionContextInterface;
Expand Down Expand Up @@ -1079,6 +1082,50 @@ public function testValidateMultipleGroups()
$this->assertCount(2, $violations);
}

public function testValidateMultipleGroupsForCollectionConstraint()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test belongs in CollectionValidatorTest. Also, I'm missing tests for what is happening when one or more groups are set on the Collection constraint itself. You should systematically test the different permutations:

  • Collection group equals that of the constraints
  • Collection group does not equal that of the constraints
  • Collection groups are a superset of the groups of the constraints
  • Collection groups are a subset of the groups of the constraints
  • Collection groups and constraing groups don't overlap

The same applies to AllValidator.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@blazarecki Can you address this comment?

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

$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, array('foo', 'bar'));

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

public function testValidateMultipleGroupsForAllConstraint()
{
$entity = new Entity();
$entity->firstName = array(1);

$this->metadata->addPropertyConstraint('firstName', new All(
array(
'constraints' => 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, array('foo', 'bar'));

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

public function testReplaceDefaultGroupByGroupSequenceObject()
{
$entity = new Entity();
Expand Down
0