Skip to content

Commit

Permalink
[Form] Fixed expanded choice field to be marked invalid when unknown …
Browse files Browse the repository at this point in the history 
…choices are submitted
  • Loading branch information
@webmozart
webmozart committed Sep 10, 2013
1 parent 30aa1de commit ed83752
Show file tree
Hide file tree
Showing 8 changed files with 539 additions and 84 deletions.
6 changes: 1 addition & 5 deletions src/Symfony/Component/Form/Extension/Core/DataTransformer/BooleanToStringTransformer.php
View file
Expand Up @@ -49,15 +49,11 @@ public function __construct($trueValue)
*/
public function transform($value)
{
if (null === $value) {

This comment has been minimized.

Sign in to view

Copy link
@rvanlaak

rvanlaak Sep 13, 2013

Contributor

Since this check has been removed, I get the TransformationFailedException from line 53 at every formField for which no default value is provided. In other words (correct me if I am wrong), now I have to always pass a default value to checkbox.

Shouldn't the transformer also handle null values?

This comment has been minimized.

Sign in to view

Copy link
@stof

stof Sep 13, 2013

Member

This has already been fixed in a later commit: 2747bdc
return null;
}

if (!is_bool($value)) {
throw new TransformationFailedException('Expected a Boolean.');
}

return true === $value ? $this->trueValue : null;
return $value ? $this->trueValue : null;
}

