Move appendParentObject into createNewInstance

[goetas]

Move appendParentObject into createNewInstance to allow creating new instances for which the parent entity will not be managed by sonata

Subject

I am targeting this branch, because i think it is a bug.

Closes #7548

Changelog

### Changed
- Move appendParentObject call into createNewInstance

[VincentLanglet]

Can you Create a 4.x file similar to https://github.com/sonata-project/SonataAdminBundle/blob/4.x/UPGRADE-3.x.md and add some explanation for people overriding the createNewInstance without calling the constructor ?

[jordisala1991]

Wouldnt it be better to open another extension point instead? I dont think it is easy to understand that createNewInstance not only creates it but adds the parent object if it is needed.

What about opening the appendParentObject and you can leave it blank if you need to? Not sure if thats better, wdyt?

If we go this way we need to add a comment to clarify what this method does exactly.

[goetas]

@VincentLanglet i've added the 4.x upgrading document

[goetas]

@jordisala1991 I think that createNewInstance should create a fully working new instance, overriding that method should give you full control on the instance, and after my changes it will be exactly that situation.

if you do not override the createNewInstance i think that the resulting behavior of setting the parent is ok (it is the same as in sonata 3.x and it has been battle tested there)

[VincentLanglet]

Wouldnt it be better to open another extension point instead? I dont think it is easy to understand that createNewInstance not only creates it but adds the parent object if it is needed.

What about opening the appendParentObject and you can leave it blank if you need to? Not sure if thats better, wdyt?

I dunno what the best option, and we might need to take some time to find the best one indeed before merging something too fast.

I would say that adding a new extension point is a bad idea. We already have 2, the create and the alter. And it could be complicated to understand the difference between appendParentObject and alterNewInstance.

IMHO I see two solutions:

• Doing it this way: The create new instance has all the logic to generate a fully working object with all the property needed for the admin. The issue is that if someone need to override the method, because the constructor require some value, the method need to be called. This could be easily forgotten. And this PR is technically a BC-break.

• Doing it the other way i proposed Skip setting the parent when creating a new entity in a child admin #7548 (comment): The logic to append the parent object is modified in order to append the parent object ONLY if it's not already set. No BC-break and the issue should be solved too.

[goetas]

@VincentLanglet Am I understanding it right that your proposed solution will still need getters/setters in the entity? if yes, i think that it still defeats the propose of trying to avoid anemic domain model 1 2

IMO the solution proposed in this MR is simple fulfills the idea "createNewInstance creates a fully working new instance". making appendParentObject even more complex is a bad idea.

[core23]

IMO the solution proposed in this MR is simple fulfills the idea "createNewInstance creates a fully working new instance". making appendParentObject even more complex is a bad idea.

👍 The method createNewInstance should create a full working instance. The current implementation is indeed problematic if you want to entities without setters.

There is one extension point (alterNewInstance) if you must change something. There is no need for more hooks.

[VincentLanglet]

@VincentLanglet Am I understanding it right that your proposed solution will still need getters/setters in the entity? if yes, i think that it still defeats the propose of trying to avoid anemic domain model 1 2

getters/setters are not needed in the entity, just a way to access to the value.

Using property accessor to accessing a value to display or set any value when editing an object is already the strategy of every Sonata show/edit/... routes. I understood you removed some setters, but I don't see how your entity could work without having getter or public readonly property too.

IMO the solution proposed in this MR is simple fulfills the idea "createNewInstance creates a fully working new instance". making appendParentObject even more complex is a bad idea.

Your solution might seems good to you because you don't want the appendParent call, but every others users will need to add this call if they override the method for another reason.

[core23]

Using property accessor to accessing a value to display or set any value when editing an object is already the strategy of every Sonata show/edit/... routes. I understood you removed some setters, but I don't see how your entity could work without having getter or public readonly property too.

We have docs for this: https://docs.sonata-project.org/projects/SonataAdminBundle/en/4.x/cookbook/recipe_data_mapper/

[VincentLanglet]

Maybe try/catching the call might solve the issue then

[franmomu]

@goetas just curious, how do you deal with required parameters in the constructor of the entities (if they are provided by the form)?

[goetas]

The data mapper works for the props in the form but i need to access the entity from the parent admin, and that it is not easy to do from the from.

My entities require in the constructor the things that are used to "identify" somehow the entity.
To make an example:

In out application a "user" belongs always to a "company", thus it is not possible to create in any point an user without a company. Also a user can never change company.

To enforce this the user entity requires the company entity in the constructor (and we have no setters to set a company). In this way the entity is never in an invalid state (because someone forgot to call the setCompany method as example).

In sonata 3.x we were achieving it by overriding the getNewInstance method, but that method is now final, thus i was trying to do it using createNewInstance.. but then there is the appendParentObject method that tries to do the same (setting the parent entity).

In 3.x our code was looking as follows.

class Company {
 /// ....
}
class User {

  function __construct (private Company $company);

}

class UserAdmin extends AbstractAdmin {
   /// ....
   function getNewInstance() {
     return new User($this->getParent()->getSubject());
   }
}

with the change i propose, migrating would be trivial, it is just about changing the method name, so:

class UserAdmin extends AbstractAdmin {
   /// ....
   function createNewInstance() {
     return new User($this->getParent()->getSubject());
   }
}

Without this change we are forced to add setters in all the entities, that will allow to potentially break the entity consistency.

[goetas]

@VincentLanglet

Maybe try/catching the call might solve the issue then

where would you put the try catch? currently we have no control over appendParentObject and the place where is called

[VincentLanglet]

@VincentLanglet

Maybe try/catching the call might solve the issue then

where would you put the try catch? currently we have no control over appendParentObject and the place where is called

If I understand correctly the appendParnetObject is throwing an exception, maybe we can add a try/catch in the sonata code around the property accessor to handle case there is nothing to access...

[goetas]

@franmomu

how do you deal with required parameters in the constructor of the entities (if they are provided by the form)?

that is a good question. currently we don't. we try to reduce the number of required parameters only to relations that are required at business-layer.
As example: a user requires a company, an access token requires an user, an so on.
Cases in which the user requires also a name (beside the company), we have a setter for it...

TBH i did not try to solve this case yet, it would be good to find a general solution for it... but i'm not sure if the data mapper has access to the parent admin subject.

[goetas]

@VincentLanglet

If I understand correctly the appendParnetObject is throwing an exception, maybe we can add a try/catch in the sonata code around the property accessor to handle case there is nothing to access...

adding a try-catch for this kind of error feels as hiding the error under the carpet.

[VincentLanglet]

adding a try-catch for this kind of error feels as hiding the error under the carpet.

I'm not sure about it, it's more about changing the appendParentObject to a appendParentObjectIfPossible method.
One big benefit is the fact it avoid an important BC break here.

But don't want to be a blocker if @sonata-project/contributors are ok with this solution.

.

[franmomu]

I used to override getNewInstance as well to return null and be able to control how the models were created with the form mapper, but not anymore, having to relax the constructors to create non-valid entities 😞

[VincentLanglet]

Changelog is also needed in the first message of the PR

[goetas]

thank you!