bug #30265 [Form] do not validate non-submitted form fields in PATCH …

…requests (xabbuh)

This PR was merged into the 3.4 branch.

Discussion
----------

[Form] do not validate non-submitted form fields in PATCH requests

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #11493, #19788, #20805, #24453, #30011
| License       | MIT
| Doc PR        |

When a form field is not embedded as part of a HTTP PATCH requests, its
validation constraints configured through the `constraints` option must
not be evaluated. The fix from #10567 achieved this by not mapping their
violations to the underlying form field. This however also means that
constraint violations caused by validating the whole underlying data
object will never cause the form to be invalid. This breaks use cases
where some constraints may, for example, depend on the value of other
properties that were changed by the submitted data.

Commits
-------

a60d802 do not validate non-submitted form fields in PATCH requests

src/Symfony/Component/Form/Extension/Validator/Constraints/FormValidator.php

@@ -41,7 +41,7 @@ public function validate($form, Constraint $formConstraint)
 
         $validator = $this->context->getValidator()->inContext($this->context);
 
-        if ($form->isSynchronized()) {
+        if ($form->isSubmitted() && $form->isSynchronized()) {
             // Validate the form data only if transformation succeeded
             $groups = self::getValidationGroups($form);
             $data = $form->getData();
@@ -90,7 +90,7 @@ public function validate($form, Constraint $formConstraint)
                     }
                 }
             }
-        } else {
+        } elseif (!$form->isSynchronized()) {
             $childrenSynchronized = true;
 
             /** @var FormInterface $child */

src/Symfony/Component/Form/Extension/Validator/ViolationMapper/ViolationMapper.php

@@ -273,9 +273,6 @@ private function reconstructPath(ViolationPath $violationPath, FormInterface $or
      */
     private function acceptsErrors(FormInterface $form)
     {
-        // Ignore non-submitted forms. This happens, for example, in PATCH
-        // requests.
-        // https://github.com/symfony/symfony/pull/10567
-        return $form->isSubmitted() && ($this->allowNonSynchronized || $form->isSynchronized());
+        return $this->allowNonSynchronized || $form->isSynchronized();
     }
 }

src/Symfony/Component/Form/Tests/Extension/Validator/Constraints/FormValidatorTest.php

@@ -61,9 +61,8 @@ public function testValidate()
     {
         $object = new \stdClass();
         $options = ['validation_groups' => ['group1', 'group2']];
-        $form = $this->getBuilder('name', '\stdClass', $options)
-            ->setData($object)
-            ->getForm();
+        $form = $this->getCompoundForm($object, $options);
+        $form->submit([]);
 
         $this->expectValidateAt(0, 'data', $object, ['group1', 'group2']);
 
@@ -82,9 +81,8 @@ public function testValidateConstraints()
             'validation_groups' => ['group1', 'group2'],
             'constraints' => [$constraint1, $constraint2],
         ];
-        $form = $this->getBuilder('name', '\stdClass', $options)
-            ->setData($object)
-            ->getForm();
+        $form = $this->getCompoundForm($object, $options);
+        $form->submit([]);
 
         // First default constraints
         $this->expectValidateAt(0, 'data', $object, ['group1', 'group2']);
@@ -110,10 +108,9 @@ public function testValidateChildIfValidConstraint()
             'validation_groups' => ['group1', 'group2'],
             'constraints' => [new Valid()],
         ];
-        $form = $this->getBuilder('name', '\stdClass', $options)->getForm();
+        $form = $this->getCompoundForm($object, $options);
         $parent->add($form);
-
-        $form->setData($object);
+        $parent->submit([]);
 
         $this->expectValidateAt(0, 'data', $object, ['group1', 'group2']);
 
@@ -146,8 +143,8 @@ public function testDontValidateIfParentWithoutValidConstraint()
     public function testMissingConstraintIndex()
     {
         $object = new \stdClass();
-        $form = new FormBuilder('name', '\stdClass', $this->dispatcher, $this->factory);
-        $form = $form->setData($object)->getForm();
+        $form = $this->getCompoundForm($object);
+        $form->submit([]);
 
         $this->expectValidateAt(0, 'data', $object, ['Default']);
 
@@ -170,10 +167,9 @@ public function testValidateConstraintsOptionEvenIfNoValidConstraint()
             'validation_groups' => ['group1', 'group2'],
             'constraints' => [$constraint1, $constraint2],
         ];
