search

sonata-project / SonataAdminBundle

The missing Symfony Admin Generator
https://docs.sonata-project.org/projects/SonataAdminBundle
MIT License
2.11k stars 1.26k forks source link

Skip setting the parent when creating a new entity in a child admin #7548

Closed goetas closed 3 years ago

goetas commented 3 years ago

Feature Request

Hi, would you be interested in a PR that inhibits somehow sonata by setting the parent entity for an entity. I'm referring more precisely to this piece of code.

https://github.com/sonata-project/SonataAdminBundle/blob/9669141cff19330b0d612e1ff3af14bb9c6fb36e/src/Admin/AbstractAdmin.php#L2189

We try to avoid having setters in our entities and especially for parent identifiers we try to pass them in the constructor. We do so by overriding createNewInstance.

Are you willing to accept a PR that allows to skip it? if yes, what it the recommended way? a protected method in the abstract admin? some property or what? (i think that it should be a per-admin setting, not a global one)

core23 commented 3 years ago

Maybe @VincentLanglet can help.

The method was introduced with this PR: https://github.com/sonata-project/SonataAdminBundle/pull/6171

VincentLanglet commented 3 years ago

The method was introduced with this PR: #6171

I did introduce the method, but the logic was here for a long time. I just try to add the same logic to a lot of other places, in order to set as possible parent object as possible.

@goetas What about checking there is not already a value (and we can also check this is the expected value) before trying to set one. This way the setter is only called if you don't set the value in the createNewInstance.

goetas commented 3 years ago

in 3.x we were overriding the getNewInstance method, but that method now is final, in the same way also appendParentObject is final.

@goetas What about checking there is not already a value (and we can also check this is the expected value) before trying to set one. This way the setter is only called if you don't set the value in the createNewInstance.

Do you mean to add more logic to appendParentObject ? I would suggest to not do so. TBH I do not understand what is the ideal of appendParentObject. If someone overrides createNewInstance it is their responsibility to provide all the needed dependencies for that class. Having appendParentObject defeats the propose of using createNewInstance.

VincentLanglet commented 3 years ago

Maybe moving the appendParentObject call from getNewInstance to createNewInstance could be the right way then

goetas commented 3 years ago

Maybe moving the appendParentObject call from getNewInstance to createNewInstance could be the right way then

@VincentLanglet That would solve it for sure! And i think that your is a better solution than what I have proposed at the beginning (some config option).

VincentLanglet commented 3 years ago

Maybe moving the appendParentObject call from getNewInstance to createNewInstance could be the right way then

@VincentLanglet That would solve it for sure! And i think that your is a better solution than what I have proposed at the beginning (some config option).

Can you open a PR ?

My only concerned is that can be considered as a BC break...

goetas commented 3 years ago

Can you open a PR ?

Sure, i will do it today.

My only concerned is that can be considered as a BC break...

That is true, but from a very strict point of view.... and sonate 4.x is very young :)

goetas commented 3 years ago

see https://github.com/sonata-project/SonataAdminBundle/pull/7549