From a2ea4b17f45be5b5a3363b873afe9d284f78aaa9 Mon Sep 17 00:00:00 2001 From: Jules Pietri Date: Fri, 3 Apr 2020 17:40:36 +0200 Subject: [PATCH] [Form] Fixed handling groups sequence validation --- .../Validator/Constraints/FormValidator.php | 52 +++++++++++++------ .../Form/Resources/config/validation.xml | 2 +- .../Constraints/FormValidatorTest.php | 38 +++++++++++++- .../Type/FormTypeValidatorExtensionTest.php | 35 +++++++++++-- .../Validator/ValidatorExtensionTest.php | 31 ++++++++++- 5 files changed, 135 insertions(+), 23 deletions(-) diff --git a/src/Symfony/Component/Form/Extension/Validator/Constraints/FormValidator.php b/src/Symfony/Component/Form/Extension/Validator/Constraints/FormValidator.php index 07ecabdad1346..66e0ffdcb365a 100644 --- a/src/Symfony/Component/Form/Extension/Validator/Constraints/FormValidator.php +++ b/src/Symfony/Component/Form/Extension/Validator/Constraints/FormValidator.php @@ -24,6 +24,13 @@ */ class FormValidator extends ConstraintValidator { + private $resolvedGroups; + + public function __construct() + { + $this->resolvedGroups = new \SplObjectStorage(); + } + /** * {@inheritdoc} */ @@ -44,7 +51,7 @@ public function validate($form, Constraint $formConstraint) if ($form->isSubmitted() && $form->isSynchronized()) { // Validate the form data only if transformation succeeded - $groups = self::getValidationGroups($form); + $groups = $this->getValidationGroups($form); if (!$groups) { return; @@ -55,31 +62,42 @@ public function validate($form, Constraint $formConstraint) // Validate the data against its own constraints if ($form->isRoot() && (\is_object($data) || \is_array($data))) { if (($groups && \is_array($groups)) || ($groups instanceof GroupSequence && $groups->groups)) { - $validator->atPath('data')->validate($form->getData(), null, $groups); + $validator->atPath('data')->validate($data, null, $groups); } } - // Validate the data against the constraints defined - // in the form + // Validate the data against the constraints defined in the form + /** @var Constraint[] $constraints */ $constraints = $config->getOption('constraints', []); if ($groups instanceof GroupSequence) { - $validator->atPath('data')->validate($form->getData(), $constraints, $groups); - // Otherwise validate a constraint only once for the first - // matching group - foreach ($groups as $group) { - if (\in_array($group, $formConstraint->groups)) { - $validator->atPath('data')->validate($form->getData(), $formConstraint, $group); - if (\count($this->context->getViolations()) > 0) { - break; + // Validate the form AND nested fields in sequence + $violationsCount = $this->context->getViolations()->count(); + $fieldPropertyPath = \is_object($data) ? 'data.%s' : '%s'; + + foreach ($groups->groups as $group) { + $validator->atPath('data')->validate($data, $constraints, $group); + + foreach ($form->all() as $field) { + if ($field->isSubmitted()) { + // remember to validate this field is one group only + // otherwise resolving the groups would reuse the same + // sequence recursively, thus some fields could fail + // in different steps without breaking early enough + $this->resolvedGroups[$field] = [$group]; + $validator->atPath(sprintf($fieldPropertyPath, $field->getPropertyPath()))->validate($field, $formConstraint); } } + + if ($violationsCount < $this->context->getViolations()->count()) { + break; + } } } else { foreach ($constraints as $constraint) { // For the "Valid" constraint, validate the data in all groups if ($constraint instanceof Valid) { - $validator->atPath('data')->validate($form->getData(), $constraint, $groups); + $validator->atPath('data')->validate($data, $constraint, $groups); continue; } @@ -88,7 +106,7 @@ public function validate($form, Constraint $formConstraint) // matching group foreach ($groups as $group) { if (\in_array($group, $constraint->groups)) { - $validator->atPath('data')->validate($form->getData(), $constraint, $group); + $validator->atPath('data')->validate($data, $constraint, $group); // Prevent duplicate validation if (!$constraint instanceof Composite) { @@ -147,7 +165,7 @@ public function validate($form, Constraint $formConstraint) * * @return string|GroupSequence|(string|GroupSequence)[] The validation groups */ - private static function getValidationGroups(FormInterface $form) + private function getValidationGroups(FormInterface $form) { // Determine the clicked button of the complete form tree $clickedButton = null; @@ -171,6 +189,10 @@ private static function getValidationGroups(FormInterface $form) return self::resolveValidationGroups($groups, $form); } + if (isset($this->resolvedGroups[$form])) { + return $this->resolvedGroups[$form]; + } + $form = $form->getParent(); } while (null !== $form); diff --git a/src/Symfony/Component/Form/Resources/config/validation.xml b/src/Symfony/Component/Form/Resources/config/validation.xml index cbd586b915451..b2b935442d467 100644 --- a/src/Symfony/Component/Form/Resources/config/validation.xml +++ b/src/Symfony/Component/Form/Resources/config/validation.xml @@ -7,7 +7,7 @@ - + diff --git a/src/Symfony/Component/Form/Tests/Extension/Validator/Constraints/FormValidatorTest.php b/src/Symfony/Component/Form/Tests/Extension/Validator/Constraints/FormValidatorTest.php index e19620e790f7c..a20b25a4a6671 100644 --- a/src/Symfony/Component/Form/Tests/Extension/Validator/Constraints/FormValidatorTest.php +++ b/src/Symfony/Component/Form/Tests/Extension/Validator/Constraints/FormValidatorTest.php @@ -402,7 +402,8 @@ public function testHandleGroupSequenceValidationGroups() $form->submit([]); $this->expectValidateAt(0, 'data', $object, new GroupSequence(['group1', 'group2'])); - $this->expectValidateAt(1, 'data', $object, new GroupSequence(['group1', 'group2'])); + $this->expectValidateAt(1, 'data', $object, 'group1'); + $this->expectValidateAt(2, 'data', $object, 'group2'); $this->validator->validate($form, new Form()); @@ -756,6 +757,39 @@ public function testCompositeConstraintValidatedInEachGroup() $this->assertSame('data[field2]', $context->getViolations()[1]->getPropertyPath()); } + public function testCompositeConstraintValidatedInSequence() + { + $form = $this->getCompoundForm([], [ + 'constraints' => [ + new Collection([ + 'field1' => new NotBlank([ + 'groups' => ['field1'], + ]), + 'field2' => new NotBlank([ + 'groups' => ['field2'], + ]), + ]), + ], + 'validation_groups' => new GroupSequence(['field1', 'field2']), + ]) + ->add($this->getForm('field1')) + ->add($this->getForm('field2')) + ; + + $form->submit([ + 'field1' => '', + 'field2' => '', + ]); + + $context = new ExecutionContext(Validation::createValidator(), $form, new IdentityTranslator()); + $this->validator->initialize($context); + $this->validator->validate($form, new Form()); + + $this->assertCount(1, $context->getViolations()); + $this->assertSame('This value should not be blank.', $context->getViolations()[0]->getMessage()); + $this->assertSame('data[field1]', $context->getViolations()[0]->getPropertyPath()); + } + protected function createValidator() { return new FormValidator(); @@ -784,7 +818,7 @@ private function getForm($name = 'name', $dataClass = null, array $options = []) private function getCompoundForm($data, array $options = []) { - return $this->getBuilder('name', \get_class($data), $options) + return $this->getBuilder('name', \is_object($data) ? \get_class($data) : null, $options) ->setData($data) ->setCompound(true) ->setDataMapper(new PropertyPathMapper()) diff --git a/src/Symfony/Component/Form/Tests/Extension/Validator/Type/FormTypeValidatorExtensionTest.php b/src/Symfony/Component/Form/Tests/Extension/Validator/Type/FormTypeValidatorExtensionTest.php index 57f92b6574e3b..b2f0c820d8c6f 100644 --- a/src/Symfony/Component/Form/Tests/Extension/Validator/Type/FormTypeValidatorExtensionTest.php +++ b/src/Symfony/Component/Form/Tests/Extension/Validator/Type/FormTypeValidatorExtensionTest.php @@ -16,9 +16,9 @@ use Symfony\Component\Form\Test\Traits\ValidatorExtensionTrait; use Symfony\Component\Form\Tests\Extension\Core\Type\FormTypeTest; use Symfony\Component\Form\Tests\Extension\Core\Type\TextTypeTest; -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; @@ -64,14 +64,43 @@ public function testGroupSequenceWithConstraintsOption() ->add('field', TextTypeTest::TESTED_TYPE, [ 'constraints' => [ new Length(['min' => 10, 'groups' => ['First']]), - new Email(['groups' => ['Second']]), + new NotBlank(['groups' => ['Second']]), ], ]) ; $form->submit(['field' => 'wrong']); - $this->assertCount(1, $form->getErrors(true)); + $errors = $form->getErrors(true); + + $this->assertCount(1, $errors); + $this->assertInstanceOf(Length::class, $errors[0]->getCause()->getConstraint()); + } + + public function testManyFieldsGroupSequenceWithConstraintsOption() + { + $form = Forms::createFormFactoryBuilder() + ->addExtension(new ValidatorExtension(Validation::createValidator())) + ->getFormFactory() + ->create(FormTypeTest::TESTED_TYPE, null, (['validation_groups' => new GroupSequence(['First', 'Second'])])) + ->add('field1', TextTypeTest::TESTED_TYPE, [ + 'constraints' => [ + new Length(['min' => 10, 'groups' => ['First']]), + ], + ]) + ->add('field2', TextTypeTest::TESTED_TYPE, [ + 'constraints' => [ + new NotBlank(['groups' => ['Second']]), + ], + ]) + ; + + $form->submit(['field1' => 'wrong_1', 'field2' => '']); + + $errors = $form->getErrors(true); + + $this->assertCount(1, $errors); + $this->assertInstanceOf(Length::class, $errors[0]->getCause()->getConstraint()); } protected function createForm(array $options = []) diff --git a/src/Symfony/Component/Form/Tests/Extension/Validator/ValidatorExtensionTest.php b/src/Symfony/Component/Form/Tests/Extension/Validator/ValidatorExtensionTest.php index 136086a5e5ba8..9619ead1a829a 100644 --- a/src/Symfony/Component/Form/Tests/Extension/Validator/ValidatorExtensionTest.php +++ b/src/Symfony/Component/Form/Tests/Extension/Validator/ValidatorExtensionTest.php @@ -13,6 +13,8 @@ use PHPUnit\Framework\TestCase; use Symfony\Component\Form\AbstractType; +use Symfony\Component\Form\Extension\Core\Type\FormType; +use Symfony\Component\Form\Extension\Core\Type\TextType; use Symfony\Component\Form\Extension\Validator\Constraints\Form as FormConstraint; use Symfony\Component\Form\Extension\Validator\ValidatorExtension; use Symfony\Component\Form\Extension\Validator\ValidatorTypeGuesser; @@ -20,6 +22,8 @@ use Symfony\Component\Form\FormBuilderInterface; use Symfony\Component\Form\FormFactoryBuilder; use Symfony\Component\OptionsResolver\OptionsResolver; +use Symfony\Component\Validator\Constraints\GroupSequence; +use Symfony\Component\Validator\Constraints\Length; use Symfony\Component\Validator\Constraints\NotBlank; use Symfony\Component\Validator\Mapping\CascadingStrategy; use Symfony\Component\Validator\Mapping\ClassMetadata; @@ -49,6 +53,8 @@ public function test2Dot5ValidationApi() $this->assertCount(1, $metadata->getConstraints()); $this->assertInstanceOf(FormConstraint::class, $metadata->getConstraints()[0]); + $this->assertSame(CascadingStrategy::NONE, $metadata->cascadingStrategy); + $this->assertSame(TraversalStrategy::IMPLICIT, $metadata->traversalStrategy); $this->assertSame(CascadingStrategy::CASCADE, $metadata->getPropertyMetadata('children')[0]->cascadingStrategy); $this->assertSame(TraversalStrategy::IMPLICIT, $metadata->getPropertyMetadata('children')[0]->traversalStrategy); } @@ -86,7 +92,28 @@ public function testFieldConstraintsInvalidateFormIfFieldIsSubmitted() $this->assertFalse($form->get('baz')->isValid()); } - private function createForm($type) + public function testFieldsValidateInSequence() + { + $form = $this->createForm(FormType::class, null, [ + 'validation_groups' => new GroupSequence(['group1', 'group2']), + ]) + ->add('foo', TextType::class, [ + 'constraints' => [new Length(['min' => 10, 'groups' => ['group1']])], + ]) + ->add('bar', TextType::class, [ + 'constraints' => [new NotBlank(['groups' => ['group2']])], + ]) + ; + + $form->submit(['foo' => 'invalid', 'bar' => null]); + + $errors = $form->getErrors(true); + + $this->assertCount(1, $errors); + $this->assertInstanceOf(Length::class, $errors[0]->getCause()->getConstraint()); + } + + private function createForm($type, $data = null, array $options = []) { $validator = Validation::createValidatorBuilder() ->setMetadataFactory(new LazyLoadingMetadataFactory(new StaticMethodLoader())) @@ -95,7 +122,7 @@ private function createForm($type) $formFactoryBuilder->addExtension(new ValidatorExtension($validator)); $formFactory = $formFactoryBuilder->getFormFactory(); - return $formFactory->create($type); + return $formFactory->create($type, $data, $options); } }