-        $form = $this->getBuilder('name', '\stdClass', $options)
-            ->setData($object)
-            ->getForm();
+        $form = $this->getCompoundForm($object, $options);
         $parent->add($form);
+        $parent->submit([]);
 
         $this->expectValidateValueAt(0, 'data', $object, $constraint1, 'group1');
         $this->expectValidateValueAt(1, 'data', $object, $constraint2, 'group2');
@@ -361,9 +357,8 @@ public function testHandleGroupSequenceValidationGroups()
     {
         $object = new \stdClass();
         $options = ['validation_groups' => new GroupSequence(['group1', 'group2'])];
-        $form = $this->getBuilder('name', '\stdClass', $options)
-            ->setData($object)
-            ->getForm();
+        $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']));
@@ -377,9 +372,8 @@ public function testHandleCallbackValidationGroups()
     {
         $object = new \stdClass();
         $options = ['validation_groups' => [$this, 'getValidationGroups']];
-        $form = $this->getBuilder('name', '\stdClass', $options)
-            ->setData($object)
-            ->getForm();
+        $form = $this->getCompoundForm($object, $options);
+        $form->submit([]);
 
         $this->expectValidateAt(0, 'data', $object, ['group1', 'group2']);
 
@@ -392,9 +386,8 @@ public function testDontExecuteFunctionNames()
     {
         $object = new \stdClass();
         $options = ['validation_groups' => 'header'];
-        $form = $this->getBuilder('name', '\stdClass', $options)
-            ->setData($object)
-            ->getForm();
+        $form = $this->getCompoundForm($object, $options);
+        $form->submit([]);
 
         $this->expectValidateAt(0, 'data', $object, ['header']);
 
@@ -409,9 +402,8 @@ public function testHandleClosureValidationGroups()
         $options = ['validation_groups' => function (FormInterface $form) {
             return ['group1', 'group2'];
         }];
-        $form = $this->getBuilder('name', '\stdClass', $options)
-            ->setData($object)
-            ->getForm();
+        $form = $this->getCompoundForm($object, $options);
+        $form->submit([]);
 
         $this->expectValidateAt(0, 'data', $object, ['group1', 'group2']);
 
@@ -455,7 +447,7 @@ public function testDontUseValidationGroupOfUnclickedButton()
             ->setCompound(true)
             ->setDataMapper(new PropertyPathMapper())
             ->getForm();
-        $form = $this->getForm('name', '\stdClass', [
+        $form = $this->getCompoundForm($object, [
             'validation_groups' => 'form_group',
             'constraints' => [new Valid()],
         ]);
@@ -465,7 +457,7 @@ public function testDontUseValidationGroupOfUnclickedButton()
             'validation_groups' => 'button_group',
         ]));
 
-        $form->setData($object);
+        $parent->submit([]);
 
         $this->expectValidateAt(0, 'data', $object, ['form_group']);
 
@@ -484,10 +476,9 @@ public function testUseInheritedValidationGroup()
             ->setDataMapper(new PropertyPathMapper())
             ->getForm();
         $formOptions = ['constraints' => [new Valid()]];
-        $form = $this->getBuilder('name', '\stdClass', $formOptions)->getForm();
+        $form = $this->getCompoundForm($object, $formOptions);
         $parent->add($form);
-
-        $form->setData($object);
+        $parent->submit([]);
 
         $this->expectValidateAt(0, 'data', $object, ['group']);
 
@@ -506,10 +497,9 @@ public function testUseInheritedCallbackValidationGroup()
             ->setDataMapper(new PropertyPathMapper())
             ->getForm();
         $formOptions = ['constraints' => [new Valid()]];
-        $form = $this->getBuilder('name', '\stdClass', $formOptions)->getForm();
+        $form = $this->getCompoundForm($object, $formOptions);
         $parent->add($form);
-
-        $form->setData($object);
+        $parent->submit([]);
 
         $this->expectValidateAt(0, 'data', $object, ['group1', 'group2']);
 
@@ -523,7 +513,7 @@ public function testUseInheritedClosureValidationGroup()
         $object = new \stdClass();
 
         $parentOptions = [
-            'validation_groups' => function (FormInterface $form) {
+            'validation_groups' => function () {
                 return ['group1', 'group2'];
             },
         ];
@@ -532,10 +522,9 @@ public function testUseInheritedClosureValidationGroup()
             ->setDataMapper(new PropertyPathMapper())
             ->getForm();
         $formOptions = ['constraints' => [new Valid()]];
-        $form = $this->getBuilder('name', '\stdClass', $formOptions)->getForm();
+        $form = $this->getCompoundForm($object, $formOptions);
         $parent->add($form);
-
-        $form->setData($object);
+        $parent->submit([]);
 
         $this->expectValidateAt(0, 'data', $object, ['group1', 'group2']);
 
@@ -547,9 +536,8 @@ public function testUseInheritedClosureValidationGroup()
     public function testAppendPropertyPath()
     {
         $object = new \stdClass();
-        $form = $this->getBuilder('name', '\stdClass')
-            ->setData($object)
-            ->getForm();
+        $form = $this->getCompoundForm($object);
+        $form->submit([]);
 
         $this->expectValidateAt(0, 'data', $object, ['Default']);
 
@@ -690,6 +678,15 @@ private function getForm($name = 'name', $dataClass = null, array $options = [])
         return $this->getBuilder($name, $dataClass, $options)->getForm();
     }
 
+    private function getCompoundForm($data, array $options = [])
+    {
+        return $this->getBuilder('name', \get_class($data), $options)
+            ->setData($data)
+            ->setCompound(true)
+            ->setDataMapper(new PropertyPathMapper())
+            ->getForm();
+    }
+
     private function getSubmitButton($name = 'name', array $options = [])
     {
         $builder = new SubmitButtonBuilder($name, $options);

src/Symfony/Component/Form/Tests/Extension/Validator/ValidatorExtensionTest.php

@@ -12,12 +12,19 @@
 namespace Symfony\Component\Form\Tests\Extension\Validator;
 
 use PHPUnit\Framework\TestCase;
+use Symfony\Component\Form\AbstractType;
 use Symfony\Component\Form\Extension\Validator\Constraints\Form as FormConstraint;
 use Symfony\Component\Form\Extension\Validator\ValidatorExtension;
 use Symfony\Component\Form\Extension\Validator\ValidatorTypeGuesser;
 use Symfony\Component\Form\Form;
+use Symfony\Component\Form\FormBuilderInterface;
+use Symfony\Component\Form\FormFactoryBuilder;
+use Symfony\Component\OptionsResolver\OptionsResolver;
+use Symfony\Component\Validator\Constraints\NotBlank;
 use Symfony\Component\Validator\Mapping\CascadingStrategy;
 use Symfony\Component\Validator\Mapping\ClassMetadata;
+use Symfony\Component\Validator\Mapping\Factory\LazyLoadingMetadataFactory;
+use Symfony\Component\Validator\Mapping\Loader\StaticMethodLoader;
 use Symfony\Component\Validator\Mapping\TraversalStrategy;
 use Symfony\Component\Validator\Tests\Fixtures\FakeMetadataFactory;
 use Symfony\Component\Validator\Validation;
@@ -45,4 +52,78 @@ public function test2Dot5ValidationApi()
         $this->assertSame(CascadingStrategy::CASCADE, $metadata->getPropertyMetadata('children')[0]->cascadingStrategy);
         $this->assertSame(TraversalStrategy::IMPLICIT, $metadata->getPropertyMetadata('children')[0]->traversalStrategy);
     }
+
+    public function testDataConstraintsInvalidateFormEvenIfFieldIsNotSubmitted()
+    {
+        $form = $this->createForm(FooType::class);
+        $form->submit(['baz' => 'foobar'], false);
+
+        $this->assertTrue($form->isSubmitted());
+        $this->assertFalse($form->isValid());
+        $this->assertFalse($form->get('bar')->isSubmitted());
+        $this->assertCount(1, $form->get('bar')->getErrors());
+    }
+
+    public function testFieldConstraintsDoNotInvalidateFormIfFieldIsNotSubmitted()
+    {
+        $form = $this->createForm(FooType::class);
+        $form->submit(['bar' => 'foobar'], false);
+
+        $this->assertTrue($form->isSubmitted());
+        $this->assertTrue($form->isValid());
+    }
+
+    public function testFieldConstraintsInvalidateFormIfFieldIsSubmitted()
+    {
+        $form = $this->createForm(FooType::class);
+        $form->submit(['bar' => 'foobar', 'baz' => ''], false);
+
+        $this->assertTrue($form->isSubmitted());
+        $this->assertFalse($form->isValid());
+        $this->assertTrue($form->get('bar')->isSubmitted());
+        $this->assertTrue($form->get('bar')->isValid());
+        $this->assertTrue($form->get('baz')->isSubmitted());
+        $this->assertFalse($form->get('baz')->isValid());
+    }
+
+    private function createForm($type)
+    {
+        $validator = Validation::createValidatorBuilder()
+            ->setMetadataFactory(new LazyLoadingMetadataFactory(new StaticMethodLoader()))
+            ->getValidator();
+        $formFactoryBuilder = new FormFactoryBuilder();
+        $formFactoryBuilder->addExtension(new ValidatorExtension($validator));
+        $formFactory = $formFactoryBuilder->getFormFactory();
+
+        return $formFactory->create($type);
+    }
+}
+
+class Foo
+{
+    public $bar;
+    public $baz;
+
+    public static function loadValidatorMetadata(ClassMetadata $metadata)
+    {
+        $metadata->addPropertyConstraint('bar', new NotBlank());
+    }
+}
+
+class FooType extends AbstractType
+{
+    public function buildForm(FormBuilderInterface $builder, array $options)
+    {
+        $builder
+            ->add('bar')
+            ->add('baz', null, [
+                'constraints' => [new NotBlank()],
+            ])
+        ;
+    }
+
+    public function configureOptions(OptionsResolver $resolver)
+    {
+        $resolver->setDefault('data_class', Foo::class);
+    }
 }

src/Symfony/Component/Form/Tests/Extension/Validator/ViolationMapper/ViolationMapperTest.php

@@ -205,7 +205,7 @@ public function testAbortDotRuleMappingIfNotSynchronized()
         $this->assertCount(0, $grandChild->getErrors(), $grandChild->getName().' should not have an error, but has one');
     }
 
-    public function testAbortMappingIfNotSubmitted()
+    public function testMappingIfNotSubmitted()
     {
         $violation = $this->getConstraintViolation('children[address].data.street');
         $parent = $this->getForm('parent');
@@ -225,10 +225,10 @@ public function testAbortMappingIfNotSubmitted()
 
         $this->assertCount(0, $parent->getErrors(), $parent->getName().' should not have an error, but has one');
         $this->assertCount(0, $child->getErrors(), $child->getName().' should not have an error, but has one');
-        $this->assertCount(0, $grandChild->getErrors(), $grandChild->getName().' should not have an error, but has one');
+        $this->assertCount(1, $grandChild->getErrors(), $grandChild->getName().' should have one error');
     }
 
-    public function testAbortDotRuleMappingIfNotSubmitted()
+    public function testDotRuleMappingIfNotSubmitted()
     {
         $violation = $this->getConstraintViolation('data.address');
         $parent = $this->getForm('parent');
@@ -250,7 +250,7 @@ public function testAbortDotRuleMappingIfNotSubmitted()
 
         $this->assertCount(0, $parent->getErrors(), $parent->getName().' should not have an error, but has one');
         $this->assertCount(0, $child->getErrors(), $child->getName().' should not have an error, but has one');
-        $this->assertCount(0, $grandChild->getErrors(), $grandChild->getName().' should not have an error, but has one');
+        $this->assertCount(1, $grandChild->getErrors(), $grandChild->getName().' should have one error');
     }
 
     public function provideDefaultTests()