/**
Expand Down
40 changes: 37 additions & 3 deletions src/Symfony/Component/Form/Extension/Core/EventListener/FixCheckboxInputListener.php
View file
Expand Up @@ -11,6 +11,7 @@

namespace Symfony\Component\Form\Extension\Core\EventListener;

use Symfony\Component\Form\Exception\TransformationFailedException;
use Symfony\Component\Form\FormEvents;
use Symfony\Component\Form\FormEvent;
use Symfony\Component\EventDispatcher\EventSubscriberInterface;
Expand Down Expand Up @@ -38,10 +39,43 @@ public function __construct(ChoiceListInterface $choiceList)

public function preBind(FormEvent $event)
{
$values = (array) $event->getData();
$indices = $this->choiceList->getIndicesForValues($values);
$data = $event->getData();

$event->setData(count($indices) > 0 ? array_combine($indices, $values) : array());
if (is_array($data)) {
// Flip the submitted values for faster lookup
// It's better to flip this array than $existingValues because
// $submittedValues is generally smaller.
$submittedValues = array_flip($data);

// Since expanded choice fields are completely loaded anyway, we
// can just as well get the values again without losing performance.
$existingValues = $this->choiceList->getValues();

// Clear the data array and fill it with correct indices
$data = array();

foreach ($existingValues as $index => $value) {
if (isset($submittedValues[$value])) {
// Value was submitted
$data[$index] = $value;
unset($submittedValues[$value]);
}
}

if (count($submittedValues) > 0) {
throw new TransformationFailedException(sprintf(
'The following choices were not found: "%s"',
implode('", "', array_keys($submittedValues))
));
}
} elseif ('' === $data || null === $data) {
// Empty values are always accepted.
$data = array();
}

// Else leave the data unchanged to provoke an error during submission

$event->setData($data);
}

public static function getSubscribedEvents()
Expand Down
18 changes: 15 additions & 3 deletions src/Symfony/Component/Form/Extension/Core/EventListener/FixRadioInputListener.php
View file
Expand Up @@ -38,10 +38,22 @@ public function __construct(ChoiceListInterface $choiceList)

public function preBind(FormEvent $event)
{
$value = $event->getData();
$index = current($this->choiceList->getIndicesForValues(array($value)));
$data = $event->getData();

$event->setData(false !== $index ? array($index => $value) : array());
// Since expanded choice fields are completely loaded anyway, we
// can just as well get the values again without losing performance.
$existingValues = $this->choiceList->getValues();

if (false !== ($index = array_search($data, $existingValues, true))) {
$data = array($index => $data);
} elseif ('' === $data || null === $data) {
// Empty values are always accepted.
$data = array();
}

// Else leave the data unchanged to provoke an error during submission

$event->setData($data);
}

public static function getSubscribedEvents()
Expand Down
15 changes: 10 additions & 5 deletions src/Symfony/Component/Form/Extension/Core/Type/CheckboxType.php
View file
Expand Up @@ -25,9 +25,14 @@ class CheckboxType extends AbstractType
*/
public function buildForm(FormBuilderInterface $builder, array $options)
{
$builder
->addViewTransformer(new BooleanToStringTransformer($options['value']))
;
// Unlike in other types, where the data is NULL by default, it
// needs to be a Boolean here. setData(null) is not acceptable
// for checkboxes and radio buttons (unless a custom model
// transformer handles this case).
// We cannot solve this case via overriding the "data" option, because
// doing so also calls setDataLocked(true).
$builder->setData(isset($options['data']) ? $options['data'] : false);

This comment has been minimized.

Sign in to view

Copy link
@vlastv

vlastv Sep 17, 2013

Contributor

This is bad idea.
If checkbox in HTML has attribute disabled, then Browser not send this field that equivalent NULL

In Django is also realized in version 1.6 this will be fixed.

https://docs.djangoproject.com/en/dev/releases/1.6/#booleanfield-no-longer-defaults-to-false

This comment has been minimized.

Sign in to view

Copy link
@rvanlaak

rvanlaak Sep 17, 2013

Contributor

I also agree on this. Aside of the html specification this also means that a default value should be passed to every checkbox. For other fields (like textfield) it is not needed to give a default value, so this also shouldn't be the case for a checkbox. :)

This comment has been minimized.

Sign in to view

Copy link
@webmozart

webmozart Sep 17, 2013

Author Contributor

@vlastv When a field is disabled, it will be ignored by the form engine. So I don't think this should be a problem, no?

@rvanlaak No, as you can see in the code above, the default will be set to false if $options['data'] is null (the default).

This comment has been minimized.

Sign in to view

Copy link
@rvanlaak

rvanlaak Sep 17, 2013

Contributor

You are right, but as mentioned earlier the transformer throws an exception if the null value is passed.

ed83752#commitcomment-4088288

That actually already was fixed in a later commit, but my concern goes to the future implementation. Always having to give a default value is not the correct way right?

This comment has been minimized.

Sign in to view

Copy link
@webmozart

webmozart Sep 17, 2013

Author Contributor

I can't tell you much about the "future implementation" (not sure what you mean by that) :) The implementation we have here seems good. If the default is null or false, the checkbox is unchecked, otherwise it is checked.

This comment has been minimized.

Sign in to view

Copy link
@vlastv

vlastv Sep 18, 2013

Contributor

@bschussek I create very simple test

$formFactory = $this->getContainer()->get('form.factory');
$form = $formFactory->createNamedBuilder('form', 'form', null, array('csrf_protection' => false))
    ->add('text', 'text')
    ->add('checkbox', 'checkbox')
    ->getForm();

$request = new Request(array(), array('form' => array('text' => 'text')));
$request
    ->setMethod('POST')
    ;

$form->handleRequest($request);

if ($form->isValid()) {
    var_dump($form->getData());
}

/* Output
array(2) {
  ["text"]=>
  string(4) "text"
  ["checkbox"]=>
  bool(false)
}
*/

Why checkbox is False, if it was not transmitted to the POST request? Must be Null

This comment has been minimized.

Sign in to view

Copy link
@stof

stof Sep 18, 2013

Member

@vlastv Please read about the form-urlencoded serialization of form data for checkboxes:

  • when they are checked, they appear in the submitted data, and the value is the one set in the input (which may even be an empty string IIRC)
  • when they are not checked, they don't appear in the submitted data.

So your testcase is exactly what happens when submitting an unchecked checkbox, which means setting it as false is intended.

This comment has been minimized.

Sign in to view

Copy link
@vlastv

vlastv Sep 18, 2013

Contributor

@stof Since you can not argue :) We will wait for the reason that in Django came from False to Null
$builder->addViewTransformer(new BooleanToStringTransformer($options['value']));
}

