8000 [Validator] Pass validation groups on Collection validate. by Aliance · Pull Request #23480 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

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

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 4 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a585590
Pass validation groups on Collection validate.
Aliance Jul 11, 2017
9d342a3
Pass validation groups to All validator.
Aliance Jul 12, 2017
7217c65
Expect Default validation group for validator mock.
Aliance Jul 12, 2017
5c3e710
Add collection validator group tests.
Aliance Sep 24, 2017
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 8000 a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

}
} else {
// 2.4 API
foreach ($value as $key => $element) {
$context->validateValue($element, $constraint->constraints, '['.$key.']');
$context->validateValue($element, $constraint->constraints, '['.$key.']', $constraint->groups);
}
}
}
Expand Down
4 changes: 2 additions & 2 deletions src/Symfony/Component/Validator/Constraints/CollectionValidator.php
8000
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
$context->validateValue($value[$field], $fieldConstraint->constraints, '['.$field.']');
$context->validateValue($value[$field], $fieldConstraint->constraints, '['.$field.']', $fieldConstraint->groups);
}
}
} elseif (!$fieldConstraint instanceof Optional && !$constraint->allowMissingFields) {
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 @@ -226,7 +226,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
112 changes: 112 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,116 @@ 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',
)
);

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

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

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

$constraint = new Collection(
array(
'fields' => array(
'baz' => array(
$fooConstraint,
$barConstraint,
),
),
'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'),
),
);
}
}
0