From 95dcf51ad50e32354f88773e92fc9c34bd79f9b3 Mon Sep 17 00:00:00 2001 From: Mathieu De Zutter Date: Tue, 1 Mar 2016 10:58:37 +0100 Subject: [PATCH 1/6] Avoid conflicts due to spl_object_hash(). When merging an entity with a to-many association, it will store the original entity data using the object hash of the to-be-merged entity instead of the managed entity. Since this to-be-merged entity is not managed by Doctrine, it can disappear from the memory. A new object can reuse the same memory location and thus have the same object hash. When one tries to persist this object as new, Doctrine will refuse it because it thinks that the entity is managed+dirty. This patch is a very naive fix: it just disables storing the original entity data in case of to-many associations. It may not be the ideal or even a good solution at all, but it solves the problem of object hash reuse. The test case relies on the immediate reusing of memory locations by PHP. The variable $user has twice the same object hash, though referring a different object. Tested on PHP 5.6.17 Without the fix, the test fails on the last line with: A managed+dirty entity Doctrine\Tests\Models\CMS\CmsUser@[...] can not be scheduled for insertion. --- lib/Doctrine/ORM/UnitOfWork.php | 2 +- .../Tests/ORM/Functional/OidReuseTest.php | 34 +++++++++++++++++++ 2 files changed, 35 insertions(+), 1 deletion(-) create mode 100644 tests/Doctrine/Tests/ORM/Functional/OidReuseTest.php diff --git a/lib/Doctrine/ORM/UnitOfWork.php b/lib/Doctrine/ORM/UnitOfWork.php index c2f037e2d..bed79caa3 100644 --- a/lib/Doctrine/ORM/UnitOfWork.php +++ b/lib/Doctrine/ORM/UnitOfWork.php @@ -3423,7 +3423,7 @@ class UnitOfWork implements PropertyChangedListener $managedCol->setOwner($managedCopy, $assoc2); $prop->setValue($managedCopy, $managedCol); - $this->originalEntityData[spl_object_hash($entity)][$name] = $managedCol; +// $this->originalEntityData[spl_object_hash($entity)][$name] = $managedCol; } if ($assoc2['isCascadeMerge']) { diff --git a/tests/Doctrine/Tests/ORM/Functional/OidReuseTest.php b/tests/Doctrine/Tests/ORM/Functional/OidReuseTest.php new file mode 100644 index 000000000..40aaf4238 --- /dev/null +++ b/tests/Doctrine/Tests/ORM/Functional/OidReuseTest.php @@ -0,0 +1,34 @@ +useModelSet('cms'); + parent::setUp(); + } + + public function testOidReuse() + { + $user = new CmsUser(); + $this->_em->merge($user); + + $user = null; + + $user = new CmsUser(); + $this->_em->persist($user); + } + +} \ No newline at end of file From b0e4e3eda4b7742c286a5f8a82bc566dcd54750b Mon Sep 17 00:00:00 2001 From: Mathieu De Zutter Date: Tue, 1 Mar 2016 19:36:57 +0100 Subject: [PATCH 2/6] Remove old code in comments. --- lib/Doctrine/ORM/UnitOfWork.php | 2 -- 1 file changed, 2 deletions(-) diff --git a/lib/Doctrine/ORM/UnitOfWork.php b/lib/Doctrine/ORM/UnitOfWork.php index bed79caa3..184b291da 100644 --- a/lib/Doctrine/ORM/UnitOfWork.php +++ b/lib/Doctrine/ORM/UnitOfWork.php @@ -3422,8 +3422,6 @@ class UnitOfWork implements PropertyChangedListener ); $managedCol->setOwner($managedCopy, $assoc2); $prop->setValue($managedCopy, $managedCol); - -// $this->originalEntityData[spl_object_hash($entity)][$name] = $managedCol; } if ($assoc2['isCascadeMerge']) { From a3d93afc4fb4b8c984f79a7b4a88d689c9b41305 Mon Sep 17 00:00:00 2001 From: Mathieu De Zutter Date: Tue, 1 Mar 2016 19:51:38 +0100 Subject: [PATCH 3/6] Additional assertion to check that unreferenced objects are not in UOW. --- tests/Doctrine/Tests/ORM/Functional/OidReuseTest.php | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/Doctrine/Tests/ORM/Functional/OidReuseTest.php b/tests/Doctrine/Tests/ORM/Functional/OidReuseTest.php index 40aaf4238..742b183fe 100644 --- a/tests/Doctrine/Tests/ORM/Functional/OidReuseTest.php +++ b/tests/Doctrine/Tests/ORM/Functional/OidReuseTest.php @@ -22,11 +22,19 @@ class OidReuseTest extends OrmFunctionalTestCase public function testOidReuse() { + $uow = $this->_em->getUnitOfWork(); + $reflexion = new \ReflectionClass(get_class($uow)); + $originalEntityDataProperty = $reflexion->getProperty('originalEntityData'); + $originalEntityDataProperty->setAccessible(true); + $user = new CmsUser(); + $oid = spl_object_hash($user); $this->_em->merge($user); $user = null; + $this->assertArrayNotHasKey($oid, $originalEntityDataProperty->getValue($uow)); + $user = new CmsUser(); $this->_em->persist($user); } From e73428a051a95edb131b27fc3642493ea53ada16 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Sat, 10 Sep 2016 20:15:33 +0200 Subject: [PATCH 4/6] #5689 moved `OidReuseTest` contents into the `UnitOfWork` tests --- tests/Doctrine/Tests/ORM/UnitOfWorkTest.php | 34 +++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php b/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php index 9623cfbf4..20ae887a2 100644 --- a/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php +++ b/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php @@ -12,6 +12,8 @@ use Doctrine\Tests\Mocks\DriverMock; use Doctrine\Tests\Mocks\EntityManagerMock; use Doctrine\Tests\Mocks\EntityPersisterMock; use Doctrine\Tests\Mocks\UnitOfWorkMock; +use Doctrine\Tests\Models\CMS\CmsPhonenumber; +use Doctrine\Tests\Models\CMS\CmsUser; use Doctrine\Tests\Models\Forum\ForumAvatar; use Doctrine\Tests\Models\Forum\ForumUser; use Doctrine\Tests\Models\GeoNames\City; @@ -362,6 +364,38 @@ class UnitOfWorkTest extends \Doctrine\Tests\OrmTestCase [new ArrayCollection()], ]; } + + /** + * @group 5689 + * @group 1465 + */ + public function testObjectHashesOfMergedEntitiesAreNotUsedInOriginalEntityDataMap() + { + $reflectionOriginalEntityData = new \ReflectionProperty('Doctrine\ORM\UnitOfWork', 'originalEntityData'); + + $reflectionOriginalEntityData->setAccessible(true); + + $user = new CmsUser(); + $user->name = 'ocramius'; + $mergedUser = $this->_unitOfWork->merge($user); + + self::assertSame([], $this->_unitOfWork->getOriginalEntityData($user), 'No original data was stored'); + self::assertSame([], $this->_unitOfWork->getOriginalEntityData($mergedUser), 'No original data was stored'); + + + $user = null; + $mergedUser = null; + + // force garbage collection of $user (frees the used object hashes, which may be recycled) + gc_collect_cycles(); + + $newUser = new CmsUser(); + $newUser->name = 'ocramius'; + + $this->_unitOfWork->persist($newUser); + + self::assertSame([], $this->_unitOfWork->getOriginalEntityData($newUser), 'No original data was stored'); + } } /** From 147f8fffff7ef4de6035509b24e375ee6bc768a3 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Sat, 10 Sep 2016 20:15:59 +0200 Subject: [PATCH 5/6] #5689 removed `OidReuseTest`, which was moved to `UnitOfWork` tests --- .../Tests/ORM/Functional/OidReuseTest.php | 42 ------------------- 1 file changed, 42 deletions(-) delete mode 100644 tests/Doctrine/Tests/ORM/Functional/OidReuseTest.php diff --git a/tests/Doctrine/Tests/ORM/Functional/OidReuseTest.php b/tests/Doctrine/Tests/ORM/Functional/OidReuseTest.php deleted file mode 100644 index 742b183fe..000000000 --- a/tests/Doctrine/Tests/ORM/Functional/OidReuseTest.php +++ /dev/null @@ -1,42 +0,0 @@ -useModelSet('cms'); - parent::setUp(); - } - - public function testOidReuse() - { - $uow = $this->_em->getUnitOfWork(); - $reflexion = new \ReflectionClass(get_class($uow)); - $originalEntityDataProperty = $reflexion->getProperty('originalEntityData'); - $originalEntityDataProperty->setAccessible(true); - - $user = new CmsUser(); - $oid = spl_object_hash($user); - $this->_em->merge($user); - - $user = null; - - $this->assertArrayNotHasKey($oid, $originalEntityDataProperty->getValue($uow)); - - $user = new CmsUser(); - $this->_em->persist($user); - } - -} \ No newline at end of file From c9161fcd6f10d15bae24073d712623c03b7eb5db Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Sat, 10 Sep 2016 20:19:15 +0200 Subject: [PATCH 6/6] #5689 removed unused reflection access --- tests/Doctrine/Tests/ORM/UnitOfWorkTest.php | 4 ---- 1 file changed, 4 deletions(-) diff --git a/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php b/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php index 20ae887a2..306fceaba 100644 --- a/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php +++ b/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php @@ -371,10 +371,6 @@ class UnitOfWorkTest extends \Doctrine\Tests\OrmTestCase */ public function testObjectHashesOfMergedEntitiesAreNotUsedInOriginalEntityDataMap() { - $reflectionOriginalEntityData = new \ReflectionProperty('Doctrine\ORM\UnitOfWork', 'originalEntityData'); - - $reflectionOriginalEntityData->setAccessible(true); - $user = new CmsUser(); $user->name = 'ocramius'; $mergedUser = $this->_unitOfWork->merge($user);