Skip to content
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

Jump to bottom

[Validator][GroupSequences][Form] filter out excess violations while validating form field constraints with sequence of groups #35556

Closed
wants to merge 1 commit into from
Closed

[Validator][GroupSequences][Form] filter out excess violations while validating form field constraints with sequence of groups #35556

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
49 changes: 49 additions & 0 deletions src/Symfony/Component/Form/Extension/Validator/Constraints/FormValidator.php
View file
Open in desktop
Expand Up @@ -65,6 +65,9 @@ public function validate($form, Constraint $formConstraint)

if ($groups instanceof GroupSequence) {
$validator->atPath('data')->validate($form->getData(), $constraints, $groups);

$this->filterFormFieldsGroupSequenceViolations($groups);
Copy link
Member

Choose a reason for hiding this comment

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

What about iterating the groups provided by the group sequence instead and check after each iteration whether or not the number of violations increased? We would then stop iterating once the number of violations changed.

With the current solution validators for constraints in "later" groups are still executed which they are not supposed to be.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I thought about that.

Fields that are validated in FromValidator have a form field order that is not related to the group validation order. Thus, in general, at this stage, we know almost nothing about the need for validation of the field. We know nothing about any other form fields that have been or will be validated, as well as of their groups.

The second point is that if we try to filter constraints by group before validation, we must reproduce all the logic of the group sequence, which is incorporated in the validator itself and does not belong to FromValidator extension. But this is not an ultimate solution, because we do not know which field with which groups go further. And we must filter out violations after each verification anyway.

Only filtering violations is the lesser evil here.


// Otherwise validate a constraint only once for the first
// matching group
foreach ($groups as $group) {
Expand Down Expand Up @@ -142,6 +145,52 @@ public function validate($form, Constraint $formConstraint)
}
}

/**
* Filter out form field violations to meet the requirements of the sequence of groups.
*
* If there is a violation with a group of current groups of the sequence,
* remove all other violations that don't belong this groups.
*
* This is necessary because each form field is validated independently.
*/
private function filterFormFieldsGroupSequenceViolations(GroupSequence $groupSequence)
{
if (\count($violations = $this->context->getViolations()) < 2) {
return;
}

$violationGroups = [];

foreach ($violations as $offset => $violation) {
$violationGroups[$offset] = array_map(static function ($group) {
return $group;
}, $violation->getConstraint()->groups);
}

$groupsToKeep = [];

foreach ($groupSequence->groups as $seqGroups) {
$seqGroups = !\is_array($seqGroups) ? [$seqGroups] : $seqGroups;

if (array_filter($violationGroups, static function ($groups) use ($seqGroups) {
return array_filter($groups, static function ($group) use ($seqGroups) {
return \in_array($group, $seqGroups, true);
});
})) {
$groupsToKeep = $seqGroups;
break;
}
}

foreach ($violationGroups as $offset => $groups) {
if (!array_filter($groups, static function ($group) use ($groupsToKeep) {
return \in_array($group, $groupsToKeep, true);
})) {
$violations->remove($offset);
}
}
}

/**
* Returns the validation groups of the given form.
*
Expand Down
127 changes: 127 additions & 0 deletions src/Symfony/Component/Form/Tests/Extension/Validator/Type/FormTypeValidatorExtensionTest.php
View file
Open in desktop
Expand Up @@ -19,6 +19,7 @@
use Symfony\Component\Validator\Constraints\Email;
use Symfony\Component\Validator\Constraints\GroupSequence;
use Symfony\Component\Validator\Constraints\Length;
use Symfony\Component\Validator\Constraints\NotBlank;
use Symfony\Component\Validator\Constraints\Valid;
use Symfony\Component\Validator\ConstraintViolationList;
use Symfony\Component\Validator\Validation;
Expand Down Expand Up @@ -74,6 +75,132 @@ public function testGroupSequenceWithConstraintsOption()
$this->assertCount(1, $form->getErrors(true));
}

/**
* @dataProvider provideGroupsSequenceAndResultData
*/
public function testGroupSequenceWithConstraintsOptionMatrix(
array $groups,
array $sequence,
$errorCount,
array $propertyPaths
) {
$form = Forms::createFormFactoryBuilder()
->addExtension(new ValidatorExtension(Validation::createValidator()))
->getFormFactory()
->create(FormTypeTest::TESTED_TYPE, null, ([
'validation_groups' => new GroupSequence($sequence),
]));

$data = [];
foreach ($groups as $fieldName => $fieldGroups) {
$form = $form->add(
$fieldName, TextTypeTest::
TESTED_TYPE,
[
'constraints' => [new NotBlank(['groups' => $fieldGroups])],
]);

$data[$fieldName] = '';
}

$form->submit($data);

$errors = $form->getErrors(true);
$this->assertCount($errorCount, $form->getErrors(true));

foreach ($errors as $i => $error) {
$this->assertEquals('children['.$propertyPaths[$i].'].data', $error->getCause()->getPropertyPath());
}
}

public function provideGroupsSequenceAndResultData()
{
return [
// two fields (sequence of groups and group order):
[
'groups' => [
'field1' => ['First'],
'field2' => ['Second'],
],
'sequence' => ['First'],
'errors' => 1,
'propertyPaths' => ['field1'],
],
[
'groups' => [
'field1' => ['First'],
'field2' => ['Second'],
],
'sequence' => ['Second'],
'errors' => 1,
'propertyPaths' => ['field2'],
],
[
'groups' => [
'field1' => ['First'],
'field2' => ['Second'],
],
'sequence' => ['First', 'Second'],
'errors' => 1,
'propertyPaths' => ['field1'],
],
[
'groups' => [
'field1' => ['First'],
'field2' => ['Second'],
],
'sequence' => ['Second', 'First'],
'errors' => 1,
'propertyPaths' => ['field2'],
],

// two fields (field with sequence of groups)
[
'groups' => [
'field1' => ['First'],
'field2' => ['Second', 'First'],
],
'sequence' => ['First'],
'errors' => 2,
'propertyPaths' => ['field1', 'field2'],
],

// three fields (sequence with multigroup)
[
'groups' => [
'field1' => ['First'],
'field2' => ['Second'],
'field3' => ['Third'],
],
'sequence' => [['First', 'Second'], 'Third'],
'errors' => 2,
'propertyPaths' => ['field1', 'field2'],
],
[
'groups' => [
'field1' => ['First'],
'field2' => ['Second'],
'field3' => ['Third'],
],
'sequence' => ['First', ['Second', 'Third']],
'errors' => 1,
'propertyPaths' => ['field1'],
],

// three fields (field with sequence of groups)
[
'groups' => [
'field1' => ['First'],
'field2' => ['Second'],
'field3' => ['Third', 'Second'],
],
'sequence' => [['First', 'Second'], 'Third'],
'errors' => 3,
'propertyPaths' => ['field1', 'field2', 'field3'],
],
];
}

protected function createForm(array $options = [])
{
return $this->factory->create(FormTypeTest::TESTED_TYPE, null, $options);
Expand Down