Admin embedded collection seems to remove entities with by_reference=false

[yann-eugone]

Disclaimer : This is a really weird bug for which I had some bad time understanding why this is happening. Explanation might not be as clear as possible. But I'm fully available fore more information/demo.

Environment

Sonata packages

sonata-project/admin-bundle              3.78.1 3.78.1 The missing Symfony Admin Generator
sonata-project/block-bundle              4.4.0  4.4.0  Symfony SonataBlockBundle
sonata-project/cache                     2.0.1  2.0.1  Cache library
sonata-project/doctrine-extensions       1.10.1 1.10.1 Doctrine2 behavioral extensions
sonata-project/doctrine-orm-admin-bundle 3.24.0 3.24.0 Integrate Doctrine ORM into the SonataAdminBundle
sonata-project/exporter                  2.4.1  2.4.1  Lightweight Exporter library
sonata-project/form-extensions           1.6.0  1.6.0  Symfony form extensions
sonata-project/twig-extensions           1.4.1  1.4.1  Sonata twig extensions

Symfony packages

symfony/asset                      v4.4.16 v4.4.16 Symfony Asset Component
symfony/cache                      v4.4.16 v4.4.16 Symfony Cache component with PSR-6, PSR-16, and tags
symfony/cache-contracts            v2.2.0  v2.2.0  Generic abstractions related to caching
symfony/config                     v4.4.16 v4.4.16 Symfony Config Component
symfony/console                    v4.4.16 v4.4.16 Symfony Console Component
symfony/debug                      v4.4.16 v4.4.16 Symfony Debug Component
symfony/debug-bundle               v4.4.16 v4.4.16 Symfony DebugBundle
symfony/dependency-injection       v4.4.16 v4.4.16 Symfony DependencyInjection Component
symfony/doctrine-bridge            v4.4.16 v4.4.16 Symfony Doctrine Bridge
symfony/dotenv                     v4.4.16 v4.4.16 Registers environment variables from a .env file
symfony/error-handler              v4.4.16 v4.4.16 Symfony ErrorHandler Component
symfony/event-dispatcher           v4.4.16 v4.4.16 Symfony EventDispatcher Component
symfony/event-dispatcher-contracts v1.1.9  v2.2.0  Generic abstractions related to dispatching event
symfony/expression-language        v4.4.16 v4.4.16 Symfony ExpressionLanguage Component
symfony/filesystem                 v4.4.16 v4.4.16 Symfony Filesystem Component
symfony/finder                     v4.4.16 v4.4.16 Symfony Finder Component
symfony/flex                       v1.9.10 v1.9.10 Composer plugin for Symfony
symfony/form                       v4.4.16 v4.4.16 Symfony Form Component
symfony/framework-bundle           v4.4.16 v4.4.16 Symfony FrameworkBundle
symfony/http-client-contracts      v2.3.1  v2.3.1  Generic abstractions related to HTTP clients
symfony/http-foundation            v4.4.16 v4.4.16 Symfony HttpFoundation Component
symfony/http-kernel                v4.4.16 v4.4.16 Symfony HttpKernel Component
symfony/inflector                  v4.4.16 v4.4.16 Symfony Inflector Component
symfony/intl                       v4.4.16 v4.4.16 A PHP replacement layer for the C intl extension that includes additional data from the ICU library.
symfony/maker-bundle               v1.23.0 v1.23.0 Symfony Maker helps you create empty commands, controllers, form classes, tests and more so you can forget about writing boilerplate code.
symfony/mime                       v4.4.16 v4.4.16 A library to manipulate MIME messages
symfony/monolog-bridge             v4.4.16 v4.4.16 Symfony Monolog Bridge
symfony/monolog-bundle             v3.6.0  v3.6.0  Symfony MonologBundle
symfony/options-resolver           v4.4.16 v4.4.16 Symfony OptionsResolver Component
symfony/polyfill-intl-grapheme     v1.20.0 v1.20.0 Symfony polyfill for intl's grapheme_* functions
symfony/polyfill-intl-icu          v1.20.0 v1.20.0 Symfony polyfill for intl's ICU-related data and classes
symfony/polyfill-intl-idn          v1.20.0 v1.20.0 Symfony polyfill for intl's idn_to_ascii and idn_to_utf8 functions
symfony/polyfill-intl-normalizer   v1.20.0 v1.20.0 Symfony polyfill for intl's Normalizer class and related functions
symfony/polyfill-mbstring          v1.20.0 v1.20.0 Symfony polyfill for the Mbstring extension
symfony/polyfill-php72             v1.20.0 v1.20.0 Symfony polyfill backporting some PHP 7.2+ features to lower PHP versions
symfony/polyfill-php73             v1.20.0 v1.20.0 Symfony polyfill backporting some PHP 7.3+ features to lower PHP versions
symfony/polyfill-php80             v1.20.0 v1.20.0 Symfony polyfill backporting some PHP 8.0+ features to lower PHP versions
symfony/property-access            v4.4.16 v4.4.16 Symfony PropertyAccess Component
symfony/routing                    v4.4.16 v4.4.16 Symfony Routing Component
symfony/security-acl               v3.1.0  v3.1.0  Symfony Security Component - ACL (Access Control List)
symfony/security-bundle            v4.4.16 v4.4.16 Symfony SecurityBundle
symfony/security-core              v4.4.16 v4.4.16 Symfony Security Component - Core Library
symfony/security-csrf              v4.4.16 v4.4.16 Symfony Security Component - CSRF Library
symfony/security-guard             v4.4.16 v4.4.16 Symfony Security Component - Guard
symfony/security-http              v4.4.16 v4.4.16 Symfony Security Component - HTTP Integration
symfony/service-contracts          v2.2.0  v2.2.0  Generic abstractions related to writing services
symfony/stopwatch                  v4.4.16 v4.4.16 Symfony Stopwatch Component
symfony/string                     v5.1.8  v5.1.8  Symfony String component
symfony/translation                v4.4.16 v4.4.16 Symfony Translation Component
symfony/translation-contracts      v2.3.0  v2.3.0  Generic abstractions related to translation
symfony/twig-bridge                v4.4.16 v4.4.16 Symfony Twig Bridge
symfony/twig-bundle                v4.4.16 v4.4.16 Symfony TwigBundle
symfony/validator                  v4.4.16 v4.4.16 Symfony Validator Component
symfony/var-dumper                 v4.4.16 v4.4.16 Symfony mechanism for exploring and dumping PHP variables
symfony/var-exporter               v4.4.16 v4.4.16 A blend of var_export() + serialize() to turn any serializable data structure to plain PHP code
symfony/web-profiler-bundle        v4.4.16 v4.4.16 Symfony WebProfilerBundle
symfony/yaml                       v4.4.16 v4.4.16 Symfony Yaml Component

