[Form] Fixed: Virtual forms are ignored when prepopulating a form

src/Symfony/Component/Form/CHANGELOG.md

@@ -85,3 +85,6 @@ CHANGELOG
    * `getClientTransformers`
  * deprecated the option "validation_constraint" in favor of the new
    option "constraints"
+ * removed superfluous methods from DataMapperInterface
+   * `mapFormToData`
+   * `mapDataToForm`

src/Symfony/Component/Form/DataMapperInterface.php

@@ -21,9 +21,5 @@ interface DataMapperInterface
      */
     function mapDataToForms($data, array $forms);
 
-    function mapDataToForm($data, FormInterface $form);
-
     function mapFormsToData(array $forms, &$data);
-
-    function mapFormToData(FormInterface $form, &$data);
 }

src/Symfony/Component/Form/Extension/Core/DataMapper/PropertyPathMapper.php

@@ -32,28 +32,19 @@ class PropertyPathMapper implements DataMapperInterface
             $iterator = new \RecursiveIteratorIterator($iterator);
 
             foreach ($iterator as $form) {
-                $this->mapDataToForm($data, $form);
+                /* @var FormInterface $form */
-            }
+                $propertyPath = $form->getPropertyPath();
-        }
+                $config = $form->getConfig();
-    }
 
-    /**
+                if (null !== $propertyPath && $config->getMapped()) {
-     * {@inheritdoc}
+                    $propertyData = $propertyPath->getValue($data);
-     */
-    public function mapDataToForm($data, FormInterface $form)
-    {
-        if (!empty($data)) {
-            $propertyPath = $form->getPropertyPath();
-            $config = $form->getConfig();
 
-            if (null !== $propertyPath && $config->getMapped()) {
+                    if (is_object($propertyData) && !$form->getAttribute('by_reference')) {
-                $propertyData = $propertyPath->getValue($data);
+                        $propertyData = clone $propertyData;
+                    }
 
-                if (is_object($propertyData) && !$form->getAttribute('by_reference')) {
+                    $form->setData($propertyData);
-                    $propertyData = clone $propertyData;
                 }
-
-                $form->setData($propertyData);
             }
         }
     }
@@ -67,28 +58,21 @@ class PropertyPathMapper implements DataMapperInterface
         $iterator = new \RecursiveIteratorIterator($iterator);
 
         foreach ($iterator as $form) {
-            $this->mapFormToData($form, $data);
+            /* @var FormInterface $form */
-        }
+            $propertyPath = $form->getPropertyPath();
-    }
+            $config = $form->getConfig();
 
