From 9b0245b57b170adcab262e2df57caa9c2966b1c6 Mon Sep 17 00:00:00 2001 From: Bernhard Schussek Date: Thu, 2 Feb 2012 10:36:03 +0100 Subject: [PATCH] [Form] Made prefix of adder and remover method configurable. Adders and removers are not called if "by_reference" is disabled. --- CHANGELOG-2.1.md | 4 +- .../EventListener/MergeCollectionListener.php | 29 +++- .../Form/Extension/Core/Type/ChoiceType.php | 14 +- .../Extension/Core/Type/CollectionType.php | 10 +- .../MergeCollectionListenerTest.php | 132 ++++++++++++++++-- 5 files changed, 170 insertions(+), 19 deletions(-) diff --git a/CHANGELOG-2.1.md b/CHANGELOG-2.1.md index 98f8eed04c..5799a29757 100644 --- a/CHANGELOG-2.1.md +++ b/CHANGELOG-2.1.md @@ -195,9 +195,11 @@ To get the diff between two versions, go to https://github.com/symfony/symfony/c * choice fields now throw a FormException if neither the "choices" nor the "choice_list" option is set * the radio type is now a child of the checkbox type - * the Collection, Choice (with multiple selection) and Entity (with multiple + * 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 ### HttpFoundation diff --git a/src/Symfony/Component/Form/Extension/Core/EventListener/MergeCollectionListener.php b/src/Symfony/Component/Form/Extension/Core/EventListener/MergeCollectionListener.php index 7bfab6888f..f69775825d 100644 --- a/src/Symfony/Component/Form/Extension/Core/EventListener/MergeCollectionListener.php +++ b/src/Symfony/Component/Form/Extension/Core/EventListener/MergeCollectionListener.php @@ -35,10 +35,31 @@ class MergeCollectionListener implements EventSubscriberInterface */ private $allowDelete; - public function __construct($allowAdd = false, $allowDelete = false) + /** + * Whether to search for and use adder and remover methods + * @var Boolean + */ + private $useAccessors; + + /** + * The prefix of the adder method to look for + * @var string + */ + private $adderPrefix; + + /** + * The prefix of the remover method to look for + * @var string + */ + private $removerPrefix; + + public function __construct($allowAdd = false, $allowDelete = false, $useAccessors = true, $adderPrefix = 'add', $removerPrefix = 'remove') { $this->allowAdd = $allowAdd; $this->allowDelete = $allowDelete; + $this->useAccessors = $useAccessors; + $this->adderPrefix = $adderPrefix; + $this->removerPrefix = $removerPrefix; } static public function getSubscribedEvents() @@ -68,14 +89,14 @@ class MergeCollectionListener implements EventSubscriberInterface } // Check if the parent has matching methods to add/remove items - if (is_object($parentData)) { + if ($this->useAccessors && is_object($parentData)) { $plural = ucfirst($form->getName()); $singulars = (array) FormUtil::singularify($plural); $reflClass = new \ReflectionClass($parentData); foreach ($singulars as $singular) { - $adderName = 'add' . $singular; - $removerName = 'remove' . $singular; + $adderName = $this->adderPrefix . $singular; + $removerName = $this->removerPrefix . $singular; if ($reflClass->hasMethod($adderName) && $reflClass->hasMethod($removerName)) { $adder = $reflClass->getMethod($adderName); diff --git a/src/Symfony/Component/Form/Extension/Core/Type/ChoiceType.php b/src/Symfony/Component/Form/Extension/Core/Type/ChoiceType.php index e629d37747..574bf15aaf 100644 --- a/src/Symfony/Component/Form/Extension/Core/Type/ChoiceType.php +++ b/src/Symfony/Component/Form/Extension/Core/Type/ChoiceType.php @@ -95,7 +95,17 @@ class ChoiceType extends AbstractType if ($options['multiple']) { $builder ->appendClientTransformer(new ChoicesToValuesTransformer($options['choice_list'])) - ->addEventSubscriber(new MergeCollectionListener(true, true)) + ->addEventSubscriber(new MergeCollectionListener( + true, + true, + // If "by_reference" is disabled (explicit calling of + // the setter is desired), disable support for + // adders/removers + // Same as in CollectionType + $options['by_reference'], + $options['adder_prefix'], + $options['remover_prefix'] + )) ; } else { $builder->appendClientTransformer(new ChoiceToValueTransformer($options['choice_list'])); @@ -146,6 +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', ); } diff --git a/src/Symfony/Component/Form/Extension/Core/Type/CollectionType.php b/src/Symfony/Component/Form/Extension/Core/Type/CollectionType.php index 748370a007..13f6e3114e 100644 --- a/src/Symfony/Component/Form/Extension/Core/Type/CollectionType.php +++ b/src/Symfony/Component/Form/Extension/Core/Type/CollectionType.php @@ -40,7 +40,13 @@ class CollectionType extends AbstractType $mergeListener = new MergeCollectionListener( $options['allow_add'], - $options['allow_delete'] + $options['allow_delete'], + // If "by_reference" is disabled (explicit calling of the setter + // is desired), disable support for adders/removers + // Same as in ChoiceType + $options['by_reference'], + $options['adder_prefix'], + $options['remover_prefix'] ); $builder @@ -84,6 +90,8 @@ class CollectionType extends AbstractType return array( 'allow_add' => false, 'allow_delete' => false, + 'adder_prefix' => 'add', + 'remover_prefix' => 'remove', '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 4f6463ca84..a8f56c46fe 100644 --- a/tests/Symfony/Tests/Component/Form/Extension/Core/EventListener/MergeCollectionListenerTest.php +++ b/tests/Symfony/Tests/Component/Form/Extension/Core/EventListener/MergeCollectionListenerTest.php @@ -24,6 +24,13 @@ class MergeCollectionListenerTest_Car public function removeAxis($axis) {} } +class MergeCollectionListenerTest_CarCustomPrefix +{ + public function fooAxis($axis) {} + + public function barAxis($axis) {} +} + abstract class MergeCollectionListenerTest extends \PHPUnit_Framework_TestCase { private $dispatcher; @@ -219,11 +226,12 @@ abstract class MergeCollectionListenerTest extends \PHPUnit_Framework_TestCase public function testCallAdderIfAllowAdd() { $parentData = $this->getMock(__CLASS__ . '_Car'); - $parentForm = $this->getForm('article'); + $parentForm = $this->getForm('car'); $parentForm->setData($parentData); $parentForm->add($this->form); - $originalData = $this->getData(array(1 => 'second')); + $originalDataArray = array(1 => 'second'); + $originalData = $this->getData($originalDataArray); $newData = $this->getData(array(0 => 'first', 1 => 'second', 2 => 'third')); $this->form->setData($originalData); @@ -239,16 +247,47 @@ abstract class MergeCollectionListenerTest extends \PHPUnit_Framework_TestCase $listener = new MergeCollectionListener(true, false); $listener->onBindNormData($event); - // The original object was modified 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 testDontCallAdderIfNotAllowAdd() { $parentData = $this->getMock(__CLASS__ . '_Car'); - $parentForm = $this->getForm('article'); + $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', 1 => 'second', 2 => 'third')); + + $this->form->setData($originalData); + + $parentData->expects($this->never()) + ->method('addAxis'); + + $event = new FilterDataEvent($this->form, $newData); + $listener = new MergeCollectionListener(false, false); + $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 testDontCallAdderIfNotUseAccessors() + { + $parentData = $this->getMock(__CLASS__ . '_Car'); + $parentForm = $this->getForm('car'); $parentForm->setData($parentData); $parentForm->add($this->form); @@ -261,23 +300,26 @@ abstract class MergeCollectionListenerTest extends \PHPUnit_Framework_TestCase ->method('addAxis'); $event = new FilterDataEvent($this->form, $newData); - $listener = new MergeCollectionListener(false, false); + $listener = new MergeCollectionListener(true, false, false); $listener->onBindNormData($event); - // The original object was modified if (is_object($originalData)) { $this->assertSame($originalData, $event->getData()); } + + // The data was modified without accessors + $this->assertEquals($newData, $event->getData()); } public function testCallRemoverIfAllowDelete() { $parentData = $this->getMock(__CLASS__ . '_Car'); - $parentForm = $this->getForm('article'); + $parentForm = $this->getForm('car'); $parentForm->setData($parentData); $parentForm->add($this->form); - $originalData = $this->getData(array(0 => 'first', 1 => 'second', 2 => 'third')); + $originalDataArray = array(0 => 'first', 1 => 'second', 2 => 'third'); + $originalData = $this->getData($originalDataArray); $newData = $this->getData(array(1 => 'second')); $this->form->setData($originalData); @@ -293,16 +335,47 @@ abstract class MergeCollectionListenerTest extends \PHPUnit_Framework_TestCase $listener = new MergeCollectionListener(false, true); $listener->onBindNormData($event); - // The original object was modified 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 testDontCallRemoverIfNotAllowDelete() { $parentData = $this->getMock(__CLASS__ . '_Car'); - $parentForm = $this->getForm('article'); + $parentForm = $this->getForm('car'); + $parentForm->setData($parentData); + $parentForm->add($this->form); + + $originalDataArray = array(0 => 'first', 1 => 'second', 2 => 'third'); + $originalData = $this->getData($originalDataArray); + $newData = $this->getData(array(1 => 'second')); + + $this->form->setData($originalData); + + $parentData->expects($this->never()) + ->method('removeAxis'); + + $event = new FilterDataEvent($this->form, $newData); + $listener = new MergeCollectionListener(false, false); + $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 testDontCallRemoverIfNotUseAccessors() + { + $parentData = $this->getMock(__CLASS__ . '_Car'); + $parentForm = $this->getForm('car'); $parentForm->setData($parentData); $parentForm->add($this->form); @@ -315,12 +388,47 @@ abstract class MergeCollectionListenerTest extends \PHPUnit_Framework_TestCase ->method('removeAxis'); $event = new FilterDataEvent($this->form, $newData); - $listener = new MergeCollectionListener(false, false); + $listener = new MergeCollectionListener(false, true, false); $listener->onBindNormData($event); - // The original object was modified if (is_object($originalData)) { $this->assertSame($originalData, $event->getData()); } + + // The data was modified directly + $this->assertEquals($newData, $event->getData()); + } + + public function testCallAccessorsWithCustomPrefixes() + { + $parentData = $this->getMock(__CLASS__ . '_CarCustomPrefix'); + $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('fooAxis') + ->with('first'); + $parentData->expects($this->once()) + ->method('barAxis') + ->with('second'); + + $event = new FilterDataEvent($this->form, $newData); + $listener = new MergeCollectionListener(true, true, true, 'foo', 'bar'); + $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()); } }