Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 11, 2012
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG-2.1.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
8 changes: 8 additions & 0 deletions UPGRADE-2.1.md
Original file line number Diff line number Diff line change
Expand Up @@ -248,3 +248,11 @@ UPGRADE FROM 2.0 to 2.1
{
return isset($options['widget']) && 'single_text' === $options['widget'] ? 'text' : 'choice';
}

* 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.
Original file line number Diff line number Diff line change
@@ -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));
}
}
}
13 changes: 3 additions & 10 deletions src/Symfony/Component/Form/Extension/Csrf/Type/CsrfType.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -46,18 +48,9 @@ public function buildForm(FormBuilder $builder, array $options)
$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))
;
}

Expand Down
23 changes: 22 additions & 1 deletion src/Symfony/Component/Form/Form.php
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
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;
Expand Down Expand Up @@ -297,6 +298,10 @@ public function isDisabled()
*/
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.');
}
Expand Down Expand Up @@ -377,6 +382,10 @@ public function getAttribute($name)
*/
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);

Expand Down Expand Up @@ -451,6 +460,10 @@ public function getExtraData()
*/
public function bind($clientData)
{
if ($this->bound) {
throw new AlreadyBoundException('A form can only be bound once');
}

if ($this->isDisabled()) {
$this->bound = true;

Expand Down Expand Up @@ -689,7 +702,7 @@ public function isEmpty()
*/
public function isValid()
{
if (!$this->isBound()) {
if (!$this->bound) {
throw new \LogicException('You cannot call isValid() on a form that is not bound.');
}

Expand Down Expand Up @@ -821,6 +834,10 @@ public function hasChildren()
*/
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);
Expand All @@ -841,6 +858,10 @@ public function add(FormInterface $child)
*/
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);

Expand Down
48 changes: 47 additions & 1 deletion tests/Symfony/Tests/Component/Form/FormTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,15 @@ public function testBind()
$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');
Expand Down Expand Up @@ -408,8 +417,8 @@ public function testNotValidIfChildNotValid()
->method('isValid')
->will($this->returnValue(false));

$this->form->bind('foobar');
$this->form->add($child);
$this->form->bind(array());

$this->assertFalse($this->form->isValid());
}
Expand Down Expand Up @@ -438,6 +447,15 @@ public function testHasNoChildren()
$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();
Expand All @@ -447,6 +465,15 @@ public function testAdd()
$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();
Expand All @@ -457,6 +484,16 @@ public function testRemove()
$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');
Expand Down Expand Up @@ -504,6 +541,15 @@ public function testNotBound()
$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
Expand Down