8000 bug #17787 [Form] Fix choice placeholder edge cases (Tobion) · symfony/symfony@d3c55cb · GitHub
[go: up one dir, main page]
Skip to content

Commit d3c55cb

Browse files
fabpotfabpot
committed
bug #17787 [Form] Fix choice placeholder edge cases (Tobion)
This PR was merged into the 2.7 branch. Discussion ---------- [Form] Fix choice placeholder edge cases | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - Fixing several problems with choice placeholder that enhances #9030 for more edge cases: - A choice with an empty value manually added in the choices array should only be considered a placeholder when it is the first element in the final choice select. This is part of the HTML spec and how browsers also behave. If you select a choice with an empty value that is not the first option, it will still pass the "required" check and thus submit the empty value. So it's not a placeholder. If in the example below you move the empty option to the first place, the browsers will error on submit that you must select a value. So only then it is a placeholder to show as initial value. ```html <select id="form_timezone" name="form[timezone]" required="required"> <option value="Africa/Abidjan">Abidjan</option> <option value="">Empty</option> </select> ``` Also the validator https://validator.w3.org/nu/ will mark the above code as error: > The first child option element of a select element with a required attribute, and without a multiple attribute, and without a size attribute whose value is greater than 1, must have either an empty value attribute, or must have no text content. Consider either adding a placeholder option label, or adding a size attribute with a value equal to the number of option elements. This is fixed by replacing`0 !== count($choiceList->getChoicesForValues(array('')))` with `$view->vars['placeholder_in_choices'] = $choiceListView->hasPlaceholder()`. Which means, the required attribute is removed automatically because the select form element is required implicitly anyway due to the nature of the choice UI. - As the above quote mentions, the `size` attribute also has impact. Namely for a select with size > 1 it can be possible to have a required attribute even without placeholder. This is because when the size > 1, there is no default choice selected (similar to select with "multiple"). - A placeholder for required radio buttons or a select with size > 1 does not make sense as it would just be fake data that can be submitted (similar to the ignored placeholder for multi-select and checkboxes). Commits ------- 0efbc30 [Form] fix edge cases with choice placeholder
2 parents de5c737 + 0efbc30 commit d3c55cb

File tree

8 files changed

+73
-8
lines changed

8 files changed

+73
-8
lines changed

‎src/Symfony/Bridge/Twig/Resources/views/Form/form_div_layout.html.twig

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@
5252
{%- endblock choice_widget_expanded -%}
5353

5454
{%- block choice_widget_collapsed -%}
55-
{%- if required and placeholder is none and not placeholder_in_choices and not multiple -%}
55+
{%- if required and placeholder is none and not placeholder_in_choices and not multiple and (attr.size is not defined or attr.size <= 1) -%}
5656
{% set required = false %}
5757
{%- endif -%}
5858
<select {{ block('widget_attributes') }}{% if multiple %} multiple="multiple"{% endif %}>

