From 0efbc30304113b987aced8f11e2d102b816a740c Mon Sep 17 00:00:00 2001 From: Tobias Schultze Date: Sat, 13 Feb 2016 05:58:41 +0100 Subject: [PATCH] [Form] fix edge cases with choice placeholder --- .../views/Form/form_div_layout.html.twig | 2 +- .../Form/choice_widget_collapsed.html.php | 2 +- .../Factory/DefaultChoiceListFactory.php | 2 +- .../Form/ChoiceList/View/ChoiceListView.php | 20 ++++++++++++++++++ .../Form/Extension/Core/Type/ChoiceType.php | 5 ++++- .../Tests/AbstractBootstrap3LayoutTest.php | 21 +++++++++++++++++++ .../Form/Tests/AbstractLayoutTest.php | 21 +++++++++++++++++++ .../Extension/Core/Type/ChoiceTypeTest.php | 8 +++---- 8 files changed, 73 insertions(+), 8 deletions(-) diff --git a/src/Symfony/Bridge/Twig/Resources/views/Form/form_div_layout.html.twig b/src/Symfony/Bridge/Twig/Resources/views/Form/form_div_layout.html.twig index e3a611c38f..ae5d7fbed4 100644 --- a/src/Symfony/Bridge/Twig/Resources/views/Form/form_div_layout.html.twig +++ b/src/Symfony/Bridge/Twig/Resources/views/Form/form_div_layout.html.twig @@ -52,7 +52,7 @@ {%- endblock choice_widget_expanded -%} {%- block choice_widget_collapsed -%} - {%- if required and placeholder is none and not placeholder_in_choices and not multiple -%} + {%- if required and placeholder is none and not placeholder_in_choices and not multiple and (attr.size is not defined or attr.size <= 1) -%} {% set required = false %} {%- endif -%} block($form, 'widget_attributes', array( diff --git a/src/Symfony/Component/Form/ChoiceList/Factory/DefaultChoiceListFactory.php b/src/Symfony/Component/Form/ChoiceList/Factory/DefaultChoiceListFactory.php index ef93ffdd76..8cd8e29259 100644 --- a/src/Symfony/Component/Form/ChoiceList/Factory/DefaultChoiceListFactory.php +++ b/src/Symfony/Component/Form/ChoiceList/Factory/DefaultChoiceListFactory.php @@ -246,7 +246,7 @@ class DefaultChoiceListFactory implements ChoiceListFactoryInterface $groupLabel = (string) $groupLabel; - // Initialize the group views if necessary. Unnnecessarily built group + // Initialize the group views if necessary. Unnecessarily built group // views will be cleaned up at the end of createView() if (!isset($preferredViews[$groupLabel])) { $preferredViews[$groupLabel] = new ChoiceGroupView($groupLabel); diff --git a/src/Symfony/Component/Form/ChoiceList/View/ChoiceListView.php b/src/Symfony/Component/Form/ChoiceList/View/ChoiceListView.php index 9641f4b1d9..99308b826f 100644 --- a/src/Symfony/Component/Form/ChoiceList/View/ChoiceListView.php +++ b/src/Symfony/Component/Form/ChoiceList/View/ChoiceListView.php @@ -48,4 +48,24 @@ class ChoiceListView $this->choices = $choices; $this->preferredChoices = $preferredChoices; } + + /** + * Returns whether a placeholder is in the choices. + * + * A placeholder must be the first child element, not be in a group and have an empty value. + * + * @return bool + */ + public function hasPlaceholder() + { + if ($this->preferredChoices) { + $firstChoice = reset($this->preferredChoices); + + return $firstChoice instanceof ChoiceView && '' === $firstChoice->value; + } + + $firstChoice = reset($this->choices); + + return $firstChoice instanceof ChoiceView && '' === $firstChoice->value; + } } diff --git a/src/Symfony/Component/Form/Extension/Core/Type/ChoiceType.php b/src/Symfony/Component/Form/Extension/Core/Type/ChoiceType.php index 58832ab2f8..d8d1a8f57e 100644 --- a/src/Symfony/Component/Form/Extension/Core/Type/ChoiceType.php +++ b/src/Symfony/Component/Form/Extension/Core/Type/ChoiceType.php @@ -199,7 +199,7 @@ class ChoiceType extends AbstractType } // Check if the choices already contain the empty value - $view->vars['placeholder_in_choices'] = 0 !== count($options['choice_list']->getChoicesForValues(array(''))); + $view->vars['placeholder_in_choices'] = $choiceListView->hasPlaceholder(); // Only add the empty value option if this is not the case if (null !== $options['placeholder'] && !$view->vars['placeholder_in_choices']) { @@ -343,6 +343,9 @@ class ChoiceType extends AbstractType if ($options['multiple']) { // never use an empty value for this case return; + } elseif ($options['required'] && ($options['expanded'] || isset($options['attr']['size']) && $options['attr']['size'] > 1)) { + // placeholder for required radio buttons or a select with size > 1 does not make sense + return; } elseif (false === $placeholder) { // an empty value should be added but the user decided otherwise return; diff --git a/src/Symfony/Component/Form/Tests/AbstractBootstrap3LayoutTest.php b/src/Symfony/Component/Form/Tests/AbstractBootstrap3LayoutTest.php index 3bafd0ea24..8c846c643e 100644 --- a/src/Symfony/Component/Form/Tests/AbstractBootstrap3LayoutTest.php +++ b/src/Symfony/Component/Form/Tests/AbstractBootstrap3LayoutTest.php @@ -232,6 +232,26 @@ abstract class AbstractBootstrap3LayoutTest extends AbstractLayoutTest ); } + public function testSelectWithSizeBiggerThanOneCanBeRequired() + { + $form = $this->factory->createNamed('name', 'choice', null, array( + 'choices' => array('a', 'b'), + 'choices_as_values' => true, + 'multiple' => false, + 'expanded' => false, + 'attr' => array('size' => 2), + )); + + $this->assertWidgetMatchesXpath($form->createView(), array('attr' => array('class' => '')), +'/select + [@name="name"] + [@required="required"] + [@size="2"] + [count(./option)=2] +' + ); + } + public function testSingleChoiceWithoutTranslation() { $form = $this->factory->createNamed('name', 'choice', '&a', array( @@ -754,6 +774,7 @@ abstract class AbstractBootstrap3LayoutTest extends AbstractLayoutTest 'multiple' => false, 'expanded' => true, 'placeholder' => 'Test&Me', + 'required' => false, )); $this->assertWidgetMatchesXpath($form->createView(), array(), diff --git a/src/Symfony/Component/Form/Tests/AbstractLayoutTest.php b/src/Symfony/Component/Form/Tests/AbstractLayoutTest.php index 0939a2b003..c31513e209 100644 --- a/src/Symfony/Component/Form/Tests/AbstractLayoutTest.php +++ b/src/Symfony/Component/Form/Tests/AbstractLayoutTest.php @@ -528,6 +528,26 @@ abstract class AbstractLayoutTest extends \Symfony\Component\Form\Test\FormInteg ); } + public function testSelectWithSizeBiggerThanOneCanBeRequired() + { + $form = $this->factory->createNamed('name', 'choice', null, array( + 'choices' => array('a', 'b'), + 'choices_as_values' => true, + 'multiple' => false, + 'expanded' => false, + 'attr' => array('size' => 2), + )); + + $this->assertWidgetMatchesXpath($form->createView(), array(), +'/select + [@name="name"] + [@required="required"] + [@size="2"] + [count(./option)=2] +' + ); + } + public function testSingleChoiceWithoutTranslation() { $form = $this->factory->createNamed('name', 'choice', '&a', array( @@ -1001,6 +1021,7 @@ abstract class AbstractLayoutTest extends \Symfony\Component\Form\Test\FormInteg 'multiple' => false, 'expanded' => true, 'placeholder' => 'Test&Me', + 'required' => false, )); $this->assertWidgetMatchesXpath($form->createView(), 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 cf74fec212..809a4a7d8d 100644 --- a/src/Symfony/Component/Form/Tests/Extension/Core/Type/ChoiceTypeTest.php +++ b/src/Symfony/Component/Form/Tests/Extension/Core/Type/ChoiceTypeTest.php @@ -1689,7 +1689,7 @@ class ChoiceTypeTest extends \Symfony\Component\Form\Test\TypeTestCase 'expanded' => $expanded, 'required' => $required, 'placeholder' => $placeholder, - 'choices' => array('A' => 'a', 'Empty' => ''), + 'choices' => array('Empty' => '', 'A' => 'a'), 'choices_as_values' => true, )); $view = $form->createView(); @@ -1716,9 +1716,9 @@ class ChoiceTypeTest extends \Symfony\Component\Form\Test\TypeTestCase 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'), + // required radios should never have a placeholder + array(false, true, true, 'foobar', null), + array(false, true, true, '', null), array(false, true, true, null, null), array(false, true, true, false, null), // multiple non-expanded