From d4f4038f6daf7cf88ca7c7ab089473cce5ebf7d8 Mon Sep 17 00:00:00 2001 From: Bernhard Schussek Date: Mon, 16 Jul 2012 12:47:05 +0200 Subject: [PATCH] [Form] Reduced the number of setData() calls by deferring a Form's initialization (+40ms) --- src/Symfony/Component/Form/Form.php | 105 ++++++++++++++---- .../Component/Form/Tests/CompoundFormTest.php | 2 +- .../Component/Form/Tests/SimpleFormTest.php | 17 +++ 3 files changed, 104 insertions(+), 20 deletions(-) diff --git a/src/Symfony/Component/Form/Form.php b/src/Symfony/Component/Form/Form.php index a2383948e7..5d6a237213 100644 --- a/src/Symfony/Component/Form/Form.php +++ b/src/Symfony/Component/Form/Form.php @@ -119,6 +119,26 @@ class Form implements \IteratorAggregate, FormInterface */ private $synchronized = true; + /** + * Whether the form's data has been initialized. + * + * When the data is initialized with its default value, that default value + * is passed through the transformer chain in order to synchronize the + * model, normalized and view format for the first time. This is done + * lazily in order to save performance when {@link setData()} is called + * manually, making the initialization with the configured default value + * superfluous. + * + * @var Boolean + */ + private $initialized = false; + + /** + * Whether setData() is currently being called. + * @var Boolean + */ + private $lockSetData = false; + /** * Creates a new form based on the given configuration. * @@ -138,8 +158,6 @@ class Form implements \IteratorAggregate, FormInterface } $this->config = $config; - - $this->setData($config->getData()); } public function __clone() @@ -327,13 +345,16 @@ class Form implements \IteratorAggregate, FormInterface /** * Updates the form with default data. * - * @param array $modelData The data formatted as expected for the underlying object + * @param mixed $modelData The data formatted as expected for the underlying object * * @return Form The current form */ public function setData($modelData) { - if ($this->bound) { + // If the form is bound while disabled, it is set to bound, but the data is not + // changed. In such cases (i.e. when the form is not initialized yet) don't + // abort this method. + if ($this->bound && $this->initialized) { throw new AlreadyBoundException('You cannot change the data of a bound form'); } @@ -346,6 +367,12 @@ class Form implements \IteratorAggregate, FormInterface $modelData = clone $modelData; } + if ($this->lockSetData) { + throw new FormException('A cycle was detected. Listeners to the PRE_SET_DATA event must not call setData(). You should call setData() on the FormEvent object instead.'); + } + + $this->lockSetData = true; + // Hook to change content of the data $event = new FormEvent($this, $modelData); $this->config->getEventDispatcher()->dispatch(FormEvents::PRE_SET_DATA, $event); @@ -395,8 +422,12 @@ class Form implements \IteratorAggregate, FormInterface $this->normData = $normData; $this->viewData = $viewData; $this->synchronized = true; + $this->initialized = true; + $this->lockSetData = false; - if ($this->config->getCompound()) { + // It is not necessary to invoke this method if the form doesn't have children, + // even if the form is compound. + if (count($this->children) > 0) { // Update child forms from the data $this->config->getDataMapper()->mapDataToForms($viewData, $this->children); } @@ -414,9 +445,29 @@ class Form implements \IteratorAggregate, FormInterface */ public function getData() { + if (!$this->initialized) { + $this->setData($this->config->getData()); + } + return $this->modelData; } + /** + * Returns the normalized data of the form. + * + * @return mixed When the form is not bound, the default data is returned. + * When the form is bound, the normalized bound data is + * returned if the form is valid, null otherwise. + */ + public function getNormData() + { + if (!$this->initialized) { + $this->setData($this->config->getData()); + } + + return $this->normData; + } + /** * Returns the data transformed by the value transformer. * @@ -424,6 +475,10 @@ class Form implements \IteratorAggregate, FormInterface */ public function getViewData() { + if (!$this->initialized) { + $this->setData($this->config->getData()); + } + return $this->viewData; } @@ -543,7 +598,9 @@ class Form implements \IteratorAggregate, FormInterface } // Merge form data from children into existing view data - if ($this->config->getCompound()) { + // It is not necessary to invoke this method if the form has no children, + // even if it is compound. + if (count($this->children) > 0) { $this->config->getDataMapper()->mapFormsToData($this->children, $viewData); } @@ -573,6 +630,7 @@ class Form implements \IteratorAggregate, FormInterface $this->viewData = $viewData; $this->extraData = $extraData; $this->synchronized = $synchronized; + $this->initialized = true; $event = new FormEvent($this, $viewData); $this->config->getEventDispatcher()->dispatch(FormEvents::POST_BIND, $event); @@ -604,18 +662,6 @@ class Form implements \IteratorAggregate, FormInterface return $this->bind($request); } - /** - * Returns the normalized data of the form. - * - * @return mixed When the form is not bound, the default data is returned. - * When the form is bound, the normalized bound data is - * returned if the form is valid, null otherwise. - */ - public function getNormData() - { - return $this->normData; - } - /** * Adds an error to this form. * @@ -833,11 +879,32 @@ class Form implements \IteratorAggregate, FormInterface throw new FormException('You cannot add children to a simple form. Maybe you should set the option "compound" to true?'); } + // Obtain the view data + $viewData = null; + + // If setData() is currently being called, there is no need to call + // mapDataToForms() here, as mapDataToForms() is called at the end + // of setData() anyway. Not doing this check leads to an endless + // recursion when initializing the form lazily and an event listener + // (such as ResizeFormListener) adds fields depending on the data: + // + // * setData() is called, the form is not initialized yet + // * add() is called by the listener (setData() is not complete, so + // the form is still not initialized) + // * getViewData() is called + // * setData() is called since the form is not initialized yet + // * ... endless recursion ... + if (!$this->lockSetData) { + $viewData = $this->getViewData(); + } + $this->children[$child->getName()] = $child; $child->setParent($this); - $this->config->getDataMapper()->mapDataToForms($this->getViewData(), array($child)); + if (!$this->lockSetData) { + $this->config->getDataMapper()->mapDataToForms($viewData, array($child)); + } return $this; } diff --git a/src/Symfony/Component/Form/Tests/CompoundFormTest.php b/src/Symfony/Component/Form/Tests/CompoundFormTest.php index 1fecee9c44..4e591858da 100644 --- a/src/Symfony/Component/Form/Tests/CompoundFormTest.php +++ b/src/Symfony/Component/Form/Tests/CompoundFormTest.php @@ -20,7 +20,7 @@ use Symfony\Component\HttpFoundation\File\UploadedFile; use Symfony\Component\EventDispatcher\EventDispatcher; use Symfony\Component\Form\Tests\Fixtures\FixedDataTransformer; -class FormTest extends AbstractFormTest +class CompoundFormTest extends AbstractFormTest { public function testValidIfAllChildrenAreValid() { diff --git a/src/Symfony/Component/Form/Tests/SimpleFormTest.php b/src/Symfony/Component/Form/Tests/SimpleFormTest.php index dcc2438a18..2d24010671 100644 --- a/src/Symfony/Component/Form/Tests/SimpleFormTest.php +++ b/src/Symfony/Component/Form/Tests/SimpleFormTest.php @@ -12,6 +12,8 @@ namespace Symfony\Component\Form\Tests; use Symfony\Component\Form\Form; +use Symfony\Component\Form\FormEvent; +use Symfony\Component\Form\FormEvents; use Symfony\Component\Form\Util\PropertyPath; use Symfony\Component\Form\FormConfig; use Symfony\Component\Form\FormView; @@ -732,6 +734,21 @@ class SimpleFormTest extends AbstractFormTest $form->setData('foo'); } + /** + * @expectedException Symfony\Component\Form\Exception\FormException + */ + public function testSetDataCannotInvokeItself() + { + // Cycle detection to prevent endless loops + $config = new FormConfig('name', 'stdClass', $this->dispatcher); + $config->addEventListener(FormEvents::PRE_SET_DATA, function (FormEvent $event) { + $event->getForm()->setData('bar'); + }); + $form = new Form($config); + + $form->setData('foo'); + } + protected function createForm() { return $this->getBuilder()->getForm();