Prevent false deprecation error for n to m associations #6411

jorrit · 2020-09-24T11:16:31Z

Subject

CRUDController->checkParentChildAssociation() checks if the associated item of the child admins subject equals the parent admins subject. This doesn't work when the parent relations is many-to-many as the child property will be a Collection and not the parent item itself. In that case, the parent item must be searched in this collection.

I am targeting this branch, because its a backwards compatible fix.

Changelog

### Fixed
- Prevent false deprecation notice when accessing an item in a child admin that has a Many to Many relationship with the parent item.

src/Controller/CRUDController.php

franmomu

These are suggestions.

franmomu · 2020-10-01T22:44:26Z

src/Controller/CRUDController.php

@@ -1554,7 +1554,13 @@ private function checkParentChildAssociation(Request $request, object $object):
        $propertyAccessor = PropertyAccess::createPropertyAccessor();
        $propertyPath = new PropertyPath($this->admin->getParentAssociationMapping());

-        if ($parentAdmin->getObject($parentId) !== $propertyAccessor->getValue($object, $propertyPath)) {
+        $parentObject = $parentAdmin->getObject($parentId);


What about calling this $parentAdminObject? And the one below $parentObject. To be honest, I can't come up with something better, but $parentObject and $objectParent sound strange.

parentAdminObject will be OK.
objectParent is bad idea, use parentAdminAvailableObjects instead.

franmomu · 2020-10-01T23:00:11Z

src/Controller/CRUDController.php

+     *
+     * @return bool true when $haystack equals $needle or $haystack is iterable and contains $needle
+     */
+    private static function equalsOrInList(object $needle, $haystack): bool


Even if it doesn't need $this, is there any benefit to create it as static?

Looking at the method name, what about:

Suggested change

private static function equalsOrInList(object $needle, $haystack): bool

private function hasParentChildAssociation(object $parent, $parentAssociation): bool

If this is static, it could be in another class instead of this big CRUDController.

IMHO we can keep it in CRUDController and add it to #6512.
Of course remove static.

src/Controller/CRUDController.php

VincentLanglet · 2021-01-12T09:35:43Z

Hi @jorrit, are you still interested in this PR ?

jorrit · 2021-01-12T09:44:24Z

Yes, sorry for not responding. Can you recommend a class to put the static method into?

VincentLanglet · 2021-01-12T12:53:20Z

Can you recommend a class to put the static method into?

cc @sonata-project/contributors ; Any idea ?

wbloszyk

@jorrit I add some comments.

wbloszyk · 2021-01-12T13:23:36Z

src/Controller/CRUDController.php

+     *
+     * @return bool true when $haystack equals $needle or $haystack is iterable and contains $needle
+     */
+    private static function equalsOrInList(object $needle, $haystack): bool


IMHO we can keep it in CRUDController and add it to #6512.
Of course remove static.

wbloszyk · 2021-01-12T14:03:17Z

src/Controller/CRUDController.php

@@ -1554,7 +1554,13 @@ private function checkParentChildAssociation(Request $request, object $object):
        $propertyAccessor = PropertyAccess::createPropertyAccessor();
        $propertyPath = new PropertyPath($this->admin->getParentAssociationMapping());

-        if ($parentAdmin->getObject($parentId) !== $propertyAccessor->getValue($object, $propertyPath)) {
+        $parentObject = $parentAdmin->getObject($parentId);


parentAdminObject will be OK.
objectParent is bad idea, use parentAdminAvailableObjects instead.

jorrit · 2021-01-19T10:04:33Z

@wbloszyk : thanks, I've made the changes you suggested.

VincentLanglet · 2021-01-31T15:19:28Z

Please review @sonata-project/contributors @wbloszyk

src/Controller/CRUDController.php

Changes are made

VincentLanglet · 2021-02-22T21:48:51Z

Thanks @jorrit

VincentLanglet previously approved these changes Sep 24, 2020

View reviewed changes

franmomu reviewed Sep 24, 2020

View reviewed changes

src/Controller/CRUDController.php Outdated Show resolved Hide resolved

jorrit dismissed VincentLanglet’s stale review via 90bb98e September 29, 2020 08:03

jorrit force-pushed the parentchildmanymany branch 3 times, most recently from d23fbb3 to 4b3945d Compare September 29, 2020 08:17

jordisala1991 reviewed Sep 29, 2020

View reviewed changes

src/Controller/CRUDController.php Outdated Show resolved Hide resolved

jorrit force-pushed the parentchildmanymany branch 3 times, most recently from 5757950 to 90c6c5d Compare October 1, 2020 13:10

VincentLanglet requested review from franmomu and jordisala1991 October 1, 2020 14:07

franmomu reviewed Oct 1, 2020

View reviewed changes

VincentLanglet added this to the 4.0 milestone Dec 24, 2020

jorrit force-pushed the parentchildmanymany branch from 90c6c5d to 869c460 Compare January 12, 2021 09:47

wbloszyk previously requested changes Jan 12, 2021

View reviewed changes

Prevent false deprecation error for n to m associations

2a6a064

jorrit force-pushed the parentchildmanymany branch from 869c460 to 2a6a064 Compare January 19, 2021 10:04

VincentLanglet requested review from wbloszyk and a team January 19, 2021 12:32

VincentLanglet added the patch label Jan 24, 2021

VincentLanglet requested a review from franmomu January 31, 2021 15:15

VincentLanglet previously approved these changes Jan 31, 2021

View reviewed changes

VincentLanglet requested a review from a team February 22, 2021 20:56

phansys reviewed Feb 22, 2021

View reviewed changes

src/Controller/CRUDController.php Outdated Show resolved Hide resolved

VincentLanglet reviewed Feb 22, 2021

View reviewed changes

src/Controller/CRUDController.php Outdated Show resolved Hide resolved

src/Controller/CRUDController.php Outdated Show resolved Hide resolved

src/Controller/CRUDController.php Outdated Show resolved Hide resolved

Update src/Controller/CRUDController.php

42b20ed

VincentLanglet dismissed their stale review via 42b20ed February 22, 2021 21:34

VincentLanglet added 2 commits February 22, 2021 22:34

Update src/Controller/CRUDController.php

cb5535b

Update src/Controller/CRUDController.php

7d86dca

phansys approved these changes Feb 22, 2021

View reviewed changes

VincentLanglet merged commit 6a7c306 into sonata-project:3.x Feb 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent false deprecation error for n to m associations #6411

Prevent false deprecation error for n to m associations #6411

jorrit commented Sep 24, 2020

franmomu left a comment

franmomu Oct 1, 2020

wbloszyk Jan 12, 2021

franmomu Oct 1, 2020

VincentLanglet Dec 24, 2020

wbloszyk Jan 12, 2021 •

edited

Loading

VincentLanglet commented Jan 12, 2021

jorrit commented Jan 12, 2021

VincentLanglet commented Jan 12, 2021

wbloszyk left a comment

wbloszyk Jan 12, 2021 •

edited

Loading

wbloszyk Jan 12, 2021

jorrit commented Jan 19, 2021

VincentLanglet commented Jan 31, 2021

VincentLanglet commented Feb 22, 2021

	private static function equalsOrInList(object $needle, $haystack): bool
	private function hasParentChildAssociation(object $parent, $parentAssociation): bool

Prevent false deprecation error for n to m associations #6411

Prevent false deprecation error for n to m associations #6411

Conversation

jorrit commented Sep 24, 2020

Subject

Changelog

franmomu left a comment

Choose a reason for hiding this comment

franmomu Oct 1, 2020

Choose a reason for hiding this comment

wbloszyk Jan 12, 2021

Choose a reason for hiding this comment

franmomu Oct 1, 2020

Choose a reason for hiding this comment

VincentLanglet Dec 24, 2020

Choose a reason for hiding this comment

wbloszyk Jan 12, 2021 • edited Loading

Choose a reason for hiding this comment

VincentLanglet commented Jan 12, 2021

jorrit commented Jan 12, 2021

VincentLanglet commented Jan 12, 2021

wbloszyk left a comment

Choose a reason for hiding this comment

wbloszyk Jan 12, 2021 • edited Loading

Choose a reason for hiding this comment

wbloszyk Jan 12, 2021

Choose a reason for hiding this comment

jorrit commented Jan 19, 2021

VincentLanglet commented Jan 31, 2021

VincentLanglet commented Feb 22, 2021

wbloszyk Jan 12, 2021 •

edited

Loading

wbloszyk Jan 12, 2021 •

edited

Loading