diff --git a/CHANGELOG-2.1.md b/CHANGELOG-2.1.md index a94752e130..d72a196779 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 * forms now don't create an empty object anymore if they are completely empty and not required. The empty value for such forms is null. diff --git a/src/Symfony/Component/Form/Extension/Core/EventListener/MergeCollectionListener.php b/src/Symfony/Component/Form/Extension/Core/EventListener/MergeCollectionListener.php index f69775825d..8249a81d2f 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() @@ -70,11 +70,18 @@ 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; - $adder = null; - $remover = null; + $addMethod = null; + $removeMethod = null; if (null === $data) { $data = array(); @@ -90,28 +97,59 @@ 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); + $addMethodNeeded = $this->allowAdd && !$this->addMethod; + $removeMethodNeeded = $this->allowDelete && !$this->removeMethod; - foreach ($singulars as $singular) { - $adderName = $this->adderPrefix . $singular; - $removerName = $this->removerPrefix . $singular; + // Any of the two methods is required, but not yet known + if ($addMethodNeeded || $removeMethodNeeded) { + $singulars = (array) FormUtil::singularify(ucfirst($form->getName())); - if ($reflClass->hasMethod($adderName) && $reflClass->hasMethod($removerName)) { - $adder = $reflClass->getMethod($adderName); - $remover = $reflClass->getMethod($removerName); + foreach ($singulars as $singular) { + // Try to find adder, but don't override preconfigured one + if ($addMethodNeeded) { + $addMethod = $this->checkMethod($reflClass, 'add' . $singular); + } - if ($adder->isPublic() && $adder->getNumberOfRequiredParameters() === 1 - && $remover->isPublic() && $remover->getNumberOfRequiredParameters() === 1) { + // Try to find remover, but don't override preconfigured one + if ($removeMethodNeeded) { + $removeMethod = $this->checkMethod($reflClass, 'remove' . $singular); + } - // We found a public, one-parameter add and remove method + // Found all that we need. Abort search. + if ((!$addMethodNeeded || $addMethod) && (!$removeMethodNeeded || $removeMethod)) { break; } // False alert - $adder = null; - $remover = null; + $addMethod = null; + $removeMethod = null; + } + } + + // Set preconfigured adder + if ($this->allowAdd && $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->allowDelete && $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() + )); } } } @@ -136,17 +174,17 @@ class MergeCollectionListener implements EventSubscriberInterface } } - if ($adder && $remover) { + if ($addMethod || $removeMethod) { // If methods to add and to remove exist, call them now, if allowed - if ($this->allowDelete) { + if ($removeMethod) { foreach ($itemsToDelete as $item) { - $remover->invoke($parentData, $item); + $parentData->$removeMethod($item); } } - if ($this->allowAdd) { + if ($addMethod) { foreach ($itemsToAdd as $item) { - $adder->invoke($parentData, $item); + $parentData->$addMethod($item); } } } elseif (!$originalData) { @@ -176,4 +214,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 a8f56c46fe..02fd3e7300 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,21 @@ 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 +{ + public function addAxis($axis) {} +} + +class MergeCollectionListenerTest_CarOnlyRemover +{ + public function removeAxis($axis) {} } abstract class MergeCollectionListenerTest extends \PHPUnit_Framework_TestCase @@ -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,9 +409,9 @@ abstract class MergeCollectionListenerTest extends \PHPUnit_Framework_TestCase $this->assertEquals($newData, $event->getData()); } - public function testCallAccessorsWithCustomPrefixes() + public function testCallAdderAndDeleterIfAllowAll() { - $parentData = $this->getMock(__CLASS__ . '_CarCustomPrefix'); + $parentData = $this->getMock(__CLASS__ . '_Car'); $parentForm = $this->getForm('car'); $parentForm->setData($parentData); $parentForm->add($this->form); @@ -413,10 +423,43 @@ abstract class MergeCollectionListenerTest extends \PHPUnit_Framework_TestCase $this->form->setData($originalData); $parentData->expects($this->once()) - ->method('fooAxis') + ->method('addAxis') ->with('first'); $parentData->expects($this->once()) - ->method('barAxis') + ->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 testCallAccessorsWithCustomNames() + { + $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->once()) + ->method('bar') ->with('second'); $event = new FilterDataEvent($this->form, $newData); @@ -431,4 +474,106 @@ 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()); } + + 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 + */ + 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); + } }