From d1864c7d63dca47c8db497a7e6cd3d0c6bff6577 Mon Sep 17 00:00:00 2001 From: Bernhard Schussek Date: Wed, 23 May 2012 14:08:46 +0200 Subject: [PATCH 1/3] [Form] Fixed: Virtual forms are ignored when prepopulating a form --- src/Symfony/Component/Form/CHANGELOG.md | 3 + .../Component/Form/DataMapperInterface.php | 4 - .../Core/DataMapper/PropertyPathMapper.php | 60 ++--- src/Symfony/Component/Form/Form.php | 2 +- src/Symfony/Component/Form/FormConfig.php | 10 +- .../DataMapper/PropertyPathMapperTest.php | 254 ++++++++++++------ src/Symfony/Component/Form/Tests/FormTest.php | 4 +- 7 files changed, 211 insertions(+), 126 deletions(-) diff --git a/src/Symfony/Component/Form/CHANGELOG.md b/src/Symfony/Component/Form/CHANGELOG.md index db8efd3724..38818a8e25 100644 --- a/src/Symfony/Component/Form/CHANGELOG.md +++ b/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` diff --git a/src/Symfony/Component/Form/DataMapperInterface.php b/src/Symfony/Component/Form/DataMapperInterface.php index a8b8fdffdd..587a4abd78 100644 --- a/src/Symfony/Component/Form/DataMapperInterface.php +++ b/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); } diff --git a/src/Symfony/Component/Form/Extension/Core/DataMapper/PropertyPathMapper.php b/src/Symfony/Component/Form/Extension/Core/DataMapper/PropertyPathMapper.php index 513f22a2e5..4f1b61a0cd 100644 --- a/src/Symfony/Component/Form/Extension/Core/DataMapper/PropertyPathMapper.php +++ b/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(); - /** - * {@inheritdoc} - */ - public function mapDataToForm($data, FormInterface $form) - { - if (!empty($data)) { - $propertyPath = $form->getPropertyPath(); - $config = $form->getConfig(); + if (null !== $propertyPath && $config->getMapped()) { + $propertyData = $propertyPath->getValue($data); - if (null !== $propertyPath && $config->getMapped()) { - $propertyData = $propertyPath->getValue($data); + if (is_object($propertyData) && !$form->getAttribute('by_reference')) { + $propertyData = clone $propertyData; + } - if (is_object($propertyData) && !$form->getAttribute('by_reference')) { - $propertyData = clone $propertyData; + $form->setData($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(); - /** - * {@inheritdoc} - */ - public function mapFormToData(FormInterface $form, &$data) - { - $propertyPath = $form->getPropertyPath(); - $config = $form->getConfig(); + // Write-back is disabled if the form is not synchronized (transformation failed) + // and if the form is disabled (modification not allowed) + 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'); - // Write-back is disabled if the form is not synchronized (transformation failed) - // and if the form is disabled (modification not allowed) - 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()); + if (!(is_object($data) && $isReference && $byReference)) { + $propertyPath->setValue($data, $form->getData()); + } } } } diff --git a/src/Symfony/Component/Form/Form.php b/src/Symfony/Component/Form/Form.php index a159aea6cc..fb3d66b17e 100644 --- a/src/Symfony/Component/Form/Form.php +++ b/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; diff --git a/src/Symfony/Component/Form/FormConfig.php b/src/Symfony/Component/Form/FormConfig.php index 2ff8bdf604..59a459367c 100644 --- a/src/Symfony/Component/Form/FormConfig.php +++ b/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 diff --git a/src/Symfony/Component/Form/Tests/Extension/Core/DataMapper/PropertyPathMapperTest.php b/src/Symfony/Component/Form/Tests/Extension/Core/DataMapper/PropertyPathMapperTest.php index d060dfaa0f..e6bb3a4b3e 100644 --- a/src/Symfony/Component/Form/Tests/Extension/Core/DataMapper/PropertyPathMapperTest.php +++ b/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() - { - $this->mapper = null; - } - + /** + * @param $path + * @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'); - - $config->expects($this->any()) - ->method('getMapped') - ->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 = $this->getMockBuilder('Symfony\Component\Form\Form') + ->setConstructorArgs(array($config)) + ->setMethods(array('isSynchronized')) + ->getMock(); $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); - $form->setData($engine); + $config = new FormConfig('name', '\stdClass', $this->dispatcher); + $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); - $form->setData($engine); + $config = new FormConfig('name', '\stdClass', $this->dispatcher); + $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); - $form->setData($engine); + $config = new FormConfig('name', '\stdClass', $this->dispatcher); + $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); - $form->setData($engine); + $config = new FormConfig('name', '\stdClass', $this->dispatcher); + $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); - $form->setData(null); + $config = new FormConfig('name', '\stdClass', $this->dispatcher); + $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); - $form->setData($engine); + $config = new FormConfig('name', '\stdClass', $this->dispatcher); + $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); - $form->setData($engine); + $config = new FormConfig('name', '\stdClass', $this->dispatcher); + $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); } } diff --git a/src/Symfony/Component/Form/Tests/FormTest.php b/src/Symfony/Component/Form/Tests/FormTest.php index 389d5b4747..6a0cd9a38f 100644 --- a/src/Symfony/Component/Form/Tests/FormTest.php +++ b/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') - ->with('bar', $child); + ->method('mapDataToForms') + ->with('bar', array($child)); $form->add($child); } From fc2370104f7e082fb29dfbca2ec8c850a358ec6f Mon Sep 17 00:00:00 2001 From: Bernhard Schussek Date: Wed, 23 May 2012 14:24:08 +0200 Subject: [PATCH 2/3] [Form] Correctly highlighted BC breaks in the CHANGELOG --- src/Symfony/Component/Form/CHANGELOG.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Symfony/Component/Form/CHANGELOG.md b/src/Symfony/Component/Form/CHANGELOG.md index 38818a8e25..14ac7cca1c 100644 --- a/src/Symfony/Component/Form/CHANGELOG.md +++ b/src/Symfony/Component/Form/CHANGELOG.md @@ -71,13 +71,13 @@ CHANGELOG * labels don't display field attributes anymore. Label attributes can be passed in the "label_attr" option/variable * added option "mapped" which should be used instead of setting "property_path" to false - * "data_class" now *must* be set if a form maps to an object and should be left empty otherwise + * [BC BREAK] "data_class" now *must* be set if a form maps to an object and should be left empty otherwise * improved error mapping on forms * dot (".") rules are now allowed to map errors assigned to a form to one of its children * errors are not mapped to unsynchronized forms anymore - * changed Form constructor to accept a single `FormConfigInterface` object - * changed argument order in the FormBuilder constructor + * [BC BREAK] changed Form constructor to accept a single `FormConfigInterface` object + * [BC BREAK] changed argument order in the FormBuilder constructor * deprecated Form methods * `getTypes` * `getErrorBubbling` From bad6d040d76f4c10a12b7946e05377ce366a6d94 Mon Sep 17 00:00:00 2001 From: Bernhard Schussek Date: Wed, 23 May 2012 14:51:26 +0200 Subject: [PATCH 3/3] [Form] Added accessor FormConfigInterface::getByReference() and let Form clone objects if not by reference --- .../Core/DataMapper/PropertyPathMapper.php | 11 ++----- .../Form/Extension/Core/Type/FormType.php | 2 +- src/Symfony/Component/Form/Form.php | 4 +++ src/Symfony/Component/Form/FormConfig.php | 28 +++++++++++++++++ .../Component/Form/FormConfigInterface.php | 7 +++++ .../DataMapper/PropertyPathMapperTest.php | 30 +++++++++---------- src/Symfony/Component/Form/Tests/FormTest.php | 19 ++++++++++++ .../Component/Form/UnmodifiableFormConfig.php | 14 +++++++++ 8 files changed, 90 insertions(+), 25 deletions(-) diff --git a/src/Symfony/Component/Form/Extension/Core/DataMapper/PropertyPathMapper.php b/src/Symfony/Component/Form/Extension/Core/DataMapper/PropertyPathMapper.php index 4f1b61a0cd..71f54c5fba 100644 --- a/src/Symfony/Component/Form/Extension/Core/DataMapper/PropertyPathMapper.php +++ b/src/Symfony/Component/Form/Extension/Core/DataMapper/PropertyPathMapper.php @@ -37,13 +37,7 @@ class PropertyPathMapper implements DataMapperInterface $config = $form->getConfig(); if (null !== $propertyPath && $config->getMapped()) { - $propertyData = $propertyPath->getValue($data); - - if (is_object($propertyData) && !$form->getAttribute('by_reference')) { - $propertyData = clone $propertyData; - } - - $form->setData($propertyData); + $form->setData($propertyPath->getValue($data)); } } } @@ -68,9 +62,8 @@ class PropertyPathMapper implements DataMapperInterface // 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)) { + if (!is_object($data) || !$isReference || !$config->getByReference()) { $propertyPath->setValue($data, $form->getData()); } } diff --git a/src/Symfony/Component/Form/Extension/Core/Type/FormType.php b/src/Symfony/Component/Form/Extension/Core/Type/FormType.php index 3ce2d18bcd..212675ee1a 100644 --- a/src/Symfony/Component/Form/Extension/Core/Type/FormType.php +++ b/src/Symfony/Component/Form/Extension/Core/Type/FormType.php @@ -46,9 +46,9 @@ class FormType extends AbstractType // BC compatibility, when "property_path" could be false ->setPropertyPath(is_string($options['property_path']) ? $options['property_path'] : null) ->setMapped($options['mapped']) + ->setByReference($options['by_reference']) ->setVirtual($options['virtual']) ->setAttribute('read_only', $options['read_only']) - ->setAttribute('by_reference', $options['by_reference']) ->setAttribute('max_length', $options['max_length']) ->setAttribute('pattern', $options['pattern']) ->setAttribute('label', $options['label'] ?: $this->humanize($builder->getName())) diff --git a/src/Symfony/Component/Form/Form.php b/src/Symfony/Component/Form/Form.php index fb3d66b17e..b9fe51376d 100644 --- a/src/Symfony/Component/Form/Form.php +++ b/src/Symfony/Component/Form/Form.php @@ -319,6 +319,10 @@ class Form implements \IteratorAggregate, FormInterface throw new AlreadyBoundException('You cannot change the data of a bound form'); } + if (is_object($appData) && !$this->config->getByReference()) { + $appData = clone $appData; + } + $event = new DataEvent($this, $appData); $this->config->getEventDispatcher()->dispatch(FormEvents::PRE_SET_DATA, $event); diff --git a/src/Symfony/Component/Form/FormConfig.php b/src/Symfony/Component/Form/FormConfig.php index 59a459367c..eae045323c 100644 --- a/src/Symfony/Component/Form/FormConfig.php +++ b/src/Symfony/Component/Form/FormConfig.php @@ -43,6 +43,11 @@ class FormConfig implements FormConfigInterface */ private $mapped = true; + /** + * @var Boolean + */ + private $byReference = true; + /** * @var Boolean */ @@ -294,6 +299,14 @@ class FormConfig implements FormConfigInterface return $this->mapped; } + /** + * {@inheritdoc} + */ + public function getByReference() + { + return $this->byReference; + } + /** * {@inheritdoc} */ @@ -550,6 +563,21 @@ class FormConfig implements FormConfigInterface return $this; } + /** + * Sets whether the form's data should be modified by reference. + * + * @param Boolean $byReference Whether the data should be + modified by reference. + * + * @return self The configuration object. + */ + public function setByReference($byReference) + { + $this->byReference = $byReference; + + return $this; + } + /** * Sets whether the form should be virtual. * diff --git a/src/Symfony/Component/Form/FormConfigInterface.php b/src/Symfony/Component/Form/FormConfigInterface.php index acceb84477..554a104667 100644 --- a/src/Symfony/Component/Form/FormConfigInterface.php +++ b/src/Symfony/Component/Form/FormConfigInterface.php @@ -47,6 +47,13 @@ interface FormConfigInterface */ function getMapped(); + /** + * Returns whether the form's data should be modified by reference. + * + * @return Boolean Whether to modify the form's data by reference. + */ + function getByReference(); + /** * Returns whether the form should be virtual. * diff --git a/src/Symfony/Component/Form/Tests/Extension/Core/DataMapper/PropertyPathMapperTest.php b/src/Symfony/Component/Form/Tests/Extension/Core/DataMapper/PropertyPathMapperTest.php index e6bb3a4b3e..040e9899dc 100644 --- a/src/Symfony/Component/Form/Tests/Extension/Core/DataMapper/PropertyPathMapperTest.php +++ b/src/Symfony/Component/Form/Tests/Extension/Core/DataMapper/PropertyPathMapperTest.php @@ -110,7 +110,7 @@ class PropertyPathMapperTest extends \PHPUnit_Framework_TestCase ->will($this->returnValue($engine)); $config = new FormConfig('name', '\stdClass', $this->dispatcher); - $config->setAttribute('by_reference', true); + $config->setByReference(true); $config->setPropertyPath($propertyPath); $form = $this->getForm($config); @@ -133,7 +133,7 @@ class PropertyPathMapperTest extends \PHPUnit_Framework_TestCase ->will($this->returnValue($engine)); $config = new FormConfig('name', '\stdClass', $this->dispatcher); - $config->setAttribute('by_reference', false); + $config->setByReference(false); $config->setPropertyPath($propertyPath); $form = $this->getForm($config); @@ -148,7 +148,7 @@ class PropertyPathMapperTest extends \PHPUnit_Framework_TestCase $car = new \stdClass(); $config = new FormConfig(null, '\stdClass', $this->dispatcher); - $config->setAttribute('by_reference', true); + $config->setByReference(true); $form = $this->getForm($config); $this->assertNull($form->getPropertyPath()); @@ -167,7 +167,7 @@ class PropertyPathMapperTest extends \PHPUnit_Framework_TestCase ->method('getValue'); $config = new FormConfig('name', '\stdClass', $this->dispatcher); - $config->setAttribute('by_reference', true); + $config->setByReference(true); $config->setMapped(false); $config->setPropertyPath($propertyPath); $form = $this->getForm($config); @@ -185,7 +185,7 @@ class PropertyPathMapperTest extends \PHPUnit_Framework_TestCase ->method('getValue'); $config = new FormConfig('name', '\stdClass', $this->dispatcher); - $config->setAttribute('by_reference', true); + $config->setByReference(true); $config->setPropertyPath($propertyPath); $form = $this->getForm($config); @@ -206,12 +206,12 @@ class PropertyPathMapperTest extends \PHPUnit_Framework_TestCase ->will($this->returnValue($engine)); $config = new FormConfig('name', '\stdClass', $this->dispatcher); - $config->setAttribute('by_reference', true); + $config->setByReference(true); $config->setVirtual(true); $form = $this->getForm($config); $config = new FormConfig('engine', '\stdClass', $this->dispatcher); - $config->setAttribute('by_reference', true); + $config->setByReference(true); $config->setPropertyPath($propertyPath); $child = $this->getForm($config); @@ -234,7 +234,7 @@ class PropertyPathMapperTest extends \PHPUnit_Framework_TestCase ->with($car, $engine); $config = new FormConfig('name', '\stdClass', $this->dispatcher); - $config->setAttribute('by_reference', false); + $config->setByReference(false); $config->setPropertyPath($propertyPath); $config->setData($engine); $form = $this->getForm($config); @@ -253,7 +253,7 @@ class PropertyPathMapperTest extends \PHPUnit_Framework_TestCase ->with($car, $engine); $config = new FormConfig('name', '\stdClass', $this->dispatcher); - $config->setAttribute('by_reference', true); + $config->setByReference(true); $config->setPropertyPath($propertyPath); $config->setData($engine); $form = $this->getForm($config); @@ -277,7 +277,7 @@ class PropertyPathMapperTest extends \PHPUnit_Framework_TestCase ->method('setValue'); $config = new FormConfig('name', '\stdClass', $this->dispatcher); - $config->setAttribute('by_reference', true); + $config->setByReference(true); $config->setPropertyPath($propertyPath); $config->setData($engine); $form = $this->getForm($config); @@ -295,7 +295,7 @@ class PropertyPathMapperTest extends \PHPUnit_Framework_TestCase ->method('setValue'); $config = new FormConfig('name', '\stdClass', $this->dispatcher); - $config->setAttribute('by_reference', true); + $config->setByReference(true); $config->setPropertyPath($propertyPath); $config->setData($engine); $config->setMapped(false); @@ -313,7 +313,7 @@ class PropertyPathMapperTest extends \PHPUnit_Framework_TestCase ->method('setValue'); $config = new FormConfig('name', '\stdClass', $this->dispatcher); - $config->setAttribute('by_reference', true); + $config->setByReference(true); $config->setPropertyPath($propertyPath); $config->setData(null); $form = $this->getForm($config); @@ -331,7 +331,7 @@ class PropertyPathMapperTest extends \PHPUnit_Framework_TestCase ->method('setValue'); $config = new FormConfig('name', '\stdClass', $this->dispatcher); - $config->setAttribute('by_reference', true); + $config->setByReference(true); $config->setPropertyPath($propertyPath); $config->setData($engine); $form = $this->getForm($config, false); @@ -349,7 +349,7 @@ class PropertyPathMapperTest extends \PHPUnit_Framework_TestCase ->method('setValue'); $config = new FormConfig('name', '\stdClass', $this->dispatcher); - $config->setAttribute('by_reference', true); + $config->setByReference(true); $config->setPropertyPath($propertyPath); $config->setData($engine); $config->setDisabled(true); @@ -380,7 +380,7 @@ class PropertyPathMapperTest extends \PHPUnit_Framework_TestCase $form = $this->getForm($config); $config = new FormConfig('engine', '\stdClass', $this->dispatcher); - $config->setAttribute('by_reference', true); + $config->setByReference(true); $config->setPropertyPath($childPath); $config->setData($engine); $child = $this->getForm($config); diff --git a/src/Symfony/Component/Form/Tests/FormTest.php b/src/Symfony/Component/Form/Tests/FormTest.php index 6a0cd9a38f..e0188d9085 100644 --- a/src/Symfony/Component/Form/Tests/FormTest.php +++ b/src/Symfony/Component/Form/Tests/FormTest.php @@ -464,6 +464,25 @@ class FormTest extends \PHPUnit_Framework_TestCase $this->form->setData(null); } + public function testSetDataClonesObjectIfNotByReference() + { + $data = new \stdClass(); + $form = $this->getBuilder('name', null, '\stdClass')->setByReference(false)->getForm(); + $form->setData($data); + + $this->assertNotSame($data, $form->getData()); + $this->assertEquals($data, $form->getData()); + } + + public function testSetDataDoesNotCloneObjectIfByReference() + { + $data = new \stdClass(); + $form = $this->getBuilder('name', null, '\stdClass')->setByReference(true)->getForm(); + $form->setData($data); + + $this->assertSame($data, $form->getData()); + } + public function testSetDataExecutesTransformationChain() { // use real event dispatcher now diff --git a/src/Symfony/Component/Form/UnmodifiableFormConfig.php b/src/Symfony/Component/Form/UnmodifiableFormConfig.php index ab925316c1..3fdb963923 100644 --- a/src/Symfony/Component/Form/UnmodifiableFormConfig.php +++ b/src/Symfony/Component/Form/UnmodifiableFormConfig.php @@ -42,6 +42,11 @@ class UnmodifiableFormConfig implements FormConfigInterface */ private $mapped; + /** + * @var Boolean + */ + private $byReference; + /** * @var Boolean */ @@ -123,6 +128,7 @@ class UnmodifiableFormConfig implements FormConfigInterface $this->name = $config->getName(); $this->propertyPath = $config->getPropertyPath(); $this->mapped = $config->getMapped(); + $this->byReference = $config->getByReference(); $this->virtual = $config->getVirtual(); $this->types = $config->getTypes(); $this->clientTransformers = $config->getClientTransformers(); @@ -170,6 +176,14 @@ class UnmodifiableFormConfig implements FormConfigInterface return $this->mapped; } + /** + * {@inheritdoc} + */ + public function getByReference() + { + return $this->byReference; + } + /** * {@inheritdoc} */