-    /**
+            // Write-back is disabled if the form is not synchronized (transformation failed)
-     * {@inheritdoc}
+            // and if the form is disabled (modification not allowed)
-     */
+            if (null !== $propertyPath && $config->getMapped() && $form->isSynchronized() && !$form->isDisabled()) {
-    public function mapFormToData(FormInterface $form, &$data)
+                // If the data is identical to the value in $data, we are
-    {
+                // dealing with a reference
-        $propertyPath = $form->getPropertyPath();
+                $isReference = $form->getData() === $propertyPath->getValue($data);
-        $config = $form->getConfig();
+                $byReference = $form->getAttribute('by_reference');
 
-        // Write-back is disabled if the form is not synchronized (transformation failed)
+                if (!(is_object($data) && $isReference && $byReference)) {
-        // and if the form is disabled (modification not allowed)
+                    $propertyPath->setValue($data, $form->getData());
-        if (null !== $propertyPath && $config->getMapped() && $form->isSynchronized() && !$form->isDisabled()) {
+                }
-            // If the data is identical to the value in $data, we are
-            // dealing with a reference
-            $isReference = $form->getData() === $propertyPath->getValue($data);
-            $byReference = $form->getAttribute('by_reference');
-
-            if (!(is_object($data) && $isReference && $byReference)) {
-                $propertyPath->setValue($data, $form->getData());
             }
         }
     }

src/Symfony/Component/Form/Form.php

@@ -805,7 +805,7 @@ class Form implements \IteratorAggregate, FormInterface
         $child->setParent($this);
 
         if ($this->config->getDataMapper()) {
-            $this->config->getDataMapper()->mapDataToForm($this->getClientData(), $child);
+            $this->config->getDataMapper()->mapDataToForms($this->getClientData(), array($child));
         }
 
         return $this;

src/Symfony/Component/Form/FormConfig.php

@@ -41,12 +41,12 @@ class FormConfig implements FormConfigInterface
     /**
      * @var Boolean
      */
-    private $mapped;
+    private $mapped = true;
 
     /**
      * @var Boolean
      */
-    private $virtual;
+    private $virtual = false;
 
     /**
      * @var array
@@ -76,17 +76,17 @@ class FormConfig implements FormConfigInterface
     /**
      * @var Boolean
      */
-    private $required;
+    private $required = true;
 
     /**
      * @var Boolean
      */
-    private $disabled;
+    private $disabled = false;
 
     /**
      * @var Boolean
      */
-    private $errorBubbling;
+    private $errorBubbling = false;
 
     /**
      * @var mixed

src/Symfony/Component/Form/Tests/Extension/Core/DataMapper/PropertyPathMapperTest.php

@@ -12,6 +12,9 @@
 namespace Symfony\Component\Form\Tests\Extension\Core\DataMapper;
 
 use Symfony\Component\Form\Tests\FormInterface;
+use Symfony\Component\Form\Form;
+use Symfony\Component\Form\FormConfig;
+use Symfony\Component\Form\FormConfigInterface;
 use Symfony\Component\Form\Util\PropertyPath;
 use Symfony\Component\Form\Extension\Core\DataMapper\PropertyPathMapper;
 
@@ -44,18 +47,30 @@ abstract class PropertyPathMapperTest_Form implements FormInterface
 
 class PropertyPathMapperTest extends \PHPUnit_Framework_TestCase
 {
+    /**
+     * @var PropertyPathMapper
+     */
     private $mapper;
 
+    /**
+     * @var \PHPUnit_Framework_MockObject_MockObject
+     */
+    private $dispatcher;
+
     protected function setUp()
     {
+        if (!class_exists('Symfony\Component\EventDispatcher\Event')) {
+            $this->markTestSkipped('The "EventDispatcher" component is not available');
+        }
+
+        $this->dispatcher = $this->getMock('Symfony\Component\EventDispatcher\EventDispatcherInterface');
         $this->mapper = new PropertyPathMapper();
     }
 
-    protected function tearDown()
+    /**
-    {
+     * @param $path
-        $this->mapper = null;
+     * @return \PHPUnit_Framework_MockObject_MockObject
-    }
+     */
-
     private function getPropertyPath($path)
     {
         return $this->getMockBuilder('Symfony\Component\Form\Util\PropertyPath')
@@ -64,44 +79,26 @@ class PropertyPathMapperTest extends \PHPUnit_Framework_TestCase
             ->getMock();
     }
 
-    private function getForm(PropertyPath $propertyPath = null, $byReference, $synchronized = true, $mapped = true, $disabled = false)
+    /**
+     * @param FormConfigInterface $config
+     * @param Boolean $synchronized
+     * @return \PHPUnit_Framework_MockObject_MockObject
+     */
+    private function getForm(FormConfigInterface $config, $synchronized = true)
     {
-        $config = $this->getMock('Symfony\Component\Form\FormConfigInterface');
+        $form = $this->getMockBuilder('Symfony\Component\Form\Form')
-
+            ->setConstructorArgs(array($config))
-        $config->expects($this->any())
+            ->setMethods(array('isSynchronized'))
-            ->method('getMapped')
+            ->getMock();
-            ->will($this->returnValue($mapped));
-
-        $form = $this->getMockBuilder(__CLASS__ . '_Form')
-            // PHPUnit's getMockForAbstractClass does not behave like in the docs..
-            // If the array is empty, all methods are mocked. If it is not
-            // empty, only abstract methods and the methods in the array are
-            // mocked.
-            ->setMethods(array('foo'))
-            ->getMockForAbstractClass();
-
-        $form->setAttribute('by_reference', $byReference);
-
-        $form->expects($this->any())
-            ->method('getConfig')
-            ->will($this->returnValue($config));
-
-        $form->expects($this->any())
-            ->method('getPropertyPath')
-            ->will($this->returnValue($propertyPath));
 
         $form->expects($this->any())
             ->method('isSynchronized')
             ->will($this->returnValue($synchronized));
 
-        $form->expects($this->any())
-            ->method('isDisabled')
-            ->will($this->returnValue($disabled));
-
         return $form;
     }
 
-    public function testMapDataToFormPassesObjectRefIfByReference()
+    public function testMapDataToFormsPassesObjectRefIfByReference()
     {
         $car = new \stdClass();
         $engine = new \stdClass();
@@ -112,16 +109,19 @@ class PropertyPathMapperTest extends \PHPUnit_Framework_TestCase
             ->with($car)
             ->will($this->returnValue($engine));
 
-        $form = $this->getForm($propertyPath, true);
+        $config = new FormConfig('name', '\stdClass', $this->dispatcher);
+        $config->setAttribute('by_reference', true);
+        $config->setPropertyPath($propertyPath);
+        $form = $this->getForm($config);
 
-        $this->mapper->mapDataToForm($car, $form);
+        $this->mapper->mapDataToForms($car, array($form));
 
         // Can't use isIdentical() above because mocks always clone their
         // arguments which can't be disabled in PHPUnit 3.6
         $this->assertSame($engine, $form->getData());
     }
 
-    public function testMapDataToFormPassesObjectCloneIfNotByReference()
+    public function testMapDataToFormsPassesObjectCloneIfNotByReference()
     {
         $car = new \stdClass();
         $engine = new \stdClass();
@@ -132,26 +132,33 @@ class PropertyPathMapperTest extends \PHPUnit_Framework_TestCase
             ->with($car)
             ->will($this->returnValue($engine));
 
-        $form = $this->getForm($propertyPath, false);
+        $config = new FormConfig('name', '\stdClass', $this->dispatcher);
+        $config->setAttribute('by_reference', false);
+        $config->setPropertyPath($propertyPath);
+        $form = $this->getForm($config);
 
-        $this->mapper->mapDataToForm($car, $form);
+        $this->mapper->mapDataToForms($car, array($form));
 
         $this->assertNotSame($engine, $form->getData());
         $this->assertEquals($engine, $form->getData());
     }
 
-    public function testMapDataToFormIgnoresEmptyPropertyPath()
+    public function testMapDataToFormsIgnoresEmptyPropertyPath()
     {
         $car = new \stdClass();
 
-        $form = $this->getForm(null, true);
+        $config = new FormConfig(null, '\stdClass', $this->dispatcher);
+        $config->setAttribute('by_reference', true);
+        $form = $this->getForm($config);
 
-        $this->mapper->mapDataToForm($car, $form);
+        $this->assertNull($form->getPropertyPath());
+
+        $this->mapper->mapDataToForms($car, array($form));
 
         $this->assertNull($form->getData());
     }
 
-    public function testMapDataToFormIgnoresUnmapped()
+    public function testMapDataToFormsIgnoresUnmapped()
     {
         $car = new \stdClass();
         $propertyPath = $this->getPropertyPath('engine');
@@ -159,24 +166,64 @@ class PropertyPathMapperTest extends \PHPUnit_Framework_TestCase
         $propertyPath->expects($this->never())
             ->method('getValue');
 
-        $form = $this->getForm($propertyPath, true, true, false);
+        $config = new FormConfig('name', '\stdClass', $this->dispatcher);
+        $config->setAttribute('by_reference', true);
+        $config->setMapped(false);
+        $config->setPropertyPath($propertyPath);
+        $form = $this->getForm($config);
 
-        $this->mapper->mapDataToForm($car, $form);
+        $this->mapper->mapDataToForms($car, array($form));
 
         $this->assertNull($form->getData());
     }
 
-    public function testMapDataToFormIgnoresEmptyData()
+    public function testMapDataToFormsIgnoresEmptyData()
     {
         $propertyPath = $this->getPropertyPath('engine');
-        $form = $this->getForm($propertyPath, true);
 
-        $this->mapper->mapDataToForm(null, $form);
+        $propertyPath->expects($this->never())
+            ->method('getValue');
+
+        $config = new FormConfig('name', '\stdClass', $this->dispatcher);
+        $config->setAttribute('by_reference', true);
+        $config->setPropertyPath($propertyPath);
+        $form = $this->getForm($config);
+
+        $this->mapper->mapDataToForms(null, array($form));
 
         $this->assertNull($form->getData());
     }
 
-    public function testMapFormToDataWritesBackIfNotByReference()
+    public function testMapDataToFormsSkipsVirtualForms()
+    {
+        $car = new \stdClass();
+        $engine = new \stdClass();
+        $propertyPath = $this->getPropertyPath('engine');
+
+        $propertyPath->expects($this->once())
+            ->method('getValue')
+            ->with($car)
+            ->will($this->returnValue($engine));
+
+        $config = new FormConfig('name', '\stdClass', $this->dispatcher);
+        $config->setAttribute('by_reference', true);
+        $config->setVirtual(true);
+        $form = $this->getForm($config);
+
+        $config = new FormConfig('engine', '\stdClass', $this->dispatcher);
+        $config->setAttribute('by_reference', true);
+        $config->setPropertyPath($propertyPath);
+        $child = $this->getForm($config);
+
+        $form->add($child);
+
+        $this->mapper->mapDataToForms($car, array($form));
+
+        $this->assertNull($form->getData());
+        $this->assertSame($engine, $child->getData());
+    }
+
+    public function testMapFormsToDataWritesBackIfNotByReference()
     {
         $car = new \stdClass();
         $engine = new \stdClass();
@@ -186,13 +233,16 @@ class PropertyPathMapperTest extends \PHPUnit_Framework_TestCase
             ->method('setValue')
             ->with($car, $engine);
 
-        $form = $this->getForm($propertyPath, false);
+        $config = new FormConfig('name', '\stdClass', $this->dispatcher);
-        $form->setData($engine);
+        $config->setAttribute('by_reference', false);
+        $config->setPropertyPath($propertyPath);
+        $config->setData($engine);
+        $form = $this->getForm($config);
 
-        $this->mapper->mapFormToData($form, $car);
+        $this->mapper->mapFormsToData(array($form), $car);
     }
 
-    public function testMapFormToDataWritesBackIfByReferenceButNoReference()
+    public function testMapFormsToDataWritesBackIfByReferenceButNoReference()
     {
         $car = new \stdClass();
         $engine = new \stdClass();
@@ -202,13 +252,16 @@ class PropertyPathMapperTest extends \PHPUnit_Framework_TestCase
             ->method('setValue')
             ->with($car, $engine);
 
-        $form = $this->getForm($propertyPath, true);
+        $config = new FormConfig('name', '\stdClass', $this->dispatcher);
-        $form->setData($engine);
+        $config->setAttribute('by_reference', true);
+        $config->setPropertyPath($propertyPath);
+        $config->setData($engine);
+        $form = $this->getForm($config);
 
-        $this->mapper->mapFormToData($form, $car);
+        $this->mapper->mapFormsToData(array($form), $car);
     }
 
-    public function testMapFormToDataWritesBackIfByReferenceAndReference()
+    public function testMapFormsToDataWritesBackIfByReferenceAndReference()
     {
         $car = new \stdClass();
         $engine = new \stdClass();
@@ -223,13 +276,16 @@ class PropertyPathMapperTest extends \PHPUnit_Framework_TestCase
         $propertyPath->expects($this->never())
             ->method('setValue');
 
-        $form = $this->getForm($propertyPath, true);
+        $config = new FormConfig('name', '\stdClass', $this->dispatcher);
-        $form->setData($engine);
+        $config->setAttribute('by_reference', true);
+        $config->setPropertyPath($propertyPath);
+        $config->setData($engine);
+        $form = $this->getForm($config);
 
-        $this->mapper->mapFormToData($form, $car);
+        $this->mapper->mapFormsToData(array($form), $car);
     }
 
-    public function testMapFormToDataIgnoresUnmapped()
+    public function testMapFormsToDataIgnoresUnmapped()
     {
         $car = new \stdClass();
         $engine = new \stdClass();
@@ -238,13 +294,17 @@ class PropertyPathMapperTest extends \PHPUnit_Framework_TestCase
         $propertyPath->expects($this->never())
             ->method('setValue');
 
-        $form = $this->getForm($propertyPath, true, true, false);
+        $config = new FormConfig('name', '\stdClass', $this->dispatcher);
-        $form->setData($engine);
+        $config->setAttribute('by_reference', true);
+        $config->setPropertyPath($propertyPath);
+        $config->setData($engine);
+        $config->setMapped(false);
+        $form = $this->getForm($config);
 
-        $this->mapper->mapFormToData($form, $car);
+        $this->mapper->mapFormsToData(array($form), $car);
     }
 
-    public function testMapFormToDataIgnoresEmptyData()
+    public function testMapFormsToDataIgnoresEmptyData()
     {
         $car = new \stdClass();
         $propertyPath = $this->getPropertyPath('engine');
@@ -252,13 +312,16 @@ class PropertyPathMapperTest extends \PHPUnit_Framework_TestCase
         $propertyPath->expects($this->never())
             ->method('setValue');
 
-        $form = $this->getForm($propertyPath, true);
+        $config = new FormConfig('name', '\stdClass', $this->dispatcher);
-        $form->setData(null);
+        $config->setAttribute('by_reference', true);
+        $config->setPropertyPath($propertyPath);
+        $config->setData(null);
+        $form = $this->getForm($config);
 
-        $this->mapper->mapFormToData($form, $car);
+        $this->mapper->mapFormsToData(array($form), $car);
     }
 
-    public function testMapFormToDataIgnoresUnsynchronized()
+    public function testMapFormsToDataIgnoresUnsynchronized()
     {
         $car = new \stdClass();
         $engine = new \stdClass();
@@ -267,13 +330,16 @@ class PropertyPathMapperTest extends \PHPUnit_Framework_TestCase
         $propertyPath->expects($this->never())
             ->method('setValue');
 
-        $form = $this->getForm($propertyPath, true, false);
+        $config = new FormConfig('name', '\stdClass', $this->dispatcher);
-        $form->setData($engine);
+        $config->setAttribute('by_reference', true);
+        $config->setPropertyPath($propertyPath);
+        $config->setData($engine);
+        $form = $this->getForm($config, false);
 
-        $this->mapper->mapFormToData($form, $car);
+        $this->mapper->mapFormsToData(array($form), $car);
     }
 
-    public function testMapFormToDataIgnoresDisabled()
+    public function testMapFormsToDataIgnoresDisabled()
     {
         $car = new \stdClass();
         $engine = new \stdClass();
@@ -282,9 +348,45 @@ class PropertyPathMapperTest extends \PHPUnit_Framework_TestCase
         $propertyPath->expects($this->never())
             ->method('setValue');
 
-        $form = $this->getForm($propertyPath, true, true, true, true);
+        $config = new FormConfig('name', '\stdClass', $this->dispatcher);
-        $form->setData($engine);
+        $config->setAttribute('by_reference', true);
+        $config->setPropertyPath($propertyPath);
+        $config->setData($engine);
+        $config->setDisabled(true);
+        $form = $this->getForm($config);
 
-        $this->mapper->mapFormToData($form, $car);
+        $this->mapper->mapFormsToData(array($form), $car);
+    }
+
+    public function testMapFormsToDataSkipsVirtualForms()
+    {
+        $car = new \stdClass();
+        $engine = new \stdClass();
+        $parentPath = $this->getPropertyPath('name');
+        $childPath = $this->getPropertyPath('engine');
+
+        $parentPath->expects($this->never())
+            ->method('getValue');
+        $parentPath->expects($this->never())
+            ->method('setValue');
+
+        $childPath->expects($this->once())
+            ->method('setValue')
+            ->with($car, $engine);
+
+        $config = new FormConfig('name', '\stdClass', $this->dispatcher);
+        $config->setPropertyPath($parentPath);
+        $config->setVirtual(true);
+        $form = $this->getForm($config);
+
+        $config = new FormConfig('engine', '\stdClass', $this->dispatcher);
+        $config->setAttribute('by_reference', true);
+        $config->setPropertyPath($childPath);
+        $config->setData($engine);
+        $child = $this->getForm($config);
+
+        $form->add($child);
+
+        $this->mapper->mapFormsToData(array($form), $car);
     }
 }

src/Symfony/Component/Form/Tests/FormTest.php

@@ -736,8 +736,8 @@ class FormTest extends \PHPUnit_Framework_TestCase
 
         $child = $this->getBuilder()->getForm();
         $mapper->expects($this->once())
-            ->method('mapDataToForm')
+            ->method('mapDataToForms')
-            ->with('bar', $child);
+            ->with('bar', array($child));
 
         $form->add($child);
     }