Improve the way we append parent object if any #6171

VincentLanglet · 2020-06-26T08:56:44Z

Subject

I am targeting this branch, because this is BC (or at least I'm trying to).

When working on Embedded Admin and OneToMany field, I had a subject without the parent set, even if I was on the parent admin. After some investigations, I find out it was coming from this call:
https://github.com/sonata-project/SonataAdminBundle/blob/3.x/src/Resources/views/CRUD/Association/edit_one_to_many_inline_table.html.twig#L47

associationadmin.formfielddescriptions is calling getFormFieldDescriptions, which build the form.
There was some code to set the parent admin
https://github.com/sonata-project/SonataAdminBundle/blob/3.x/src/Admin/AbstractAdmin.php#L3356-L3373
but, it seems like the association admin was (obviously) returning false to isChild. I could instead use the parentFieldDescription like I did in some other situation.
https://github.com/sonata-project/SonataAdminBundle/blob/3.x/src/Form/Type/AdminType.php#L112

Moreover, as an improvement, I move the code who set the parent object to the subject in the newInstance method. Indeed:

When building a createForm, the getNewInstance method is called before buildForm. This way, the parent object is set sooner.
When building an editForm, the buildForm shouldn't modify the object.

Finally, I fixed the following bug #6166, by calling the child setter instead of the parent adder.

Close #6166

Changelog

### Fixed
- EmbeddedAdmin now correctly set the parent object when creating a new instance.
- Error message is correctly displayed for CollectionType

SonataCI · 2020-06-26T09:00:08Z

Could you please rebase your PR and fix merge conflicts?

src/Admin/AdminHelper.php

UPGRADE-3.x.md

core23 · 2020-06-28T17:00:51Z

src/Admin/AdminHelper.php

+    }
+
+    /**
+     * @template T of object


You should use @phpstan-template, there were some problem with the general @template annotation, as it may be used for symfony templates

You should also move the template generic to the AdminHelper class, not the method

You should also move the template generic to the AdminHelper class, not the method

It's allowed for method https://arnaud.le-blanc.net/post/phpstan-generics.html

In this case, I don't think the template should be moved to the class. This is not class-related.
I can use the method with an object Foo and then use it with an object Bar.

IMHO, you add the template tag when it's related to a property of the class (with construct, add, get, ...).

I still changed template for phpstan-template.

jordisala1991 · 2020-06-28T20:17:12Z

If this is fixing a bug why is not a patch instead of minor?

VincentLanglet · 2020-06-28T20:28:44Z

If this is fixing a bug why is not a patch instead of minor?

I added deprecation.

core23 · 2020-07-07T12:45:44Z

src/Admin/AbstractAdmin.php

+    /**
+     * Set the parent object, if any, to the provided object.
+     */
+    final protected function appendParentObject(object $object): void


Do you need this method or can you make it private

I don't need it.
But since it's called in the getNewInstance and I know some users are overridden this method (maybe without calling the parent method), I was thinking it could be useful to expose the appendParentObject method.

src/Form/Type/AdminType.php

tests/Admin/AdminTest.php

UPGRADE-3.x.md

src/Form/Type/AdminType.php

tests/Admin/AdminHelperTest.php

VincentLanglet · 2020-07-12T14:07:38Z

@core23 Your approval is required to merge this bugfix. Thanks :)

src/Form/Type/AdminType.php

src/Manipulator/ObjectManipulator.php

ossinkine · 2020-09-28T13:03:24Z

src/Form/Type/AdminType.php

-                        if (null !== $this->adminHelper) {
-                            $subject = $this->adminHelper->addNewInstance($parentSubject, $parentFieldDescription);
-                        }
+                        $subject = ObjectManipulator::setObject($admin->getNewInstance(), $parentSubject, $parentFieldDescription);


@VincentLanglet There is BC break here, parent object is set to new instance instead of adding new instance to parent collection, and this does not work for me

That's intended, to solve #6166
And it works well for me since month.

If you think a special case is missed create an issue with a precise description.
But without any information, I would say that your setter or mapping has something wrong.

Kal74 · 2020-10-16T08:00:51Z

src/Admin/AbstractAdmin.php

@@ -3382,26 +3386,6 @@ protected function buildForm()

        $this->loaded['form'] = true;

