Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
goetas opened this issue Oct 18, 2021 · 8 comments · Fixed by #7549
Closed

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

goetas opened this issue Oct 18, 2021 · 8 comments · Fixed by #7549
Labels

Comments

@goetas
Copy link
Contributor

goetas commented Oct 18, 2021

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.

final protected function appendParentObject(object $object): void

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)

@goetas goetas added the feature label Oct 18, 2021
@core23
Copy link
Member

core23 commented Oct 18, 2021

Maybe @VincentLanglet can help.

The method was introduced with this PR: #6171

@VincentLanglet
Copy link
Member

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
Copy link
Contributor Author

goetas commented Oct 19, 2021

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
Copy link
Member

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

@goetas
Copy link
Contributor Author

goetas commented Oct 19, 2021

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
Copy link
Member

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
Copy link
Contributor Author

goetas commented Oct 19, 2021

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
Copy link
Contributor Author

goetas commented Oct 19, 2021

see #7549

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants