bug #9211 [Form] Fixed memory leak in FormValidator (bschussek)

This PR was merged into the 2.3 branch.

Discussion
----------

[Form] Fixed memory leak in FormValidator

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #9187
| License       | MIT
| Doc PR        | -

Commits
-------

1bb7e4d [Form] Added method Form::getClickedButton() to remove memory leak in FormValidator
5329ab5 [Form] Fixed memory leak in FormValidator

src/Symfony/Component/Form/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.');

src/Symfony/Component/Form/Extension/Validator/Constraints/FormValidator.php

@@ -22,11 +22,6 @@ use Symfony\Component\Validator\ConstraintValidator;
  */
 class FormValidator extends ConstraintValidator
 {
-    /**
-     * @var \SplObjectStorage
-     */
-    private static $clickedButtons;
-
     /**
      * @var ServerParams
      */
@@ -52,16 +47,6 @@ class FormValidator extends ConstraintValidator
             return;
         }
 
-        if (null === static::$clickedButtons) {
-            static::$clickedButtons = new \SplObjectStorage();
-        }
-
-        // If the form was previously validated, remove it from the cache in
-        // case the clicked button has changed
-        if (static::$clickedButtons->contains($form)) {
-            static::$clickedButtons->detach($form);
-        }
-
         /* @var FormInterface $form */
         $config = $form->getConfig();
 
@@ -187,20 +172,15 @@ class FormValidator extends ConstraintValidator
      */
     private static function getValidationGroups(FormInterface $form)
     {
-        $root = $form->getRoot();
-
         // Determine the clicked button of the complete form tree
-        if (!static::$clickedButtons->contains($root)) {
+        $clickedButton = null;
-            // Only call findClickedButton() once to prevent an exponential
+
-            // runtime
+        if (method_exists($form, 'getClickedButton')) {
-            // https://github.com/symfony/symfony/issues/8317
+            $clickedButton = $form->getClickedButton();
-            static::$clickedButtons->attach($root, self::findClickedButton($root));
         }
 
-        $button = static::$clickedButtons->offsetGet($root);
+        if (null !== $clickedButton) {
-
+            $groups = $clickedButton->getConfig()->getOption('validation_groups');
-        if (null !== $button) {
-            $groups = $button->getConfig()->getOption('validation_groups');
 
             if (null !== $groups) {
                 return self::resolveValidationGroups($groups, $form);
@@ -220,28 +200,6 @@ class FormValidator extends ConstraintValidator
         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.
      *

src/Symfony/Component/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
@@ -553,6 +559,20 @@ class Form implements \IteratorAggregate, FormInterface
                     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();
+                        }
                     }
                 }
 
@@ -732,6 +752,25 @@ class Form implements \IteratorAggregate, FormInterface
         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}
      */

src/Symfony/Component/Form/Tests/CompoundFormTest.php

@@ -13,8 +13,10 @@ namespace Symfony\Component\Form\Tests;
 
 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;
@@ -831,6 +833,88 @@ class CompoundFormTest extends AbstractFormTest
         $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()

src/Symfony/Component/Form/Tests/Extension/Validator/Constraints/FormValidatorPerformanceTest.php

@@ -38,7 +38,7 @@ class FormValidatorPerformanceTest extends FormPerformanceTestCase
 
         $builder = $this->factory->createBuilder('form');
 
-        for ($i = 0; $i < 100; ++$i) {
+        for ($i = 0; $i < 40; ++$i) {
             $builder->add($i, 'form');
 
             $builder->get($i)

src/Symfony/Component/Form/Tests/Extension/Validator/Constraints/FormValidatorTest.php

@@ -430,11 +430,11 @@ class FormValidatorTest extends \PHPUnit_Framework_TestCase
         ));
 
         $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 @@ class FormValidatorTest extends \PHPUnit_Framework_TestCase
         return $builder->getForm();
     }
 
-    private function getClickedSubmitButton($name = 'name', array $options = array())
-    {
-        return $this->getSubmitButton($name, $options)->submit('');
-    }
-
     /**
      * @return \PHPUnit_Framework_MockObject_MockObject
      */