Description
Symfony version(s) affected: 5.2.9
Description
ChoiceType constructor early return prevents $translator from being set and used
The constructor of ChoiceType (Symfony\Component\Form\Extension\Core\Type\ChoiceType) uses a conditional check
if ($this->choiceListFactory instanceof CachingFactoryDecorator) {
return;
}
which causes an early return from the costructor if $choiceListFactory is an instance of CachingFactoryDecorator - which is the default case in symfony (also a default value), so this condition is met in a lot of cases.
Later on the constructor would set the $translator property passed in the constructor argument, but it won't do that, as it returned early.
public function __construct(ChoiceListFactoryInterface $choiceListFactory = null, $translator = null)
{
$this->choiceListFactory = $choiceListFactory ?? new CachingFactoryDecorator(
new PropertyAccessDecorator(
new DefaultChoiceListFactory()
)
);
// BC, to be removed in 6.0
if ($this->choiceListFactory instanceof CachingFactoryDecorator) {
return; // <----------------------------- leaving too early
}
// [...] some other code here, not relevant
$this->translator = $translator; // <------------------- this translator is not set, as constructor leaved already
}
Obviously the $translator is used by the ChoiceType class, e.g. to translate an error message when some unknown values are found in the submitted form element. It would translate if it was set, but instead it uses the original message:
if ($options['multiple']) {
$messageTemplate = $options['invalid_message'] ?? 'The value {{ value }} is not valid.';
$builder->addEventListener(FormEvents::POST_SUBMIT, function (FormEvent $event) use (&$unknownValues, $messageTemplate) {
// Throw exception if unknown values were submitted
if (\count($unknownValues) > 0) {
$form = $event->getForm();
$clientDataAsString = is_scalar($form->getViewData()) ? (string) $form->getViewData() : (\is_array($form->getViewData()) ? implode('", "', array_keys($unknownValues)) : \gettype($form->getViewData()));
if (null !== $this->translator) {
$message = $this->translator->trans($messageTemplate, ['{{ value }}' => $clientDataAsString], 'validators');
} else { // <-------------------------- no translator has been set, so untranslated error message is used
$message = strtr($messageTemplate, ['{{ value }}' => $clientDataAsString]);
}
$form->addError(new FormError($message, $messageTemplate, ['{{ value }}' => $clientDataAsString], null, new TransformationFailedException(sprintf('The choices "%s" do not exist in the choice list.', $clientDataAsString))));
}
});
// <select> tag with "multiple" option or list of checkbox inputs
$builder->addViewTransformer(new ChoicesToValuesTransformer($choiceList));
}
How to reproduce
The easiest way to understand the issue it probably to analyze the constructor and the code above. This seems to be an obvious mistake. Other way to reproduce would be to submit a choice form element with unknown value (e.g. use browser to change a value of the element to one that was not defined by the original form, then select it and submit it).
Possible Solution
Change the order of lines in the constructor so that $translator is set before the early return.
Or do not early return in this constructor at all, as it doesn't seem to make much of sense there anyway.
Additional context
This is maybe a rare bug, it will occur when someone is manually tampering with form values before submitting the form. Yet, if you want your application to handle it in a clean way and you use translations everywhere else, you'd like to translate this error message as well. Having a class depend on a $translator, and not set that dependency or not use that $translator because of some unrelated condition doesn't seem to be justified.