From 01d51bfca391e49bbb596c90d1a9c4766533155a 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 7ac24cc60..403ab668d 100644 --- a/lib/Doctrine/ORM/UnitOfWork.php +++ b/lib/Doctrine/ORM/UnitOfWork.php @@ -3405,7 +3405,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 b8c7d871bee7a4290551439ce65c7a66cf92041d 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 403ab668d..59ee1c1cb 100644 --- a/lib/Doctrine/ORM/UnitOfWork.php +++ b/lib/Doctrine/ORM/UnitOfWork.php @@ -3404,8 +3404,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 44af69c5d2299e46f90a94e91c35711afe8392de 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 75bf197e115ce7f66ff49adc7c96f2f72c896762 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 | 33 +++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php b/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php index 6a2c882e8..a604641f8 100644 --- a/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php +++ b/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php @@ -14,6 +14,7 @@ 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; @@ -459,6 +460,38 @@ class UnitOfWorkTest extends OrmTestCase 'second null string, two fields' => [$secondNullString, ['id1' => $secondNullString->id1, 'id2' => null]], ]; } + + /** + * @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 f8436b2165ea6878e0fb45dbd3c30493443f544d 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 503b211a22db02ea825942aacf6dbff87d438aaa 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 a604641f8..0ff2572b9 100644 --- a/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php +++ b/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php @@ -467,10 +467,6 @@ class UnitOfWorkTest extends 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);