8000 [Form] fixed BC break with pre selection of choices with `ChoiceType` and its children by HeahDude · Pull Request #18180 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Form] fixed BC break with pre selection of choices with ChoiceType and its children #18180

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.

Sign up for GitHub

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 2 commits into from
Apr 28, 2016
Merged

[Form] fixed BC break with pre selection of choices with ChoiceType and its children #18180

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
71841c7
Revert "[Form] refactor `RadioListMapper::mapDataToForm()`"
HeahDude Mar 15, 2016
ea5375c
[Form] refactor CheckboxListMapper and RadioListMapper
HeahDude Mar 15, 2016
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
2 changes: 1 addition & 1 deletion src/Symfony/Bridge/Doctrine/Tests/Form/Type/EntityTypeTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,7 +261,7 @@ public function testSubmitSingleExpandedNull()
$field->submit(null);

$this->assertNull($field->getData());
$this->assertNull($field->getViewData());
$this->assertSame('', $field->getViewData(), 'View data is always a string');
}

public function testSubmitSingleNonExpandedNull()
Expand Down
2 changes: 1 addition & 1 deletion src/Symfony/Bridge/Doctrine/composer.json
View file
Open in desktop
10000
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
41 changes: 8 additions & 33 deletions src/Symfony/Component/Form/Extension/Core/DataMapper/CheckboxListMapper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -26,16 +25,6 @@
*/
class CheckboxListMapper implements DataMapperInterface
{
/**
* @var ChoiceListInterface
*/
private $choiceList;

public function __construct(ChoiceListInterface $choiceList)
{
$this->choiceList = $choiceList;
}

/**
* {@inheritdoc}
*/
Expand All @@ -46,22 +35,12 @@ public function mapDataToForms($choices, $checkboxes)
}

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');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

same here

}

foreach ($checkboxes as $checkbox) {
$value = $checkbox->getConfig()->getOption('value');
$checkbox->setData(isset($valueMap[$value]) ? true : false);
$checkbox->setData(in_array($value, $choices, true));
}
}

Expand All @@ -70,6 +49,10 @@ public function mapDataToForms($choices, $checkboxes)
*/
public function mapFormsToData($checkboxes, &$choices)
{
if (!is_array($choices)) {
throw new UnexpectedTypeException($choices, 'array');
}

$values = array();

foreach ($checkboxes as $checkbox) {
Expand All @@ -79,14 +62,6 @@ public function mapFormsToData($checkboxes, &$choices)
}
}

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;
}
}
27 changes: 12 additions & 15 deletions src/Symfony/Component/Form/Extension/Core/DataMapper/RadioListMapper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -25,24 +25,18 @@
*/
class RadioListMapper implements DataMapperInterface
{
/**
* @var ChoiceListInterface
*/
private $choiceList;

public function __construct(ChoiceListInterface $choiceList)
{
$this->choiceList = $choiceList;
}

/**
* {@inheritdoc}
*/
public function mapDataToForms($data, $radios)
public function mapDataToForms($choice, $radios)
{
if (!is_string($choice)) {
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 I test null of force a string casting before throwing here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@webmozart I re-check the code and here the view data should always be a string.

In contrary, since mapsFormsToData() happens before transformation we should accept null there as well.

Do you agree or am I missing something ? Thanks.

throw new UnexpectedTypeException($choice, 'string');
}

foreach ($radios as $radio) {
$value = $radio->getConfig()->getOption('value');
$radio->setData($value === $data ? true : false);
$radio->setData($choice === $value);
}
}

Expand All @@ -51,6 +45,10 @@ public function mapDataToForms($data, $radios)
*/
public function mapFormsToData($radios, &$choice)
{
if (null !== $choice && !is_string($choice)) {
throw new UnexpectedTypeException($choice, 'null or string');
}

$choice = null;

foreach ($radios as $radio) {
Expand All @@ -59,8 +57,7 @@ public function mapFormsToData($radios, &$choice)
return;
}

$value = $radio->getConfig()->getOption('value');
$choice = current($this->choiceList->getChoicesForValues(array($value)));
$choice = $radio->getConfig()->getOption('value');

return;
}
Expand Down
35 changes: 10 additions & 25 deletions src/Symfony/Component/Form/Extension/Core/Type/ChoiceType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,9 +60,7 @@ public function __construct(ChoiceListFactoryInterface $choiceListFactory = null
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
Expand Down Expand Up @@ -133,30 +131,13 @@ public function buildForm(FormBuilderInterface $builder, array $options)

$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);
}
});
// For radio lists, pre selection of the choice needs to pre set data
// with the string value so it can be matched in
// {@link \Symfony\Component\Form\Extension\Core\DataMapper\RadioListMapper::mapDataToForms()}
$builder->addEventListener(FormEvents::PRE_SET_DATA, function (FormEvent $event) {
$choiceList = $event->getForm()->getConfig()->getOption('choice_list');
$value = current($choiceList->getValuesForChoices(array($event->getData())));
$event->setData((string) $value);
});
}
} elseif ($options['multiple']) {
// <select> tag with "multiple" option
if ($options['multiple']) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't fully understand why this is necessary. It seems to me the transformer and the data mappers are doing the same thing already. Can you add some explanation?

// <select> tag with "multiple" option or list of checkbox inputs
$builder->addViewTransformer(new ChoicesToValuesTransformer($options['choice_list']));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@webmozart it also prevents this hack

} else {
// <select> tag without "multiple" option
// <select> tag without "multiple" option or list of radio inputs
$builder->addViewTransformer(new ChoiceToValueTransformer($options['choice_list']));
}

Expand Down Expand Up @@ -255,7 +236,11 @@ public function configureOptions(OptionsResolver $resolver)
$choiceListFactory = $this->choiceListFactory;

$emptyData = function (Options $options) {
if ($options['multiple'] || $options['expanded']) {
if ($options['expanded'] && !$options['multiple']) {
return;
}

if ($options['multiple']) {
return array();
}

Expand Down
Loading
0