merged branch bschussek/issue3239 (PR #3256)

Commits
-------

8714d79 [Form] Simplified code in MergeCollectionListener
8ab982a [Form] Fixed: Custom add and remove method are not invoked if disallowed
02f61ad [Form] Renamed choice and collection options "adder_prefix" and "remover_prefix" to "add_method" and "remove_method" and allowed to specify full method names
b393774 [Form] Used direct method access in MergeCollectionListener instead of Reflection to avoid problems when using class hierarchies
d208f4e [Form] Made it possible to use models with only either addXxx() or removeXxx()

Discussion
----------

[Form] Fixed edge cases in MergeCollectionListener

Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -

![Travis Build Status](https://secure.travis-ci.org/bschussek/symfony.png?branch=issue3239)

Fixes an issue mentioned in the comments of #3239

see https://github.com/symfony/symfony/pull/3239#issuecomment-3776312

---------------------------------------------------------------------------

by bschussek at 2012-02-02T12:12:17Z

Wait a minute before merging this.

---------------------------------------------------------------------------

by bschussek at 2012-02-02T13:01:55Z

@fabpot Ready to merge

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
+ * added options "add_method" and "remove_method" to collection and choice type
-   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.
 

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 <bschussek@gmail.com>
@@ -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->addMethod = $addMethod;
-        $this->removerPrefix = $removerPrefix;
+        $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;
+        $addMethod = null;
-        $remover = 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) {
+            // Any of the two methods is required, but not yet known
-                $adderName = $this->adderPrefix . $singular;
+            if ($addMethodNeeded || $removeMethodNeeded) {
-                $removerName = $this->removerPrefix . $singular;
+                $singulars = (array) FormUtil::singularify(ucfirst($form->getName()));
 
-                if ($reflClass->hasMethod($adderName) && $reflClass->hasMethod($removerName)) {
+                foreach ($singulars as $singular) {
-                    $adder = $reflClass->getMethod($adderName);
+                    // Try to find adder, but don't override preconfigured one
-                    $remover = $reflClass->getMethod($removerName);
+                    if ($addMethodNeeded) {
+                        $addMethod = $this->checkMethod($reflClass, 'add' . $singular);
+                    }
 
-                    if ($adder->isPublic() && $adder->getNumberOfRequiredParameters() === 1
+                    // Try to find remover, but don't override preconfigured one
-                        && $remover->isPublic() && $remover->getNumberOfRequiredParameters() === 1) {
+                    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;
+                    $addMethod = null;
-                    $remover = 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;
+    }
 }

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['add_method'],
-                        $options['remover_prefix']
+                        $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',
+            'add_method'        => null,
-            'remover_prefix'    => 'remove',
+            'remove_method'     => null,
         );
     }
 

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['add_method'],
-            $options['remover_prefix']
+            $options['remove_method']
         );
 
         $builder
@@ -90,8 +90,8 @@ class CollectionType extends AbstractType
         return array(
             'allow_add'      => false,
             'allow_delete'   => false,
-            'adder_prefix'   => 'add',
+            'add_method'     => null,
-            'remover_prefix' => 'remove',
+            'remove_method'  => null,
             'prototype'      => true,
             'prototype_name' => '__name__',
             'type'           => 'text',

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);
+    }
 }