merged branch bschussek/issue3022 (PR #3322)

Commits
-------

cde34fd [Form] Throwing an AlreadyBoundException in `add`, `remove`, `setParent`, `bind` and `setData` if called on a bound form

Discussion
----------

[Form] Throwing an AlreadyBoundException in `add`, `remove`, `setParent`, `bind` and `setData` if called on a bound form

Bug fix: yes
Feature addition: no
Backwards compatibility break: **yes**
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -

![Travis Build Status](https://secure.travis-ci.org/bschussek/symfony.png?branch=issue3022)

The above mentioned methods now throw an exception because when invoked on a bound form they might cause strange side effects. You should rely on event listeners instead of modifying bound forms.

See also #3022

CHANGELOG-2.1.md

@@ -210,6 +210,8 @@ To get the diff between two versions, go to https://github.com/symfony/symfony/c
  * added constant Guess::VERY_HIGH_CONFIDENCE
  * FormType::getDefaultOptions() now sees default options defined by parent types
  * [BC BREAK] FormType::getParent() does not see default options anymore
+ * [BC BREAK] The methods `add`, `remove`, `setParent`, `bind` and `setData`
+   in class Form now throw an exception if the form is already bound
 
 ### HttpFoundation
 

UPGRADE-2.1.md

@@ -291,3 +291,11 @@ UPGRADE FROM 2.0 to 2.1
 
   Any session storage driver that wants to use custom save handlers should
   implement `Symfony\Component\HttpFoundation\Session\Storage\SaveHandlerInterface`
+
+* The methods `add`, `remove`, `setParent`, `bind` and `setData` in class Form
+  now throw an exception if the form is already bound
+
+    If you used these methods on bound forms, you should consider moving your
+    logic to an event listener listening to either of the events
+    FormEvents::PRE_BIND, FormEvents::BIND_CLIENT_DATA or
+    FormEvents::BIND_NORM_DATA instead. 

src/Symfony/Component/Form/Extension/Csrf/EventListener/CsrfValidationListener.php

@@ -0,0 +1,68 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\Form\Extension\Csrf\EventListener;
+
+use Symfony\Component\EventDispatcher\EventSubscriberInterface;
+use Symfony\Component\Form\FormEvents;
+use Symfony\Component\Form\FormError;
+use Symfony\Component\Form\Event\DataEvent;
+use Symfony\Component\Form\Extension\Csrf\CsrfProvider\CsrfProviderInterface;
+
+/**
+ * @author Bernhard Schussek <bschussek@gmail.com>
+ */
+class CsrfValidationListener implements EventSubscriberInterface
+{
+    /**
+     * The provider for generating and validating CSRF tokens
+     * @var CsrfProviderInterface
+     */
+    private $csrfProvider;
+
+    /**
+     * A text mentioning the intention of the CSRF token
+     *
+     * Validation of the token will only succeed if it was generated in the
+     * same session and with the same intention.
+     *
+     * @var string
+     */
+    private $intention;
+
+    static public function getSubscribedEvents()
+    {
+        return array(
+            FormEvents::BIND_CLIENT_DATA => 'onBindClientData',
+        );
+    }
+
+    public function __construct(CsrfProviderInterface $csrfProvider, $intention)
+    {
+        $this->csrfProvider = $csrfProvider;
+        $this->intention = $intention;
+    }
+
+    public function onBindClientData(DataEvent $event)
+    {
+        $form = $event->getForm();
+        $data = $event->getData();
+
+        if ((!$form->hasParent() || $form->getParent()->isRoot())
+            && !$this->csrfProvider->isCsrfTokenValid($this->intention, $data)) {
+            $form->addError(new FormError('The CSRF token is invalid. Please try to resubmit the form'));
+
+            // If the session timed out, the token is invalid now.
+            // Regenerate the token so that a resubmission is possible.
+            $event->setData($this->csrfProvider->generateCsrfToken($this->intention));
+        }
+    }
+}

src/Symfony/Component/Form/Extension/Csrf/Type/CsrfType.php

@@ -11,10 +11,12 @@
 
 namespace Symfony\Component\Form\Extension\Csrf\Type;
 
+
 use Symfony\Component\Form\AbstractType;
 use Symfony\Component\Form\FormInterface;
 use Symfony\Component\Form\FormBuilder;
 use Symfony\Component\Form\FormError;
+use Symfony\Component\Form\Extension\Csrf\EventListener\CsrfValidationListener;
 use Symfony\Component\Form\Extension\Csrf\CsrfProvider\CsrfProviderInterface;
 use Symfony\Component\Form\CallbackValidator;
 
@@ -46,18 +48,9 @@ class CsrfType extends AbstractType
         $csrfProvider = $options['csrf_provider'];
         $intention = $options['intention'];
 
-        $validator = function (FormInterface $form) use ($csrfProvider, $intention)
-        {
-            if ((!$form->hasParent() || $form->getParent()->isRoot())
-                && !$csrfProvider->isCsrfTokenValid($intention, $form->getData())) {
-                $form->addError(new FormError('The CSRF token is invalid. Please try to resubmit the form'));
-                $form->setData($csrfProvider->generateCsrfToken($intention));
-            }
-        };
-
         $builder
             ->setData($csrfProvider->generateCsrfToken($intention))
-            ->addValidator(new CallbackValidator($validator))
+            ->addEventSubscriber(new CsrfValidationListener($csrfProvider, $intention))
         ;
     }
 

src/Symfony/Component/Form/Form.php

@@ -14,6 +14,7 @@ namespace Symfony\Component\Form;
 use Symfony\Component\Form\Event\DataEvent;
 use Symfony\Component\Form\Event\FilterDataEvent;
 use Symfony\Component\Form\Exception\FormException;
+use Symfony\Component\Form\Exception\AlreadyBoundException;
 use Symfony\Component\Form\Exception\UnexpectedTypeException;
 use Symfony\Component\Form\Exception\TransformationFailedException;
 use Symfony\Component\HttpFoundation\Request;
@@ -297,6 +298,10 @@ class Form implements \IteratorAggregate, FormInterface
      */
     public function setParent(FormInterface $parent = null)
     {
+        if ($this->bound) {
+            throw new AlreadyBoundException('You cannot set the parent of a bound form');
+        }
+
         if ('' === $this->getName()) {
             throw new FormException('Form with empty name can not have parent form.');
         }
@@ -377,6 +382,10 @@ class Form implements \IteratorAggregate, FormInterface
      */
     public function setData($appData)
     {
+        if ($this->bound) {
+            throw new AlreadyBoundException('You cannot change the data of a bound form');
+        }
+
         $event = new DataEvent($this, $appData);
         $this->dispatcher->dispatch(FormEvents::PRE_SET_DATA, $event);
 
@@ -451,6 +460,10 @@ class Form implements \IteratorAggregate, FormInterface
      */
     public function bind($clientData)
     {
+        if ($this->bound) {
+            throw new AlreadyBoundException('A form can only be bound once');
+        }
+
         if ($this->isDisabled()) {
             $this->bound = true;
 
@@ -689,7 +702,7 @@ class Form implements \IteratorAggregate, FormInterface
      */
     public function isValid()
     {
-        if (!$this->isBound()) {
+        if (!$this->bound) {
             throw new \LogicException('You cannot call isValid() on a form that is not bound.');
         }
 
@@ -821,6 +834,10 @@ class Form implements \IteratorAggregate, FormInterface
      */
     public function add(FormInterface $child)
     {
+        if ($this->bound) {
+            throw new AlreadyBoundException('You cannot add children to a bound form');
+        }
+
         $this->children[$child->getName()] = $child;
 
         $child->setParent($this);
@@ -841,6 +858,10 @@ class Form implements \IteratorAggregate, FormInterface
      */
     public function remove($name)
     {
+        if ($this->bound) {
+            throw new AlreadyBoundException('You cannot remove children from a bound form');
+        }
+
         if (isset($this->children[$name])) {
             $this->children[$name]->setParent(null);
 

tests/Symfony/Tests/Component/Form/FormTest.php

@@ -199,6 +199,15 @@ class FormTest extends \PHPUnit_Framework_TestCase
         $this->assertEquals(array('firstName' => 'Bernhard'), $this->form->getData());
     }
 
+    /**
+     * @expectedException Symfony\Component\Form\Exception\AlreadyBoundException
+     */
+    public function testBindThrowsExceptionIfAlreadyBound()
+    {
+        $this->form->bind(array());
+        $this->form->bind(array());
+    }
+
     public function testBindForwardsNullIfValueIsMissing()
     {
         $child = $this->getMockForm('firstName');
@@ -408,8 +417,8 @@ class FormTest extends \PHPUnit_Framework_TestCase
             ->method('isValid')
             ->will($this->returnValue(false));
 
-        $this->form->bind('foobar');
         $this->form->add($child);
+        $this->form->bind(array());
 
         $this->assertFalse($this->form->isValid());
     }
@@ -438,6 +447,15 @@ class FormTest extends \PHPUnit_Framework_TestCase
         $this->assertFalse($this->form->hasChildren());
     }
 
+    /**
+     * @expectedException Symfony\Component\Form\Exception\AlreadyBoundException
+     */
+    public function testSetParentThrowsExceptionIfAlreadyBound()
+    {
+        $this->form->bind(array());
+        $this->form->setParent($this->getBuilder('parent')->getForm());
+    }
+
     public function testAdd()
     {
         $child = $this->getBuilder('foo')->getForm();
@@ -447,6 +465,15 @@ class FormTest extends \PHPUnit_Framework_TestCase
         $this->assertSame(array('foo' => $child), $this->form->getChildren());
     }
 
+    /**
+     * @expectedException Symfony\Component\Form\Exception\AlreadyBoundException
+     */
+    public function testAddThrowsExceptionIfAlreadyBound()
+    {
+        $this->form->bind(array());
+        $this->form->add($this->getBuilder('foo')->getForm());
+    }
+
     public function testRemove()
     {
         $child = $this->getBuilder('foo')->getForm();
@@ -457,6 +484,16 @@ class FormTest extends \PHPUnit_Framework_TestCase
         $this->assertFalse($this->form->hasChildren());
     }
 
+    /**
+     * @expectedException Symfony\Component\Form\Exception\AlreadyBoundException
+     */
+    public function testRemoveThrowsExceptionIfAlreadyBound()
+    {
+        $this->form->add($this->getBuilder('foo')->getForm());
+        $this->form->bind(array('foo' => 'bar'));
+        $this->form->remove('foo');
+    }
+
     public function testRemoveIgnoresUnknownName()
     {
         $this->form->remove('notexisting');
@@ -504,6 +541,15 @@ class FormTest extends \PHPUnit_Framework_TestCase
         $this->assertFalse($this->form->isBound());
     }
 
+    /**
+     * @expectedException Symfony\Component\Form\Exception\AlreadyBoundException
+     */
+    public function testSetDataThrowsExceptionIfAlreadyBound()
+    {
+        $this->form->bind(array());
+        $this->form->setData(null);
+    }
+
     public function testSetDataExecutesTransformationChain()
     {
         // use real event dispatcher now