From e5e3166747f75892e799ace32061faf7354094ca Mon Sep 17 00:00:00 2001 From: Dmitry Bannik Date: Tue, 13 Feb 2024 14:15:19 +0300 Subject: [PATCH 1/5] #11090 - Fix obtaining an identifier in cases where the hydration has not yet fully completed on eagerLoadCollections --- src/UnitOfWork.php | 12 +++- .../WithFetchEager/AbstractRemoveControl.php | 49 ++++++++++++++ .../WithFetchEager/MobileRemoteControl.php | 14 ++++ .../WithFetchEager/User.php | 35 ++++++++++ .../AbstractRemoveControl.php | 49 ++++++++++++++ .../WithoutFetchEager/MobileRemoteControl.php | 14 ++++ .../WithoutFetchEager/User.php | 35 ++++++++++ .../ORM/Functional/AbstractFetchEagerTest.php | 64 +++++++++++++++++++ 8 files changed, 271 insertions(+), 1 deletion(-) create mode 100644 tests/Tests/Models/AbstractFetchEager/WithFetchEager/AbstractRemoveControl.php create mode 100644 tests/Tests/Models/AbstractFetchEager/WithFetchEager/MobileRemoteControl.php create mode 100644 tests/Tests/Models/AbstractFetchEager/WithFetchEager/User.php create mode 100644 tests/Tests/Models/AbstractFetchEager/WithoutFetchEager/AbstractRemoveControl.php create mode 100644 tests/Tests/Models/AbstractFetchEager/WithoutFetchEager/MobileRemoteControl.php create mode 100644 tests/Tests/Models/AbstractFetchEager/WithoutFetchEager/User.php create mode 100644 tests/Tests/ORM/Functional/AbstractFetchEagerTest.php diff --git a/src/UnitOfWork.php b/src/UnitOfWork.php index 00698e56c60..07903c3e6c0 100644 --- a/src/UnitOfWork.php +++ b/src/UnitOfWork.php @@ -3252,7 +3252,17 @@ private function eagerLoadCollections(array $collections, array $mapping): void foreach ($found as $targetValue) { $sourceEntity = $targetProperty->getValue($targetValue); - $id = $this->identifierFlattener->flattenIdentifier($class, $class->getIdentifierValues($sourceEntity)); + // In cases where the hydration $targetValue has not yet fully completed + if ($sourceEntity === null && isset($targetClass->associationMappings[$mappedBy]['joinColumns'])) { + $data = $this->getOriginalEntityData($targetValue); + $id = []; + foreach ($targetClass->associationMappings[$mappedBy]['joinColumns'] as $joinColumn) { + $id[] = $data[$joinColumn['name']]; + } + } else { + $id = $this->identifierFlattener->flattenIdentifier($class, $class->getIdentifierValues($sourceEntity)); + } + $idHash = implode(' ', $id); if (isset($mapping['indexBy'])) { diff --git a/tests/Tests/Models/AbstractFetchEager/WithFetchEager/AbstractRemoveControl.php b/tests/Tests/Models/AbstractFetchEager/WithFetchEager/AbstractRemoveControl.php new file mode 100644 index 00000000000..7c69c15d75a --- /dev/null +++ b/tests/Tests/Models/AbstractFetchEager/WithFetchEager/AbstractRemoveControl.php @@ -0,0 +1,49 @@ + + */ + public $users; + + public function __construct(string $name) + { + $this->name = $name; + $this->users = new ArrayCollection(); + } +} diff --git a/tests/Tests/Models/AbstractFetchEager/WithFetchEager/MobileRemoteControl.php b/tests/Tests/Models/AbstractFetchEager/WithFetchEager/MobileRemoteControl.php new file mode 100644 index 00000000000..cdbca6f3f0f --- /dev/null +++ b/tests/Tests/Models/AbstractFetchEager/WithFetchEager/MobileRemoteControl.php @@ -0,0 +1,14 @@ +remoteControl = $control; + } +} diff --git a/tests/Tests/Models/AbstractFetchEager/WithoutFetchEager/AbstractRemoveControl.php b/tests/Tests/Models/AbstractFetchEager/WithoutFetchEager/AbstractRemoveControl.php new file mode 100644 index 00000000000..82bb2f705f6 --- /dev/null +++ b/tests/Tests/Models/AbstractFetchEager/WithoutFetchEager/AbstractRemoveControl.php @@ -0,0 +1,49 @@ + + */ + public $users; + + public function __construct(string $name) + { + $this->name = $name; + $this->users = new ArrayCollection(); + } +} diff --git a/tests/Tests/Models/AbstractFetchEager/WithoutFetchEager/MobileRemoteControl.php b/tests/Tests/Models/AbstractFetchEager/WithoutFetchEager/MobileRemoteControl.php new file mode 100644 index 00000000000..6628234450f --- /dev/null +++ b/tests/Tests/Models/AbstractFetchEager/WithoutFetchEager/MobileRemoteControl.php @@ -0,0 +1,14 @@ +remoteControl = $control; + } +} diff --git a/tests/Tests/ORM/Functional/AbstractFetchEagerTest.php b/tests/Tests/ORM/Functional/AbstractFetchEagerTest.php new file mode 100644 index 00000000000..14e50f70b4b --- /dev/null +++ b/tests/Tests/ORM/Functional/AbstractFetchEagerTest.php @@ -0,0 +1,64 @@ +createSchemaForModels( + AbstractRemoveControl::class, + User::class + ); + + $control = new MobileRemoteControl('smart'); + $user = new User($control); + + $entityManage = $this->getEntityManager(); + + $entityManage->persist($control); + $entityManage->persist($user); + $entityManage->flush(); + $entityManage->clear(); + + $user = $entityManage->find(User::class, $user->id); + + self::assertNotNull($user); + self::assertEquals('smart', $user->remoteControl->name); + self::assertTrue($user->remoteControl->users->contains($user)); + } + + public function testWithoutAbstractFetchEager(): void + { + $this->createSchemaForModels( + AbstractRemoveControlWithoutFetchEager::class, + UserWithoutFetchEager::class + ); + + $control = new MobileRemoteControlWithoutFetchEager('smart'); + $user = new UserWithoutFetchEager($control); + + $entityManage = $this->getEntityManager(); + + $entityManage->persist($control); + $entityManage->persist($user); + $entityManage->flush(); + $entityManage->clear(); + + $user = $entityManage->find(UserWithoutFetchEager::class, $user->id); + + self::assertNotNull($user); + self::assertEquals('smart', $user->remoteControl->name); + self::assertTrue($user->remoteControl->users->contains($user)); + } +} From 16f355f0cc9180375c19755372210a11b7d6df1e Mon Sep 17 00:00:00 2001 From: Benjamin Eberlei Date: Sat, 16 Mar 2024 20:31:09 +0100 Subject: [PATCH 2/5] Remove tests for already working case as they add no value other than exploration, and we only need the regression test. --- .../AbstractRemoveControl.php | 2 +- .../MobileRemoteControl.php | 2 +- .../{WithFetchEager => }/User.php | 2 +- .../AbstractRemoveControl.php | 49 ------------------- .../WithoutFetchEager/MobileRemoteControl.php | 14 ------ .../WithoutFetchEager/User.php | 35 ------------- .../ORM/Functional/AbstractFetchEagerTest.php | 33 ++----------- 7 files changed, 6 insertions(+), 131 deletions(-) rename tests/Tests/Models/AbstractFetchEager/{WithFetchEager => }/AbstractRemoveControl.php (93%) rename tests/Tests/Models/AbstractFetchEager/{WithFetchEager => }/MobileRemoteControl.php (69%) rename tests/Tests/Models/AbstractFetchEager/{WithFetchEager => }/User.php (89%) delete mode 100644 tests/Tests/Models/AbstractFetchEager/WithoutFetchEager/AbstractRemoveControl.php delete mode 100644 tests/Tests/Models/AbstractFetchEager/WithoutFetchEager/MobileRemoteControl.php delete mode 100644 tests/Tests/Models/AbstractFetchEager/WithoutFetchEager/User.php diff --git a/tests/Tests/Models/AbstractFetchEager/WithFetchEager/AbstractRemoveControl.php b/tests/Tests/Models/AbstractFetchEager/AbstractRemoveControl.php similarity index 93% rename from tests/Tests/Models/AbstractFetchEager/WithFetchEager/AbstractRemoveControl.php rename to tests/Tests/Models/AbstractFetchEager/AbstractRemoveControl.php index 7c69c15d75a..a2874469304 100644 --- a/tests/Tests/Models/AbstractFetchEager/WithFetchEager/AbstractRemoveControl.php +++ b/tests/Tests/Models/AbstractFetchEager/AbstractRemoveControl.php @@ -2,7 +2,7 @@ declare(strict_types=1); -namespace Doctrine\Tests\Models\AbstractFetchEager\WithFetchEager; +namespace Doctrine\Tests\Models\AbstractFetchEager; use Doctrine\Common\Collections\ArrayCollection; use Doctrine\Common\Collections\Collection; diff --git a/tests/Tests/Models/AbstractFetchEager/WithFetchEager/MobileRemoteControl.php b/tests/Tests/Models/AbstractFetchEager/MobileRemoteControl.php similarity index 69% rename from tests/Tests/Models/AbstractFetchEager/WithFetchEager/MobileRemoteControl.php rename to tests/Tests/Models/AbstractFetchEager/MobileRemoteControl.php index cdbca6f3f0f..adb7c22fd08 100644 --- a/tests/Tests/Models/AbstractFetchEager/WithFetchEager/MobileRemoteControl.php +++ b/tests/Tests/Models/AbstractFetchEager/MobileRemoteControl.php @@ -2,7 +2,7 @@ declare(strict_types=1); -namespace Doctrine\Tests\Models\AbstractFetchEager\WithFetchEager; +namespace Doctrine\Tests\Models\AbstractFetchEager; use Doctrine\ORM\Mapping as ORM; diff --git a/tests/Tests/Models/AbstractFetchEager/WithFetchEager/User.php b/tests/Tests/Models/AbstractFetchEager/User.php similarity index 89% rename from tests/Tests/Models/AbstractFetchEager/WithFetchEager/User.php rename to tests/Tests/Models/AbstractFetchEager/User.php index 6c34501e730..0ee7b24891b 100644 --- a/tests/Tests/Models/AbstractFetchEager/WithFetchEager/User.php +++ b/tests/Tests/Models/AbstractFetchEager/User.php @@ -2,7 +2,7 @@ declare(strict_types=1); -namespace Doctrine\Tests\Models\AbstractFetchEager\WithFetchEager; +namespace Doctrine\Tests\Models\AbstractFetchEager; use Doctrine\ORM\Mapping as ORM; diff --git a/tests/Tests/Models/AbstractFetchEager/WithoutFetchEager/AbstractRemoveControl.php b/tests/Tests/Models/AbstractFetchEager/WithoutFetchEager/AbstractRemoveControl.php deleted file mode 100644 index 82bb2f705f6..00000000000 --- a/tests/Tests/Models/AbstractFetchEager/WithoutFetchEager/AbstractRemoveControl.php +++ /dev/null @@ -1,49 +0,0 @@ - - */ - public $users; - - public function __construct(string $name) - { - $this->name = $name; - $this->users = new ArrayCollection(); - } -} diff --git a/tests/Tests/Models/AbstractFetchEager/WithoutFetchEager/MobileRemoteControl.php b/tests/Tests/Models/AbstractFetchEager/WithoutFetchEager/MobileRemoteControl.php deleted file mode 100644 index 6628234450f..00000000000 --- a/tests/Tests/Models/AbstractFetchEager/WithoutFetchEager/MobileRemoteControl.php +++ /dev/null @@ -1,14 +0,0 @@ -remoteControl = $control; - } -} diff --git a/tests/Tests/ORM/Functional/AbstractFetchEagerTest.php b/tests/Tests/ORM/Functional/AbstractFetchEagerTest.php index 14e50f70b4b..975a97fa7a9 100644 --- a/tests/Tests/ORM/Functional/AbstractFetchEagerTest.php +++ b/tests/Tests/ORM/Functional/AbstractFetchEagerTest.php @@ -4,12 +4,9 @@ namespace Doctrine\Tests\ORM\Functional; -use Doctrine\Tests\Models\AbstractFetchEager\WithFetchEager\AbstractRemoveControl; -use Doctrine\Tests\Models\AbstractFetchEager\WithFetchEager\MobileRemoteControl; -use Doctrine\Tests\Models\AbstractFetchEager\WithFetchEager\User; -use Doctrine\Tests\Models\AbstractFetchEager\WithoutFetchEager\AbstractRemoveControl as AbstractRemoveControlWithoutFetchEager; -use Doctrine\Tests\Models\AbstractFetchEager\WithoutFetchEager\MobileRemoteControl as MobileRemoteControlWithoutFetchEager; -use Doctrine\Tests\Models\AbstractFetchEager\WithoutFetchEager\User as UserWithoutFetchEager; +use Doctrine\Tests\Models\AbstractFetchEager\AbstractRemoveControl; +use Doctrine\Tests\Models\AbstractFetchEager\MobileRemoteControl; +use Doctrine\Tests\Models\AbstractFetchEager\User; use Doctrine\Tests\OrmFunctionalTestCase; final class AbstractFetchEagerTest extends OrmFunctionalTestCase @@ -37,28 +34,4 @@ public function testWithAbstractFetchEager(): void self::assertEquals('smart', $user->remoteControl->name); self::assertTrue($user->remoteControl->users->contains($user)); } - - public function testWithoutAbstractFetchEager(): void - { - $this->createSchemaForModels( - AbstractRemoveControlWithoutFetchEager::class, - UserWithoutFetchEager::class - ); - - $control = new MobileRemoteControlWithoutFetchEager('smart'); - $user = new UserWithoutFetchEager($control); - - $entityManage = $this->getEntityManager(); - - $entityManage->persist($control); - $entityManage->persist($user); - $entityManage->flush(); - $entityManage->clear(); - - $user = $entityManage->find(UserWithoutFetchEager::class, $user->id); - - self::assertNotNull($user); - self::assertEquals('smart', $user->remoteControl->name); - self::assertTrue($user->remoteControl->users->contains($user)); - } } From e399d21fb3594599e322cab4695e732687f3a73c Mon Sep 17 00:00:00 2001 From: Benjamin Eberlei Date: Sat, 16 Mar 2024 20:41:24 +0100 Subject: [PATCH 3/5] Simplify condition, improve comment on this edge case. --- src/UnitOfWork.php | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/UnitOfWork.php b/src/UnitOfWork.php index 07903c3e6c0..ba9765fbafa 100644 --- a/src/UnitOfWork.php +++ b/src/UnitOfWork.php @@ -3252,8 +3252,10 @@ private function eagerLoadCollections(array $collections, array $mapping): void foreach ($found as $targetValue) { $sourceEntity = $targetProperty->getValue($targetValue); - // In cases where the hydration $targetValue has not yet fully completed - if ($sourceEntity === null && isset($targetClass->associationMappings[$mappedBy]['joinColumns'])) { + if ($sourceEntity === null) { + // case where the hydration $targetValue itself has not yet fully completed, for example + // in case a bi-directional association is being hydrated and deferring eager loading is + // not possible due to subclassing. $data = $this->getOriginalEntityData($targetValue); $id = []; foreach ($targetClass->associationMappings[$mappedBy]['joinColumns'] as $joinColumn) { From 6501890ab5fad43adfe259fbbbc3339b07185710 Mon Sep 17 00:00:00 2001 From: Benjamin Eberlei Date: Sat, 16 Mar 2024 20:48:15 +0100 Subject: [PATCH 4/5] Static analysis enforces the extra isset() even though that just masks no sense. --- src/UnitOfWork.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/UnitOfWork.php b/src/UnitOfWork.php index ba9765fbafa..0b14fba2dc4 100644 --- a/src/UnitOfWork.php +++ b/src/UnitOfWork.php @@ -3252,7 +3252,7 @@ private function eagerLoadCollections(array $collections, array $mapping): void foreach ($found as $targetValue) { $sourceEntity = $targetProperty->getValue($targetValue); - if ($sourceEntity === null) { + if ($sourceEntity === null && isset($targetClass->associationMappings[$mappedBy]['joinColumns'])) { // case where the hydration $targetValue itself has not yet fully completed, for example // in case a bi-directional association is being hydrated and deferring eager loading is // not possible due to subclassing. From 1b6cf58a1aa19a403b587373de2b5d690734bf21 Mon Sep 17 00:00:00 2001 From: Benjamin Eberlei Date: Sat, 16 Mar 2024 21:08:30 +0100 Subject: [PATCH 5/5] Rename tables to avoid pg related illegal table name --- ...AbstractRemoveControl.php => AbstractRemoteControl.php} | 3 ++- .../Models/AbstractFetchEager/MobileRemoteControl.php | 2 +- tests/Tests/Models/AbstractFetchEager/User.php | 7 ++++--- tests/Tests/ORM/Functional/AbstractFetchEagerTest.php | 4 ++-- 4 files changed, 9 insertions(+), 7 deletions(-) rename tests/Tests/Models/AbstractFetchEager/{AbstractRemoveControl.php => AbstractRemoteControl.php} (90%) diff --git a/tests/Tests/Models/AbstractFetchEager/AbstractRemoveControl.php b/tests/Tests/Models/AbstractFetchEager/AbstractRemoteControl.php similarity index 90% rename from tests/Tests/Models/AbstractFetchEager/AbstractRemoveControl.php rename to tests/Tests/Models/AbstractFetchEager/AbstractRemoteControl.php index a2874469304..59d69da28a4 100644 --- a/tests/Tests/Models/AbstractFetchEager/AbstractRemoveControl.php +++ b/tests/Tests/Models/AbstractFetchEager/AbstractRemoteControl.php @@ -10,11 +10,12 @@ /** * @ORM\Entity() + * @ORM\Table(name="abstract_fetch_eager_remote_control") * @ORM\InheritanceType("SINGLE_TABLE") * @ORM\DiscriminatorColumn(name="type", type="string") * @ORM\DiscriminatorMap({"mobile"="MobileRemoteControl"}) */ -abstract class AbstractRemoveControl +abstract class AbstractRemoteControl { /** * @ORM\Id diff --git a/tests/Tests/Models/AbstractFetchEager/MobileRemoteControl.php b/tests/Tests/Models/AbstractFetchEager/MobileRemoteControl.php index adb7c22fd08..50bdc25470b 100644 --- a/tests/Tests/Models/AbstractFetchEager/MobileRemoteControl.php +++ b/tests/Tests/Models/AbstractFetchEager/MobileRemoteControl.php @@ -9,6 +9,6 @@ /** * @ORM\Entity() */ -class MobileRemoteControl extends AbstractRemoveControl +class MobileRemoteControl extends AbstractRemoteControl { } diff --git a/tests/Tests/Models/AbstractFetchEager/User.php b/tests/Tests/Models/AbstractFetchEager/User.php index 0ee7b24891b..d8b7b2c012a 100644 --- a/tests/Tests/Models/AbstractFetchEager/User.php +++ b/tests/Tests/Models/AbstractFetchEager/User.php @@ -8,6 +8,7 @@ /** * @ORM\Entity() + * @ORM\Table(name="abstract_fetch_eager_user") */ class User { @@ -21,14 +22,14 @@ class User public $id; /** - * @ORM\ManyToOne(targetEntity="AbstractRemoveControl", inversedBy="users") + * @ORM\ManyToOne(targetEntity="AbstractRemoteControl", inversedBy="users") * @ORM\JoinColumn(nullable=false) * - * @var AbstractRemoveControl + * @var AbstractRemoteControl */ public $remoteControl; - public function __construct(AbstractRemoveControl $control) + public function __construct(AbstractRemoteControl $control) { $this->remoteControl = $control; } diff --git a/tests/Tests/ORM/Functional/AbstractFetchEagerTest.php b/tests/Tests/ORM/Functional/AbstractFetchEagerTest.php index 975a97fa7a9..24e100a684c 100644 --- a/tests/Tests/ORM/Functional/AbstractFetchEagerTest.php +++ b/tests/Tests/ORM/Functional/AbstractFetchEagerTest.php @@ -4,7 +4,7 @@ namespace Doctrine\Tests\ORM\Functional; -use Doctrine\Tests\Models\AbstractFetchEager\AbstractRemoveControl; +use Doctrine\Tests\Models\AbstractFetchEager\AbstractRemoteControl; use Doctrine\Tests\Models\AbstractFetchEager\MobileRemoteControl; use Doctrine\Tests\Models\AbstractFetchEager\User; use Doctrine\Tests\OrmFunctionalTestCase; @@ -14,7 +14,7 @@ final class AbstractFetchEagerTest extends OrmFunctionalTestCase public function testWithAbstractFetchEager(): void { $this->createSchemaForModels( - AbstractRemoveControl::class, + AbstractRemoteControl::class, User::class );