Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

[Form] Fix choice placeholder edge cases #17787

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 18, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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 -%}
<select {{ block('widget_attributes') }}{% if multiple %} multiple="multiple"{% endif %}>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<select
<?php if ($required && null === $placeholder && $placeholder_in_choices === false && $multiple === false):
<?php if ($required && null === $placeholder && $placeholder_in_choices === false && $multiple === false && (!isset($attr['size']) || $attr['size'] <= 1)):
$required = false;
endif; ?>
<?php echo $view['form']->block($form, 'widget_attributes', array(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,7 @@ private static function addChoiceViewGroupedBy($groupBy, $choice, $value, $label

$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);
Expand Down
20 changes: 20 additions & 0 deletions 20 src/Symfony/Component/Form/ChoiceList/View/ChoiceListView.php
Original file line number Diff line number Diff line change
Expand Up @@ -48,4 +48,24 @@ public function __construct(array $choices = array(), array $preferredChoices =
$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;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ public function buildView(FormView $view, FormInterface $form, array $options)
}

// 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']) {
Expand Down Expand Up @@ -343,6 +343,9 @@ public function configureOptions(OptionsResolver $resolver)
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;
Expand Down
21 changes: 21 additions & 0 deletions 21 src/Symfony/Component/Form/Tests/AbstractBootstrap3LayoutTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,26 @@ public function testSingleChoice()
);
}

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(
Expand Down Expand Up @@ -754,6 +774,7 @@ public function testSingleChoiceExpandedWithPlaceholder()
'multiple' => false,
'expanded' => true,
'placeholder' => 'Test&Me',
'required' => false,
));

$this->assertWidgetMatchesXpath($form->createView(), array(),
Expand Down
21 changes: 21 additions & 0 deletions 21 src/Symfony/Component/Form/Tests/AbstractLayoutTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -528,6 +528,26 @@ public function testSingleChoice()
);
}

public function testSelectWithSizeBiggerThanOneCanBeRequired()
{
$form = $this->factory->createNamed('name', 'choice', null, array(
'choices' => array('a', 'b'),
'choices_as_values' => true,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be removed in 3.0 or 3.1

'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(
Expand Down Expand Up @@ -1001,6 +1021,7 @@ public function testSingleChoiceExpandedWithPlaceholder()
'multiple' => false,
'expanded' => true,
'placeholder' => 'Test&Me',
'required' => false,
));

$this->assertWidgetMatchesXpath($form->createView(), array(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1689,7 +1689,7 @@ public function testDontPassPlaceholderIfContainedInChoices($multiple, $expanded
'expanded' => $expanded,
'required' => $required,
'placeholder' => $placeholder,
'choices' => array('A' => 'a', 'Empty' => ''),
'choices' => array('Empty' => '', 'A' => 'a'),
'choices_as_values' => true,
));
$view = $form->createView();
Expand All @@ -1716,9 +1716,9 @@ public function getOptionsWithPlaceholder()
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
Expand Down
Morty Proxy This is a proxified and sanitized view of the page, visit original site.