From dfb61c204c3d2149e36bcce7a9aefd020ff2f0c2 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 | 79 ++++++++++++++----- .../Form/Resources/config/validation.xml | 2 +- .../Constraints/FormValidatorTest.php | 39 ++++++++- .../Type/FormTypeValidatorExtensionTest.php | 65 ++++++++++++++- .../Validator/ValidatorExtensionTest.php | 56 ++++++++++++- 5 files changed, 212 insertions(+), 29 deletions(-) diff --git a/src/Symfony/Component/Form/Extension/Validator/Constraints/FormValidator.php b/src/Symfony/Component/Form/Extension/Validator/Constraints/FormValidator.php index 07ecabdad134..14158f4c1cf7 100644 --- a/src/Symfony/Component/Form/Extension/Validator/Constraints/FormValidator.php +++ b/src/Symfony/Component/Form/Extension/Validator/Constraints/FormValidator.php @@ -24,6 +24,8 @@ */ class FormValidator extends ConstraintValidator { + private $resolvedGroups; + /** * {@inheritdoc} */ @@ -44,42 +46,68 @@ 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; } $data = $form->getData(); - // 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); - } - } + $validateDataGraph = $form->isRoot() + && (\is_object($data) || \is_array($data)) + && (($groups && \is_array($groups)) || ($groups instanceof GroupSequence && $groups->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 data, the form AND nested fields in sequence + $violationsCount = $this->context->getViolations()->count(); + $fieldPropertyPath = \is_object($data) ? 'children[%s]' : 'children%s'; + $hasChildren = $form->count() > 0; + $this->resolvedGroups = $hasChildren ? new \SplObjectStorage() : null; + + foreach ($groups->groups as $group) { + if ($validateDataGraph) { + $validator->atPath('data')->validate($data, null, $group); + } + + if ($groupedConstraints = self::getConstraintsInGroups($constraints, $group)) { + $validator->atPath('data')->validate($data, $groupedConstraints, $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] = (array) $group; + $validator->atPath(sprintf($fieldPropertyPath, $field->getPropertyPath()))->validate($field, $formConstraint); } } + + if ($violationsCount < $this->context->getViolations()->count()) { + break; + } + } + + if ($hasChildren) { + // destroy storage at the end of the sequence to avoid memory leaks + $this->resolvedGroups = null; } } else { + if ($validateDataGraph) { + $validator->atPath('data')->validate($data, null, $groups); + } + 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 +116,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 +175,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 +199,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); @@ -197,4 +229,11 @@ private static function resolveValidationGroups($groups, FormInterface $form) return (array) $groups; } + + private static function getConstraintsInGroups($constraints, $group) + { + return array_filter($constraints, static function (Constraint $constraint) use ($group) { + return \in_array($group, $constraint->groups, true); + }); + } } diff --git a/src/Symfony/Component/Form/Resources/config/validation.xml b/src/Symfony/Component/Form/Resources/config/validation.xml index cbd586b91545..b2b935442d46 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 e19620e790f7..5181e4122516 100644 --- a/src/Symfony/Component/Form/Tests/Extension/Validator/Constraints/FormValidatorTest.php +++ b/src/Symfony/Component/Form/Tests/Extension/Validator/Constraints/FormValidatorTest.php @@ -401,8 +401,8 @@ public function testHandleGroupSequenceValidationGroups() $form = $this->getCompoundForm($object, $options); $form->submit([]); - $this->expectValidateAt(0, 'data', $object, new GroupSequence(['group1', 'group2'])); - $this->expectValidateAt(1, 'data', $object, new GroupSequence(['group1', 'group2'])); + $this->expectValidateAt(0, 'data', $object, 'group1'); + $this->expectValidateAt(1, 'data', $object, 'group2'); $this->validator->validate($form, new Form()); @@ -756,6 +756,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 +817,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 57f92b6574e3..4c90cc6316db 100644 --- a/src/Symfony/Component/Form/Tests/Extension/Validator/Type/FormTypeValidatorExtensionTest.php +++ b/src/Symfony/Component/Form/Tests/Extension/Validator/Type/FormTypeValidatorExtensionTest.php @@ -12,15 +12,19 @@ namespace Symfony\Component\Form\Tests\Extension\Validator\Type; use Symfony\Component\Form\Extension\Validator\ValidatorExtension; +use Symfony\Component\Form\Form; use Symfony\Component\Form\Forms; 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\Form\Tests\Fixtures\Author; 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\Mapping\ClassMetadata; +use Symfony\Component\Validator\Mapping\Factory\MetadataFactoryInterface; use Symfony\Component\Validator\Validation; class FormTypeValidatorExtensionTest extends BaseValidatorExtensionTest @@ -64,14 +68,69 @@ 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() + { + $formMetadata = new ClassMetadata(Form::class); + $authorMetadata = (new ClassMetadata(Author::class)) + ->addPropertyConstraint('firstName', new NotBlank(['groups' => 'Second'])) + ; + $metadataFactory = $this->createMock(MetadataFactoryInterface::class); + $metadataFactory->expects($this->any()) + ->method('getMetadataFor') + ->willReturnCallback(static function ($classOrObject) use ($formMetadata, $authorMetadata) { + if (Author::class === $classOrObject || $classOrObject instanceof Author) { + return $authorMetadata; + } + + if (Form::class === $classOrObject || $classOrObject instanceof Form) { + return $formMetadata; + } + + return new ClassMetadata(\is_string($classOrObject) ? $classOrObject : \get_class($classOrObject)); + }) + ; + + $validator = Validation::createValidatorBuilder() + ->setMetadataFactory($metadataFactory) + ->getValidator() + ; + $form = Forms::createFormFactoryBuilder() + ->addExtension(new ValidatorExtension($validator)) + ->getFormFactory() + ->create(FormTypeTest::TESTED_TYPE, new Author(), (['validation_groups' => new GroupSequence(['First', 'Second'])])) + ->add('firstName', TextTypeTest::TESTED_TYPE) + ->add('lastName', TextTypeTest::TESTED_TYPE, [ + 'constraints' => [ + new Length(['min' => 10, 'groups' => ['First']]), + ], + ]) + ->add('australian', TextTypeTest::TESTED_TYPE, [ + 'constraints' => [ + new NotBlank(['groups' => ['Second']]), + ], + ]) + ; + + $form->submit(['firstName' => '', 'lastName' => 'wrong_1', 'australian' => '']); + + $errors = $form->getErrors(true); + + $this->assertCount(1, $errors); + $this->assertInstanceOf(Length::class, $errors[0]->getCause()->getConstraint()); + $this->assertSame('children[lastName].data', $errors[0]->getCause()->getPropertyPath()); } 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 136086a5e5ba..cb9b93abdbf6 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,53 @@ 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()); + } + + public function testFieldsValidateInSequenceWithNestedGroupsArray() + { + $form = $this->createForm(FormType::class, null, [ + 'validation_groups' => new GroupSequence([['group1', 'group2'], 'group3']), + ]) + ->add('foo', TextType::class, [ + 'constraints' => [new Length(['min' => 10, 'groups' => ['group1']])], + ]) + ->add('bar', TextType::class, [ + 'constraints' => [new Length(['min' => 10, 'groups' => ['group2']])], + ]) + ->add('baz', TextType::class, [ + 'constraints' => [new NotBlank(['groups' => ['group3']])], + ]) + ; + + $form->submit(['foo' => 'invalid', 'bar' => 'invalid', 'baz' => null]); + + $errors = $form->getErrors(true); + + $this->assertCount(2, $errors); + $this->assertInstanceOf(Length::class, $errors[0]->getCause()->getConstraint()); + $this->assertInstanceOf(Length::class, $errors[1]->getCause()->getConstraint()); + } + + private function createForm($type, $data = null, array $options = []) { $validator = Validation::createValidatorBuilder() ->setMetadataFactory(new LazyLoadingMetadataFactory(new StaticMethodLoader())) @@ -95,7 +147,7 @@ private function createForm($type) $formFactoryBuilder->addExtension(new ValidatorExtension($validator)); $formFactory = $formFactoryBuilder->getFormFactory(); - return $formFactory->create($type); + return $formFactory->create($type, $data, $options); } }