From d208f4e0deb58db5cf677cfdc0ca8bad862da182 Mon Sep 17 00:00:00 2001 From: Bernhard Schussek Date: Thu, 2 Feb 2012 12:45:45 +0100 Subject: [PATCH 1/5] [Form] Made it possible to use models with only either addXxx() or removeXxx() --- .../EventListener/MergeCollectionListener.php | 45 ++++++++++++----- .../MergeCollectionListenerTest.php | 49 +++++++++++++++++-- 2 files changed, 78 insertions(+), 16 deletions(-) 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'); From b39377402bf7c9284854b6e2be7cd23b5d12f64e Mon Sep 17 00:00:00 2001 From: Bernhard Schussek Date: Thu, 2 Feb 2012 12:50:16 +0100 Subject: [PATCH 2/5] [Form] Used direct method access in MergeCollectionListener instead of Reflection to avoid problems when using class hierarchies --- .../EventListener/MergeCollectionListener.php | 42 +++++++++---------- 1 file changed, 20 insertions(+), 22 deletions(-) diff --git a/src/Symfony/Component/Form/Extension/Core/EventListener/MergeCollectionListener.php b/src/Symfony/Component/Form/Extension/Core/EventListener/MergeCollectionListener.php index 52a73a8f0b..fcd7ed3099 100644 --- a/src/Symfony/Component/Form/Extension/Core/EventListener/MergeCollectionListener.php +++ b/src/Symfony/Component/Form/Extension/Core/EventListener/MergeCollectionListener.php @@ -80,8 +80,8 @@ class MergeCollectionListener implements EventSubscriberInterface $form = $event->getForm(); $data = $event->getData(); $parentData = $form->hasParent() ? $form->getParent()->getData() : null; - $adder = null; - $remover = null; + $adderName = null; + $removerName = null; if (null === $data) { $data = array(); @@ -102,36 +102,34 @@ class MergeCollectionListener implements EventSubscriberInterface $reflClass = new \ReflectionClass($parentData); foreach ($singulars as $singular) { - $adderName = $this->adderPrefix . $singular; - $removerName = $this->removerPrefix . $singular; + $maybeAdderName = $this->adderPrefix . $singular; + $maybeRemoverName = $this->removerPrefix . $singular; - if ($this->allowAdd && $reflClass->hasMethod($adderName)) { - $adder = $reflClass->getMethod($adderName); + if ($this->allowAdd && $reflClass->hasMethod($maybeAdderName)) { + $adder = $reflClass->getMethod($maybeAdderName); - if (!$adder->isPublic() || $adder->getNumberOfRequiredParameters() !== 1) { - // False alert - $adder = null; + if ($adder->isPublic() && $adder->getNumberOfRequiredParameters() === 1) { + $adderName = $maybeAdderName; } } - if ($this->allowDelete && $reflClass->hasMethod($removerName)) { - $remover = $reflClass->getMethod($removerName); + if ($this->allowDelete && $reflClass->hasMethod($maybeRemoverName)) { + $remover = $reflClass->getMethod($maybeRemoverName); - if (!$remover->isPublic() || $remover->getNumberOfRequiredParameters() !== 1) { - // False alert - $remover = null; + if ($remover->isPublic() && $remover->getNumberOfRequiredParameters() === 1) { + $removerName = $maybeRemoverName; } } // 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)) { + if (!($this->allowAdd && !$adderName) && !($this->allowDelete && !$removerName)) { break; } // False alert - $adder = null; - $remover = null; + $adderName = null; + $removerName = null; } } @@ -155,17 +153,17 @@ class MergeCollectionListener implements EventSubscriberInterface } } - if ($adder || $remover) { + if ($adderName || $removerName) { // If methods to add and to remove exist, call them now, if allowed - if ($remover) { + if ($removerName) { foreach ($itemsToDelete as $item) { - $remover->invoke($parentData, $item); + $parentData->$removerName($item); } } - if ($adder) { + if ($adderName) { foreach ($itemsToAdd as $item) { - $adder->invoke($parentData, $item); + $parentData->$adderName($item); } } } elseif (!$originalData) { From 02f61adcc54252d8828ac6629cb806bd20f8e7a1 Mon Sep 17 00:00:00 2001 From: Bernhard Schussek Date: Thu, 2 Feb 2012 13:44:00 +0100 Subject: [PATCH 3/5] [Form] Renamed choice and collection options "adder_prefix" and "remover_prefix" to "add_method" and "remove_method" and allowed to specify full method names --- CHANGELOG-2.1.md | 3 +- .../EventListener/MergeCollectionListener.php | 119 +++++++++++------- .../Form/Extension/Core/Type/ChoiceType.php | 8 +- .../Extension/Core/Type/CollectionType.php | 8 +- .../MergeCollectionListenerTest.php | 54 ++++++-- 5 files changed, 133 insertions(+), 59 deletions(-) diff --git a/CHANGELOG-2.1.md b/CHANGELOG-2.1.md index 5799a29757..893255effe 100644 --- a/CHANGELOG-2.1.md +++ b/CHANGELOG-2.1.md @@ -198,8 +198,7 @@ To get the diff between two versions, go to https://github.com/symfony/symfony/c * the collection, choice (with multiple selection) and entity (with multiple selection) types now make use of addXxx() and removeXxx() methods in your model - * added options "adder_prefix" and "remover_prefix" to collection and choice - type + * added options "add_method" and "remove_method" to collection and choice type ### HttpFoundation diff --git a/src/Symfony/Component/Form/Extension/Core/EventListener/MergeCollectionListener.php b/src/Symfony/Component/Form/Extension/Core/EventListener/MergeCollectionListener.php index fcd7ed3099..35d086391b 100644 --- a/src/Symfony/Component/Form/Extension/Core/EventListener/MergeCollectionListener.php +++ b/src/Symfony/Component/Form/Extension/Core/EventListener/MergeCollectionListener.php @@ -11,12 +11,12 @@ namespace Symfony\Component\Form\Extension\Core\EventListener; -use Symfony\Component\Form\Util\FormUtil; - +use Symfony\Component\EventDispatcher\EventSubscriberInterface; use Symfony\Component\Form\FormEvents; use Symfony\Component\Form\Event\FilterDataEvent; -use Symfony\Component\EventDispatcher\EventSubscriberInterface; use Symfony\Component\Form\Exception\UnexpectedTypeException; +use Symfony\Component\Form\Exception\FormException; +use Symfony\Component\Form\Util\FormUtil; /** * @author Bernhard Schussek @@ -42,24 +42,24 @@ class MergeCollectionListener implements EventSubscriberInterface private $useAccessors; /** - * The prefix of the adder method to look for + * The name of the adder method to look for * @var string */ - private $adderPrefix; + private $addMethod; /** - * The prefix of the remover method to look for + * The name of the remover method to look for * @var string */ - private $removerPrefix; + private $removeMethod; - public function __construct($allowAdd = false, $allowDelete = false, $useAccessors = true, $adderPrefix = 'add', $removerPrefix = 'remove') + public function __construct($allowAdd = false, $allowDelete = false, $useAccessors = true, $addMethod = null, $removeMethod = null) { $this->allowAdd = $allowAdd; $this->allowDelete = $allowDelete; $this->useAccessors = $useAccessors; - $this->adderPrefix = $adderPrefix; - $this->removerPrefix = $removerPrefix; + $this->addMethod = $addMethod; + $this->removeMethod = $removeMethod; } static public function getSubscribedEvents() @@ -80,8 +80,8 @@ class MergeCollectionListener implements EventSubscriberInterface $form = $event->getForm(); $data = $event->getData(); $parentData = $form->hasParent() ? $form->getParent()->getData() : null; - $adderName = null; - $removerName = null; + $addMethod = null; + $removeMethod = null; if (null === $data) { $data = array(); @@ -97,39 +97,62 @@ class MergeCollectionListener implements EventSubscriberInterface // Check if the parent has matching methods to add/remove items if ($this->useAccessors && is_object($parentData)) { - $plural = ucfirst($form->getName()); - $singulars = (array) FormUtil::singularify($plural); $reflClass = new \ReflectionClass($parentData); - foreach ($singulars as $singular) { - $maybeAdderName = $this->adderPrefix . $singular; - $maybeRemoverName = $this->removerPrefix . $singular; + // Any of the two methods is required, but not yet known + if (($this->allowAdd && !$this->addMethod) || ($this->allowDelete && !$this->removeMethod)) { + $plural = ucfirst($form->getName()); + $singulars = (array) FormUtil::singularify($plural); - if ($this->allowAdd && $reflClass->hasMethod($maybeAdderName)) { - $adder = $reflClass->getMethod($maybeAdderName); - - if ($adder->isPublic() && $adder->getNumberOfRequiredParameters() === 1) { - $adderName = $maybeAdderName; + foreach ($singulars as $singular) { + // Try to find adder, but don't override preconfigured one + if ($this->allowAdd && !$this->addMethod) { + $addMethod = $this->checkMethod($reflClass, 'add' . $singular); } - } - if ($this->allowDelete && $reflClass->hasMethod($maybeRemoverName)) { - $remover = $reflClass->getMethod($maybeRemoverName); - - if ($remover->isPublic() && $remover->getNumberOfRequiredParameters() === 1) { - $removerName = $maybeRemoverName; + // Try to find remover, but don't override preconfigured one + if ($this->allowDelete && !$this->removeMethod) { + $removeMethod = $this->checkMethod($reflClass, 'remove' . $singular); } - } - // When we want to both add and delete, we look for an adder and - // remover with the same name - if (!($this->allowAdd && !$adderName) && !($this->allowDelete && !$removerName)) { - break; - } + $addMethodFound = !$this->allowAdd || $addMethod || $this->addMethod; + $removeMethodFound = !$this->allowDelete || $removeMethod || $this->removeMethod; - // False alert - $adderName = null; - $removerName = null; + // Found all that we need. Abort search. + if ($addMethodFound && $removeMethodFound) { + break; + } + + // False alert + $addMethod = null; + $removeMethod = null; + } + } + + // Set preconfigured adder + if ($this->addMethod) { + $addMethod = $this->checkMethod($reflClass, $this->addMethod); + + if (!$addMethod) { + throw new FormException(sprintf( + 'The method "%s" could not be found on class %s', + $this->addMethod, + $reflClass->getName() + )); + } + } + + // Set preconfigured remover + if ($this->removeMethod) { + $removeMethod = $this->checkMethod($reflClass, $this->removeMethod); + + if (!$removeMethod) { + throw new FormException(sprintf( + 'The method "%s" could not be found on class %s', + $this->removeMethod, + $reflClass->getName() + )); + } } } @@ -153,17 +176,17 @@ class MergeCollectionListener implements EventSubscriberInterface } } - if ($adderName || $removerName) { + if ($addMethod || $removeMethod) { // If methods to add and to remove exist, call them now, if allowed - if ($removerName) { + if ($removeMethod) { foreach ($itemsToDelete as $item) { - $parentData->$removerName($item); + $parentData->$removeMethod($item); } } - if ($adderName) { + if ($addMethod) { foreach ($itemsToAdd as $item) { - $parentData->$adderName($item); + $parentData->$addMethod($item); } } } elseif (!$originalData) { @@ -193,4 +216,16 @@ class MergeCollectionListener implements EventSubscriberInterface $event->setData($originalData); } + + private function checkMethod(\ReflectionClass $reflClass, $methodName) { + if ($reflClass->hasMethod($methodName)) { + $method = $reflClass->getMethod($methodName); + + if ($method->isPublic() && $method->getNumberOfRequiredParameters() === 1) { + return $methodName; + } + } + + return null; + } } diff --git a/src/Symfony/Component/Form/Extension/Core/Type/ChoiceType.php b/src/Symfony/Component/Form/Extension/Core/Type/ChoiceType.php index 574bf15aaf..e76967ce20 100644 --- a/src/Symfony/Component/Form/Extension/Core/Type/ChoiceType.php +++ b/src/Symfony/Component/Form/Extension/Core/Type/ChoiceType.php @@ -103,8 +103,8 @@ class ChoiceType extends AbstractType // adders/removers // Same as in CollectionType $options['by_reference'], - $options['adder_prefix'], - $options['remover_prefix'] + $options['add_method'], + $options['remove_method'] )) ; } else { @@ -156,8 +156,8 @@ class ChoiceType extends AbstractType 'empty_data' => $multiple || $expanded ? array() : '', 'empty_value' => $multiple || $expanded || !isset($options['empty_value']) ? null : '', 'error_bubbling' => false, - 'adder_prefix' => 'add', - 'remover_prefix' => 'remove', + 'add_method' => null, + 'remove_method' => null, ); } diff --git a/src/Symfony/Component/Form/Extension/Core/Type/CollectionType.php b/src/Symfony/Component/Form/Extension/Core/Type/CollectionType.php index 13f6e3114e..fbdb35383a 100644 --- a/src/Symfony/Component/Form/Extension/Core/Type/CollectionType.php +++ b/src/Symfony/Component/Form/Extension/Core/Type/CollectionType.php @@ -45,8 +45,8 @@ class CollectionType extends AbstractType // is desired), disable support for adders/removers // Same as in ChoiceType $options['by_reference'], - $options['adder_prefix'], - $options['remover_prefix'] + $options['add_method'], + $options['remove_method'] ); $builder @@ -90,8 +90,8 @@ class CollectionType extends AbstractType return array( 'allow_add' => false, 'allow_delete' => false, - 'adder_prefix' => 'add', - 'remover_prefix' => 'remove', + 'add_method' => null, + 'remove_method' => null, 'prototype' => true, 'prototype_name' => '__name__', 'type' => 'text', 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 84f17ada78..9f81626b4c 100644 --- a/tests/Symfony/Tests/Component/Form/Extension/Core/EventListener/MergeCollectionListenerTest.php +++ b/tests/Symfony/Tests/Component/Form/Extension/Core/EventListener/MergeCollectionListenerTest.php @@ -24,11 +24,11 @@ class MergeCollectionListenerTest_Car public function removeAxis($axis) {} } -class MergeCollectionListenerTest_CarCustomPrefix +class MergeCollectionListenerTest_CarCustomNames { - public function fooAxis($axis) {} + public function foo($axis) {} - public function barAxis($axis) {} + public function bar($axis) {} } class MergeCollectionListenerTest_CarOnlyAdder @@ -442,9 +442,9 @@ abstract class MergeCollectionListenerTest extends \PHPUnit_Framework_TestCase $this->assertEquals($this->getData($originalDataArray), $event->getData()); } - public function testCallAccessorsWithCustomPrefixes() + public function testCallAccessorsWithCustomNames() { - $parentData = $this->getMock(__CLASS__ . '_CarCustomPrefix'); + $parentData = $this->getMock(__CLASS__ . '_CarCustomNames'); $parentForm = $this->getForm('car'); $parentForm->setData($parentData); $parentForm->add($this->form); @@ -456,10 +456,10 @@ abstract class MergeCollectionListenerTest extends \PHPUnit_Framework_TestCase $this->form->setData($originalData); $parentData->expects($this->once()) - ->method('fooAxis') + ->method('foo') ->with('first'); $parentData->expects($this->once()) - ->method('barAxis') + ->method('bar') ->with('second'); $event = new FilterDataEvent($this->form, $newData); @@ -474,4 +474,44 @@ abstract class MergeCollectionListenerTest extends \PHPUnit_Framework_TestCase // Thus it should not be written back into the parent data! $this->assertEquals($this->getData($originalDataArray), $event->getData()); } + + /** + * @expectedException Symfony\Component\Form\Exception\FormException + */ + public function testThrowExceptionIfInvalidAdder() + { + $parentData = $this->getMock(__CLASS__ . '_CarCustomNames'); + $parentForm = $this->getForm('car'); + $parentForm->setData($parentData); + $parentForm->add($this->form); + + $originalData = $this->getData(array(1 => 'second')); + $newData = $this->getData(array(0 => 'first')); + + $this->form->setData($originalData); + + $event = new FilterDataEvent($this->form, $newData); + $listener = new MergeCollectionListener(true, false, true, 'doesnotexist'); + $listener->onBindNormData($event); + } + + /** + * @expectedException Symfony\Component\Form\Exception\FormException + */ + public function testThrowExceptionIfInvalidRemover() + { + $parentData = $this->getMock(__CLASS__ . '_CarCustomNames'); + $parentForm = $this->getForm('car'); + $parentForm->setData($parentData); + $parentForm->add($this->form); + + $originalData = $this->getData(array(1 => 'second')); + $newData = $this->getData(array(0 => 'first')); + + $this->form->setData($originalData); + + $event = new FilterDataEvent($this->form, $newData); + $listener = new MergeCollectionListener(false, true, true, null, 'doesnotexist'); + $listener->onBindNormData($event); + } } From 8ab982afc86dfea86fb6d4f6678d1394b8cc3e3c Mon Sep 17 00:00:00 2001 From: Bernhard Schussek Date: Thu, 2 Feb 2012 13:53:44 +0100 Subject: [PATCH 4/5] [Form] Fixed: Custom add and remove method are not invoked if disallowed --- .../EventListener/MergeCollectionListener.php | 4 +- .../MergeCollectionListenerTest.php | 62 +++++++++++++++++++ 2 files changed, 64 insertions(+), 2 deletions(-) diff --git a/src/Symfony/Component/Form/Extension/Core/EventListener/MergeCollectionListener.php b/src/Symfony/Component/Form/Extension/Core/EventListener/MergeCollectionListener.php index 35d086391b..2586f677ac 100644 --- a/src/Symfony/Component/Form/Extension/Core/EventListener/MergeCollectionListener.php +++ b/src/Symfony/Component/Form/Extension/Core/EventListener/MergeCollectionListener.php @@ -130,7 +130,7 @@ class MergeCollectionListener implements EventSubscriberInterface } // Set preconfigured adder - if ($this->addMethod) { + if ($this->allowAdd && $this->addMethod) { $addMethod = $this->checkMethod($reflClass, $this->addMethod); if (!$addMethod) { @@ -143,7 +143,7 @@ class MergeCollectionListener implements EventSubscriberInterface } // Set preconfigured remover - if ($this->removeMethod) { + if ($this->allowDelete && $this->removeMethod) { $removeMethod = $this->checkMethod($reflClass, $this->removeMethod); if (!$removeMethod) { 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 9f81626b4c..02fd3e7300 100644 --- a/tests/Symfony/Tests/Component/Form/Extension/Core/EventListener/MergeCollectionListenerTest.php +++ b/tests/Symfony/Tests/Component/Form/Extension/Core/EventListener/MergeCollectionListenerTest.php @@ -475,6 +475,68 @@ abstract class MergeCollectionListenerTest extends \PHPUnit_Framework_TestCase $this->assertEquals($this->getData($originalDataArray), $event->getData()); } + public function testDontCallAdderWithCustomNameIfDisallowed() + { + $parentData = $this->getMock(__CLASS__ . '_CarCustomNames'); + $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->never()) + ->method('foo'); + $parentData->expects($this->once()) + ->method('bar') + ->with('second'); + + $event = new FilterDataEvent($this->form, $newData); + $listener = new MergeCollectionListener(false, true, true, 'foo', 'bar'); + $listener->onBindNormData($event); + + if (is_object($originalData)) { + $this->assertSame($originalData, $event->getData()); + } + + // The data was not modified + $this->assertEquals($this->getData($originalDataArray), $event->getData()); + } + + public function testDontCallRemoverWithCustomNameIfDisallowed() + { + $parentData = $this->getMock(__CLASS__ . '_CarCustomNames'); + $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('foo') + ->with('first'); + $parentData->expects($this->never()) + ->method('bar'); + + $event = new FilterDataEvent($this->form, $newData); + $listener = new MergeCollectionListener(true, false, true, 'foo', 'bar'); + $listener->onBindNormData($event); + + if (is_object($originalData)) { + $this->assertSame($originalData, $event->getData()); + } + + // The data was not modified + $this->assertEquals($this->getData($originalDataArray), $event->getData()); + } + /** * @expectedException Symfony\Component\Form\Exception\FormException */ From 8714d7924e65c26e15f93cb5ff745807a533d1be Mon Sep 17 00:00:00 2001 From: Bernhard Schussek Date: Thu, 2 Feb 2012 13:58:41 +0100 Subject: [PATCH 5/5] [Form] Simplified code in MergeCollectionListener --- .../EventListener/MergeCollectionListener.php | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/src/Symfony/Component/Form/Extension/Core/EventListener/MergeCollectionListener.php b/src/Symfony/Component/Form/Extension/Core/EventListener/MergeCollectionListener.php index 2586f677ac..8249a81d2f 100644 --- a/src/Symfony/Component/Form/Extension/Core/EventListener/MergeCollectionListener.php +++ b/src/Symfony/Component/Form/Extension/Core/EventListener/MergeCollectionListener.php @@ -98,28 +98,26 @@ class MergeCollectionListener implements EventSubscriberInterface // Check if the parent has matching methods to add/remove items if ($this->useAccessors && is_object($parentData)) { $reflClass = new \ReflectionClass($parentData); + $addMethodNeeded = $this->allowAdd && !$this->addMethod; + $removeMethodNeeded = $this->allowDelete && !$this->removeMethod; // Any of the two methods is required, but not yet known - if (($this->allowAdd && !$this->addMethod) || ($this->allowDelete && !$this->removeMethod)) { - $plural = ucfirst($form->getName()); - $singulars = (array) FormUtil::singularify($plural); + if ($addMethodNeeded || $removeMethodNeeded) { + $singulars = (array) FormUtil::singularify(ucfirst($form->getName())); foreach ($singulars as $singular) { // Try to find adder, but don't override preconfigured one - if ($this->allowAdd && !$this->addMethod) { + if ($addMethodNeeded) { $addMethod = $this->checkMethod($reflClass, 'add' . $singular); } // Try to find remover, but don't override preconfigured one - if ($this->allowDelete && !$this->removeMethod) { + if ($removeMethodNeeded) { $removeMethod = $this->checkMethod($reflClass, 'remove' . $singular); } - $addMethodFound = !$this->allowAdd || $addMethod || $this->addMethod; - $removeMethodFound = !$this->allowDelete || $removeMethod || $this->removeMethod; - // Found all that we need. Abort search. - if ($addMethodFound && $removeMethodFound) { + if ((!$addMethodNeeded || $addMethod) && (!$removeMethodNeeded || $removeMethod)) { break; }