[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

@@ -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
 
 ### HttpFoundation
 

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()
@@ -80,8 +80,8 @@ class MergeCollectionListener implements EventSubscriberInterface
         $form = $event->getForm();
         $data = $event->getData();
         $parentData = $form->hasParent() ? $form->getParent()->getData() : null;
-        $adderName = null;
+        $addMethod = null;
-        $removerName = 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) {
+            // Any of the two methods is required, but not yet known
-                $maybeAdderName = $this->adderPrefix . $singular;
+            if (($this->allowAdd && !$this->addMethod) || ($this->allowDelete && !$this->removeMethod)) {
-                $maybeRemoverName = $this->removerPrefix . $singular;
+                $plural = ucfirst($form->getName());
+                $singulars = (array) FormUtil::singularify($plural);
 
-                if ($this->allowAdd && $reflClass->hasMethod($maybeAdderName)) {
+                foreach ($singulars as $singular) {
-                    $adder = $reflClass->getMethod($maybeAdderName);
+                    // Try to find adder, but don't override preconfigured one
-
+                    if ($this->allowAdd && !$this->addMethod) {
-                    if ($adder->isPublic() && $adder->getNumberOfRequiredParameters() === 1) {
+                        $addMethod = $this->checkMethod($reflClass, 'add' . $singular);
-                        $adderName = $maybeAdderName;
                     }
-                }
 
-                if ($this->allowDelete && $reflClass->hasMethod($maybeRemoverName)) {
+                    // Try to find remover, but don't override preconfigured one
-                    $remover = $reflClass->getMethod($maybeRemoverName);
+                    if ($this->allowDelete && !$this->removeMethod) {
-
+                        $removeMethod = $this->checkMethod($reflClass, 'remove' . $singular);
-                    if ($remover->isPublic() && $remover->getNumberOfRequiredParameters() === 1) {
-                        $removerName = $maybeRemoverName;
                     }
-                }
 
-                // When we want to both add and delete, we look for an adder and
+                    $addMethodFound = !$this->allowAdd || $addMethod || $this->addMethod;
-                // remover with the same name
+                    $removeMethodFound = !$this->allowDelete || $removeMethod || $this->removeMethod;
-                if (!($this->allowAdd && !$adderName) && !($this->allowDelete && !$removerName)) {
-                    break;
-                }
 
-                // False alert
+                    // Found all that we need. Abort search.
-                $adderName = null;
+                    if ($addMethodFound && $removeMethodFound) {
-                $removerName = null;
+                        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;
+    }
 }

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