-        // append parent object if any
-        // todo : clean the way the Admin class can retrieve set the object
-        if ($this->isChild() && $this->getParentAssociationMapping()) {


Hi, @VincentLanglet There is BC break here.
In case of a create form in a subadmin, the parent is not anymore associated.

In my case, to workaround it, I need to associate the parent entity in the subAdmin::preCreate.

Can you create an issue with as much information as you have.
Or directly create the PR to call appendParentObject if needed and explain why ?

I called appendParentObject directly in getNewInstance which should be enough IMO, do you override this method ? @Kal74

Right, I overrided the getNewInstance!
Thanks.

peter-gribanov · 2020-10-23T14:32:13Z

src/Manipulator/ObjectManipulator.php

+             * NEXT_MAJOR: Use BadMethodCallException instead
+             */
+            throw new \RuntimeException(
+                sprintf('Method %s::%s() does not exist.', ClassUtils::getClass($instance), $method)


@VincentLanglet Why did you stop using a Symfony PropertyAccess? I got this exception because my DTO has no setters and getters, but only public properties.

There are multiple issue with PropertyAccess, see symfony/symfony#5013 (comment)

In our case, we wanted

To have different behavior when it's passed by reference or not.

To sometimes use Setter over Adder.

But I didn't change the strategy in this PR, it was already like this in AdminHelper:: addNewInstance.

We can still add a check for a public property. Do you want to create the PR ?

But I didn't change the strategy in this PR, it was already like this in AdminHelper:: addNewInstance.

So i messed up something.

But I didn't change the strategy in this PR, it was already like this in AdminHelper:: addNewInstance.

So i messed up something.

I did review again, and I changed something

Previously, only getter and adder was used.
Now, getter and setter or adder are used.

Instead of $parent->addChild(), I sometimes call $child->setParent().

antonymous · 2022-08-20T10:20:52Z

src/Admin/AbstractAdmin.php

+            }
+        } elseif ($this->hasParentFieldDescription()) {
+            $parentAdmin = $this->getParentFieldDescription()->getAdmin();
+            $parentObject = $parentAdmin->getObject($this->request->get($parentAdmin->getIdParameter()));


@VincentLanglet

This fails with embedded documents (MongoDB) when adding new document (let's say it's called Level2NestedDoc) at 2nd level of nesting: Class "App\Document\Level1NestedDoc" does not have an identifier.

Maybe nesting such deep is not a good design (isn't it OK-ish with MongoDB's embedded documents?), but it worked on Sonata Admin v.3.

Now, after update to v.4, to work around the issue we have to override AbstractAdmin::createNewInstance() to omit this method's call. But maybe instead we could

first check the mapping for embedded flag, and/or

then verify that request's id parameter matches parent admin by looking at request's admin code?

What do you think? Should I open an issue?

Better open an issue, this PR is 2 years old.

But, we won't check embedded in SonataAdmin since it's not related to any persistence bundle.

getObject is doing

final public function getObject($id): ?object { if (null === $id) { return null; } $object = $this->getModelManager()->find($this->getClass(), $id); if (null === $object) { return null; } $this->alterObject($object); foreach ($this->getExtensions() as $extension) { $extension->alterObject($this, $object); } return $object; }

Where does come from the Exception ?

If it's in the

$this->getModelManager()->find($this->getClass(), $id);

call, we could add some check in https://github.com/sonata-project/SonataDoctrineMongoDBAdminBundle/blob/4.x/src/Model/ModelManager.php

But its weird to have an entity without identifier, since all the admin (and parent admin) are supposed to be related to an entity/document.

VincentLanglet force-pushed the parentObject branch 2 times, most recently from 690aa10 to 479f1b4 Compare June 27, 2020 08:40

VincentLanglet marked this pull request as ready for review June 27, 2020 08:40

VincentLanglet force-pushed the parentObject branch 5 times, most recently from fdbca2e to f7bfe9e Compare June 27, 2020 22:25

VincentLanglet requested a review from a team June 27, 2020 22:42

greg0ire added bug minor Needs: Upgrade note labels Jun 28, 2020

greg0ire reviewed Jun 28, 2020

View reviewed changes

src/Admin/AdminHelper.php Show resolved Hide resolved

src/Admin/AdminHelper.php Outdated Show resolved Hide resolved

VincentLanglet force-pushed the parentObject branch from f7bfe9e to 27b2958 Compare June 28, 2020 09:03

greg0ire reviewed Jun 28, 2020

View reviewed changes

UPGRADE-3.x.md Outdated Show resolved Hide resolved

UPGRADE-3.x.md Outdated Show resolved Hide resolved

VincentLanglet force-pushed the parentObject branch 3 times, most recently from 942f72e to 46c9d00 Compare June 28, 2020 09:33

VincentLanglet requested a review from a team June 28, 2020 09:47

greg0ire previously approved these changes Jun 28, 2020

View reviewed changes

greg0ire requested a review from a team June 28, 2020 09:59

core23 requested changes Jun 28, 2020

View reviewed changes

VincentLanglet dismissed greg0ire’s stale review via c34c0e6 June 28, 2020 17:05

VincentLanglet force-pushed the parentObject branch from 46c9d00 to c34c0e6 Compare June 28, 2020 17:05

VincentLanglet requested a review from core23 June 28, 2020 19:55

jordisala1991 removed the Needs: Upgrade note label Jun 28, 2020

core23 previously approved these changes Jun 28, 2020

View reviewed changes

core23 reviewed Jul 7, 2020

View reviewed changes

VincentLanglet requested review from core23, greg0ire, jordisala1991 and phansys July 9, 2020 20:35

phansys requested changes Jul 10, 2020

View reviewed changes

VincentLanglet force-pushed the parentObject branch from f077ee8 to c1119bc Compare July 11, 2020 17:05

VincentLanglet requested a review from phansys July 11, 2020 17:06

phansys requested changes Jul 11, 2020

View reviewed changes

UPGRADE-3.x.md Outdated Show resolved Hide resolved

src/Form/Type/AdminType.php Outdated Show resolved Hide resolved

tests/Admin/AdminHelperTest.php Show resolved Hide resolved

VincentLanglet force-pushed the parentObject branch from c1119bc to e31ebc6 Compare July 11, 2020 17:34

phansys previously approved these changes Jul 11, 2020

View reviewed changes

VincentLanglet requested a review from a team July 12, 2020 13:47

core23 reviewed Jul 12, 2020

View reviewed changes

src/Form/Type/AdminType.php Outdated Show resolved Hide resolved

core23 requested changes Jul 12, 2020

View reviewed changes

src/Manipulator/ObjectManipulator.php Outdated Show resolved Hide resolved

Improve the way we append parent object if any

c50c60a

VincentLanglet dismissed phansys’s stale review via c50c60a July 12, 2020 18:49

VincentLanglet force-pushed the parentObject branch from e31ebc6 to c50c60a Compare July 12, 2020 18:49

VincentLanglet requested a review from core23 July 12, 2020 18:57

core23 approved these changes Jul 12, 2020

View reviewed changes

phansys approved these changes Jul 12, 2020

View reviewed changes

VincentLanglet merged commit 77c7c68 into sonata-project:3.x Jul 13, 2020

VincentLanglet deleted the parentObject branch July 13, 2020 09:39

ossinkine reviewed Sep 28, 2020

View reviewed changes

ossinkine mentioned this pull request Sep 28, 2020

Admin with children forms does not associate entities #6426

Closed

Kal74 reviewed Oct 16, 2020

View reviewed changes

peter-gribanov reviewed Oct 23, 2020

View reviewed changes

core23 mentioned this pull request Oct 18, 2021

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

Closed

antonymous reviewed Aug 30, 2022

View reviewed changes

antonymous mentioned this pull request Oct 30, 2022

MongoDB's embedded documents nested twice stopped working in Sonata v.4 sonata-project/SonataDoctrineMongoDBAdminBundle#789

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve the way we append parent object if any #6171

Improve the way we append parent object if any #6171

VincentLanglet commented Jun 26, 2020 •

edited

Loading

SonataCI commented Jun 26, 2020

core23 Jun 28, 2020

core23 Jun 28, 2020

VincentLanglet Jun 28, 2020

jordisala1991 commented Jun 28, 2020

VincentLanglet commented Jun 28, 2020

core23 Jul 7, 2020

VincentLanglet Jul 7, 2020 •

edited

Loading

VincentLanglet commented Jul 12, 2020 •

edited

Loading

ossinkine Sep 28, 2020

VincentLanglet Sep 28, 2020

Kal74 Oct 16, 2020

VincentLanglet Oct 16, 2020

Kal74 Oct 16, 2020

peter-gribanov Oct 23, 2020

VincentLanglet Oct 23, 2020

VincentLanglet Oct 23, 2020

peter-gribanov Oct 23, 2020

VincentLanglet Oct 23, 2020

antonymous Aug 20, 2022

VincentLanglet Aug 30, 2022

Improve the way we append parent object if any #6171

Improve the way we append parent object if any #6171

Conversation

VincentLanglet commented Jun 26, 2020 • edited Loading

Subject

Changelog

SonataCI commented Jun 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jordisala1991 commented Jun 28, 2020

VincentLanglet commented Jun 28, 2020

Choose a reason for hiding this comment

VincentLanglet Jul 7, 2020 • edited Loading

Choose a reason for hiding this comment

VincentLanglet commented Jul 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VincentLanglet commented Jun 26, 2020 •

edited

Loading

VincentLanglet Jul 7, 2020 •

edited

Loading

VincentLanglet commented Jul 12, 2020 •

edited

Loading