Bug on OneToMany CollectionType with Translations

benjamin-hubert commented 3 years ago

Environment

Sonata packages

show

``` sonata-project/admin-bundle 3.104.0 3.104.0 The missing Symfony... sonata-project/block-bundle 3.23.2 4.6.0 Symfony SonataBlock... sonata-project/cache 2.0.1 2.1.1 Cache library sonata-project/core-bundle 3.20.0 3.20.0 Symfony SonataCoreB... Package sonata-project/core-bundle is abandoned, you should avoid using it. No replacement was suggested. sonata-project/datagrid-bundle 3.3.0 3.3.0 Symfony SonataDatag... sonata-project/doctrine-extensions 1.13.1 1.13.1 Doctrine2 behaviora... sonata-project/doctrine-orm-admin-bundle 3.35.0 3.35.0 Integrate Doctrine ... sonata-project/exporter 2.7.0 2.7.0 Lightweight Exporte... sonata-project/form-extensions 0.1.2 1.9.0 Symfony form extens... sonata-project/formatter-bundle 4.5.0 4.5.0 Symfony SonataForma... sonata-project/intl-bundle 2.10.1 2.10.1 Symfony SonataIntlB... sonata-project/media-bundle 3.32.0 3.32.0 Symfony SonataMedia... sonata-project/seo-bundle 2.13.1 2.13.1 Symfony SonataSeoBu... sonata-project/translation-bundle 2.8.1 2.8.1 SonataTranslationBu... sonata-project/twig-extensions 0.1.1 1.7.0 Sonata twig extensions ```

Symfony packages

show

``` symfony/asset v4.4.27 v5.3.4 Manages URL generation an... symfony/cache v4.4.27 v5.3.4 Provides an extended PSR-... symfony/cache-contracts v2.4.0 v2.4.0 Generic abstractions rela... symfony/config v4.4.27 v5.3.4 Helps you find, load, com... symfony/console v4.4.27 v5.3.4 Eases the creation of bea... symfony/css-selector v4.4.27 v5.3.4 Converts CSS selectors to... symfony/debug v4.4.27 v4.4.27 Provides tools to ease de... symfony/dependency-injection v4.4.27 v5.3.4 Allows you to standardize... symfony/deprecation-contracts v2.4.0 v2.4.0 A generic function and co... symfony/doctrine-bridge v4.4.27 v5.3.4 Provides integration for ... symfony/dotenv v4.4.27 v5.3.4 Registers environment var... symfony/error-handler v4.4.27 v5.3.4 Provides tools to manage ... symfony/event-dispatcher v4.4.27 v5.3.4 Provides tools that allow... symfony/event-dispatcher-contracts v1.1.9 v2.4.0 Generic abstractions rela... symfony/expression-language v4.4.27 v5.3.4 Provides an engine that c... symfony/filesystem v4.4.27 v5.3.4 Provides basic utilities ... symfony/finder v4.4.27 v5.3.4 Finds files and directori... symfony/flex v1.13.3 v1.13.3 Composer plugin for Symfony symfony/form v4.4.27 v5.3.4 Allows to easily create, ... symfony/framework-bundle v4.4.27 v5.3.4 Provides a tight integrat... symfony/http-client v4.4.27 v5.3.4 Provides powerful methods... symfony/http-client-contracts v2.4.0 v2.4.0 Generic abstractions rela... symfony/http-foundation v4.4.27 v5.3.4 Defines an object-oriente... symfony/http-kernel v4.4.28 v5.3.5 Provides a structured pro... symfony/inflector v4.4.27 v5.3.4 Converts words between th... symfony/intl v4.4.27 v5.3.4 Provides a PHP replacemen... symfony/mailer v4.4.27 v5.3.4 Helps sending emails symfony/mailgun-mailer v4.4.27 v5.3.4 Symfony Mailgun Mailer Br... symfony/mime v4.4.27 v5.3.4 Allows manipulating MIME ... symfony/monolog-bridge v4.4.27 v5.3.4 Provides integration for ... symfony/monolog-bundle v3.7.0 v3.7.0 Symfony MonologBundle symfony/options-resolver v4.4.27 v5.3.4 Provides an improved repl... symfony/phpunit-bridge v4.4.27 v5.3.4 Provides utilities for PH... symfony/polyfill-intl-grapheme v1.23.0 v1.23.0 Symfony polyfill for intl... symfony/polyfill-intl-icu v1.23.0 v1.23.0 Symfony polyfill for intl... symfony/polyfill-intl-idn v1.23.0 v1.23.0 Symfony polyfill for intl... symfony/polyfill-intl-normalizer v1.23.0 v1.23.0 Symfony polyfill for intl... symfony/polyfill-mbstring v1.23.0 v1.23.0 Symfony polyfill for the ... symfony/polyfill-php72 v1.23.0 v1.23.0 Symfony polyfill backport... symfony/polyfill-php73 v1.23.0 v1.23.0 Symfony polyfill backport... symfony/polyfill-php80 v1.23.0 v1.23.0 Symfony polyfill backport... symfony/polyfill-php81 v1.23.0 v1.23.0 Symfony polyfill backport... symfony/polyfill-uuid v1.23.0 v1.23.0 Symfony polyfill for uuid... symfony/process v4.4.27 v5.3.4 Executes commands in sub-... symfony/property-access v4.4.27 v5.3.4 Provides functions to rea... symfony/routing v4.4.27 v5.3.4 Maps an HTTP request to a... symfony/security-acl v3.2.0 v3.2.0 Symfony Security Componen... symfony/security-bundle v4.4.27 v5.3.4 Provides a tight integrat... symfony/security-core v4.4.27 v5.3.4 Symfony Security Componen... symfony/security-csrf v4.4.27 v5.3.4 Symfony Security Componen... symfony/security-guard v4.4.27 v5.3.4 Symfony Security Componen... symfony/security-http v4.4.27 v5.3.4 Symfony Security Componen... symfony/serializer v4.4.27 v5.3.4 Handles serializing and d... symfony/service-contracts v2.4.0 v2.4.0 Generic abstractions rela... symfony/stopwatch v4.4.27 v5.3.4 Provides a way to profile... symfony/string v5.3.3 v5.3.3 Provides an object-orient... symfony/templating v4.4.27 v5.3.4 Provides all the tools ne... symfony/translation v4.4.27 v5.3.4 Provides tools to interna... symfony/translation-contracts v2.4.0 v2.4.0 Generic abstractions rela... symfony/twig-bridge v4.4.27 v5.3.4 Provides integration for ... symfony/twig-bundle v4.4.27 v5.3.4 Provides a tight integrat... symfony/validator v4.4.27 v5.3.4 Provides tools to validat... symfony/var-dumper v4.4.27 v5.3.4 Provides mechanisms for w... symfony/var-exporter v4.4.27 v5.3.4 Allows exporting any seri... symfony/web-profiler-bundle v4.4.28 v5.3.5 Provides a development to... symfony/web-server-bundle v4.4.27 v4.4.27 Provides commands for run... symfony/webpack-encore-bundle v1.12.0 v1.12.0 Integration with your Sym... symfony/yaml v4.4.27 v5.3.4 Loads and dumps YAML files ```

