a2lix / TranslationFormBundle

Ease translations with some dedicated Symfony form types
https://a2lix.fr/bundles/translation-form
MIT License
330 stars 138 forks source link

:bug: Fix form validation when locale is required (knp behaviors) #369

Closed grizzlylab closed 2 years ago

grizzlylab commented 2 years ago

I am targeting master because a tag 3.0.11 will be probably needed. And to be honest Iam not sure which branch to target, maybe the version 2 is relevant (but automatic merge was not possible). Let me know how I can help. I think the subject of this PR is very important and Iam ready to follow rules and modify it in order to see it merged. Thank you for your time.

Closes #361 Closes #356

## Changelog

Fix form validation for required locales.

### Fixed

Form validation for required locales.

## To do

Check if this fix is only relevant when using knp behaviors or if it can be applied globally.

## Subject

A check is added for required locale when submiting the form, the element is not removed if the locale is required in order to trigger the "NotBlank" constraint. Otherwise, there is no validation (can submit all fields being empty).
grizzlylab commented 2 years ago

To reproduce the issue, it is needed to work with collection.

Where en is a required locale

Before fix :

Possible to edit 1 item by clearing all fields and then save the item, it will then remove all translations, even the translation being required (here en).

After fix :

Form validation triggered about en field for items not having a string, as the element for en is not removed (behavior in the fix), validation is applied on it ("This value should not be blank.")

Screenshot from 2022-01-11 16-39-04

tchapi commented 2 years ago

hi @grizzlylab and thanks a lot for this PR

I will try to review it asap. Could you just remove the modifications in composer.json ? I don't think they're related and it's better if we keep the commit history clean and tidy (but I may be wrong)

grizzlylab commented 2 years ago

@tchapi I removed the composer.json modifications.

tchapi commented 2 years ago

Sorry I'm being overwhelmed with many other things to do at work and haven't had the chance to look yet at the PR. Still in my priority list though!

tchapi commented 2 years ago

@webda2l it seems ok to do this for Knp. For other providers, I don't know what to think.. if empty($translation) is true but the locale is required, we should not remove the field (easy) but then, we cannot set the locale on the translation object (since it's empty) so $translation->setLocale($locale) would fail.

Can we just not remove the Element, without setting the locale ? Would that work ? Something along those lines :

foreach ($data as $locale => $translation) {
    $isRequiredLocale = \in_array($locale, $formOptions['required_locales'], true);
    // Remove useless Translation object
    if (!$isRequiredLocale && (
        (method_exists($translation, 'isEmpty') && $translation->isEmpty()) // Knp
        || empty($translation) // Default
    )) {
        $data->removeElement($translation);
        continue;
    }

    if (!empty($translation)) {
        $translation->setLocale($locale);
    }
}
webda2l commented 2 years ago

Hi, Ok to me, thank for your contribution. With or without your code @tchapi, as you want! (I think very few people use this bundle with a provider different than KNP)

grizzlylab commented 2 years ago

@tchapi @webda2l Thank you !

grizzlylab commented 2 years ago

Hello @tchapi, can we release a tag for this ?

webda2l commented 2 years ago

Hello @grizzlylab, Done https://github.com/a2lix/TranslationFormBundle/tree/3.2.1 Thanks