From ea5375c7b78ecb71790475935edfe6855c15dc7d Mon Sep 17 00:00:00 2001 From: Jules Pietri Date: Tue, 15 Mar 2016 17:34:45 +0100 Subject: [PATCH] [Form] refactor CheckboxListMapper and RadioListMapper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit fixes #14712 and #17789. `ChoiceType` now always use `ChoiceToValueTransformer` or `ChoicesToValuesTransformer` depending on `multiple` option. Hence `CheckboxListMapper` and `RadioListMapper` don’t handle the transformation anymore. Fixes pre selection of choice with model values such as `null`, `false` or empty string. --- .../Tests/Form/Type/EntityTypeTest.php | 2 +- src/Symfony/Bridge/Doctrine/composer.json | 2 +- .../Core/DataMapper/CheckboxListMapper.php | 41 ++------- .../Core/DataMapper/RadioListMapper.php | 25 +++--- .../Form/Extension/Core/Type/ChoiceType.php | 27 +++--- .../Extension/Core/Type/ChoiceTypeTest.php | 84 +++++++++++++------ 6 files changed, 88 insertions(+), 93 deletions(-) diff --git a/src/Symfony/Bridge/Doctrine/Tests/Form/Type/EntityTypeTest.php b/src/Symfony/Bridge/Doctrine/Tests/Form/Type/EntityTypeTest.php index 25ead8c557..36bdb1a48f 100644 --- a/src/Symfony/Bridge/Doctrine/Tests/Form/Type/EntityTypeTest.php +++ b/src/Symfony/Bridge/Doctrine/Tests/Form/Type/EntityTypeTest.php @@ -261,7 +261,7 @@ class EntityTypeTest extends TypeTestCase $field->submit(null); $this->assertNull($field->getData()); - $this->assertNull($field->getViewData()); + $this->assertSame('', $field->getViewData(), 'View data is always a string'); } public function testSubmitSingleNonExpandedNull() diff --git a/src/Symfony/Bridge/Doctrine/composer.json b/src/Symfony/Bridge/Doctrine/composer.json index f727880491..4c106a2568 100644 --- a/src/Symfony/Bridge/Doctrine/composer.json +++ b/src/Symfony/Bridge/Doctrine/composer.json @@ -22,7 +22,7 @@ "require-dev": { "symfony/stopwatch": "~2.2", "symfony/dependency-injection": "~2.2", - "symfony/form": "~2.7,>=2.7.1", + "symfony/form": "~2.7.12|~2.8.5", "symfony/http-kernel": "~2.2", "symfony/property-access": "~2.3", "symfony/security": "~2.2", diff --git a/src/Symfony/Component/Form/Extension/Core/DataMapper/CheckboxListMapper.php b/src/Symfony/Component/Form/Extension/Core/DataMapper/CheckboxListMapper.php index b1169ceafa..b85fb0025f 100644 --- a/src/Symfony/Component/Form/Extension/Core/DataMapper/CheckboxListMapper.php +++ b/src/Symfony/Component/Form/Extension/Core/DataMapper/CheckboxListMapper.php @@ -11,9 +11,8 @@ namespace Symfony\Component\Form\Extension\Core\DataMapper; -use Symfony\Component\Form\ChoiceList\ChoiceListInterface; use Symfony\Component\Form\DataMapperInterface; -use Symfony\Component\Form\Exception\TransformationFailedException; +use Symfony\Component\Form\Exception\UnexpectedTypeException; /** * Maps choices to/from checkbox forms. @@ -26,16 +25,6 @@ use Symfony\Component\Form\Exception\TransformationFailedException; */ class CheckboxListMapper implements DataMapperInterface { - /** - * @var ChoiceListInterface - */ - private $choiceList; - - public function __construct(ChoiceListInterface $choiceList) - { - $this->choiceList = $choiceList; - } - /** * {@inheritdoc} */ @@ -46,22 +35,12 @@ class CheckboxListMapper implements DataMapperInterface } if (!is_array($choices)) { - throw new TransformationFailedException('Expected an array.'); - } - - try { - $valueMap = array_flip($this->choiceList->getValuesForChoices($choices)); - } catch (\Exception $e) { - throw new TransformationFailedException( - 'Can not read the choices from the choice list.', - $e->getCode(), - $e - ); + throw new UnexpectedTypeException($choices, 'array'); } foreach ($checkboxes as $checkbox) { $value = $checkbox->getConfig()->getOption('value'); - $checkbox->setData(isset($valueMap[$value]) ? true : false); + $checkbox->setData(in_array($value, $choices, true)); } } @@ -70,6 +49,10 @@ class CheckboxListMapper implements DataMapperInterface */ public function mapFormsToData($checkboxes, &$choices) { + if (!is_array($choices)) { + throw new UnexpectedTypeException($choices, 'array'); + } + $values = array(); foreach ($checkboxes as $checkbox) { @@ -79,14 +62,6 @@ class CheckboxListMapper implements DataMapperInterface } } - try { - $choices = $this->choiceList->getChoicesForValues($values); - } catch (\Exception $e) { - throw new TransformationFailedException( - 'Can not read the values from the choice list.', - $e->getCode(), - $e - ); - } + $choices = $values; } } diff --git a/src/Symfony/Component/Form/Extension/Core/DataMapper/RadioListMapper.php b/src/Symfony/Component/Form/Extension/Core/DataMapper/RadioListMapper.php index 19db183a28..6f757c601f 100644 --- a/src/Symfony/Component/Form/Extension/Core/DataMapper/RadioListMapper.php +++ b/src/Symfony/Component/Form/Extension/Core/DataMapper/RadioListMapper.php @@ -11,8 +11,8 @@ namespace Symfony\Component\Form\Extension\Core\DataMapper; -use Symfony\Component\Form\ChoiceList\ChoiceListInterface; use Symfony\Component\Form\DataMapperInterface; +use Symfony\Component\Form\Exception\UnexpectedTypeException; /** * Maps choices to/from radio forms. @@ -25,26 +25,18 @@ use Symfony\Component\Form\DataMapperInterface; */ class RadioListMapper implements DataMapperInterface { - /** - * @var ChoiceListInterface - */ - private $choiceList; - - public function __construct(ChoiceListInterface $choiceList) - { - $this->choiceList = $choiceList; - } - /** * {@inheritdoc} */ public function mapDataToForms($choice, $radios) { - $valueMap = array_flip($this->choiceList->getValuesForChoices(array($choice))); + if (!is_string($choice)) { + throw new UnexpectedTypeException($choice, 'string'); + } foreach ($radios as $radio) { $value = $radio->getConfig()->getOption('value'); - $radio->setData(isset($valueMap[$value]) ? true : false); + $radio->setData($choice === $value); } } @@ -53,6 +45,10 @@ class RadioListMapper implements DataMapperInterface */ public function mapFormsToData($radios, &$choice) { + if (null !== $choice && !is_string($choice)) { + throw new UnexpectedTypeException($choice, 'null or string'); + } + $choice = null; foreach ($radios as $radio) { @@ -61,8 +57,7 @@ class RadioListMapper implements DataMapperInterface return; } - $value = $radio->getConfig()->getOption('value'); - $choice = current($this->choiceList->getChoicesForValues(array($value))); + $choice = $radio->getConfig()->getOption('value'); return; } diff --git a/src/Symfony/Component/Form/Extension/Core/Type/ChoiceType.php b/src/Symfony/Component/Form/Extension/Core/Type/ChoiceType.php index 29798d037a..a61317de85 100644 --- a/src/Symfony/Component/Form/Extension/Core/Type/ChoiceType.php +++ b/src/Symfony/Component/Form/Extension/Core/Type/ChoiceType.php @@ -60,9 +60,7 @@ class ChoiceType extends AbstractType public function buildForm(FormBuilderInterface $builder, array $options) { if ($options['expanded']) { - $builder->setDataMapper($options['multiple'] - ? new CheckboxListMapper($options['choice_list']) - : new RadioListMapper($options['choice_list'])); + $builder->setDataMapper($options['multiple'] ? new CheckboxListMapper() : new RadioListMapper()); // Initialize all choices before doing the index check below. // This helps in cases where index checks are optimized for non @@ -133,22 +131,13 @@ class ChoiceType extends AbstractType $event->setData($data); }); + } - if (!$options['multiple']) { - // For radio lists, transform empty arrays to null - // This is kind of a hack necessary because the RadioListMapper - // is not invoked for forms without choices - $builder->addEventListener(FormEvents::SUBMIT, function (FormEvent $event) { - if (array() === $event->getData()) { - $event->setData(null); - } - }); - } - } elseif ($options['multiple']) { - // tag with "multiple" option or list of checkbox inputs $builder->addViewTransformer(new ChoicesToValuesTransformer($options['choice_list'])); } else { - // tag without "multiple" option or list of radio inputs $builder->addViewTransformer(new ChoiceToValueTransformer($options['choice_list'])); } @@ -247,7 +236,11 @@ class ChoiceType extends AbstractType $choiceListFactory = $this->choiceListFactory; $emptyData = function (Options $options) { - if ($options['multiple'] || $options['expanded']) { + if ($options['expanded'] && !$options['multiple']) { + return; + } + + if ($options['multiple']) { return array(); } 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 9dc6bfd6b5..a00fcacdb7 100644 --- a/src/Symfony/Component/Form/Tests/Extension/Core/Type/ChoiceTypeTest.php +++ b/src/Symfony/Component/Form/Tests/Extension/Core/Type/ChoiceTypeTest.php @@ -32,6 +32,12 @@ class ChoiceTypeTest extends \Symfony\Component\Form\Test\TypeTestCase 'n/a' => '', ); + private $booleanChoicesWithNull = array( + 'Yes' => true, + 'No' => false, + 'n/a' => null, + ); + private $numericChoicesFlipped = array( 0 => 'Bernhard', 1 => 'Fabien', @@ -183,6 +189,19 @@ class ChoiceTypeTest extends \Symfony\Component\Form\Test\TypeTestCase $this->assertTrue($view->children[2]->vars['checked'], 'Empty value should be pre selected'); } + public function testExpandedChoiceListWithBooleanAndNullValues() + { + $view = $this->factory->create('choice', null, array( + 'choices' => $this->booleanChoicesWithNull, + '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( @@ -197,6 +216,19 @@ class ChoiceTypeTest extends \Symfony\Component\Form\Test\TypeTestCase $this->assertFalse($view->children[2]->vars['checked'], 'Empty value should not be pre selected'); } + public function testExpandedChoiceListWithBooleanAndNullValuesAndFalseAsPreSetData() + { + $view = $this->factory->create('choice', false, array( + 'choices' => $this->booleanChoicesWithNull, + 'choices_as_values' => true, + 'expanded' => true, + ))->createView(); + + $this->assertFalse($view->children[0]->vars['checked'], 'True value should not be pre selected'); + $this->assertTrue($view->children[1]->vars['checked'], 'False value should be pre selected'); + $this->assertFalse($view->children[2]->vars['checked'], 'Null value should not be pre selected'); + } + public function testPlaceholderPresentOnNonRequiredExpandedSingleChoice() { $form = $this->factory->create('choice', null, array( @@ -319,7 +351,7 @@ class ChoiceTypeTest extends \Symfony\Component\Form\Test\TypeTestCase $view = $form->createView(); - $this->assertSame('', $view->vars['value'], 'Value should be empty'); + $this->assertSame('', $view->vars['value'], 'Value should be an empty string'); $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'); @@ -940,8 +972,8 @@ class ChoiceTypeTest extends \Symfony\Component\Form\Test\TypeTestCase $form->submit(null); $this->assertNull($form->getData()); - $this->assertNull($form->getViewData()); - $this->assertEmpty($form->getExtraData()); + $this->assertSame('', $form->getViewData(), 'View data should always be a string'); + $this->assertSame(array(), $form->getExtraData(), 'ChoiceType is compound when expanded, extra data should always be an array'); $this->assertTrue($form->isSynchronized()); $this->assertFalse($form[0]->getData()); @@ -972,8 +1004,8 @@ class ChoiceTypeTest extends \Symfony\Component\Form\Test\TypeTestCase $form->submit(null); $this->assertNull($form->getData()); - $this->assertNull($form->getViewData()); - $this->assertEmpty($form->getExtraData()); + $this->assertSame('', $form->getViewData(), 'View data should always be a string'); + $this->assertSame(array(), $form->getExtraData(), 'ChoiceType is compound when expanded, extra data should always be an array'); $this->assertTrue($form->isSynchronized()); } @@ -990,8 +1022,8 @@ class ChoiceTypeTest extends \Symfony\Component\Form\Test\TypeTestCase $form->submit(''); $this->assertNull($form->getData()); - $this->assertNull($form->getViewData()); - $this->assertEmpty($form->getExtraData()); + $this->assertSame('', $form->getViewData(), 'View data should always be a string'); + $this->assertSame(array(), $form->getExtraData(), 'ChoiceType is compound when expanded, extra data should always be an array'); $this->assertTrue($form->isSynchronized()); $this->assertFalse($form[0]->getData()); @@ -1022,8 +1054,8 @@ class ChoiceTypeTest extends \Symfony\Component\Form\Test\TypeTestCase $form->submit(''); $this->assertNull($form->getData()); - $this->assertNull($form->getViewData()); - $this->assertEmpty($form->getExtraData()); + $this->assertSame('', $form->getViewData(), 'View data should always be a string'); + $this->assertSame(array(), $form->getExtraData(), 'ChoiceType is compound when expanded, extra data should always be an array'); $this->assertTrue($form->isSynchronized()); } @@ -1040,8 +1072,8 @@ class ChoiceTypeTest extends \Symfony\Component\Form\Test\TypeTestCase $form->submit(false); $this->assertNull($form->getData()); - $this->assertNull($form->getViewData()); - $this->assertEmpty($form->getExtraData()); + $this->assertSame('', $form->getViewData(), 'View data should always be a string'); + $this->assertSame(array(), $form->getExtraData(), 'ChoiceType is compound when expanded, extra data should always be an array'); $this->assertTrue($form->isSynchronized()); $this->assertFalse($form[0]->getData()); @@ -1072,8 +1104,8 @@ class ChoiceTypeTest extends \Symfony\Component\Form\Test\TypeTestCase $form->submit(false); $this->assertNull($form->getData()); - $this->assertNull($form->getViewData()); - $this->assertEmpty($form->getExtraData()); + $this->assertSame('', $form->getViewData(), 'View data should always be a string'); + $this->assertSame(array(), $form->getExtraData(), 'ChoiceType is compound when expanded, extra data should always be an array'); $this->assertTrue($form->isSynchronized()); } @@ -1090,8 +1122,8 @@ class ChoiceTypeTest extends \Symfony\Component\Form\Test\TypeTestCase $form->submit(null); $this->assertNull($form->getData()); - $this->assertNull($form->getViewData()); - $this->assertEmpty($form->getExtraData()); + $this->assertSame('', $form->getViewData(), 'View data should always be a string'); + $this->assertSame(array(), $form->getExtraData(), 'ChoiceType is compound when expanded, extra data should always be an array'); $this->assertTrue($form->isSynchronized()); $this->assertTrue($form['placeholder']->getData()); @@ -1124,8 +1156,8 @@ class ChoiceTypeTest extends \Symfony\Component\Form\Test\TypeTestCase $form->submit(null); $this->assertNull($form->getData()); - $this->assertNull($form->getViewData()); - $this->assertEmpty($form->getExtraData()); + $this->assertSame('', $form->getViewData(), 'View data should always be a string'); + $this->assertSame(array(), $form->getExtraData(), 'ChoiceType is compound when expanded, extra data should always be an array'); $this->assertTrue($form->isSynchronized()); } @@ -1142,8 +1174,8 @@ class ChoiceTypeTest extends \Symfony\Component\Form\Test\TypeTestCase $form->submit(''); $this->assertNull($form->getData()); - $this->assertNull($form->getViewData()); - $this->assertEmpty($form->getExtraData()); + $this->assertSame('', $form->getViewData(), 'View data should always be a string'); + $this->assertSame(array(), $form->getExtraData(), 'ChoiceType is compound when expanded, extra data should always be an array'); $this->assertTrue($form->isSynchronized()); $this->assertTrue($form['placeholder']->getData()); @@ -1176,8 +1208,8 @@ class ChoiceTypeTest extends \Symfony\Component\Form\Test\TypeTestCase $form->submit(''); $this->assertNull($form->getData()); - $this->assertNull($form->getViewData()); - $this->assertEmpty($form->getExtraData()); + $this->assertSame('', $form->getViewData(), 'View data should always be a string'); + $this->assertSame(array(), $form->getExtraData(), 'ChoiceType is compound when expanded, extra data should always be an array'); $this->assertTrue($form->isSynchronized()); } @@ -1194,8 +1226,8 @@ class ChoiceTypeTest extends \Symfony\Component\Form\Test\TypeTestCase $form->submit(false); $this->assertNull($form->getData()); - $this->assertNull($form->getViewData()); - $this->assertEmpty($form->getExtraData()); + $this->assertSame('', $form->getViewData(), 'View data should always be a string'); + $this->assertSame(array(), $form->getExtraData(), 'ChoiceType is compound when expanded, extra data should always be an array'); $this->assertTrue($form->isSynchronized()); $this->assertTrue($form['placeholder']->getData()); @@ -1228,8 +1260,8 @@ class ChoiceTypeTest extends \Symfony\Component\Form\Test\TypeTestCase $form->submit(false); $this->assertNull($form->getData()); - $this->assertNull($form->getViewData()); - $this->assertEmpty($form->getExtraData()); + $this->assertSame('', $form->getViewData(), 'View data should always be a string'); + $this->assertSame(array(), $form->getExtraData(), 'ChoiceType is compound when expanded, extra data should always be an array'); $this->assertTrue($form->isSynchronized()); } @@ -1247,7 +1279,7 @@ class ChoiceTypeTest extends \Symfony\Component\Form\Test\TypeTestCase $form->submit(''); - $this->assertNull($form->getData()); + $this->assertSame('', $form->getData()); $this->assertTrue($form->isSynchronized()); $this->assertTrue($form[0]->getData());