PHP version

PHP 7.4.20 (cli) (built: Jun  4 2021 21:24:55) ( NTS )
Copyright (c) The PHP Group
Zend Engine v3.4.0, Copyright (c) Zend Technologies
    with Zend OPcache v7.4.20, Copyright (c), by Zend Technologies

Subject

I have a Product entity with an attributes field (OneToMany) pointing to a collection of ProductAttribute entity. Both entities are translatables.

I'm declaring a CollectionType in my ProductAdmin class

$formMapper->add(
                "attributes",
                CollectionType::class,
                [
                    'label'        => false,
                    'by_reference' => false,
                    'required'     => false,
                ],
                [
                    'edit'     => 'inline',
                    'inline'   => 'table',
                    'sortable' => 'position',
                ]
            )

Expected results

On the Product administration page :

I click on the add new button of the attributes table
I fill my new line
I click again on the add new button
I have a new blank line to fill

Actual results

On the Product administration page :

I click on the add new button of the attributes table
I fill my new line
I click again on the add new button
It resets my first line i have filled

benjamin-hubert commented 3 years ago

After research, it seems to have a bug on the AdminHelper->appendFormFieldElement function.

            $newInstance = ObjectManipulator::addInstance(
                $form->getData(),
                $associationAdmin->getNewInstance(),
                $fieldDescription
            );

This line call the getNewInstance function which seems the source of this bug. After digging, when the new ProductAttribute instance is created, it calls the parent admin and the alterObject function. This function is caught by TranslatableAdminExtension which refresh the main object and mess with our form data.

StackTrace :

AdminHelper->appendFormFieldElement
AbstractAdmin->getNewInstance
AbstractAdmin->apprendParentObject
AbstractAdmin->getObject (for the parent)
AbstractAdmin->alterObject
TranslatableAdminExtension->alterObject

This seems more of a SonataAdmin logic issue than a bug of the TranslationBundle.

VincentLanglet commented 3 years ago

If I understand correctly, the issue is because of the following line: https://github.com/sonata-project/SonataTranslationBundle/blob/2.x/src/Admin/Extension/Gedmo/TranslatableAdminExtension.php#L88

I wouldn't say it's a normal behavior to call the objectManager in the alterObject method...

benjamin-hubert commented 3 years ago

You are right, this is the line. You would recommend moving this issue to sonata-project/SonataTranslationBundle ?

VincentLanglet commented 3 years ago

You are right, this is the line. You would recommend moving this issue to sonata-project/SonataTranslationBundle ?

Yes, I moved it because it would be nice to find a solution inside the SonataTranslationBundle.

It would require

to understand the purpose of the refresh call.
to see if it's avoidable, at least in the case of the form usage.

benjamin-hubert commented 3 years ago

The refresh call is meant to have the right translation displayed in the back office. This is from the gedmo translatable documentation https://github.com/doctrine-extensions/DoctrineExtensions/blob/main/doc/translatable.md#basic-usage-examples

