bug #18935 [Form] Consider a violation even if the form is not submit…

…ted (egeloen)

This PR was merged into the 2.7 branch.

Discussion
----------

[Form] Consider a violation even if the form is not submitted

| Q             | A
| ------------- | ---
| Branch?       | 2.7
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | yes (only for the behavior)
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #11493
| License       | MIT
| Doc PR        |

Hey!

I'm currently implementing an API using the form component in order to validate the payload sent (in conjonction with the FOSRestBundle). Unfortunatelly, we dig into an issue about the PATCH request which don't map some of our validation rules to the form. Basically, the violations are lost in the middle of the process.

### Use case

We have an entity with the following fields "type", "image" & "video". The field "type"can be either "default", "image" or "video" and then accordingly we use the appropriate field (none for the "default" type, video for the "video" type and image for the "image" type. Then, in our form, we change the validation groups according to our entity type in order to make the "image" field mandatory if the type is "image" and the same for the video field if the type is "video".

### Current behavior

The current behavior (since 2.5) seems to not propages a violation to a form if this form is not submitted but in our use case, changing the field "type" via a PATCH request triggers some new validation which should be reported to end user (inform that a field (video or image) is missing in the PATCH request).

### Expected behavior

The current behavior was introduced in #10567 but IMO, this update is a bug as suggested by @webmozart in #11493 (comment) Instead, the form component should still map validation errors to the form even if the field was not submitted. If the initial data is not valid, then your initial data was buggy from the beginning but the form should not accept it and instead of silently ignoring the errors, end users should be informed and fix it.

WDYT?

Commits
-------

c483a0f [Form] Consider a violation even if the form is not submitted

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

@@ -275,9 +275,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/ViolationMapper/ViolationMapperTest.php

@@ -212,7 +212,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');
@@ -230,12 +230,12 @@ public function testAbortMappingIfNotSubmitted()
 
         $this->mapper->mapViolation($violation, $parent);
 
-        $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(0, $parent->getErrors(), $parent->getName().' should not have an error');
+        $this->assertCount(0, $child->getErrors(), $child->getName().' should not have an error');
+        $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');
@@ -255,9 +255,9 @@ public function testAbortDotRuleMappingIfNotSubmitted()
 
         $this->mapper->mapViolation($violation, $parent);
 
-        $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(0, $parent->getErrors(), $parent->getName().' should not have an error');
+        $this->assertCount(0, $child->getErrors(), $child->getName().' should not have an error');
+        $this->assertCount(1, $grandChild->getErrors(), $grandChild->getName().' should have an error');
     }
 
     public function provideDefaultTests()