Skip to content

Commit

Permalink
[Form] refactor RadioListMapper::mapDataToForm()
Browse files Browse the repository at this point in the history 
This fixes "false" choice pre selection when `ChoiceType`
is `expanded` and not `multiple`
  • Loading branch information
@HeahDude
HeahDude committed Feb 22, 2016
1 parent 3eac469 commit 8f918e5
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 4 deletions.
6 changes: 2 additions & 4 deletions src/Symfony/Component/Form/Extension/Core/DataMapper/RadioListMapper.php
View file
Expand Up @@ -38,13 +38,11 @@ public function __construct(ChoiceListInterface $choiceList)
/**
* {@inheritdoc}
*/
public function mapDataToForms($choice, $radios)
public function mapDataToForms($data, $radios)
{
$valueMap = array_flip($this->choiceList->getValuesForChoices(array($choice)));

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

Expand Down
8 changes: 8 additions & 0 deletions src/Symfony/Component/Form/Extension/Core/Type/ChoiceType.php
View file
Expand Up @@ -143,6 +143,14 @@ public function buildForm(FormBuilderInterface $builder, array $options)
$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);

This comment has been minimized.

Sign in to view

Copy link
@koemeet

koemeet Mar 15, 2016

@HeahDude Why does this have to be cast to a string? This causes data transformers to never have it's $value set to NULL (since they happen after the PRE_SET_DATA event). A lot of libraries check if the $value is NULL.

@fabpot In the symfony docs the implementation of a data transformer is that is checks the value to be null, here a reference of the official docs:

public function transform($issue)
{
    if (null === $issue) {
        return '';
    }

    return $issue->getId();
}

A lot of data transformers of other libraries have that strict if statement and it breaks with the current changes. Is a good solution to do a less strict check for empty values, such as if (empty($value))? What is your opinion on this?

This comment has been minimized.

Sign in to view

Copy link
@HeahDude

HeahDude Mar 15, 2016

Author Contributor

Hi @steffenbrem, thanks for reporting this problem.

I did not think of about transformers data when I worked on than fix.

Casting to string with the way this fix is written is mandatory as current() may return false then testing empty($value) would also be useless.

I'm actually working on another patch, meanwhile could you please try this workaround:

$choiceList = $event->getForm()->getConfig()->getOption('choice_list');
$value = current($choiceList->getValuesForChoices(array($event->getData())));
$data = $value ? (string) $value : null;
$event->setData($data);

Thank you.

This comment has been minimized.

Sign in to view

Copy link
@koemeet

koemeet Mar 15, 2016

That would fix it indeed. When $value is null it should stay null. I think the following is also sufficient:

$data = !$value ?: (string) $value; // this way whatever falsy value it had, it will stay like that

This comment has been minimized.

Sign in to view

Copy link
@HeahDude

HeahDude Mar 15, 2016

Author Contributor 
$data = false === $value ? null : (string) $value;

should work better.

This comment has been minimized.

Sign in to view

Copy link
@HeahDude

HeahDude Mar 15, 2016

Author Contributor

Still you pointed out that this is not valid in general as any transformer would expect the actual choice model data, not its string value.

This comment has been minimized.

Sign in to view

Copy link
@koemeet

koemeet Mar 15, 2016

@HeahDude 👍 Do you work on that patch? Otherwise I can do it tomorrow.

This comment has been minimized.

Sign in to view

Copy link
@HeahDude

HeahDude Mar 15, 2016

Author Contributor

I'm on it, actually trying to find a way to get it more consistent by first reverting this commit.
Pre selection of choices gives headache, I've already added some other failing tests...

This comment has been minimized.

Sign in to view

Copy link
@HeahDude

HeahDude Mar 16, 2016

Author Contributor

@steffenbrem See #18180. Thank you again for reporting this.
});
}
} elseif ($options['multiple']) {
// <select> tag with "multiple" option
Expand Down

0 comments on commit 8f918e5

Please sign in to comment.