bug #17760 [2.7] [Form] fix choice value "false" in ChoiceType (HeahDude)

This PR was merged into the 2.7 branch.

Discussion
----------

[2.7] [Form] fix choice value "false" in ChoiceType

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #17292, #14712, #17789
| License       | MIT
| Doc PR        | -

- [x] Add tests for choices with `boolean` and `null` values, and with a placeholder
- [x] Fix FQCN in 2.8 tests, see #17759
- [x] Remove `choices_as_values` in 3.0 tests, see #17886

Commits
-------

8f918e5 [Form] refactor `RadioListMapper::mapDataToForm()`
3eac469 [Form] fix choice value "false" in ChoiceType

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;

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);
         }
     }
 

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)) {
+        if (null !== $value && !is_string($value)) {
-            throw new TransformationFailedException('Expected a scalar.');
+            throw new TransformationFailedException('Expected a string or null.');
         }
 
         $choices = $this->choiceList->getChoicesForValues(array((string) $value));

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']) {
             // <select> tag with "multiple" option

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')));
+    }
 }

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('', '', '', '0'),
-            array(false, '1'),
+            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('', '', '0', ''),
-            array('1', false),
+            array('0', false, '1', false),
-            array('2', 'X'),
+            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);
     }
 }

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));
     }
 
     /**

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(