‎src/Symfony/Bundle/FrameworkBundle/Resources/views/Form/choice_widget_collapsed.html.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
<select
2-
<?php if ($required && null === $placeholder && $placeholder_in_choices === false && $multiple === false):
2+
<?php if ($required && null === $placeholder && $placeholder_in_choices === false && $multiple === false && (!isset($attr['size']) || $attr['size'] <= 1)):
33
$required = false;
44
endif; ?>
55
<?php echo $view['form']->block($form, 'widget_attributes', array(

‎src/Symfony/Component/Form/ChoiceList/Factory/DefaultChoiceListFactory.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -246,7 +246,7 @@ private static function addChoiceViewGroupedBy($groupBy, $choice, $value, $label
246246

247247
$groupLabel = (string) $groupLabel;
248248

249-
// Initialize the group views if necessary. Unnnecessarily built group
249+
// Initialize the group views if necessary. Unnecessarily built group
250250
// views will be cleaned up at the end of createView()
251251
if (!isset($preferredViews[$groupLabel])) {
252252
$preferredViews[$groupLabel] = new ChoiceGroupView($groupLabel);

‎src/Symfony/Component/Form/ChoiceList/View/ChoiceListView.php

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,4 +48,24 @@ public function __construct(array $choices = array(), array $preferredChoices =
4848
$this->choices = $choices;
4949
$this->preferredChoices = $preferredChoices;
5050
}
51+
52+
/**
53+
* Returns whether a placeholder is in the choices.
54+
*
55+
* A placeholder must be the first child element, not be in a group and have an empty value.
56+
*
57+
* @return bool
58+
*/
59+
public function hasPlaceholder()
60+
{
61+
if ($this->preferredChoices) {
62+
$firstChoice = reset($this->preferredChoices);
63+
64+
return $firstChoice instanceof ChoiceView && '' === $firstChoice->value;
65+
}
66+
67+
$firstChoice = reset($this->choices);
68+
69+
return $firstChoice instanceof ChoiceView && '' === $firstChoice->value;
70+
}
5171
}

‎src/Symfony/Component/Form/Extension/Core/Type/ChoiceType.php

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -199,7 +199,7 @@ public function buildView(FormView $view, FormInterface $form, array $options)
199199
}
200200

201201
// Check if the choices already contain the empty value
202-
$view->vars['placeholder_in_choices'] = 0 !== count($options['choice_list']->getChoicesForValues(array('')));
202+
$view->vars['placeholder_in_choices'] = $choiceListView->hasPlaceholder();
203203

204204
// Only add the empty value option if this is not the case
205205
if (null !== $options['placeholder'] && !$view->vars['placeholder_in_choices']) {
@@ -343,6 +343,9 @@ public function configureOptions(OptionsResolver $resolver)
343343
if ($options['multiple']) {
344344
// never use an empty value for this case
345345
return;
346+
} elseif ($options['required'] && ($options['expanded'] || isset($options['attr']['size']) && $options['attr']['size'] > 1)) {
347+
// placeholder for required radio buttons or a select with size > 1 does not make sense
348+
return;
346349
} elseif (false === $placeholder) {
347350
// an empty value should be added but the user decided otherwise
348351
return;

‎src/Symfony/Component/Form/Tests/AbstractBootstrap3LayoutTest.php

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -232,6 +232,26 @@ public function testSingleChoice()
232232
);
233233
}
234234

235+
public function testSelectWithSizeBiggerThanOneCanBeRequired()
236+
{
237+
$form = $this->factory->createNamed('name', 'choice', null, array(
238+
'choices' => array('a', 'b'),
239+
'choices_as_values' => true,
240+
'multiple' => false,
241+
'expanded' => false,
242+
'attr' => array('size' => 2),
243+
));
244+
245+
$this->assertWidgetMatchesXpath($form->createView(), array('attr' => array('class' => '')),
246+
'/select
247+
[@name="name"]
248+
[@required="required"]
249+
[@size="2"]
250+
[count(./option)=2]
251+
'
252+
);
253+
}
254+
235255
public function testSingleChoiceWithoutTranslation()
236256
{
237257
$form = $this->factory->createNamed('name', 'choice', '&a', array(
@@ -754,6 +774,7 @@ public function testSingleChoiceExpandedWithPlaceholder()
754774
'multiple' => false,
755775
'expanded' => true,
756776
'placeholder' => 'Test&Me',
777+
'required' => false,
757778
));
758779

759780
$this->assertWidgetMatchesXpath($form->createView(), array(),

‎src/Symfony/Component/Form/Tests/AbstractLayoutTest.php

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -528,6 +528,26 @@ public function testSingleChoice()
528528
);
529529
}
530530

531+
public function testSelectWithSizeBiggerThanOneCanBeRequired()
532+
{
533+
$form = $this->factory->createNamed('name', 'choice', null, array(
534+
'choices' => array('a', 'b'),
535+
'choices_as_values' => true,
536+
'multiple' => false,
537+
'expanded' => false,
538+
'attr' => array('size' => 2),
539+
));
540+
541+
$this->assertWidgetMatchesXpath($form->createView(), array(),
542+
'/select
543+
[@name="name"]
544+
[@required="required"]
545+
[@size="2"]
546+
[count(./option)=2]
547+
'
548+
);
549+
}
550+
531551
public function testSingleChoiceWithoutTranslation()
532552
{
533553
$form = $this->factory->createNamed('name', 'choice', '&a', array(
@@ -1001,6 +1021,7 @@ public function testSingleChoiceExpandedWithPlaceholder()
10011021
'multiple' => false,
10021022
'expanded' => true,
10031023
'placeholder' => 'Test&Me',
1024+
'required' => false,
10041025
));
10051026

10061027
$this->assertWidgetMatchesXpath($form->createView(), array(),

‎src/Symfony/Component/Form/Tests/Extension/Core/Type/ChoiceTypeTest.php

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1689,7 +1689,7 @@ public function testDontPassPlaceholderIfContainedInChoices($multiple, $expanded
16891689
'expanded' => $expanded,
16901690
'required' => $required,
16911691
'placeholder' => $placeholder,
1692-
'choices' => array('A' => 'a', 'Empty' => ''),
1692+
'choices' => array('Empty' => '', 'A' => 'a'),
16931693
'choices_as_values' => true,
16941694
));
16951695
$view = $form->createView();
@@ -1716,9 +1716,9 @@ public function getOptionsWithPlaceholder()
17161716
array(false, true, false, '', 'None'),
17171717
array(false, true, false, null, null),
17181718
array(false, true, false, false, null),
1719-
array(false, true, true, 'foobar', 'foobar'),
1720-
// radios should never have an empty label
1721-
array(false, true, true, '', 'None'),
1719+
// required radios should never have a placeholder
1720+
array(false, true, true, 'foobar', null),
1721+
array(false, true, true, '', null),
17221722
array(false, true, true, null, null),
17231723
array(false, true, true, false, null),
17241724
// multiple non-expanded

0 commit comments

Comments
 (0)
0