Skip to content

Commit 1839a8d

Browse files
fabpotfabpot
committed
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
2 parents 07f4991 + c338c85 commit 1839a8d

File tree

6 files changed

+133
-57
lines changed

6 files changed

+133
-57
lines changed

‎ButtonBuilder.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,7 @@ class ButtonBuilder implements \IteratorAggregate, FormBuilderInterface
6060
*
6161
* @throws InvalidArgumentException If the name is empty.
6262
*/
63-
public function __construct($name, array $options)
63+
public function __construct($name, array $options = array())
6464
{
6565
if (empty($name) && 0 != $name) {
6666
throw new InvalidArgumentException('Buttons cannot have empty names.');

‎Extension/Validator/Constraints/FormValidator.php

Lines changed: 6 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -22,11 +22,6 @@
2222
*/
2323
class FormValidator extends ConstraintValidator
2424
{
25-
/**
26-
* @var \SplObjectStorage
27-
*/
28-
private static $clickedButtons;
29-
3025
/**
3126
* @var ServerParams
3227
*/
@@ -52,16 +47,6 @@ public function validate($form, Constraint $constraint)
5247
return;
5348
}
5449

55-
if (null === static::$clickedButtons) {
56-
static::$clickedButtons = new \SplObjectStorage();
57-
}
58-
59-
// If the form was previously validated, remove it from the cache in
60-
// case the clicked button has changed
61-
if (static::$clickedButtons->contains($form)) {
62-
static::$clickedButtons->detach($form);
63-
}
64-
6550
/* @var FormInterface $form */
6651
$config = $form->getConfig();
6752

@@ -187,20 +172,15 @@ private static function allowDataWalking(FormInterface $form)
187172
*/
188173
private static function getValidationGroups(FormInterface $form)
189174
{
190-
$root = $form->getRoot();
191-
192175
// Determine the clicked button of the complete form tree
193-
if (!static::$clickedButtons->contains($root)) {
194-
// Only call findClickedButton() once to prevent an exponential
195-
// runtime
196-
// https://github.com/symfony/symfony/issues/8317
197-
static::$clickedButtons->attach($root, self::findClickedButton($root));
198-
}
176+
$clickedButton = null;
199177

200-
$button = static::$clickedButtons->offsetGet($root);
178+
if (method_exists($form, 'getClickedButton')) {
179+
$clickedButton = $form->getClickedButton();
180+
}
201181

202-
if (null !== $button) {
203-
$groups = $button->getConfig()->getOption('validation_groups');
182+
if (null !== $clickedButton) {
183+
$groups = $clickedButton->getConfig()->getOption('validation_groups');
204184

205185
if (null !== $groups) {
206186
return self::resolveValidationGroups($groups, $form);
@@ -220,28 +200,6 @@ private static function getValidationGroups(FormInterface $form)
220200
return array(Constraint::DEFAULT_GROUP);
221201
}
222202

223-
/**
224-
* Extracts a clicked button from a form tree, if one exists.
225-
*
226-
* @param FormInterface $form The root form.
227-
*
228-
* @return ClickableInterface|null The clicked button or null.
229-
*/
230-
private static function findClickedButton(FormInterface $form)
231-
{
232-
if ($form instanceof ClickableInterface && $form->isClicked()) {
233-
return $form;
234-
}
235-
236-
foreach ($form as $child) {
237-
if (null !== ($button = self::findClickedButton($child))) {
238-
return $button;
239-
}
240-
}
241-
242-
return null;
243-
}
244-
245203
/**
246204
* Post-processes the validation groups option for a given form.
247205
*

‎Form.php

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,12 @@ class Form implements \IteratorAggregate, FormInterface
9090
*/
9191
private $submitted = false;
9292

93+
/**
94+
* The button that was used to submit the form
95+
* @var Button
96+
*/
97+
private $clickedButton;
98+
9399
/**
94100
* The form data in model format
95101
* @var mixed
@@ -553,6 +559,20 @@ public function submit($submittedData, $clearMissing = true)
553559
if (isset($submittedData[$name]) || $clearMissing) {
554560
$child->submit(isset($submittedData[$name]) ? $submittedData[$name] : null, $clearMissing);
555561
unset($submittedData[$name]);
562+
563+
if (null !== $this->clickedButton) {
564+
continue;
565+
}
566+
567+
if ($child instanceof ClickableInterface && $child->isClicked()) {
568+
$this->clickedButton = $child;
569+
570+
continue;
571+
}
572+
573+
if (method_exists($child, 'getClickedButton') && null !== $child->getClickedButton()) {
574+
$this->clickedButton = $child->getClickedButton();
575+
}
556576
}
557577
}
558578

@@ -732,6 +752,25 @@ public function isValid()
732752
return true;
733753
}
734754

755+
/**
756+
* Returns the button that was used to submit the form.
757+
*
758+
* @return Button|null The clicked button or NULL if the form was not
759+
* submitted
760+
*/
761+
public function getClickedButton()
762+
{
763+
if ($this->clickedButton) {
764+
return $this->clickedButton;
765+
}
766+
767+
if ($this->parent && method_exists($this->parent, 'getClickedButton')) {
768+
return $this->parent->getClickedButton();
769+
}
770+
771+
return null;
772+
}
773+
735774
/**
736775
* {@inheritdoc}
737776
*/

‎Tests/CompoundFormTest.php

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,10 @@
1313

1414
use Symfony\Component\Form\Extension\Core\DataMapper\PropertyPathMapper;
1515
use Symfony\Component\Form\Extension\HttpFoundation\HttpFoundationRequestHandler;
16+
use Symfony\Component\Form\FormBuilder;
1617
use Symfony\Component\Form\FormError;
1718
use Symfony\Component\Form\Forms;
19+
use Symfony\Component\Form\SubmitButtonBuilder;
1820
use Symfony\Component\HttpFoundation\Request;
1921
use Symfony\Component\HttpFoundation\File\UploadedFile;
2022
use Symfony\Component\Form\Tests\Fixtures\FixedDataTransformer;
@@ -831,6 +833,88 @@ public function testGetErrorsAsStringDeep()
831833
$this->assertEquals("name:\n ERROR: Error!\nfoo:\n No errors\n", $parent->getErrorsAsString());
832834
}
833835

836+
public function testNoClickedButtonBeforeSubmission()
837+
{
838+
$this->assertNull($this->form->getClickedButton());
839+
}
840+
841+
public function testNoClickedButton()
842+
{
843+
$button = $this->getMockBuilder('Symfony\Component\Form\SubmitButton')
844+
->setConstructorArgs(array(new SubmitButtonBuilder('submit')))
845+
->setMethods(array('isClicked'))
846+
->getMock();
847+
848+
$button->expects($this->any())
849+
->method('isClicked')
850+
->will($this->returnValue(false));
851+
852+
$parentForm = $this->getBuilder('parent')->getForm();
853+
$nestedForm = $this->getBuilder('nested')->getForm();
854+
855+
$this->form->setParent($parentForm);
856+
$this->form->add($button);
857+
$this->form->add($nestedForm);
858+
$this->form->submit(array());
859+
860+
$this->assertNull($this->form->getClickedButton());
861+
}
862+
863+
public function testClickedButton()
864+
{
865+
$button = $this->getMockBuilder('Symfony\Component\Form\SubmitButton')
866+
->setConstructorArgs(array(new SubmitButtonBuilder('submit')))
867+
->setMethods(array('isClicked'))
868+
->getMock();
869+
870+
$button->expects($this->any())
871+
->method('isClicked')
872+
->will($this->returnValue(true));
873+
874+
$this->form->add($button);
875+
$this->form->submit(array());
876+
877+
$this->assertSame($button, $this->form->getClickedButton());
878+
}
879+
880+
public function testClickedButtonFromNestedForm()
881+
{
882+
$button = $this->getBuilder('submit')->getForm();
883+
884+
$nestedForm = $this->getMockBuilder('Symfony\Component\Form\Form')
885+
->setConstructorArgs(array($this->getBuilder('nested')))
886+
->setMethods(array('getClickedButton'))
887+
->getMock();
888+
889+
$nestedForm->expects($this->any())
890+
->method('getClickedButton')
891+
->will($this->returnValue($button));
892+
893+
$this->form->add($nestedForm);
894+
$this->form->submit(array());
895+
896+
$this->assertSame($button, $this->form->getClickedButton());
897+
}
898+
899+
public function testClickedButtonFromParentForm()
900+
{
901+
$button = $this->getBuilder('submit')->getForm();
902+
903+
$parentForm = $this->getMockBuilder('Symfony\Component\Form\Form')
904+
->setConstructorArgs(array($this->getBuilder('parent')))
905+
->setMethods(array('getClickedButton'))
906+
->getMock();
907+
908+
$parentForm->expects($this->any())
909+
->method('getClickedButton')
910+
->will($this->returnValue($button));
911+
912+
$this->form->setParent($parentForm);
913+
$this->form->submit(array());
914+
915+
$this->assertSame($button, $this->form->getClickedButton());
916+
}
917+
834918
protected function createForm()
835919
{
836920
return $this->getBuilder()

‎Tests/Extension/Validator/Constraints/FormValidatorPerformanceTest.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ public function testValidationPerformance()
3838

3939
$builder = $this->factory->createBuilder('form');
4040

41-
for ($i = 0; $i < 100; ++$i) {
41+
for ($i = 0; $i < 40; ++$i) {
4242
$builder->add($i, 'form');
4343

4444
$builder->get($i)

‎Tests/Extension/Validator/Constraints/FormValidatorTest.php

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -430,11 +430,11 @@ public function testUseValidationGroupOfClickedButton()
430430
));
431431

432432
$parent->add($form);
433-
$parent->add($this->getClickedSubmitButton('submit', array(
433+
$parent->add($this->getSubmitButton('submit', array(
434434
'validation_groups' => 'button_group',
435435
)));
436436

437-
$form->setData($object);
437+
$parent->submit(array('name' => $object, 'submit' => ''));
438438

439439
$context->expects($this->once())
440440
->method('validate')
@@ -733,11 +733,6 @@ private function getSubmitButton($name = 'name', array $options = array())
733733
return $builder->getForm();
734734
}
735735

736-
private function getClickedSubmitButton($name = 'name', array $options = array())
737-
{
738-
return $this->getSubmitButton($name, $options)->submit('');
739-
}
740-
741736
/**
742737
* @return \PHPUnit_Framework_MockObject_MockObject
743738
*/

0 commit comments

Comments
 (0)