diff --git a/src/Symfony/Component/Form/CHANGELOG.md b/src/Symfony/Component/Form/CHANGELOG.md index 6cc800d0af..a696c7be9a 100644 --- a/src/Symfony/Component/Form/CHANGELOG.md +++ b/src/Symfony/Component/Form/CHANGELOG.md @@ -33,6 +33,7 @@ CHANGELOG unless FormInterface::setData() is called manually * fixed CSRF error message to be translated * custom CSRF error messages can now be set through the "csrf_message" option + * fixed: expanded single-choice fields now show a radio button for the empty value 2.2.0 ----- diff --git a/src/Symfony/Component/Form/Extension/Core/ChoiceList/ChoiceListInterface.php b/src/Symfony/Component/Form/Extension/Core/ChoiceList/ChoiceListInterface.php index c81c560f4f..e0ba818b18 100644 --- a/src/Symfony/Component/Form/Extension/Core/ChoiceList/ChoiceListInterface.php +++ b/src/Symfony/Component/Form/Extension/Core/ChoiceList/ChoiceListInterface.php @@ -95,6 +95,8 @@ interface ChoiceListInterface /** * Returns the choices corresponding to the given values. * + * The choices can have any data type. + * * @param array $values An array of choice values. Not existing values in * this array are ignored. * @@ -105,6 +107,8 @@ interface ChoiceListInterface /** * Returns the values corresponding to the given choices. * + * The values must be a strings. + * * @param array $choices An array of choices. Not existing choices in this * array are ignored. * @@ -116,20 +120,30 @@ interface ChoiceListInterface /** * Returns the indices corresponding to the given choices. * + * The indices must be positive integers or strings accepted by + * {@link FormConfigBuilder::validateName()}. + * + * The index "placeholder" is internally reserved. + * * @param array $choices An array of choices. Not existing choices in this * array are ignored. * - * @return array An array of indices with ascending, 0-based numeric keys + * @return array An array of indices with ascending, 0-based numeric keys. */ public function getIndicesForChoices(array $choices); /** * Returns the indices corresponding to the given values. * + * The indices must be positive integers or strings accepted by + * {@link FormConfigBuilder::validateName()}. + * + * The index "placeholder" is internally reserved. + * * @param array $values An array of choice values. Not existing values in * this array are ignored. * - * @return array An array of indices with ascending, 0-based numeric keys + * @return array An array of indices with ascending, 0-based numeric keys. */ public function getIndicesForValues(array $values); } diff --git a/src/Symfony/Component/Form/Extension/Core/DataTransformer/ChoiceToBooleanArrayTransformer.php b/src/Symfony/Component/Form/Extension/Core/DataTransformer/ChoiceToBooleanArrayTransformer.php index 59fe9dbcea..edd58e144d 100644 --- a/src/Symfony/Component/Form/Extension/Core/DataTransformer/ChoiceToBooleanArrayTransformer.php +++ b/src/Symfony/Component/Form/Extension/Core/DataTransformer/ChoiceToBooleanArrayTransformer.php @@ -23,14 +23,18 @@ class ChoiceToBooleanArrayTransformer implements DataTransformerInterface { private $choiceList; + private $placeholderPresent; + /** * Constructor. * * @param ChoiceListInterface $choiceList + * @param Boolean $placeholderPresent */ - public function __construct(ChoiceListInterface $choiceList) + public function __construct(ChoiceListInterface $choiceList, $placeholderPresent) { $this->choiceList = $choiceList; + $this->placeholderPresent = $placeholderPresent; } /** @@ -63,6 +67,10 @@ class ChoiceToBooleanArrayTransformer implements DataTransformerInterface $values[$i] = $i === $index; } + if ($this->placeholderPresent) { + $values['placeholder'] = false === $index; + } + return $values; } @@ -97,6 +105,8 @@ class ChoiceToBooleanArrayTransformer implements DataTransformerInterface if ($selected) { if (isset($choices[$i])) { return $choices[$i] === '' ? null : $choices[$i]; + } elseif ($this->placeholderPresent && 'placeholder' === $i) { + return null; } else { throw new TransformationFailedException(sprintf('The choice "%s" does not exist', $i)); } diff --git a/src/Symfony/Component/Form/Extension/Core/EventListener/FixRadioInputListener.php b/src/Symfony/Component/Form/Extension/Core/EventListener/FixRadioInputListener.php index b045ca8fd1..bf03792fed 100644 --- a/src/Symfony/Component/Form/Extension/Core/EventListener/FixRadioInputListener.php +++ b/src/Symfony/Component/Form/Extension/Core/EventListener/FixRadioInputListener.php @@ -26,14 +26,18 @@ class FixRadioInputListener implements EventSubscriberInterface { private $choiceList; + private $placeholderPresent; + /** * Constructor. * * @param ChoiceListInterface $choiceList + * @param Boolean $placeholderPresent */ - public function __construct(ChoiceListInterface $choiceList) + public function __construct(ChoiceListInterface $choiceList, $placeholderPresent) { $this->choiceList = $choiceList; + $this->placeholderPresent = $placeholderPresent; } public function preSubmit(FormEvent $event) @@ -41,7 +45,7 @@ class FixRadioInputListener implements EventSubscriberInterface $value = $event->getData(); $index = current($this->choiceList->getIndicesForValues(array($value))); - $event->setData(false !== $index ? array($index => $value) : array()); + $event->setData(false !== $index ? array($index => $value) : ($this->placeholderPresent ? array('placeholder' => '') : array())) ; } /** diff --git a/src/Symfony/Component/Form/Extension/Core/Type/ChoiceType.php b/src/Symfony/Component/Form/Extension/Core/Type/ChoiceType.php index 446a74dd6e..9a3fdef12b 100644 --- a/src/Symfony/Component/Form/Extension/Core/Type/ChoiceType.php +++ b/src/Symfony/Component/Form/Extension/Core/Type/ChoiceType.php @@ -12,6 +12,7 @@ namespace Symfony\Component\Form\Extension\Core\Type; use Symfony\Component\Form\AbstractType; +use Symfony\Component\Form\Extension\Core\View\ChoiceView; use Symfony\Component\Form\FormBuilderInterface; use Symfony\Component\Form\FormInterface; use Symfony\Component\Form\FormView; @@ -45,19 +46,34 @@ class ChoiceType extends AbstractType } if ($options['expanded']) { - $this->addSubForms($builder, $options['choice_list']->getPreferredViews(), $options); - $this->addSubForms($builder, $options['choice_list']->getRemainingViews(), $options); + // Initialize all choices before doing the index check below. + // This helps in cases where index checks are optimized for non + // initialized choice lists. For example, when using an SQL driver, + // the index check would read in one SQL query and the initialization + // requires another SQL query. When the initialization is done first, + // one SQL query is sufficient. + $preferredViews = $options['choice_list']->getPreferredViews(); + $remainingViews = $options['choice_list']->getRemainingViews(); + + // Check if the choices already contain the empty value + // Only add the empty value option if this is not the case + if (null !== $options['empty_value'] && 0 === count($options['choice_list']->getIndicesForValues(array('')))) { + $placeholderView = new ChoiceView(null, '', $options['empty_value']); + + // "placeholder" is a reserved index + // see also ChoiceListInterface::getIndicesForChoices() + $this->addSubForms($builder, array('placeholder' => $placeholderView), $options); + } + + $this->addSubForms($builder, $preferredViews, $options); + $this->addSubForms($builder, $remainingViews, $options); if ($options['multiple']) { - $builder - ->addViewTransformer(new ChoicesToBooleanArrayTransformer($options['choice_list'])) - ->addEventSubscriber(new FixCheckboxInputListener($options['choice_list']), 10) - ; + $builder->addViewTransformer(new ChoicesToBooleanArrayTransformer($options['choice_list'])); + $builder->addEventSubscriber(new FixCheckboxInputListener($options['choice_list']), 10); } else { - $builder - ->addViewTransformer(new ChoiceToBooleanArrayTransformer($options['choice_list'])) - ->addEventSubscriber(new FixRadioInputListener($options['choice_list']), 10) - ; + $builder->addViewTransformer(new ChoiceToBooleanArrayTransformer($options['choice_list'], $builder->has('placeholder'))); + $builder->addEventSubscriber(new FixRadioInputListener($options['choice_list'], $builder->has('placeholder')), 10); } } else { if ($options['multiple']) { @@ -104,7 +120,7 @@ class ChoiceType extends AbstractType // Check if the choices already contain the empty value // Only add the empty value option if this is not the case - if (0 === count($options['choice_list']->getIndicesForValues(array('')))) { + if (null !== $options['empty_value'] && 0 === count($options['choice_list']->getIndicesForValues(array('')))) { $view->vars['empty_value'] = $options['empty_value']; } @@ -170,12 +186,15 @@ class ChoiceType extends AbstractType }; $emptyValueNormalizer = function (Options $options, $emptyValue) { - if ($options['multiple'] || $options['expanded']) { - // never use an empty value for these cases + if ($options['multiple']) { + // never use an empty value for this case return null; } elseif (false === $emptyValue) { // an empty value should be added but the user decided otherwise return null; + } elseif ($options['expanded'] && '' === $emptyValue) { + // never use an empty label for radio buttons + return 'None'; } // empty value has been set explicitly diff --git a/src/Symfony/Component/Form/Tests/AbstractLayoutTest.php b/src/Symfony/Component/Form/Tests/AbstractLayoutTest.php index 1a48fae194..8f632a2b50 100644 --- a/src/Symfony/Component/Form/Tests/AbstractLayoutTest.php +++ b/src/Symfony/Component/Form/Tests/AbstractLayoutTest.php @@ -634,7 +634,7 @@ abstract class AbstractLayoutTest extends \Symfony\Component\Form\Test\FormInteg ); } - public function testMultipleChoiceSkipEmptyValue() + public function testMultipleChoiceSkipsEmptyValue() { $form = $this->factory->createNamed('name', 'choice', array('&a'), array( 'choices' => array('&a' => 'Choice&A', '&b' => 'Choice&B'), @@ -700,7 +700,7 @@ abstract class AbstractLayoutTest extends \Symfony\Component\Form\Test\FormInteg ); } - public function testSingleChoiceExpandedSkipEmptyValue() + public function testSingleChoiceExpandedWithEmptyValue() { $form = $this->factory->createNamed('name', 'choice', '&a', array( 'choices' => array('&a' => 'Choice&A', '&b' => 'Choice&B'), @@ -712,13 +712,15 @@ abstract class AbstractLayoutTest extends \Symfony\Component\Form\Test\FormInteg $this->assertWidgetMatchesXpath($form->createView(), array(), '/div [ - ./input[@type="radio"][@name="name"][@id="name_0"][@checked] + ./input[@type="radio"][@name="name"][@id="name_placeholder"][not(@checked)] + /following-sibling::label[@for="name_placeholder"][.="[trans]Test&Me[/trans]"] + /following-sibling::input[@type="radio"][@name="name"][@id="name_0"][@checked] /following-sibling::label[@for="name_0"][.="[trans]Choice&A[/trans]"] /following-sibling::input[@type="radio"][@name="name"][@id="name_1"][not(@checked)] /following-sibling::label[@for="name_1"][.="[trans]Choice&B[/trans]"] /following-sibling::input[@type="hidden"][@id="name__token"] ] - [count(./input)=3] + [count(./input)=4] ' ); } diff --git a/src/Symfony/Component/Form/Tests/Extension/Core/EventListener/FixRadioInputListenerTest.php b/src/Symfony/Component/Form/Tests/Extension/Core/EventListener/FixRadioInputListenerTest.php index 955f56663f..a5d5c78a81 100644 --- a/src/Symfony/Component/Form/Tests/Extension/Core/EventListener/FixRadioInputListenerTest.php +++ b/src/Symfony/Component/Form/Tests/Extension/Core/EventListener/FixRadioInputListenerTest.php @@ -17,7 +17,7 @@ use Symfony\Component\Form\Extension\Core\ChoiceList\SimpleChoiceList; class FixRadioInputListenerTest extends \PHPUnit_Framework_TestCase { - private $listener; + private $choiceList; protected function setUp() { @@ -27,15 +27,14 @@ class FixRadioInputListenerTest extends \PHPUnit_Framework_TestCase parent::setUp(); - $list = new SimpleChoiceList(array(0 => 'A', 1 => 'B')); - $this->listener = new FixRadioInputListener($list); + $this->choiceList = new SimpleChoiceList(array('' => 'Empty', 0 => 'A', 1 => 'B')); } protected function tearDown() { parent::tearDown(); - $this->listener = null; + $listener = null; } public function testFixRadio() @@ -44,9 +43,11 @@ class FixRadioInputListenerTest extends \PHPUnit_Framework_TestCase $form = $this->getMock('Symfony\Component\Form\Test\FormInterface'); $event = new FormEvent($form, $data); - $this->listener->preSubmit($event); + $listener = new FixRadioInputListener($this->choiceList, true); + $listener->preSubmit($event); - $this->assertEquals(array(1 => '1'), $event->getData()); + // Indices in SimpleChoiceList are zero-based generated integers + $this->assertEquals(array(2 => '1'), $event->getData()); } public function testFixZero() @@ -55,18 +56,50 @@ class FixRadioInputListenerTest extends \PHPUnit_Framework_TestCase $form = $this->getMock('Symfony\Component\Form\Test\FormInterface'); $event = new FormEvent($form, $data); - $this->listener->preSubmit($event); + $listener = new FixRadioInputListener($this->choiceList, true); + $listener->preSubmit($event); - $this->assertEquals(array(0 => '0'), $event->getData()); + // Indices in SimpleChoiceList are zero-based generated integers + $this->assertEquals(array(1 => '0'), $event->getData()); } - public function testIgnoreEmptyString() + public function testFixEmptyString() { $data = ''; $form = $this->getMock('Symfony\Component\Form\Test\FormInterface'); $event = new FormEvent($form, $data); - $this->listener->preSubmit($event); + $listener = new FixRadioInputListener($this->choiceList, true); + $listener->preSubmit($event); + + // Indices in SimpleChoiceList are zero-based generated integers + $this->assertEquals(array(0 => ''), $event->getData()); + } + + public function testConvertEmptyStringToPlaceholderIfNotFound() + { + $list = new SimpleChoiceList(array(0 => 'A', 1 => 'B')); + + $data = ''; + $form = $this->getMock('Symfony\Component\Form\Test\FormInterface'); + $event = new FormEvent($form, $data); + + $listener = new FixRadioInputListener($list, true); + $listener->preSubmit($event); + + $this->assertEquals(array('placeholder' => ''), $event->getData()); + } + + public function testDontConvertEmptyStringToPlaceholderIfNoPlaceholderUsed() + { + $list = new SimpleChoiceList(array(0 => 'A', 1 => 'B')); + + $data = ''; + $form = $this->getMock('Symfony\Component\Form\Test\FormInterface'); + $event = new FormEvent($form, $data); + + $listener = new FixRadioInputListener($list, false); + $listener->preSubmit($event); $this->assertEquals(array(), $event->getData()); } 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 9499d7607f..219e8181c0 100644 --- a/src/Symfony/Component/Form/Tests/Extension/Core/Type/ChoiceTypeTest.php +++ b/src/Symfony/Component/Form/Tests/Extension/Core/Type/ChoiceTypeTest.php @@ -88,8 +88,7 @@ class ChoiceTypeTest extends \Symfony\Component\Form\Test\TypeTestCase public function testChoiceListAndChoicesCanBeEmpty() { - $this->factory->create('choice', null, array( - )); + $this->factory->create('choice'); } public function testExpandedChoicesOptionsTurnIntoChildren() @@ -99,7 +98,62 @@ class ChoiceTypeTest extends \Symfony\Component\Form\Test\TypeTestCase 'choices' => $this->choices, )); - $this->assertCount($form->count(), $this->choices, 'Each choice should become a new field'); + $this->assertCount(count($this->choices), $form, 'Each choice should become a new field'); + } + + public function testPlaceholderPresentOnNonRequiredExpandedSingleChoice() + { + $form = $this->factory->create('choice', null, array( + 'multiple' => false, + 'expanded' => true, + 'required' => false, + 'choices' => $this->choices, + )); + + $this->assertTrue(isset($form['placeholder'])); + $this->assertCount(count($this->choices) + 1, $form, 'Each choice should become a new field'); + } + + public function testPlaceholderNotPresentIfRequired() + { + $form = $this->factory->create('choice', null, array( + 'multiple' => false, + 'expanded' => true, + 'required' => true, + 'choices' => $this->choices, + )); + + $this->assertFalse(isset($form['placeholder'])); + $this->assertCount(count($this->choices), $form, 'Each choice should become a new field'); + } + + public function testPlaceholderNotPresentIfMultiple() + { + $form = $this->factory->create('choice', null, array( + 'multiple' => true, + 'expanded' => true, + 'required' => false, + 'choices' => $this->choices, + )); + + $this->assertFalse(isset($form['placeholder'])); + $this->assertCount(count($this->choices), $form, 'Each choice should become a new field'); + } + + public function testPlaceholderNotPresentIfEmptyChoice() + { + $form = $this->factory->create('choice', null, array( + 'multiple' => false, + 'expanded' => true, + 'required' => false, + 'choices' => array( + '' => 'Empty', + 1 => 'Not empty', + ), + )); + + $this->assertFalse(isset($form['placeholder'])); + $this->assertCount(2, $form, 'Each choice should become a new field'); } public function testExpandedChoicesOptionsAreFlattened() @@ -236,17 +290,26 @@ class ChoiceTypeTest extends \Symfony\Component\Form\Test\TypeTestCase $this->assertEquals(array('2', '3'), $form->getViewData()); } - public function testSubmitSingleExpanded() + public function testSubmitSingleExpandedRequired() { $form = $this->factory->create('choice', null, array( 'multiple' => false, 'expanded' => true, + 'required' => true, 'choices' => $this->choices, )); $form->submit('b'); $this->assertSame('b', $form->getData()); + $this->assertSame(array( + 0 => false, + 1 => true, + 2 => false, + 3 => false, + 4 => false, + ), $form->getViewData()); + $this->assertFalse($form[0]->getData()); $this->assertTrue($form[1]->getData()); $this->assertFalse($form[2]->getData()); @@ -259,17 +322,61 @@ class ChoiceTypeTest extends \Symfony\Component\Form\Test\TypeTestCase $this->assertNull($form[4]->getViewData()); } - public function testSubmitSingleExpandedNothingChecked() + public function testSubmitSingleExpandedNonRequired() { $form = $this->factory->create('choice', null, array( 'multiple' => false, 'expanded' => true, + 'required' => false, + 'choices' => $this->choices, + )); + + $form->submit('b'); + + $this->assertSame('b', $form->getData()); + $this->assertSame(array( + 0 => false, + 1 => true, + 2 => false, + 3 => false, + 4 => false, + 'placeholder' => false, + ), $form->getViewData()); + + $this->assertFalse($form['placeholder']->getData()); + $this->assertFalse($form[0]->getData()); + $this->assertTrue($form[1]->getData()); + $this->assertFalse($form[2]->getData()); + $this->assertFalse($form[3]->getData()); + $this->assertFalse($form[4]->getData()); + $this->assertNull($form['placeholder']->getViewData()); + $this->assertNull($form[0]->getViewData()); + $this->assertSame('b', $form[1]->getViewData()); + $this->assertNull($form[2]->getViewData()); + $this->assertNull($form[3]->getViewData()); + $this->assertNull($form[4]->getViewData()); + } + + public function testSubmitSingleExpandedRequiredNothingChecked() + { + $form = $this->factory->create('choice', null, array( + 'multiple' => false, + 'expanded' => true, + 'required' => true, 'choices' => $this->choices, )); $form->submit(null); $this->assertNull($form->getData()); + $this->assertSame(array( + 0 => false, + 1 => false, + 2 => false, + 3 => false, + 4 => false, + ), $form->getViewData()); + $this->assertFalse($form[0]->getData()); $this->assertFalse($form[1]->getData()); $this->assertFalse($form[2]->getData()); @@ -282,11 +389,62 @@ class ChoiceTypeTest extends \Symfony\Component\Form\Test\TypeTestCase $this->assertNull($form[4]->getViewData()); } - public function testSubmitSingleExpandedWithFalseDoesNotHaveExtraChildren() + public function testSubmitSingleExpandedNonRequiredNothingChecked() { $form = $this->factory->create('choice', null, array( 'multiple' => false, 'expanded' => true, + 'required' => false, + 'choices' => $this->choices, + )); + + $form->submit(null); + + $this->assertNull($form->getData()); + $this->assertSame(array( + 0 => false, + 1 => false, + 2 => false, + 3 => false, + 4 => false, + 'placeholder' => true, + ), $form->getViewData()); + + $this->assertTrue($form['placeholder']->getData()); + $this->assertFalse($form[0]->getData()); + $this->assertFalse($form[1]->getData()); + $this->assertFalse($form[2]->getData()); + $this->assertFalse($form[3]->getData()); + $this->assertFalse($form[4]->getData()); + $this->assertSame('', $form['placeholder']->getViewData()); + $this->assertNull($form[0]->getViewData()); + $this->assertNull($form[1]->getViewData()); + $this->assertNull($form[2]->getViewData()); + $this->assertNull($form[3]->getViewData()); + $this->assertNull($form[4]->getViewData()); + } + + public function testSubmitFalseToSingleExpandedRequiredDoesNotProduceExtraChildrenError() + { + $form = $this->factory->create('choice', null, array( + 'multiple' => false, + 'expanded' => true, + 'required' => true, + 'choices' => $this->choices, + )); + + $form->submit(false); + + $this->assertEmpty($form->getExtraData()); + $this->assertNull($form->getData()); + } + + public function testSubmitFalseToSingleExpandedNonRequiredDoesNotProduceExtraChildrenError() + { + $form = $this->factory->create('choice', null, array( + 'multiple' => false, + 'expanded' => true, + 'required' => false, 'choices' => $this->choices, )); @@ -568,10 +726,11 @@ class ChoiceTypeTest extends \Symfony\Component\Form\Test\TypeTestCase $this->assertTrue($view->vars['expanded']); } - public function testNotPassedEmptyValueToViewIsNull() + public function testEmptyValueIsNullByDefaultIfRequired() { $form = $this->factory->create('choice', null, array( 'multiple' => false, + 'required' => true, 'choices' => $this->choices, )); $view = $form->createView(); @@ -579,7 +738,7 @@ class ChoiceTypeTest extends \Symfony\Component\Form\Test\TypeTestCase $this->assertNull($view->vars['empty_value']); } - public function testPassEmptyValueToViewIsEmpty() + public function testEmptyValueIsEmptyStringByDefaultIfNotRequired() { $form = $this->factory->create('choice', null, array( 'multiple' => false, @@ -588,7 +747,7 @@ class ChoiceTypeTest extends \Symfony\Component\Form\Test\TypeTestCase )); $view = $form->createView(); - $this->assertEmpty($view->vars['empty_value']); + $this->assertSame('', $view->vars['empty_value']); } /** @@ -628,15 +787,44 @@ class ChoiceTypeTest extends \Symfony\Component\Form\Test\TypeTestCase public function getOptionsWithEmptyValue() { return array( + // single non-expanded array(false, false, false, 'foobar', 'foobar'), - array(true, false, false, 'foobar', null), - array(false, true, false, 'foobar', null), + array(false, false, false, '', ''), + array(false, false, false, null, null), + array(false, false, false, false, null), array(false, false, true, 'foobar', 'foobar'), array(false, false, true, '', ''), array(false, false, true, null, null), - array(false, true, true, 'foobar', null), + array(false, false, true, false, null), + // single expanded + array(false, true, false, 'foobar', 'foobar'), + // radios should never have an empty label + array(false, true, false, '', 'None'), + array(false, true, false, null, null), + array(false, true, false, false, null), + array(false, true, true, 'foobar', 'foobar'), + // radios should never have an empty label + array(false, true, true, '', 'None'), + array(false, true, true, null, null), + array(false, true, true, false, null), + // multiple non-expanded + array(true, false, false, 'foobar', null), + array(true, false, false, '', null), + array(true, false, false, null, null), + array(true, false, false, false, null), + array(true, false, true, 'foobar', null), + array(true, false, true, '', null), + array(true, false, true, null, null), + array(true, false, true, false, null), + // multiple expanded array(true, true, false, 'foobar', null), + array(true, true, false, '', null), + array(true, true, false, null, null), + array(true, true, false, false, null), array(true, true, true, 'foobar', null), + array(true, true, true, '', null), + array(true, true, true, null, null), + array(true, true, true, false, null), ); }