From 00bc2708bce4f1c16a77c371de871c922c31079f Mon Sep 17 00:00:00 2001 From: Bernhard Schussek Date: Thu, 15 Aug 2013 21:04:26 +0200 Subject: [PATCH 1/4] [Form] Fixed: submit() reacts to dynamic modifications of the form children --- src/Symfony/Component/Form/Form.php | 9 +- .../Component/Form/Tests/AbstractFormTest.php | 3 + .../Component/Form/Tests/CompoundFormTest.php | 55 ++++++-- .../Util/VirtualFormAwareIteratorTest.php | 122 ++++++++++++++++++ .../Form/Util/VirtualFormAwareIterator.php | 74 ++++++++++- 5 files changed, 242 insertions(+), 21 deletions(-) create mode 100644 src/Symfony/Component/Form/Tests/Util/VirtualFormAwareIteratorTest.php diff --git a/src/Symfony/Component/Form/Form.php b/src/Symfony/Component/Form/Form.php index 55d80c2f9f2f8..2cf0c9dff89c7 100644 --- a/src/Symfony/Component/Form/Form.php +++ b/src/Symfony/Component/Form/Form.php @@ -552,7 +552,12 @@ public function bind($submittedData) $submittedData = array(); } - foreach ($this->children as $name => $child) { + reset($this->children); + + for (reset($this->children); false !== current($this->children); next($this->children)) { + $child = current($this->children); + $name = key($this->children); + $child->bind(isset($submittedData[$name]) ? $submittedData[$name] : null); unset($submittedData[$name]); } @@ -829,7 +834,7 @@ public function getClientTransformers() /** * {@inheritdoc} */ - public function all() + public function &all() { return $this->children; } diff --git a/src/Symfony/Component/Form/Tests/AbstractFormTest.php b/src/Symfony/Component/Form/Tests/AbstractFormTest.php index 5e13863aa9b8f..46be43eb0d905 100644 --- a/src/Symfony/Component/Form/Tests/AbstractFormTest.php +++ b/src/Symfony/Component/Form/Tests/AbstractFormTest.php @@ -81,6 +81,9 @@ protected function getMockForm($name = 'name') $form->expects($this->any()) ->method('getName') ->will($this->returnValue($name)); + $form->expects($this->any()) + ->method('getConfig') + ->will($this->returnValue($this->getMock('Symfony\Component\Form\FormConfigInterface'))); return $form; } diff --git a/src/Symfony/Component/Form/Tests/CompoundFormTest.php b/src/Symfony/Component/Form/Tests/CompoundFormTest.php index ca97c8ec2a0b8..d719133da1690 100644 --- a/src/Symfony/Component/Form/Tests/CompoundFormTest.php +++ b/src/Symfony/Component/Form/Tests/CompoundFormTest.php @@ -11,7 +11,6 @@ namespace Symfony\Component\Form\Tests; -use Symfony\Component\Form\Form; use Symfony\Component\Form\FormError; use Symfony\Component\Form\Extension\HttpFoundation\EventListener\BindRequestListener; use Symfony\Component\HttpFoundation\Request; @@ -46,19 +45,6 @@ public function testInvalidIfChildIsInvalid() $this->assertFalse($this->form->isValid()); } - public function testBindForwardsNullIfValueIsMissing() - { - $child = $this->getMockForm('firstName'); - - $this->form->add($child); - - $child->expects($this->once()) - ->method('bind') - ->with($this->equalTo(null)); - - $this->form->bind(array()); - } - public function testCloneChildren() { $child = $this->getBuilder('child')->getForm(); @@ -322,6 +308,47 @@ public function testSetDataMapsViewDataToChildren() $form->setData('foo'); } + public function testBindForwardsNullIfValueIsMissing() + { + $child = $this->getMockForm('firstName'); + + $this->form->add($child); + + $child->expects($this->once()) + ->method('bind') + ->with($this->equalTo(null)); + + $this->form->bind(array()); + } + + public function testBindSupportsDynamicAdditionAndRemovalOfChildren() + { + $child = $this->getMockForm('child'); + $childToBeRemoved = $this->getMockForm('removed'); + $childToBeAdded = $this->getMockForm('added'); + + $this->form->add($child); + $this->form->add($childToBeRemoved); + + $form = $this->form; + + $child->expects($this->once()) + ->method('bind') + ->will($this->returnCallback(function () use ($form, $childToBeAdded) { + $form->remove('removed'); + $form->add($childToBeAdded); + })); + + $childToBeRemoved->expects($this->never()) + ->method('bind'); + + $childToBeAdded->expects($this->once()) + ->method('bind'); + + // pass NULL to all children + $this->form->bind(array()); + } + public function testBindMapsBoundChildrenOntoExistingViewData() { $test = $this; diff --git a/src/Symfony/Component/Form/Tests/Util/VirtualFormAwareIteratorTest.php b/src/Symfony/Component/Form/Tests/Util/VirtualFormAwareIteratorTest.php new file mode 100644 index 0000000000000..805acad903b68 --- /dev/null +++ b/src/Symfony/Component/Form/Tests/Util/VirtualFormAwareIteratorTest.php @@ -0,0 +1,122 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Symfony\Component\Form\Tests\Util; + +use Symfony\Component\Form\Util\VirtualFormAwareIterator; + +/** + * @author Bernhard Schussek + */ +class VirtualFormAwareIteratorTest extends \PHPUnit_Framework_TestCase +{ + public function testSupportDynamicModification() + { + $form = $this->getMockForm('form'); + $formToBeAdded = $this->getMockForm('added'); + $formToBeRemoved = $this->getMockForm('removed'); + + $forms = array('form' => $form, 'removed' => $formToBeRemoved); + $iterator = new VirtualFormAwareIterator($forms); + + $iterator->rewind(); + $this->assertTrue($iterator->valid()); + $this->assertSame('form', $iterator->key()); + $this->assertSame($form, $iterator->current()); + + // dynamic modification + unset($forms['removed']); + $forms['added'] = $formToBeAdded; + + // continue iteration + $iterator->next(); + $this->assertTrue($iterator->valid()); + $this->assertSame('added', $iterator->key()); + $this->assertSame($formToBeAdded, $iterator->current()); + + // end of array + $iterator->next(); + $this->assertFalse($iterator->valid()); + } + + public function testSupportDynamicModificationInRecursiveCall() + { + $virtualForm = $this->getMockForm('virtual'); + $form = $this->getMockForm('form'); + $formToBeAdded = $this->getMockForm('added'); + $formToBeRemoved = $this->getMockForm('removed'); + + $virtualForm->getConfig()->expects($this->any()) + ->method('getVirtual') + ->will($this->returnValue(true)); + + $virtualForm->add($form); + $virtualForm->add($formToBeRemoved); + + $forms = array('virtual' => $virtualForm); + $iterator = new VirtualFormAwareIterator($forms); + + $iterator->rewind(); + $this->assertTrue($iterator->valid()); + $this->assertSame('virtual', $iterator->key()); + $this->assertSame($virtualForm, $iterator->current()); + $this->assertTrue($iterator->hasChildren()); + + // enter nested iterator + $nestedIterator = $iterator->getChildren(); + $this->assertSame('form', $nestedIterator->key()); + $this->assertSame($form, $nestedIterator->current()); + $this->assertFalse($nestedIterator->hasChildren()); + + // dynamic modification + $virtualForm->remove('removed'); + $virtualForm->add($formToBeAdded); + + // continue iteration - nested iterator discovers change in the form + $nestedIterator->next(); + $this->assertTrue($nestedIterator->valid()); + $this->assertSame('added', $nestedIterator->key()); + $this->assertSame($formToBeAdded, $nestedIterator->current()); + + // end of array + $nestedIterator->next(); + $this->assertFalse($nestedIterator->valid()); + } + + /** + * @param string $name + * + * @return \PHPUnit_Framework_MockObject_MockObject + */ + protected function getMockForm($name = 'name') + { + $config = $this->getMock('Symfony\Component\Form\FormConfigInterface'); + + $config->expects($this->any()) + ->method('getName') + ->will($this->returnValue($name)); + $config->expects($this->any()) + ->method('getCompound') + ->will($this->returnValue(true)); + $config->expects($this->any()) + ->method('getDataMapper') + ->will($this->returnValue($this->getMock('Symfony\Component\Form\DataMapperInterface'))); + $config->expects($this->any()) + ->method('getEventDispatcher') + ->will($this->returnValue($this->getMock('Symfony\Component\EventDispatcher\EventDispatcher'))); + + return $this->getMockBuilder('Symfony\Component\Form\Form') + ->setConstructorArgs(array($config)) + ->disableArgumentCloning() + ->setMethods(array('getViewData')) + ->getMock(); + } +} diff --git a/src/Symfony/Component/Form/Util/VirtualFormAwareIterator.php b/src/Symfony/Component/Form/Util/VirtualFormAwareIterator.php index c1322bb35d939..9b9abb36e7b62 100644 --- a/src/Symfony/Component/Form/Util/VirtualFormAwareIterator.php +++ b/src/Symfony/Component/Form/Util/VirtualFormAwareIterator.php @@ -12,22 +12,86 @@ namespace Symfony\Component\Form\Util; /** - * Iterator that traverses fields of a field group + * Iterator that traverses an array of forms. * - * If the iterator encounters a virtual field group, it enters the field - * group and traverses its children as well. + * Contrary to \ArrayIterator, this iterator recognizes changes in the original + * array during iteration. + * + * You can wrap the iterator into a {@link \RecursiveIterator} in order to + * enter any virtual child form and iterate the children of that virtual form. * * @author Bernhard Schussek */ -class VirtualFormAwareIterator extends \ArrayIterator implements \RecursiveIterator +class VirtualFormAwareIterator implements \RecursiveIterator { + /** + * @var \Symfony\Component\Form\FormInterface[] + */ + private $forms; + + /** + * Creates a new iterator. + * + * @param \Symfony\Component\Form\FormInterface[] $forms An array of forms + */ + public function __construct(array &$forms) + { + $this->forms = &$forms; + } + + /** + *{@inheritdoc} + */ + public function current() + { + return current($this->forms); + } + + /** + *{@inheritdoc} + */ + public function next() + { + next($this->forms); + } + + /** + *{@inheritdoc} + */ + public function key() + { + return key($this->forms); + } + + /** + *{@inheritdoc} + */ + public function valid() + { + return null !== key($this->forms); + } + + /** + *{@inheritdoc} + */ + public function rewind() + { + reset($this->forms); + } + + /** + *{@inheritdoc} + */ public function getChildren() { return new self($this->current()->all()); } + /** + *{@inheritdoc} + */ public function hasChildren() { - return $this->current()->getConfig()->getVirtual(); + return (bool) $this->current()->getConfig()->getVirtual(); } } From 19b483f2eacb118592c3f7cc7e654148b341124e Mon Sep 17 00:00:00 2001 From: Bernhard Schussek Date: Thu, 22 Aug 2013 13:38:57 +0200 Subject: [PATCH 2/4] [Form] Removed superfluous reset() call --- src/Symfony/Component/Form/Form.php | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/Symfony/Component/Form/Form.php b/src/Symfony/Component/Form/Form.php index 2cf0c9dff89c7..c4f744f1de11a 100644 --- a/src/Symfony/Component/Form/Form.php +++ b/src/Symfony/Component/Form/Form.php @@ -552,8 +552,6 @@ public function bind($submittedData) $submittedData = array(); } - reset($this->children); - for (reset($this->children); false !== current($this->children); next($this->children)) { $child = current($this->children); $name = key($this->children); From ccaaedfcbb274960a5e847dd0cdd5e81e41aeeb1 Mon Sep 17 00:00:00 2001 From: Bernhard Schussek Date: Thu, 22 Aug 2013 14:10:15 +0200 Subject: [PATCH 3/4] [Form] PropertyPathMapper::mapDataToForms() *always* calls setData() on every child to ensure that all *_DATA events were fired when the initialization phase is over (except for virtual forms) --- .../Core/DataMapper/PropertyPathMapper.php | 14 +++---- .../DataMapper/PropertyPathMapperTest.php | 39 +++++++++++++++++-- 2 files changed, 43 insertions(+), 10 deletions(-) diff --git a/src/Symfony/Component/Form/Extension/Core/DataMapper/PropertyPathMapper.php b/src/Symfony/Component/Form/Extension/Core/DataMapper/PropertyPathMapper.php index f691ecca21502..3280e008f5f38 100644 --- a/src/Symfony/Component/Form/Extension/Core/DataMapper/PropertyPathMapper.php +++ b/src/Symfony/Component/Form/Extension/Core/DataMapper/PropertyPathMapper.php @@ -44,11 +44,9 @@ public function __construct(PropertyAccessorInterface $propertyAccessor = null) */ public function mapDataToForms($data, array $forms) { - if (null === $data || array() === $data) { - return; - } + $empty = null === $data || array() === $data; - if (!is_array($data) && !is_object($data)) { + if (!$empty && !is_array($data) && !is_object($data)) { throw new UnexpectedTypeException($data, 'object, array or empty'); } @@ -56,12 +54,14 @@ public function mapDataToForms($data, array $forms) $iterator = new \RecursiveIteratorIterator($iterator); foreach ($iterator as $form) { - /* @var FormInterface $form */ + /* @var \Symfony\Component\Form\FormInterface $form */ $propertyPath = $form->getPropertyPath(); $config = $form->getConfig(); - if (null !== $propertyPath && $config->getMapped()) { + if (!$empty && null !== $propertyPath && $config->getMapped()) { $form->setData($this->propertyAccessor->getValue($data, $propertyPath)); + } else { + $form->setData($form->getConfig()->getData()); } } } @@ -83,7 +83,7 @@ public function mapFormsToData(array $forms, &$data) $iterator = new \RecursiveIteratorIterator($iterator); foreach ($iterator as $form) { - /* @var FormInterface $form */ + /* @var \Symfony\Component\Form\FormInterface $form */ $propertyPath = $form->getPropertyPath(); $config = $form->getConfig(); 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 8af2fd5f07f15..0a76ec9576097 100644 --- a/src/Symfony/Component/Form/Tests/Extension/Core/DataMapper/PropertyPathMapperTest.php +++ b/src/Symfony/Component/Form/Tests/Extension/Core/DataMapper/PropertyPathMapperTest.php @@ -165,8 +165,9 @@ public function testMapDataToFormsIgnoresUnmapped() $this->assertNull($form->getData()); } - public function testMapDataToFormsIgnoresEmptyData() + public function testMapDataToFormsSetsDefaultDataIfPassedDataIsNull() { + $default = new \stdClass(); $propertyPath = $this->getPropertyPath('engine'); $this->propertyAccessor->expects($this->never()) @@ -175,11 +176,43 @@ public function testMapDataToFormsIgnoresEmptyData() $config = new FormConfigBuilder('name', '\stdClass', $this->dispatcher); $config->setByReference(true); $config->setPropertyPath($propertyPath); - $form = $this->getForm($config); + $config->setData($default); + + $form = $this->getMockBuilder('Symfony\Component\Form\Form') + ->setConstructorArgs(array($config)) + ->setMethods(array('setData')) + ->getMock(); + + $form->expects($this->once()) + ->method('setData') + ->with($default); $this->mapper->mapDataToForms(null, array($form)); + } - $this->assertNull($form->getData()); + public function testMapDataToFormsSetsDefaultDataIfPassedDataIsEmptyArray() + { + $default = new \stdClass(); + $propertyPath = $this->getPropertyPath('engine'); + + $this->propertyAccessor->expects($this->never()) + ->method('getValue'); + + $config = new FormConfigBuilder('name', '\stdClass', $this->dispatcher); + $config->setByReference(true); + $config->setPropertyPath($propertyPath); + $config->setData($default); + + $form = $this->getMockBuilder('Symfony\Component\Form\Form') + ->setConstructorArgs(array($config)) + ->setMethods(array('setData')) + ->getMock(); + + $form->expects($this->once()) + ->method('setData') + ->with($default); + + $this->mapper->mapDataToForms(array(), array($form)); } public function testMapDataToFormsSkipsVirtualForms() From cd27e1facb9b75f1e0a80b4dbbd8175da8562d75 Mon Sep 17 00:00:00 2001 From: Bernhard Schussek Date: Fri, 23 Aug 2013 13:16:42 +0200 Subject: [PATCH 4/4] [Form] Extracted ReferencingArrayIterator out of VirtualFormAwareIterator --- .../Form/Util/ReferencingArrayIterator.php | 78 +++++++++++++++++++ .../Form/Util/VirtualFormAwareIterator.php | 61 +-------------- 2 files changed, 81 insertions(+), 58 deletions(-) create mode 100644 src/Symfony/Component/Form/Util/ReferencingArrayIterator.php diff --git a/src/Symfony/Component/Form/Util/ReferencingArrayIterator.php b/src/Symfony/Component/Form/Util/ReferencingArrayIterator.php new file mode 100644 index 0000000000000..9bb64d79d2ed4 --- /dev/null +++ b/src/Symfony/Component/Form/Util/ReferencingArrayIterator.php @@ -0,0 +1,78 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Symfony\Component\Form\Util; + +/** + * Iterator that traverses an array. + * + * Contrary to {@link \ArrayIterator}, this iterator recognizes changes in the + * original array during iteration. + * + * @author Bernhard Schussek + */ +class ReferencingArrayIterator implements \Iterator +{ + /** + * @var array + */ + private $array; + + /** + * Creates a new iterator. + * + * @param array $array An array + */ + public function __construct(array &$array) + { + $this->array = &$array; + } + + /** + *{@inheritdoc} + */ + public function current() + { + return current($this->array); + } + + /** + *{@inheritdoc} + */ + public function next() + { + next($this->array); + } + + /** + *{@inheritdoc} + */ + public function key() + { + return key($this->array); + } + + /** + *{@inheritdoc} + */ + public function valid() + { + return null !== key($this->array); + } + + /** + *{@inheritdoc} + */ + public function rewind() + { + reset($this->array); + } +} diff --git a/src/Symfony/Component/Form/Util/VirtualFormAwareIterator.php b/src/Symfony/Component/Form/Util/VirtualFormAwareIterator.php index 9b9abb36e7b62..2648c3a501052 100644 --- a/src/Symfony/Component/Form/Util/VirtualFormAwareIterator.php +++ b/src/Symfony/Component/Form/Util/VirtualFormAwareIterator.php @@ -14,71 +14,16 @@ /** * Iterator that traverses an array of forms. * - * Contrary to \ArrayIterator, this iterator recognizes changes in the original - * array during iteration. + * Contrary to {@link \ArrayIterator}, this iterator recognizes changes in the + * original array during iteration. * * You can wrap the iterator into a {@link \RecursiveIterator} in order to * enter any virtual child form and iterate the children of that virtual form. * * @author Bernhard Schussek */ -class VirtualFormAwareIterator implements \RecursiveIterator +class VirtualFormAwareIterator extends ReferencingArrayIterator implements \RecursiveIterator { - /** - * @var \Symfony\Component\Form\FormInterface[] - */ - private $forms; - - /** - * Creates a new iterator. - * - * @param \Symfony\Component\Form\FormInterface[] $forms An array of forms - */ - public function __construct(array &$forms) - { - $this->forms = &$forms; - } - - /** - *{@inheritdoc} - */ - public function current() - { - return current($this->forms); - } - - /** - *{@inheritdoc} - */ - public function next() - { - next($this->forms); - } - - /** - *{@inheritdoc} - */ - public function key() - { - return key($this->forms); - } - - /** - *{@inheritdoc} - */ - public function valid() - { - return null !== key($this->forms); - } - - /** - *{@inheritdoc} - */ - public function rewind() - { - reset($this->forms); - } - /** *{@inheritdoc} */