// reload in different language
$article = $em->find('Entity\Article', 1 /*article id*/);
$article->setLocale('ru_ru');
$em->refresh($article);

That's why i posted first this issue in Sonata as the SonataTranslationBundle seemed to follow Gedmo Translatable's rules and it was hard to expect such behaviour from the bundle side.

benjamin-hubert commented 3 years ago

It seems difficult to me to expect changes on SonataTranslationBundle side as i believe there is no bad design implementation from it.

My opinion is that the AdminHelper->appendFormFieldElement should be rewritten where the ObjectManipulator is mixing existing data and new data.

VincentLanglet commented 3 years ago

In your example, setLocale is called, then refresh. In the SonataTranslationExtension, refresh is call before setLocale ; don't know the reason.

It seems difficult to me to expect changes on SonataTranslationBundle side as i believe there is no bad design implementation from it.

I disagree on this.

Calling refresh is basically the same than calling persist or flush. All the object manager manipulation should be let to the Admin and his modelManager.

My opinion is that the AdminHelper->appendFormFieldElement should be rewritten where the ObjectManipulator is mixing existing data and new data.

appendFormFieldElement is just calling getNewInstance to generate new instance. And the getNewInstance is done this way:

public function getNewInstance()
{
        $object = $this->createNewInstance();

        $this->appendParentObject($object);
        $this->alterNewInstance($object);

        foreach ($this->getExtensions() as $extension) {
            $extension->alterNewInstance($this, $object);
        }

        return $object;
}

Creating a new entity
Attaching this new entity to the existent one
Allowing extensions to do their job

This won't change.

benjamin-hubert commented 3 years ago

In your example, setLocale is called, then refresh. In the SonataTranslationExtension, refresh is call before setLocale ; don't know the reason.

Because SonataTranslationBundle is explicitly calling the TranslationListener just before. The $object->setLocale() is just an update of the entity's $locale property. This has no incidence on the translation process.

Your decision means we would have a huge rewrite of the main behaviour of this bundle due to a bad conception. We won't be able to use alterObject anymore to handle the loading of translations. This could be a huge BC break

VincentLanglet commented 3 years ago

This could be a huge BC break

Like rewriting appendFormFieldElement would be a BC-break too.

Your decision means we would have a huge rewrite of the main behaviour of this bundle due to a bad conception. We won't be able to use alterObject anymore to handle the loading of translations.

Prior to something like version 3.61, we end up with case where $this->getSubject() could return null in the configureFormField() method, because some OneToOne or OneToMany relations weren't set. You could also generate a new entity in a childAdmin which wasn't correctly attached to the parent entity.

Both those bugs and others related were fixed. I'm sorry for not wanting to reintroduce those bugs.

The bad conception is in the fact to use the object manager here.

Looking at the refresh method phpdoc

Refreshes the persistent state of an object from the database, overriding any local changes that have not yet been persisted.

This is definitely a method which shouldn't be call here during the form edition.

Personally I don't understand why the local is updated for every call to getObject. I would have expect a check if the locale has changed.

First time: you load the object and set the locale
Then you add some child in the collection
Second time: you load the object, the locale is already set so no need for a refresh.

Wouldn't it be possible ?

benjamin-hubert commented 3 years ago

Personally I don't understand why the local is updated for every call to getObject.

me neither

I would have expect a check if the locale has changed.

I was actually working on a PR following this idea because i agree with you on the excessive refresh. However, it only fix the issue when we are working on the main language of the project.

VincentLanglet commented 3 years ago

I was actually working on a PR following this idea because i agree with you on the excessive refresh. However, it only fix the issue when we are working on the main language of the project.

I'm not sure about this according to the current result:

I click on the add new button of the attributes table I fill my new line I click again on the add new button It resets my first line i have filled

The first add new click should set the wanted locale. And then the second add new button won't refresh and then won't reset the first line.

But I can't be sure...

VincentLanglet commented 2 years ago

@franmomu I think you're using this bundle, do you encountered this bug ? Would you have time to take a look ? If we introduce a BC-break it would be nice to do it on 3.x before the stable release.

wokenlex commented 2 years ago

I never saw it work properly, especially with a second level cache - it can show you translation of another locale, or nothing. I have made some hacks and only in this way it's working.

I think it's not a BC, it's just a bug what was here at least 3 years.

I'm faking default locale specially for sonata_translation:

sonata_translation:
    locales: ['en', 'es']
    locale_switcher: false
    locale_switcher_show_country_flags: false       
    default_locale: '%env(currentLocale:DEFAULT_LOCALE)%'
    gedmo:
        enabled: true

with a

stof_doctrine_extensions:
    default_locale: '%locale%'
    translation_fallback: false
    persist_default_translation: true

wokenlex commented 2 years ago

It has some kind of regression (maybe?) after migration from 3 to 4:

I had this problem again with the persist_default_translation: true, what has worked before. It's damaging main language now. I did persist_default_translation: false, and it's probably works now (I has given it to the tests)

sonata-project / SonataTranslationBundle