Fix for issue #7242 : broken translations for enums in forms

[FoxCie]

With symfony/form >= v7.3.5, EnumType uses the keys of the choices option as labels for the enum cases, breaking the translation that happened previously when enum implements TranslatableInterface. The translation now occurs only when the choices option has integer keys (more precisely, currently only if it satisfies array_list()). Fixing this by changing the way ChoiceFields are configured with the ChoiceConfigurator, to define the choices option with integer keys when the enum is translatable.

[Seb33300]

This sounds like a bug in Symfony?

[FoxCie]

They acknowledged that the previous behaviour was actually the bug. When dealing with choices, the keys of the array should be the labels. It is not explicit on EnumType, but on its parent ChoiceType, the doc states that clearly :

Source : https://symfony.com/doc/current/reference/forms/types/choice.html#choices .

The thing is that it was actually impossible to change an enum's labels with the choices array with the previous behaviour. Now it is possible, but the old behaviour is still present if the keys are integers (e.g. when giving the ::cases() as the choices).

EDIT : the associated bug in Symfony is here : symfony/symfony#61927 .

[Seb33300]

This still looks wrong to me. The EnumType should transform the data to make it compatible with ChoiceType

[FoxCie]

It is what is done. It is just the label that can be changed by passing a string value as the key of the array. EnumType does what should be done so that the values are properly handled and converted. If you think this is not appropriate, you could open an issue on the Symfony side.

[siggidiel]

Hey guys. What's the status of this? The translations of enum based ChoiceFields on form pages are broken for a while already. Any updates on this? :)

[rdevaissiere]

Thanks for the PR @FoxCie. It works well for me in forms but there's an issue with the selected value(s) as they come up empty in index/detail pages. I can't quite figure out a fix for now but choiceMessages is not populated in ChoiceConfigurator::configure(). Hope this is helpful.

[FoxCie]

Thanks for the PR @FoxCie. It works well for me in forms but there's an issue with the selected value(s) as they come up empty in index/detail pages. I can't quite figure out a fix for now but choiceMessages is not populated in ChoiceConfigurator::configure(). Hope this is helpful.

Nice catch, we should only change forms, not the other pages. I just pushed a version that should fix that (by just checking if the page is index or details, but I am not sure if this is the correct thing to do, there maybe a better way by changing more what was done before).

[rdevaissiere]

Thanks @FoxCie now all my tests pass! Hopefully someone can accept this PR so those of us affected by the issue can move forward with Symfony upgrades.

[ckaotik]

Thanks a bunch for creating a proper merge request for this and also for adding tests! ❤️ It works just fine for my use case 👍
Edit: I could not get the Enum field to render in my project. The form rendering works, but the view rendering is empty:

$type = ChoiceField::new('type')
  ->setTranslatableChoices(CustomEnumType::cases())
  ->renderExpanded();

I'm wondering why we need to override the $guessedFieldFqcn? I'm also not too fond of explicitly checking for the detail/index CRUD page, as a project might use custom pages which could cause weird side effects?
Edit: nvm, the $isIndexOrDetail was already used before.

I had a minimal custom patch before that seemed to work for my (limited and quite simple) use case. I feel like I must have missed something obvious. Unfortunately, I don't have a suitable environment to run this against your tests locally (it's complicated...).

--- src/Field/Configurator/ChoiceConfigurator.php
+++ src/Field/Configurator/ChoiceConfigurator.php
@@ -78,6 +78,8 @@ final class ChoiceConfigurator implement
         if ($areChoicesTranslatable && !$choicesSupportTranslatableInterface) {
             $field->setFormTypeOptionIfNotSet('choices', array_keys($choices));
             $field->setFormTypeOptionIfNotSet('choice_label', static fn ($value) => $choices[$value]);
+        } elseif ($areChoicesTranslatable && $choicesSupportTranslatableInterface) {
+            $field->setFormTypeOptionIfNotSet('choices', array_values($choices));
         } else {
             $field->setFormTypeOptionIfNotSet('choices', $choices);
         }

[FoxCie]

$type = ChoiceField::new('type')
  ->setTranslatableChoices(CustomEnumType::cases())
  ->renderExpanded();