/**
Expand All @@ -46,8 +51,8 @@ public function buildView(FormView $view, FormInterface $form, array $options)
*/
public function setDefaultOptions(OptionsResolverInterface $resolver)
{
$emptyData = function (FormInterface $form, $clientData) {
return $clientData;
$emptyData = function (FormInterface $form, $viewData) {
return $viewData;
};

$resolver->setDefaults(array(
Expand Down
109 changes: 60 additions & 49 deletions src/Symfony/Component/Form/Form.php
View file
Expand Up @@ -531,68 +531,73 @@ public function bind($submittedData)

$dispatcher = $this->config->getEventDispatcher();

// Hook to change content of the data bound by the browser
if ($dispatcher->hasListeners(FormEvents::PRE_BIND) || $dispatcher->hasListeners(FormEvents::BIND_CLIENT_DATA)) {
$event = new FormEvent($this, $submittedData);
$dispatcher->dispatch(FormEvents::PRE_BIND, $event);
// BC until 2.3
if ($dispatcher->hasListeners(FormEvents::BIND_CLIENT_DATA)) {
trigger_error('The FormEvents::BIND_CLIENT_DATA event is deprecated since 2.1 and will be removed in 2.3. Use the FormEvents::PRE_BIND event instead.', E_USER_DEPRECATED);
}
$dispatcher->dispatch(FormEvents::BIND_CLIENT_DATA, $event);
$submittedData = $event->getData();
}
$modelData = null;
$normData = null;
$viewData = null;

// Check whether the form is compound.
// This check is preferable over checking the number of children,
// since forms without children may also be compound.
// (think of empty collection forms)
if ($this->config->getCompound()) {
if (!is_array($submittedData)) {
$submittedData = array();
try {
// Hook to change content of the data bound by the browser
if ($dispatcher->hasListeners(FormEvents::PRE_BIND) || $dispatcher->hasListeners(FormEvents::BIND_CLIENT_DATA)) {
$event = new FormEvent($this, $submittedData);
$dispatcher->dispatch(FormEvents::PRE_BIND, $event);
// BC until 2.3
if ($dispatcher->hasListeners(FormEvents::BIND_CLIENT_DATA)) {
trigger_error('The FormEvents::BIND_CLIENT_DATA event is deprecated since 2.1 and will be removed in 2.3. Use the FormEvents::PRE_BIND event instead.', E_USER_DEPRECATED);
}
$dispatcher->dispatch(FormEvents::BIND_CLIENT_DATA, $event);
$submittedData = $event->getData();
}

for (reset($this->children); false !== current($this->children); next($this->children)) {
$child = current($this->children);
$name = key($this->children);
// Check whether the form is compound.
// This check is preferable over checking the number of children,
// since forms without children may also be compound.
// (think of empty collection forms)
if ($this->config->getCompound()) {
if (null === $submittedData) {
$submittedData = array();
}

$child->bind(isset($submittedData[$name]) ? $submittedData[$name] : null);
unset($submittedData[$name]);
}
if (!is_array($submittedData)) {
throw new TransformationFailedException('Compound forms expect an array or NULL on submission.');
}

$this->extraData = $submittedData;
for (reset($this->children); false !== current($this->children); next($this->children)) {
$child = current($this->children);
$name = key($this->children);

// If the form is compound, the default data in view format
// is reused. The data of the children is merged into this
// default data using the data mapper.
$viewData = $this->viewData;
} else {
// If the form is not compound, the submitted data is also the data in view format.
$viewData = $submittedData;
}
$child->bind(isset($submittedData[$name]) ? $submittedData[$name] : null);
unset($submittedData[$name]);
}

if (FormUtil::isEmpty($viewData)) {
$emptyData = $this->config->getEmptyData();
$this->extraData = $submittedData;

if ($emptyData instanceof \Closure) {
/* @var \Closure $emptyData */
$emptyData = $emptyData($this, $viewData);
// If the form is compound, the default data in view format
// is reused. The data of the children is merged into this
// default data using the data mapper.
$viewData = $this->viewData;
} else {
// If the form is not compound, the submitted data is also the data in view format.
$viewData = $submittedData;
}

$viewData = $emptyData;
}
if (FormUtil::isEmpty($viewData)) {
$emptyData = $this->config->getEmptyData();

// Merge form data from children into existing view data
// 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);
}
if ($emptyData instanceof \Closure) {
/* @var \Closure $emptyData */
$emptyData = $emptyData($this, $viewData);
}

$modelData = null;
$normData = null;
$viewData = $emptyData;
}

// Merge form data from children into existing view data
// 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);
}

try {
// Normalize data to unified representation
$normData = $this->viewToNorm($viewData);

Expand All @@ -614,6 +619,12 @@ public function bind($submittedData)
$viewData = $this->normToView($normData);
} catch (TransformationFailedException $e) {
$this->synchronized = false;

// If $viewData was not yet set, set it to $submittedData so that
// the erroneous data is accessible on the form.
if (null === $viewData) {
$viewData = $submittedData;
}
}

$this->bound = true;
Expand Down
24 changes: 18 additions & 6 deletions ...ny/Component/Form/Tests/Extension/Core/DataTransformer/BooleanToStringTransformerTest.php
View file
Expand Up @@ -17,6 +17,9 @@ class BooleanToStringTransformerTest extends \PHPUnit_Framework_TestCase
{
const TRUE_VALUE = '1';

/**
* @var BooleanToStringTransformer
*/
protected $transformer;

protected function setUp()
Expand All @@ -33,20 +36,29 @@ public function testTransform()
{
$this->assertEquals(self::TRUE_VALUE, $this->transformer->transform(true));
$this->assertNull($this->transformer->transform(false));
$this->assertNull($this->transformer->transform(null));
}

public function testTransformExpectsBoolean()
/**
* @expectedException \Symfony\Component\Form\Exception\TransformationFailedException
*/
public function testTransformFailsIfNull()
{
$this->setExpectedException('Symfony\Component\Form\Exception\TransformationFailedException');
$this->transformer->transform(null);
}

/**
* @expectedException \Symfony\Component\Form\Exception\TransformationFailedException
*/
public function testTransformFailsIfString()
{
$this->transformer->transform('1');
}

public function testReverseTransformExpectsString()
/**
* @expectedException \Symfony\Component\Form\Exception\TransformationFailedException
*/
public function testReverseTransformFailsIfInteger()
{
$this->setExpectedException('Symfony\Component\Form\Exception\TransformationFailedException');

$this->transformer->reverseTransform(1);
}

Expand Down
31 changes: 21 additions & 10 deletions src/Symfony/Component/Form/Tests/Extension/Core/Type/CheckboxTypeTest.php
View file
Expand Up @@ -15,6 +15,15 @@

class CheckboxTypeTest extends TypeTestCase
{
public function testDataIsFalseByDefault()
{
$form = $this->factory->create('checkbox');

$this->assertFalse($form->getData());
$this->assertFalse($form->getNormData());
$this->assertNull($form->getViewData());
}

public function testPassValueToView()
{
$form = $this->factory->create('checkbox', null, array('value' => 'foobar'));
Expand Down Expand Up @@ -106,35 +115,37 @@ public function testBindWithEmptyValueUnchecked()
}

/**
* @dataProvider provideTransformedData
* @dataProvider provideCustomModelTransformerData
*/
public function testTransformedData($data, $expected)
public function testCustomModelTransformer($data, $checked)
{
// present a binary status field as a checkbox
$transformer = new CallbackTransformer(
function ($value) {
return 'expedited' == $value;
return 'checked' == $value;
},
function ($value) {
return $value ? 'expedited' : 'standard';
return $value ? 'checked' : 'unchecked';
}
);

$form = $this->builder
->create('expedited_shipping', 'checkbox')
$form = $this->factory->createBuilder('checkbox')
->addModelTransformer($transformer)
->getForm();

$form->setData($data);
$view = $form->createView();

$this->assertEquals($expected, $view->vars['checked']);
$this->assertSame($data, $form->getData());
$this->assertSame($checked, $form->getNormData());
$this->assertEquals($checked, $view->vars['checked']);
}

public function provideTransformedData()
public function provideCustomModelTransformerData()
{
return array(
array('expedited', true),
array('standard', false),
array('checked', true),
array('unchecked', false),
);
}
}

0 comments on commit ed83752

Please sign in to comment.