PHP version

PHP 7.4.12 (cli) (built: Oct 27 2020 15:01:52) ( NTS )
Copyright (c) The PHP Group
Zend Engine v3.4.0, Copyright (c) Zend Technologies
    with Zend OPcache v7.4.12, Copyright (c), by Zend Technologies

Subject

When you embed a collection of admin, using CollectionType & setting by_reference=false option.
All entries in collection are removed and re-created as new objects.

Steps to reproduce

I've created a sample project, with minimal code/dependencies : https://github.com/yann-eugone/sonata-collection-error.
In this project, I've added a UniqueEntity constraint onto Experience entity, in order to highlight the issue.

If you create a Person entity, with 1 Experience, and if you press Update right after object creation you will have the following error :

Expected results

Collection entries should be preserved.

Actual results

Collection entries are removed and re-created.

Further investigations

I've spent some time debugging why/where/when this is happening. Had some times in the property path classes, but not sure if this is the place to look.
I tested this behavior in a Symfony only project, and this behavior is not reproduced.
This is why i've opened this issue here.

I've also tried to downgrade my demo project to a lower version of sonata, but I also has to downgrade symfony : on the sonata highest version allowed with symfony 4.3, the issue is not reproduced either.

[VincentLanglet]

Thanks for the report @yann-eugone.

It's working in 3.77.
Seems related to #6438

The CollectionType by_reference => false option is now passed to the AdminType.
We need to know if the collection type has by_reference or not in order to call the right method addInstance/setObject.

But it seems that changing the by_reference option of the AdminType has other impact.

So it's maybe better to provide a collection_by_reference option
#6565

Then a PR would be neede here: https://github.com/sonata-project/SonataDoctrineORMAdminBundle/blob/3.x/src/Builder/FormContractor.php#L227
To change to

$typeOptions['collection_by_reference'] = $formOptions['by_reference'];

I currently don't see another solution.

[yann-eugone]

Thanks for this very quick answer @VincentLanglet :)

Looks like you were right : downgrading to 3.77 looks like a solution.

I tried to figure out which was the impacts of by_reference option, and there is only 2 in Symfony :

• Symfony\Component\Form\Form::setData causing PersistentCollection to be duplicated : but this exists for years now
• Symfony\Component\Form\Extension\Core\DataMapper\PropertyPathMapper::mapFormsToData causing collection to be "replaced" on submit : and this exists for a while too

Maybe it is just me not undserstanding how this works, but it can't see what part of the code produced between 3.77.0 & 3.78.1 is messing the whole thing up...

[VincentLanglet]

Maybe it is just me not understanding how this works, but it can't see what part of the code produced between 3.77.0 & 3.78.1 is messing the whole thing up...

Since 3.78 we pass the CollectionType options to persistence bundle.
See https://github.com/sonata-project/SonataAdminBundle/pull/6438/files#diff-9516ba91e79cc249e2c83431f248056fa195d97a4e3e533f6aab8657ef2ba093R134

And then, the persistence bundle send back type_options => ['by_reference' => $options['by_reference']]
See https://github.com/sonata-project/SonataDoctrineORMAdminBundle/blob/3.x/src/Builder/FormContractor.php#L227

And the bug occur because of

• Symfony\Component\Form\Form::setData causing PersistentCollection to be duplicated : but this exists for years now