From 3eac4693ad8e63283bb715375ab690999717d92b Mon Sep 17 00:00:00 2001 From: Jules Pietri Date: Thu, 11 Feb 2016 09:01:47 +0100 Subject: [PATCH 1/2] [Form] fix choice value "false" in ChoiceType --- .../Form/ChoiceList/ArrayChoiceList.php | 6 +- .../ChoiceToValueTransformer.php | 4 +- .../Tests/ChoiceList/ArrayChoiceListTest.php | 25 +++ .../ChoiceToValueTransformerTest.php | 40 +++-- .../ChoicesToValuesTransformerTest.php | 20 ++- .../Extension/Core/Type/ChoiceTypeTest.php | 152 ++++++++++++++++++ 6 files changed, 230 insertions(+), 17 deletions(-) diff --git a/src/Symfony/Component/Form/ChoiceList/ArrayChoiceList.php b/src/Symfony/Component/Form/ChoiceList/ArrayChoiceList.php index e979455ef3..b89d7b324c 100644 --- a/src/Symfony/Component/Form/ChoiceList/ArrayChoiceList.php +++ b/src/Symfony/Component/Form/ChoiceList/ArrayChoiceList.php @@ -76,7 +76,7 @@ class ArrayChoiceList implements ChoiceListInterface if (null === $value && $this->castableToString($choices)) { $value = function ($choice) { - return (string) $choice; + return false === $choice ? '0' : (string) $choice; }; } @@ -235,11 +235,11 @@ class ArrayChoiceList implements ChoiceListInterface continue; } elseif (!is_scalar($choice)) { return false; - } elseif (isset($cache[(string) $choice])) { + } elseif (isset($cache[$choice])) { return false; } - $cache[(string) $choice] = true; + $cache[$choice] = true; } return true; diff --git a/src/Symfony/Component/Form/Extension/Core/DataTransformer/ChoiceToValueTransformer.php b/src/Symfony/Component/Form/Extension/Core/DataTransformer/ChoiceToValueTransformer.php index ffffddedc3..c7a6655b4e 100644 --- a/src/Symfony/Component/Form/Extension/Core/DataTransformer/ChoiceToValueTransformer.php +++ b/src/Symfony/Component/Form/Extension/Core/DataTransformer/ChoiceToValueTransformer.php @@ -39,8 +39,8 @@ class ChoiceToValueTransformer implements DataTransformerInterface public function reverseTransform($value) { - if (null !== $value && !is_scalar($value)) { - throw new TransformationFailedException('Expected a scalar.'); + if (null !== $value && !is_string($value)) { + throw new TransformationFailedException('Expected a string or null.'); } $choices = $this->choiceList->getChoicesForValues(array((string) $value)); diff --git a/src/Symfony/Component/Form/Tests/ChoiceList/ArrayChoiceListTest.php b/src/Symfony/Component/Form/Tests/ChoiceList/ArrayChoiceListTest.php index 9ea5f631a1..07574013bf 100644 --- a/src/Symfony/Component/Form/Tests/ChoiceList/ArrayChoiceListTest.php +++ b/src/Symfony/Component/Form/Tests/ChoiceList/ArrayChoiceListTest.php @@ -137,4 +137,29 @@ class ArrayChoiceListTest extends AbstractChoiceListTest $this->assertSame(array(0 => null), $choiceList->getChoicesForValues(array('0'))); } + + public function testGetChoicesForValuesWithContainingFalseAndNull() + { + $choiceList = new ArrayChoiceList(array('False' => false, 'Null' => null)); + + $this->assertSame(array(0 => null), $choiceList->getChoicesForValues(array('1'))); + $this->assertSame(array(0 => false), $choiceList->getChoicesForValues(array('0'))); + } + + public function testGetChoicesForValuesWithContainingEmptyStringAndNull() + { + $choiceList = new ArrayChoiceList(array('Empty String' => '', 'Null' => null)); + + $this->assertSame(array(0 => ''), $choiceList->getChoicesForValues(array('0'))); + $this->assertSame(array(0 => null), $choiceList->getChoicesForValues(array('1'))); + } + + public function testGetChoicesForValuesWithContainingEmptyStringAndBooleans() + { + $choiceList = new ArrayChoiceList(array('Empty String' => '', 'True' => true, 'False' => false)); + + $this->assertSame(array(0 => ''), $choiceList->getChoicesForValues(array(''))); + $this->assertSame(array(0 => true), $choiceList->getChoicesForValues(array('1'))); + $this->assertSame(array(0 => false), $choiceList->getChoicesForValues(array('0'))); + } } diff --git a/src/Symfony/Component/Form/Tests/Extension/Core/DataTransformer/ChoiceToValueTransformerTest.php b/src/Symfony/Component/Form/Tests/Extension/Core/DataTransformer/ChoiceToValueTransformerTest.php index f60ef05abc..5362ab9fc7 100644 --- a/src/Symfony/Component/Form/Tests/Extension/Core/DataTransformer/ChoiceToValueTransformerTest.php +++ b/src/Symfony/Component/Form/Tests/Extension/Core/DataTransformer/ChoiceToValueTransformerTest.php @@ -17,34 +17,41 @@ use Symfony\Component\Form\Extension\Core\DataTransformer\ChoiceToValueTransform class ChoiceToValueTransformerTest extends \PHPUnit_Framework_TestCase { protected $transformer; + protected $transformerWithNull; protected function setUp() { - $list = new ArrayChoiceList(array('', false, 'X')); + $list = new ArrayChoiceList(array('', false, 'X', true)); + $listWithNull = new ArrayChoiceList(array('', false, 'X', null)); $this->transformer = new ChoiceToValueTransformer($list); + $this->transformerWithNull = new ChoiceToValueTransformer($listWithNull); } protected function tearDown() { $this->transformer = null; + $this->transformerWithNull = null; } public function transformProvider() { return array( // more extensive test set can be found in FormUtilTest - array('', '0'), - array(false, '1'), + array('', '', '', '0'), + array(false, '0', false, '1'), + array('X', 'X', 'X', '2'), + array(true, '1', null, '3'), ); } /** * @dataProvider transformProvider */ - public function testTransform($in, $out) + public function testTransform($in, $out, $inWithNull, $outWithNull) { $this->assertSame($out, $this->transformer->transform($in)); + $this->assertSame($outWithNull, $this->transformerWithNull->transform($inWithNull)); } public function reverseTransformProvider() @@ -52,25 +59,38 @@ class ChoiceToValueTransformerTest extends \PHPUnit_Framework_TestCase return array( // values are expected to be valid choice keys already and stay // the same - array('0', ''), - array('1', false), - array('2', 'X'), + array('', '', '0', ''), + array('0', false, '1', false), + array('X', 'X', '2', 'X'), + array('1', true, '3', null), ); } /** * @dataProvider reverseTransformProvider */ - public function testReverseTransform($in, $out) + public function testReverseTransform($in, $out, $inWithNull, $outWithNull) { $this->assertSame($out, $this->transformer->reverseTransform($in)); + $this->assertSame($outWithNull, $this->transformerWithNull->reverseTransform($inWithNull)); + } + + public function reverseTransformExpectsStringOrNullProvider() + { + return array( + array(0), + array(true), + array(false), + array(array()), + ); } /** + * @dataProvider reverseTransformExpectsStringOrNullProvider * @expectedException \Symfony\Component\Form\Exception\TransformationFailedException */ - public function testReverseTransformExpectsScalar() + public function testReverseTransformExpectsStringOrNull($value) { - $this->transformer->reverseTransform(array()); + $this->transformer->reverseTransform($value); } } diff --git a/src/Symfony/Component/Form/Tests/Extension/Core/DataTransformer/ChoicesToValuesTransformerTest.php b/src/Symfony/Component/Form/Tests/Extension/Core/DataTransformer/ChoicesToValuesTransformerTest.php index f7747aaccd..d6fe4edd31 100644 --- a/src/Symfony/Component/Form/Tests/Extension/Core/DataTransformer/ChoicesToValuesTransformerTest.php +++ b/src/Symfony/Component/Form/Tests/Extension/Core/DataTransformer/ChoicesToValuesTransformerTest.php @@ -17,24 +17,34 @@ use Symfony\Component\Form\Extension\Core\DataTransformer\ChoicesToValuesTransfo class ChoicesToValuesTransformerTest extends \PHPUnit_Framework_TestCase { protected $transformer; + protected $transformerWithNull; protected function setUp() { $list = new ArrayChoiceList(array('', false, 'X')); + $listWithNull = new ArrayChoiceList(array('', false, 'X', null)); + $this->transformer = new ChoicesToValuesTransformer($list); + $this->transformerWithNull = new ChoicesToValuesTransformer($listWithNull); } protected function tearDown() { $this->transformer = null; + $this->transformerWithNull = null; } public function testTransform() { $in = array('', false, 'X'); - $out = array('0', '1', '2'); + $out = array('', '0', 'X'); $this->assertSame($out, $this->transformer->transform($in)); + + $in[] = null; + $outWithNull = array('0', '1', '2', '3'); + + $this->assertSame($outWithNull, $this->transformerWithNull->transform($in)); } public function testTransformNull() @@ -53,15 +63,21 @@ class ChoicesToValuesTransformerTest extends \PHPUnit_Framework_TestCase public function testReverseTransform() { // values are expected to be valid choices and stay the same - $in = array('0', '1', '2'); + $in = array('', '0', 'X'); $out = array('', false, 'X'); $this->assertSame($out, $this->transformer->reverseTransform($in)); + // values are expected to be valid choices and stay the same + $inWithNull = array('0','1','2','3'); + $out[] = null; + + $this->assertSame($out, $this->transformerWithNull->reverseTransform($inWithNull)); } public function testReverseTransformNull() { $this->assertSame(array(), $this->transformer->reverseTransform(null)); + $this->assertSame(array(), $this->transformerWithNull->reverseTransform(null)); } /** diff --git a/src/Symfony/Component/Form/Tests/Extension/Core/Type/ChoiceTypeTest.php b/src/Symfony/Component/Form/Tests/Extension/Core/Type/ChoiceTypeTest.php index cf74fec212..6128b3dfdd 100644 --- a/src/Symfony/Component/Form/Tests/Extension/Core/Type/ChoiceTypeTest.php +++ b/src/Symfony/Component/Form/Tests/Extension/Core/Type/ChoiceTypeTest.php @@ -26,6 +26,12 @@ class ChoiceTypeTest extends \Symfony\Component\Form\Test\TypeTestCase 'Roman' => 'e', ); + private $scalarChoices = array( + 'Yes' => true, + 'No' => false, + 'n/a' => '', + ); + private $numericChoicesFlipped = array( 0 => 'Bernhard', 1 => 'Fabien', @@ -139,6 +145,58 @@ class ChoiceTypeTest extends \Symfony\Component\Form\Test\TypeTestCase $this->assertCount(count($this->choices), $form, 'Each choice should become a new field'); } + public function testChoiceListWithScalarValues() + { + $view = $this->factory->create('choice', null, array( + 'choices' => $this->scalarChoices, + 'choices_as_values' => true, + ))->createView(); + + $this->assertSame('1', $view->vars['choices'][0]->value); + $this->assertSame('0', $view->vars['choices'][1]->value); + $this->assertSame('', $view->vars['choices'][2]->value); + $this->assertFalse($view->vars['is_selected']($view->vars['choices'][0], $view->vars['value']), 'True value should not be pre selected'); + $this->assertFalse($view->vars['is_selected']($view->vars['choices'][1], $view->vars['value']), 'False value should not be pre selected'); + $this->assertFalse($view->vars['is_selected']($view->vars['choices'][2], $view->vars['value']), 'Empty value should not be pre selected'); + } + + public function testChoiceListWithScalarValuesAndFalseAsPreSetData() + { + $view = $this->factory->create('choice', false, array( + 'choices' => $this->scalarChoices, + 'choices_as_values' => true, + ))->createView(); + + $this->assertTrue($view->vars['is_selected']($view->vars['choices'][1]->value, $view->vars['value']), 'False value should be pre selected'); + } + + public function testExpandedChoiceListWithScalarValues() + { + $view = $this->factory->create('choice', null, array( + 'choices' => $this->scalarChoices, + 'choices_as_values' => true, + 'expanded' => true, + ))->createView(); + + $this->assertFalse($view->children[0]->vars['checked'], 'True value should not be pre selected'); + $this->assertFalse($view->children[1]->vars['checked'], 'False value should not be pre selected'); + $this->assertTrue($view->children[2]->vars['checked'], 'Empty value should be pre selected'); + } + + public function testExpandedChoiceListWithScalarValuesAndFalseAsPreSetData() + { + $view = $this->factory->create('choice', false, array( + 'choices' => $this->scalarChoices, + 'choices_as_values' => true, + 'expanded' => true, + ))->createView(); + + $this->assertSame('1', $view->vars['choices'][0]->value); + $this->assertSame('0', $view->vars['choices'][1]->value); + $this->assertTrue($view->children[1]->vars['checked'], 'False value should be pre selected'); + $this->assertFalse($view->children[2]->vars['checked'], 'Empty value should not be pre selected'); + } + public function testPlaceholderPresentOnNonRequiredExpandedSingleChoice() { $form = $this->factory->create('choice', null, array( @@ -198,6 +256,100 @@ class ChoiceTypeTest extends \Symfony\Component\Form\Test\TypeTestCase $this->assertCount(2, $form, 'Each choice should become a new field'); } + public function testPlaceholderWithBooleanChoices() + { + $form = $this->factory->create('choice', null, array( + 'multiple' => false, + 'expanded' => false, + 'required' => false, + 'choices' => array( + 'Yes' => true, + 'No' => false, + ), + 'placeholder' => 'Select an option', + 'choices_as_values' => true, + )); + + $view = $form->createView(); + + $this->assertSame('', $view->vars['value'], 'Value should be empty'); + $this->assertSame('1', $view->vars['choices'][0]->value); + $this->assertSame('0', $view->vars['choices'][1]->value, 'Choice "false" should have "0" as value'); + $this->assertFalse($view->vars['is_selected']($view->vars['choices'][1]->value, $view->vars['value']), 'Choice "false" should not be selected'); + } + + public function testPlaceholderWithBooleanChoicesWithFalseAsPreSetData() + { + $form = $this->factory->create('choice', false, array( + 'multiple' => false, + 'expanded' => false, + 'required' => false, + 'choices' => array( + 'Yes' => true, + 'No' => false, + ), + 'placeholder' => 'Select an option', + 'choices_as_values' => true, + )); + + $view = $form->createView(); + + $this->assertSame('0', $view->vars['value'], 'Value should be "0"'); + $this->assertSame('1', $view->vars['choices'][0]->value); + $this->assertSame('0', $view->vars['choices'][1]->value, 'Choice "false" should have "0" as value'); + $this->assertTrue($view->vars['is_selected']($view->vars['choices'][1]->value, $view->vars['value']), 'Choice "false" should be selected'); + } + + public function testPlaceholderWithExpandedBooleanChoices() + { + $form = $this->factory->create('choice', null, array( + 'multiple' => false, + 'expanded' => true, + 'required' => false, + 'choices' => array( + 'Yes' => true, + 'No' => false, + ), + 'placeholder' => 'Select an option', + 'choices_as_values' => true, + )); + + $this->assertTrue(isset($form['placeholder']), 'Placeholder should be set'); + $this->assertCount(3, $form, 'Each choice should become a new field, placeholder included'); + + $view = $form->createView(); + + $this->assertSame('', $view->vars['value'], 'Value should be empty'); + $this->assertSame('1', $view->vars['choices'][0]->value); + $this->assertSame('0', $view->vars['choices'][1]->value, 'Choice "false" should have "0" as value'); + $this->assertFalse($view->children[1]->vars['checked'], 'Choice "false" should not be selected'); + } + + public function testPlaceholderWithExpandedBooleanChoicesAndWithFalseAsPreSetData() + { + $form = $this->factory->create('choice', false, array( + 'multiple' => false, + 'expanded' => true, + 'required' => false, + 'choices' => array( + 'Yes' => true, + 'No' => false, + ), + 'placeholder' => 'Select an option', + 'choices_as_values' => true, + )); + + $this->assertTrue(isset($form['placeholder']), 'Placeholder should be set'); + $this->assertCount(3, $form, 'Each choice should become a new field, placeholder included'); + + $view = $form->createView(); + + $this->assertSame('0', $view->vars['value'], 'Value should be "0"'); + $this->assertSame('1', $view->vars['choices'][0]->value); + $this->assertSame('0', $view->vars['choices'][1]->value, 'Choice "false" should have "0" as value'); + $this->assertTrue($view->children[1]->vars['checked'], 'Choice "false" should be selected'); + } + public function testExpandedChoicesOptionsAreFlattened() { $form = $this->factory->create('choice', null, array( From 8f918e5f84c53826d025cbb1f3a0bfe093acc4e0 Mon Sep 17 00:00:00 2001 From: Jules Pietri Date: Fri, 19 Feb 2016 15:39:54 +0100 Subject: [PATCH 2/2] [Form] refactor `RadioListMapper::mapDataToForm()` This fixes "false" choice pre selection when `ChoiceType` is `expanded` and not `multiple` --- .../Form/Extension/Core/DataMapper/RadioListMapper.php | 6 ++---- .../Component/Form/Extension/Core/Type/ChoiceType.php | 8 ++++++++ 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/src/Symfony/Component/Form/Extension/Core/DataMapper/RadioListMapper.php b/src/Symfony/Component/Form/Extension/Core/DataMapper/RadioListMapper.php index 19db183a28..d08a603b5c 100644 --- a/src/Symfony/Component/Form/Extension/Core/DataMapper/RadioListMapper.php +++ b/src/Symfony/Component/Form/Extension/Core/DataMapper/RadioListMapper.php @@ -38,13 +38,11 @@ class RadioListMapper implements DataMapperInterface /** * {@inheritdoc} */ - public function mapDataToForms($choice, $radios) + public function mapDataToForms($data, $radios) { - $valueMap = array_flip($this->choiceList->getValuesForChoices(array($choice))); - foreach ($radios as $radio) { $value = $radio->getConfig()->getOption('value'); - $radio->setData(isset($valueMap[$value]) ? true : false); + $radio->setData($value === $data ? true : false); } } diff --git a/src/Symfony/Component/Form/Extension/Core/Type/ChoiceType.php b/src/Symfony/Component/Form/Extension/Core/Type/ChoiceType.php index 58832ab2f8..ffabf8db46 100644 --- a/src/Symfony/Component/Form/Extension/Core/Type/ChoiceType.php +++ b/src/Symfony/Component/Form/Extension/Core/Type/ChoiceType.php @@ -143,6 +143,14 @@ class ChoiceType extends AbstractType $event->setData(null); } }); + // For radio lists, pre selection of the choice needs to pre set data + // with the string value so it can be matched in + // {@link \Symfony\Component\Form\Extension\Core\DataMapper\RadioListMapper::mapDataToForms()} + $builder->addEventListener(FormEvents::PRE_SET_DATA, function (FormEvent $event) { + $choiceList = $event->getForm()->getConfig()->getOption('choice_list'); + $value = current($choiceList->getValuesForChoices(array($event->getData()))); + $event->setData((string) $value); + }); } } elseif ($options['multiple']) { //