diff --git a/src/Symfony/Component/Form/Extension/Core/EventListener/MergeCollectionListener.php b/src/Symfony/Component/Form/Extension/Core/EventListener/MergeCollectionListener.php index f69775825d..52a73a8f0b 100644 --- a/src/Symfony/Component/Form/Extension/Core/EventListener/MergeCollectionListener.php +++ b/src/Symfony/Component/Form/Extension/Core/EventListener/MergeCollectionListener.php @@ -70,6 +70,13 @@ class MergeCollectionListener implements EventSubscriberInterface public function onBindNormData(FilterDataEvent $event) { $originalData = $event->getForm()->getData(); + + // If we are not allowed to change anything, return immediately + if (!$this->allowAdd && !$this->allowDelete) { + $event->setData($originalData); + return; + } + $form = $event->getForm(); $data = $event->getData(); $parentData = $form->hasParent() ? $form->getParent()->getData() : null; @@ -98,21 +105,33 @@ class MergeCollectionListener implements EventSubscriberInterface $adderName = $this->adderPrefix . $singular; $removerName = $this->removerPrefix . $singular; - if ($reflClass->hasMethod($adderName) && $reflClass->hasMethod($removerName)) { + if ($this->allowAdd && $reflClass->hasMethod($adderName)) { $adder = $reflClass->getMethod($adderName); + + if (!$adder->isPublic() || $adder->getNumberOfRequiredParameters() !== 1) { + // False alert + $adder = null; + } + } + + if ($this->allowDelete && $reflClass->hasMethod($removerName)) { $remover = $reflClass->getMethod($removerName); - if ($adder->isPublic() && $adder->getNumberOfRequiredParameters() === 1 - && $remover->isPublic() && $remover->getNumberOfRequiredParameters() === 1) { - - // We found a public, one-parameter add and remove method - break; + if (!$remover->isPublic() || $remover->getNumberOfRequiredParameters() !== 1) { + // False alert + $remover = null; } - - // False alert - $adder = null; - $remover = null; } + + // When we want to both add and delete, we look for an adder and + // remover with the same name + if (!($this->allowAdd && !$adder) && !($this->allowDelete && !$remover)) { + break; + } + + // False alert + $adder = null; + $remover = null; } } @@ -136,15 +155,15 @@ class MergeCollectionListener implements EventSubscriberInterface } } - if ($adder && $remover) { + if ($adder || $remover) { // If methods to add and to remove exist, call them now, if allowed - if ($this->allowDelete) { + if ($remover) { foreach ($itemsToDelete as $item) { $remover->invoke($parentData, $item); } } - if ($this->allowAdd) { + if ($adder) { foreach ($itemsToAdd as $item) { $adder->invoke($parentData, $item); } diff --git a/tests/Symfony/Tests/Component/Form/Extension/Core/EventListener/MergeCollectionListenerTest.php b/tests/Symfony/Tests/Component/Form/Extension/Core/EventListener/MergeCollectionListenerTest.php index a8f56c46fe..84f17ada78 100644 --- a/tests/Symfony/Tests/Component/Form/Extension/Core/EventListener/MergeCollectionListenerTest.php +++ b/tests/Symfony/Tests/Component/Form/Extension/Core/EventListener/MergeCollectionListenerTest.php @@ -31,6 +31,16 @@ class MergeCollectionListenerTest_CarCustomPrefix public function barAxis($axis) {} } +class MergeCollectionListenerTest_CarOnlyAdder +{ + public function addAxis($axis) {} +} + +class MergeCollectionListenerTest_CarOnlyRemover +{ + public function removeAxis($axis) {} +} + abstract class MergeCollectionListenerTest extends \PHPUnit_Framework_TestCase { private $dispatcher; @@ -177,7 +187,7 @@ abstract class MergeCollectionListenerTest extends \PHPUnit_Framework_TestCase { $newData = 'no array or traversable'; $event = new FilterDataEvent($this->form, $newData); - $listener = new MergeCollectionListener(false, false); + $listener = new MergeCollectionListener(true, false); $listener->onBindNormData($event); } @@ -225,7 +235,7 @@ abstract class MergeCollectionListenerTest extends \PHPUnit_Framework_TestCase public function testCallAdderIfAllowAdd() { - $parentData = $this->getMock(__CLASS__ . '_Car'); + $parentData = $this->getMock(__CLASS__ . '_CarOnlyAdder'); $parentForm = $this->getForm('car'); $parentForm->setData($parentData); $parentForm->add($this->form); @@ -313,7 +323,7 @@ abstract class MergeCollectionListenerTest extends \PHPUnit_Framework_TestCase public function testCallRemoverIfAllowDelete() { - $parentData = $this->getMock(__CLASS__ . '_Car'); + $parentData = $this->getMock(__CLASS__ . '_CarOnlyRemover'); $parentForm = $this->getForm('car'); $parentForm->setData($parentData); $parentForm->add($this->form); @@ -399,6 +409,39 @@ abstract class MergeCollectionListenerTest extends \PHPUnit_Framework_TestCase $this->assertEquals($newData, $event->getData()); } + public function testCallAdderAndDeleterIfAllowAll() + { + $parentData = $this->getMock(__CLASS__ . '_Car'); + $parentForm = $this->getForm('car'); + $parentForm->setData($parentData); + $parentForm->add($this->form); + + $originalDataArray = array(1 => 'second'); + $originalData = $this->getData($originalDataArray); + $newData = $this->getData(array(0 => 'first')); + + $this->form->setData($originalData); + + $parentData->expects($this->once()) + ->method('addAxis') + ->with('first'); + $parentData->expects($this->once()) + ->method('removeAxis') + ->with('second'); + + $event = new FilterDataEvent($this->form, $newData); + $listener = new MergeCollectionListener(true, true, true); + $listener->onBindNormData($event); + + if (is_object($originalData)) { + $this->assertSame($originalData, $event->getData()); + } + + // The data was not modified directly + // Thus it should not be written back into the parent data! + $this->assertEquals($this->getData($originalDataArray), $event->getData()); + } + public function testCallAccessorsWithCustomPrefixes() { $parentData = $this->getMock(__CLASS__ . '_CarCustomPrefix');