The thing is that you use setTranslatableChoices, which should (and now does indeed) work just as a ChoiceType, using this as the array for the choices without consideration for the enum. If you remove it, it should work. It may be a bug, but I'm not sure it is related to this particular issue, it may have been broken beforehand.

I'm wondering why we need to override the $guessedFieldFqcn? I'm also not too fond of explicitly checking for the detail/index CRUD page, as a project might use custom pages which could cause weird side effects? Edit: nvm, the $isIndexOrDetail was already used before.

I strongly agree with that, but I didn't find any way to indicate that fields are rendered as forms or templates in the bundle. I guess that if you use custom pages, you would have to monkey patch the configurator to have it working (or use a copy of the configurator in your project).

I had a minimal custom patch before that seemed to work for my (limited and quite simple) use case. I feel like I must have missed something obvious. Unfortunately, I don't have a suitable environment to run this against your tests locally (it's complicated...).

--- src/Field/Configurator/ChoiceConfigurator.php
+++ src/Field/Configurator/ChoiceConfigurator.php
@@ -78,6 +78,8 @@ final class ChoiceConfigurator implement
         if ($areChoicesTranslatable && !$choicesSupportTranslatableInterface) {
             $field->setFormTypeOptionIfNotSet('choices', array_keys($choices));
             $field->setFormTypeOptionIfNotSet('choice_label', static fn ($value) => $choices[$value]);
+        } elseif ($areChoicesTranslatable && $choicesSupportTranslatableInterface) {
+            $field->setFormTypeOptionIfNotSet('choices', array_values($choices));
         } else {
             $field->setFormTypeOptionIfNotSet('choices', $choices);
         }

That may work, but I tried to modify the original behaviour as less as possible in case some code depends on specificities.

[rdevaissiere]

For what it's worth, @ckaotik's minimal patch works for me.

[siggidiel]

The thing is, if using setTranslatableChoices, the select-option values turn into indexed integers which would be wrong in our case. Select-Options need to be the enum case values as they are stored in the database. When using translatable choices and visiting the form page, it won't be able to pre-set the option stored in the database.

It should be:

<select>
  <option value="foo">Translated Foo</option>
  <option value="bar">Some bar translation</option>
</select>

But it turns into:

<select>
  <option value="0">Translated Foo</option>
  <option value="1">Some bar translation</option>
</select>

Seems like the index number represents the order of the cases inside an enum class. But what happens if the order is changed?

[Seb33300]

So this could be fixed by calling both lines?

$field->setFormTypeOptionIfNotSet('choices', ...);
$field->setFormTypeOptionIfNotSet('choice_label', ...);

[FoxCie]

The thing is, if using setTranslatableChoices, the select-option values turn into indexed integers which would be wrong in our case. Select-Options need to be the enum case values as they are stored in the database. When using translatable choices and visiting the form page, it won't be able to pre-set the option stored in the database.

It should be:

<select>
  <option value="foo">Translated Foo</option>
  <option value="bar">Some bar translation</option>
</select>

But it turns into:

<select>
  <option value="0">Translated Foo</option>
  <option value="1">Some bar translation</option>
</select>

Seems like the index number represents the order of the cases inside an enum class. But what happens if the order is changed?

This is not the case. This is backed by an EnumType from Symfony. and looking at https://github.com/symfony/symfony/blob/8.1/src/Symfony/Component/Form/Extension/Core/Type/EnumType.php, we can see that they set up choice_label and choice_value if the index is an int, so the value ends up being the value of the enum, as you want it to be.

Nevertheless, even if it were replaced by an int, it would not be a problem at all, because the order will not change between the time the form is rendered and the time you submit it (this is very unlikely, you would have to deploy a new version, keeping all sessions, etc...).

The whole point of this MR is just to adapt the code to what Symfony now does with EnumType, which is what they were doing previously, but now only when keys are integers (meaning you did not provide choices that are supposed to be display => value, they were previously wrongly not enforcing that part that should be inherited from ChoiceType).

[FoxCie]

So this could be fixed by calling both lines?

$field->setFormTypeOptionIfNotSet('choices', ...);
$field->setFormTypeOptionIfNotSet('choice_label', ...);

You may need to call choice_value too, but yes, this is exactly what Symfony's EnumType does when keys are integers (see my previous comment).