[Form] Added method Form::getClickedButton() to remove memory leak in FormValidator

Author: webmozart

ButtonBuilder.php

@@ -60,7 +60,7 @@ class ButtonBuilder implements \IteratorAggregate, FormBuilderInterface
      *
      * @throws InvalidArgumentException If the name is empty.
      */
-    public function __construct($name, array $options)
+    public function __construct($name, array $options = array())
     {
         if (empty($name) && 0 != $name) {
             throw new InvalidArgumentException('Buttons cannot have empty names.');

Extension/Validator/Constraints/FormValidator.php

@@ -22,11 +22,6 @@
  */
 class FormValidator extends ConstraintValidator
 {
-    /**
-     * @var array
-     */
-    private static $clickedButtons = array();
-
     /**
      * @var ServerParams
      */
@@ -52,10 +47,6 @@ public function validate($form, Constraint $constraint)
             return;
         }
 
-        // If the form was previously validated, remove it from the cache in
-        // case the clicked button has changed
-        unset(self::$clickedButtons[spl_object_hash($form)]);
-
         /* @var FormInterface $form */
         $config = $form->getConfig();
 
@@ -181,21 +172,15 @@ private static function allowDataWalking(FormInterface $form)
      */
     private static function getValidationGroups(FormInterface $form)
     {
-        $root = $form->getRoot();
-        $rootHash = spl_object_hash($root);
-
         // Determine the clicked button of the complete form tree
-        if (!array_key_exists($rootHash, self::$clickedButtons)) {
-            // Only call findClickedButton() once to prevent an exponential
-            // runtime
-            // https://github.com/symfony/symfony/issues/8317
-            self::$clickedButtons[$rootHash] = self::findClickedButton($root);
-        }
+        $clickedButton = null;
 
-        $button = self::$clickedButtons[$rootHash];
+        if (method_exists($form, 'getClickedButton')) {
+            $clickedButton = $form->getClickedButton();
+        }
 
-        if (null !== $button) {
-            $groups = $button->getConfig()->getOption('validation_groups');
+        if (null !== $clickedButton) {
+            $groups = $clickedButton->getConfig()->getOption('validation_groups');
 
             if (null !== $groups) {
                 return self::resolveValidationGroups($groups, $form);
@@ -215,28 +200,6 @@ private static function getValidationGroups(FormInterface $form)
         return array(Constraint::DEFAULT_GROUP);
     }
 
-    /**
-     * Extracts a clicked button from a form tree, if one exists.
-     *
-     * @param FormInterface $form The root form.
-     *
-     * @return ClickableInterface|null The clicked button or null.
-     */
-    private static function findClickedButton(FormInterface $form)
-    {
-        if ($form instanceof ClickableInterface && $form->isClicked()) {
-            return $form;
-        }
-
-        foreach ($form as $child) {
-            if (null !== ($button = self::findClickedButton($child))) {
-                return $button;
-            }
-        }
-
-        return null;
-    }
-
     /**
      * Post-processes the validation groups option for a given form.
      *

Form.php

@@ -90,6 +90,12 @@ class Form implements \IteratorAggregate, FormInterface
      */
     private $submitted = false;
 
+    /**
+     * The button that was used to submit the form
+     * @var Button
+     */
+    private $clickedButton;
+
     /**
      * The form data in model format
      * @var mixed
@@ -551,6 +557,20 @@ public function submit($submittedData, $clearMissing = true)
                     if (isset($submittedData[$name]) || $clearMissing) {
                         $child->submit(isset($submittedData[$name]) ? $submittedData[$name] : null, $clearMissing);
                         unset($submittedData[$name]);
+
+                        if (null !== $this->clickedButton) {
+                            continue;
+                        }
+
+                        if ($child instanceof ClickableInterface && $child->isClicked()) {
+                            $this->clickedButton = $child;
+
+                            continue;
+                        }
+
+                        if (method_exists($child, 'getClickedButton') && null !== $child->getClickedButton()) {
+                            $this->clickedButton = $child->getClickedButton();
+                        }
                     }
                 }
 
@@ -730,6 +750,25 @@ public function isValid()
         return true;
     }
 
+    /**
+     * Returns the button that was used to submit the form.
+     *
+     * @return Button|null The clicked button or NULL if the form was not
+     *                     submitted
+     */
+    public function getClickedButton()
+    {
+        if ($this->clickedButton) {
+            return $this->clickedButton;
+        }
+
+        if ($this->parent && method_exists($this->parent, 'getClickedButton')) {
+            return $this->parent->getClickedButton();
+        }
+
+        return null;
+    }
+
     /**
      * {@inheritdoc}
      */

Tests/CompoundFormTest.php

@@ -13,8 +13,10 @@
 
 use Symfony\Component\Form\Extension\Core\DataMapper\PropertyPathMapper;
 use Symfony\Component\Form\Extension\HttpFoundation\HttpFoundationRequestHandler;
+use Symfony\Component\Form\FormBuilder;
 use Symfony\Component\Form\FormError;
 use Symfony\Component\Form\Forms;
+use Symfony\Component\Form\SubmitButtonBuilder;
 use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\HttpFoundation\File\UploadedFile;
 use Symfony\Component\Form\Tests\Fixtures\FixedDataTransformer;
@@ -830,6 +832,88 @@ public function testGetErrorsAsStringDeep()
         $this->assertEquals("name:\n    ERROR: Error!\nfoo:\n    No errors\n", $parent->getErrorsAsString());
     }
 
+    public function testNoClickedButtonBeforeSubmission()
+    {
+        $this->assertNull($this->form->getClickedButton());
+    }
+
+    public function testNoClickedButton()
+    {
+        $button = $this->getMockBuilder('Symfony\Component\Form\SubmitButton')
+            ->setConstructorArgs(array(new SubmitButtonBuilder('submit')))
+            ->setMethods(array('isClicked'))
+            ->getMock();
+
+        $button->expects($this->any())
+            ->method('isClicked')
+            ->will($this->returnValue(false));
+
+        $parentForm = $this->getBuilder('parent')->getForm();
+        $nestedForm = $this->getBuilder('nested')->getForm();
+
+        $this->form->setParent($parentForm);
+        $this->form->add($button);
+        $this->form->add($nestedForm);
+        $this->form->submit(array());
+
+        $this->assertNull($this->form->getClickedButton());
+    }
+
+    public function testClickedButton()
+    {
+        $button = $this->getMockBuilder('Symfony\Component\Form\SubmitButton')
+            ->setConstructorArgs(array(new SubmitButtonBuilder('submit')))
+            ->setMethods(array('isClicked'))
+            ->getMock();
+
+        $button->expects($this->any())
+            ->method('isClicked')
+            ->will($this->returnValue(true));
+
+        $this->form->add($button);
+        $this->form->submit(array());
+
+        $this->assertSame($button, $this->form->getClickedButton());
+    }
+
+    public function testClickedButtonFromNestedForm()
+    {
+        $button = $this->getBuilder('submit')->getForm();
+
+        $nestedForm = $this->getMockBuilder('Symfony\Component\Form\Form')
+            ->setConstructorArgs(array($this->getBuilder('nested')))
+            ->setMethods(array('getClickedButton'))
+            ->getMock();
+
+        $nestedForm->expects($this->any())
+            ->method('getClickedButton')
+            ->will($this->returnValue($button));
+
+        $this->form->add($nestedForm);
+        $this->form->submit(array());
+
+        $this->assertSame($button, $this->form->getClickedButton());
+    }
+
+    public function testClickedButtonFromParentForm()
+    {
+        $button = $this->getBuilder('submit')->getForm();
+
+        $parentForm = $this->getMockBuilder('Symfony\Component\Form\Form')
+            ->setConstructorArgs(array($this->getBuilder('parent')))
+            ->setMethods(array('getClickedButton'))
+            ->getMock();
+
+        $parentForm->expects($this->any())
+            ->method('getClickedButton')
+            ->will($this->returnValue($button));
+
+        $this->form->setParent($parentForm);
+        $this->form->submit(array());
+
+        $this->assertSame($button, $this->form->getClickedButton());
+    }
+
     protected function createForm()
     {
         return $this->getBuilder()

Tests/Extension/Validator/Constraints/FormValidatorTest.php

@@ -430,11 +430,11 @@ public function testUseValidationGroupOfClickedButton()
         ));
 
         $parent->add($form);
-        $parent->add($this->getClickedSubmitButton('submit', array(
+        $parent->add($this->getSubmitButton('submit', array(
             'validation_groups' => 'button_group',
         )));
 
-        $form->setData($object);
+        $parent->submit(array('name' => $object, 'submit' => ''));
 
         $context->expects($this->once())
             ->method('validate')
@@ -733,11 +733,6 @@ private function getSubmitButton($name = 'name', array $options = array())
         return $builder->getForm();
     }
 
-    private function getClickedSubmitButton($name = 'name', array $options = array())
-    {
-        return $this->getSubmitButton($name, $options)->submit('');
-    }
-
     /**
      * @return \PHPUnit_Framework_MockObject_MockObject
      */