From 1d961780975ae245eb86e7e095d31f2522b8780c Mon Sep 17 00:00:00 2001 From: bilouwan Date: Thu, 15 Dec 2016 12:19:56 +0100 Subject: [PATCH 01/21] Create failing test to reveal the issue --- .../Functional/EntityListenersOnMergeTest.php | 50 +++++++++++++++++++ 1 file changed, 50 insertions(+) create mode 100644 tests/Doctrine/Tests/ORM/Functional/EntityListenersOnMergeTest.php diff --git a/tests/Doctrine/Tests/ORM/Functional/EntityListenersOnMergeTest.php b/tests/Doctrine/Tests/ORM/Functional/EntityListenersOnMergeTest.php new file mode 100644 index 000000000..a3575a821 --- /dev/null +++ b/tests/Doctrine/Tests/ORM/Functional/EntityListenersOnMergeTest.php @@ -0,0 +1,50 @@ +_schemaTool->createSchema( + [ + $this->_em->getClassMetadata(DDC3597Root::class), + $this->_em->getClassMetadata(DDC3597Media::class), + $this->_em->getClassMetadata(DDC3597Image::class), + ] + ); + } + + protected function tearDown() + { + parent::tearDown(); + $this->_schemaTool->dropSchema( + [ + $this->_em->getClassMetadata(DDC3597Root::class), + $this->_em->getClassMetadata(DDC3597Media::class), + $this->_em->getClassMetadata(DDC3597Image::class), + ] + ); + } + + public function testMergeNewEntityLifecyleEventsModificationsShouldBeKept() + { + $imageEntity = new DDC3597Image('foobar'); + $imageEntity->setFormat('JPG'); + $imageEntity->setSize(123); + $imageEntity->getDimension()->setWidth(300); + $imageEntity->getDimension()->setHeight(500); + + $imageEntity = $this->_em->merge($imageEntity); + + $this->assertNotNull($imageEntity->getCreatedAt()); + $this->assertNotNull($imageEntity->getUpdatedAt()); + } +} \ No newline at end of file From 25efabdb7495585a772aaf3b38f34c047ec8c578 Mon Sep 17 00:00:00 2001 From: bilouwan Date: Thu, 15 Dec 2016 12:49:11 +0100 Subject: [PATCH 02/21] doMerge will mergeEntityStateIntoManagedCopy BEFORE persistNew to let lifecyle events changes be persisted --- lib/Doctrine/ORM/UnitOfWork.php | 46 +++++++++++++++++++-------------- 1 file changed, 27 insertions(+), 19 deletions(-) diff --git a/lib/Doctrine/ORM/UnitOfWork.php b/lib/Doctrine/ORM/UnitOfWork.php index ed7e52e69..ac1251425 100644 --- a/lib/Doctrine/ORM/UnitOfWork.php +++ b/lib/Doctrine/ORM/UnitOfWork.php @@ -1831,6 +1831,7 @@ class UnitOfWork implements PropertyChangedListener if ( ! $id) { $managedCopy = $this->newInstance($class); + $this->mergeEntityStateIntoManagedCopy($entity, $managedCopy); $this->persistNew($class, $managedCopy); } else { $flatId = ($class->containsForeignIdentifier) @@ -1862,30 +1863,16 @@ class UnitOfWork implements PropertyChangedListener $managedCopy = $this->newInstance($class); $class->setIdentifierValues($managedCopy, $id); + $this->mergeEntityStateIntoManagedCopy($entity, $managedCopy); $this->persistNew($class, $managedCopy); - } - } - - if ($class->isVersioned && $this->isLoaded($managedCopy) && $this->isLoaded($entity)) { - $reflField = $class->reflFields[$class->versionField]; - $managedCopyVersion = $reflField->getValue($managedCopy); - $entityVersion = $reflField->getValue($entity); - - // Throw exception if versions don't match. - if ($managedCopyVersion != $entityVersion) { - throw OptimisticLockException::lockFailedVersionMismatch($entity, $entityVersion, $managedCopyVersion); + }else{ + $this->ensureVersionMatch($class, $entity, $managedCopy); + $this->mergeEntityStateIntoManagedCopy($entity, $managedCopy); } } $visited[$oid] = $managedCopy; // mark visited - - if ($this->isLoaded($entity)) { - if ($managedCopy instanceof Proxy && ! $managedCopy->__isInitialized()) { - $managedCopy->__load(); - } - - $this->mergeEntityStateIntoManagedCopy($entity, $managedCopy); - } + // $this->mergeEntityStateIntoManagedCopy($entity, $managedCopy); if ($class->isChangeTrackingDeferredExplicit()) { $this->scheduleForDirtyCheck($entity); @@ -1904,6 +1891,19 @@ class UnitOfWork implements PropertyChangedListener return $managedCopy; } + private function ensureVersionMatch(ClassMetadata $class, $entity, $managedCopy) { + if ($class->isVersioned && $this->isLoaded($managedCopy) && $this->isLoaded($entity)) { + $reflField = $class->reflFields[$class->versionField]; + $managedCopyVersion = $reflField->getValue($managedCopy); + $entityVersion = $reflField->getValue($entity); + + // Throw exception if versions don't match. + if ($managedCopyVersion != $entityVersion) { + throw OptimisticLockException::lockFailedVersionMismatch($entity, $entityVersion, $managedCopyVersion); + } + } + } + /** * Tests if an entity is loaded - must either be a loaded proxy or not a proxy * @@ -3356,6 +3356,14 @@ class UnitOfWork implements PropertyChangedListener */ private function mergeEntityStateIntoManagedCopy($entity, $managedCopy) { + if (!$this->isLoaded($entity)) { + return; + } + + if (!$this->isLoaded($managedCopy)) { + $managedCopy->__load(); + } + $class = $this->em->getClassMetadata(get_class($entity)); foreach ($this->reflectionPropertiesGetter->getProperties($class->name) as $prop) { From 295523cdca6269c60c494271d6bb301b28819d4b Mon Sep 17 00:00:00 2001 From: bilouwan Date: Thu, 15 Dec 2016 13:03:53 +0100 Subject: [PATCH 03/21] Cherry pick unit test from PR #5570 (Fix PrePersist EventListener when using merge instead of persist) --- .../Company/CompanyContractListener.php | 29 ++++++++++-- .../Functional/EntityListenersOnMergeTest.php | 46 +++++++++++++++++++ 2 files changed, 71 insertions(+), 4 deletions(-) diff --git a/tests/Doctrine/Tests/Models/Company/CompanyContractListener.php b/tests/Doctrine/Tests/Models/Company/CompanyContractListener.php index 61c1d4719..c58628878 100644 --- a/tests/Doctrine/Tests/Models/Company/CompanyContractListener.php +++ b/tests/Doctrine/Tests/Models/Company/CompanyContractListener.php @@ -4,19 +4,23 @@ namespace Doctrine\Tests\Models\Company; class CompanyContractListener { + const PRE_PERSIST = 0; + public $postPersistCalls; public $prePersistCalls; - + public $postUpdateCalls; public $preUpdateCalls; - + public $postRemoveCalls; public $preRemoveCalls; public $preFlushCalls; - + public $postLoadCalls; - + + public $snapshots = []; + /** * @PostPersist */ @@ -30,6 +34,7 @@ class CompanyContractListener */ public function prePersistHandler(CompanyContract $contract) { + $this->snapshots[self::PRE_PERSIST][] = $this->takeSnapshot($contract); $this->prePersistCalls[] = func_get_args(); } @@ -81,4 +86,20 @@ class CompanyContractListener $this->postLoadCalls[] = func_get_args(); } + public function takeSnapshot(CompanyContract $contract) + { + $snapshot = []; + $reflexion = new \ReflectionClass($contract); + foreach ($reflexion->getProperties() as $property) { + $property->setAccessible(true); + $value = $property->getValue($contract); + if (is_object($value) || is_array($value)) { + continue; + } + $snapshot[$property->getName()] = $property->getValue($contract); + } + + return $snapshot; + } + } diff --git a/tests/Doctrine/Tests/ORM/Functional/EntityListenersOnMergeTest.php b/tests/Doctrine/Tests/ORM/Functional/EntityListenersOnMergeTest.php index a3575a821..5354c0a30 100644 --- a/tests/Doctrine/Tests/ORM/Functional/EntityListenersOnMergeTest.php +++ b/tests/Doctrine/Tests/ORM/Functional/EntityListenersOnMergeTest.php @@ -2,17 +2,28 @@ namespace Doctrine\Tests\ORM\Functional; +use Doctrine\Tests\Models\Company\CompanyContractListener; +use Doctrine\Tests\Models\Company\CompanyFixContract; use Doctrine\Tests\Models\DDC3597\DDC3597Image; use Doctrine\Tests\Models\DDC3597\DDC3597Media; use Doctrine\Tests\Models\DDC3597\DDC3597Root; /** + * @group DDC-1955 */ class EntityListenersOnMergeTest extends \Doctrine\Tests\OrmFunctionalTestCase { + + /** + * @var \Doctrine\Tests\Models\Company\CompanyContractListener + */ + private $listener; + protected function setUp() { + $this->useModelSet('company'); parent::setUp(); + $this->_schemaTool->createSchema( [ $this->_em->getClassMetadata(DDC3597Root::class), @@ -20,6 +31,10 @@ class EntityListenersOnMergeTest extends \Doctrine\Tests\OrmFunctionalTestCase $this->_em->getClassMetadata(DDC3597Image::class), ] ); + + $this->listener = $this->_em->getConfiguration() + ->getEntityListenerResolver() + ->resolve('Doctrine\Tests\Models\Company\CompanyContractListener'); } protected function tearDown() @@ -47,4 +62,35 @@ class EntityListenersOnMergeTest extends \Doctrine\Tests\OrmFunctionalTestCase $this->assertNotNull($imageEntity->getCreatedAt()); $this->assertNotNull($imageEntity->getUpdatedAt()); } + + public function testPrePersistListeners() + { + $fix = new CompanyFixContract(); + $fix->setFixPrice(2000); + + $this->listener->prePersistCalls = []; + + $fix = $this->_em->merge($fix); + $this->_em->flush(); + + $this->assertCount(1, $this->listener->prePersistCalls); + + $this->assertSame($fix, $this->listener->prePersistCalls[0][0]); + + $this->assertInstanceOf( + 'Doctrine\Tests\Models\Company\CompanyFixContract', + $this->listener->prePersistCalls[0][0] + ); + + $this->assertInstanceOf( + 'Doctrine\ORM\Event\LifecycleEventArgs', + $this->listener->prePersistCalls[0][1] + ); + + $this->assertArrayHasKey('fixPrice', $this->listener->snapshots[CompanyContractListener::PRE_PERSIST][0]); + $this->assertEquals( + $fix->getFixPrice(), + $this->listener->snapshots[CompanyContractListener::PRE_PERSIST][0]['fixPrice'] + ); + } } \ No newline at end of file From 569c08ce558c518b418b66930cc3975dfe9c5f3f Mon Sep 17 00:00:00 2001 From: bilouwan Date: Thu, 15 Dec 2016 15:12:29 +0100 Subject: [PATCH 04/21] Rename test --- .../Tests/ORM/Functional/EntityListenersOnMergeTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/Doctrine/Tests/ORM/Functional/EntityListenersOnMergeTest.php b/tests/Doctrine/Tests/ORM/Functional/EntityListenersOnMergeTest.php index 5354c0a30..6123fe997 100644 --- a/tests/Doctrine/Tests/ORM/Functional/EntityListenersOnMergeTest.php +++ b/tests/Doctrine/Tests/ORM/Functional/EntityListenersOnMergeTest.php @@ -63,7 +63,7 @@ class EntityListenersOnMergeTest extends \Doctrine\Tests\OrmFunctionalTestCase $this->assertNotNull($imageEntity->getUpdatedAt()); } - public function testPrePersistListeners() + public function testPrePersistListenersShouldBeFiredWithCorrectEntityData() { $fix = new CompanyFixContract(); $fix->setFixPrice(2000); From cfb7461f512e67ce1c14f98e897016def3a5cc7d Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Sun, 18 Dec 2016 14:27:35 +0100 Subject: [PATCH 05/21] #6174 #5570 CS - alignment --- lib/Doctrine/ORM/UnitOfWork.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/Doctrine/ORM/UnitOfWork.php b/lib/Doctrine/ORM/UnitOfWork.php index ac1251425..90e30f839 100644 --- a/lib/Doctrine/ORM/UnitOfWork.php +++ b/lib/Doctrine/ORM/UnitOfWork.php @@ -1891,7 +1891,8 @@ class UnitOfWork implements PropertyChangedListener return $managedCopy; } - private function ensureVersionMatch(ClassMetadata $class, $entity, $managedCopy) { + private function ensureVersionMatch(ClassMetadata $class, $entity, $managedCopy) + { if ($class->isVersioned && $this->isLoaded($managedCopy) && $this->isLoaded($entity)) { $reflField = $class->reflFields[$class->versionField]; $managedCopyVersion = $reflField->getValue($managedCopy); From cf941ce54fbe6342b7a3d2d86b33a1d211a20815 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Sun, 18 Dec 2016 14:32:31 +0100 Subject: [PATCH 06/21] #6174 #5570 documenting thrown exception types --- lib/Doctrine/ORM/UnitOfWork.php | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/lib/Doctrine/ORM/UnitOfWork.php b/lib/Doctrine/ORM/UnitOfWork.php index 90e30f839..9e213199a 100644 --- a/lib/Doctrine/ORM/UnitOfWork.php +++ b/lib/Doctrine/ORM/UnitOfWork.php @@ -1799,7 +1799,7 @@ class UnitOfWork implements PropertyChangedListener * @throws OptimisticLockException If the entity uses optimistic locking through a version * attribute and the version check against the managed copy fails. * @throws ORMInvalidArgumentException If the entity instance is NEW. - * @throws EntityNotFoundException + * @throws EntityNotFoundException if an assigned identifier is used in the entity, but none is provided */ private function doMerge($entity, array &$visited, $prevManagedCopy = null, $assoc = null) { @@ -1891,6 +1891,15 @@ class UnitOfWork implements PropertyChangedListener return $managedCopy; } + /** + * @param ClassMetadata $class + * @param object $entity + * @param object $managedCopy + * + * @return void + * + * @throws OptimisticLockException + */ private function ensureVersionMatch(ClassMetadata $class, $entity, $managedCopy) { if ($class->isVersioned && $this->isLoaded($managedCopy) && $this->isLoaded($entity)) { From eaee924180f4ed869f3cb07fb7c55bafe584fd8b Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Sun, 18 Dec 2016 14:36:39 +0100 Subject: [PATCH 07/21] #6174 #5570 flattened nested conditionals --- lib/Doctrine/ORM/UnitOfWork.php | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/lib/Doctrine/ORM/UnitOfWork.php b/lib/Doctrine/ORM/UnitOfWork.php index 9e213199a..3867b1b14 100644 --- a/lib/Doctrine/ORM/UnitOfWork.php +++ b/lib/Doctrine/ORM/UnitOfWork.php @@ -1902,16 +1902,20 @@ class UnitOfWork implements PropertyChangedListener */ private function ensureVersionMatch(ClassMetadata $class, $entity, $managedCopy) { - if ($class->isVersioned && $this->isLoaded($managedCopy) && $this->isLoaded($entity)) { - $reflField = $class->reflFields[$class->versionField]; - $managedCopyVersion = $reflField->getValue($managedCopy); - $entityVersion = $reflField->getValue($entity); - - // Throw exception if versions don't match. - if ($managedCopyVersion != $entityVersion) { - throw OptimisticLockException::lockFailedVersionMismatch($entity, $entityVersion, $managedCopyVersion); - } + if (! ($class->isVersioned && $this->isLoaded($managedCopy) && $this->isLoaded($entity))) { + return; } + + $reflField = $class->reflFields[$class->versionField]; + $managedCopyVersion = $reflField->getValue($managedCopy); + $entityVersion = $reflField->getValue($entity); + + // Throw exception if versions don't match. + if ($managedCopyVersion == $entityVersion) { + return; + } + + throw OptimisticLockException::lockFailedVersionMismatch($entity, $entityVersion, $managedCopyVersion); } /** From 576a4d7e311702e91e6ae229258b62adf3edb3f3 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Sun, 18 Dec 2016 14:38:10 +0100 Subject: [PATCH 08/21] #6174 #5570 CS - spacing --- lib/Doctrine/ORM/UnitOfWork.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/Doctrine/ORM/UnitOfWork.php b/lib/Doctrine/ORM/UnitOfWork.php index 3867b1b14..2bf9deb9e 100644 --- a/lib/Doctrine/ORM/UnitOfWork.php +++ b/lib/Doctrine/ORM/UnitOfWork.php @@ -3370,11 +3370,11 @@ class UnitOfWork implements PropertyChangedListener */ private function mergeEntityStateIntoManagedCopy($entity, $managedCopy) { - if (!$this->isLoaded($entity)) { + if (! $this->isLoaded($entity)) { return; } - if (!$this->isLoaded($managedCopy)) { + if (! $this->isLoaded($managedCopy)) { $managedCopy->__load(); } From d9821d3fda098f034979af48f459cadfc6d83c63 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Sun, 18 Dec 2016 14:39:46 +0100 Subject: [PATCH 09/21] #6174 #5570 CS - spacing --- lib/Doctrine/ORM/UnitOfWork.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Doctrine/ORM/UnitOfWork.php b/lib/Doctrine/ORM/UnitOfWork.php index 2bf9deb9e..6570f1a41 100644 --- a/lib/Doctrine/ORM/UnitOfWork.php +++ b/lib/Doctrine/ORM/UnitOfWork.php @@ -1865,7 +1865,7 @@ class UnitOfWork implements PropertyChangedListener $this->mergeEntityStateIntoManagedCopy($entity, $managedCopy); $this->persistNew($class, $managedCopy); - }else{ + } else { $this->ensureVersionMatch($class, $entity, $managedCopy); $this->mergeEntityStateIntoManagedCopy($entity, $managedCopy); } From dac1a169645a6b7ff155a5a82a8c36e40c357eb1 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Sun, 18 Dec 2016 14:45:08 +0100 Subject: [PATCH 10/21] #6174 #5570 removed unused/dead code --- lib/Doctrine/ORM/UnitOfWork.php | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/Doctrine/ORM/UnitOfWork.php b/lib/Doctrine/ORM/UnitOfWork.php index 6570f1a41..fcbd3ab94 100644 --- a/lib/Doctrine/ORM/UnitOfWork.php +++ b/lib/Doctrine/ORM/UnitOfWork.php @@ -1872,7 +1872,6 @@ class UnitOfWork implements PropertyChangedListener } $visited[$oid] = $managedCopy; // mark visited - // $this->mergeEntityStateIntoManagedCopy($entity, $managedCopy); if ($class->isChangeTrackingDeferredExplicit()) { $this->scheduleForDirtyCheck($entity); From 12e8ab216abada6e2f2f863281b37ce484ec7237 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Sun, 18 Dec 2016 14:47:16 +0100 Subject: [PATCH 11/21] #6174 #5570 CS - spacing/variable naming --- .../Tests/Models/Company/CompanyContractListener.php | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/tests/Doctrine/Tests/Models/Company/CompanyContractListener.php b/tests/Doctrine/Tests/Models/Company/CompanyContractListener.php index c58628878..ad4133d50 100644 --- a/tests/Doctrine/Tests/Models/Company/CompanyContractListener.php +++ b/tests/Doctrine/Tests/Models/Company/CompanyContractListener.php @@ -89,17 +89,19 @@ class CompanyContractListener public function takeSnapshot(CompanyContract $contract) { $snapshot = []; - $reflexion = new \ReflectionClass($contract); - foreach ($reflexion->getProperties() as $property) { + + foreach ((new \ReflectionClass($contract))->getProperties() as $property) { $property->setAccessible(true); + $value = $property->getValue($contract); + if (is_object($value) || is_array($value)) { continue; } + $snapshot[$property->getName()] = $property->getValue($contract); } return $snapshot; } - } From 26fc8d60e6cc5af02f4771df2a4db907087170f1 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Sun, 18 Dec 2016 14:47:55 +0100 Subject: [PATCH 12/21] #6174 #5570 adding group annotation to newly introduced tests --- .../Tests/ORM/Functional/EntityListenersOnMergeTest.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/Doctrine/Tests/ORM/Functional/EntityListenersOnMergeTest.php b/tests/Doctrine/Tests/ORM/Functional/EntityListenersOnMergeTest.php index 6123fe997..e6f3d5fe8 100644 --- a/tests/Doctrine/Tests/ORM/Functional/EntityListenersOnMergeTest.php +++ b/tests/Doctrine/Tests/ORM/Functional/EntityListenersOnMergeTest.php @@ -10,6 +10,8 @@ use Doctrine\Tests\Models\DDC3597\DDC3597Root; /** * @group DDC-1955 + * @group 5570 + * @group 6174 */ class EntityListenersOnMergeTest extends \Doctrine\Tests\OrmFunctionalTestCase { From beef8acdf5dadf40d71a7306bc94c9c9536644de Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Sun, 18 Dec 2016 14:53:54 +0100 Subject: [PATCH 13/21] #6174 #5570 CS fixes around the `EntityListenersOnMergeTest` --- .../Functional/EntityListenersOnMergeTest.php | 49 ++++++++----------- 1 file changed, 21 insertions(+), 28 deletions(-) diff --git a/tests/Doctrine/Tests/ORM/Functional/EntityListenersOnMergeTest.php b/tests/Doctrine/Tests/ORM/Functional/EntityListenersOnMergeTest.php index e6f3d5fe8..6c38d9326 100644 --- a/tests/Doctrine/Tests/ORM/Functional/EntityListenersOnMergeTest.php +++ b/tests/Doctrine/Tests/ORM/Functional/EntityListenersOnMergeTest.php @@ -2,22 +2,23 @@ namespace Doctrine\Tests\ORM\Functional; +use Doctrine\ORM\Event\LifecycleEventArgs; use Doctrine\Tests\Models\Company\CompanyContractListener; use Doctrine\Tests\Models\Company\CompanyFixContract; use Doctrine\Tests\Models\DDC3597\DDC3597Image; use Doctrine\Tests\Models\DDC3597\DDC3597Media; use Doctrine\Tests\Models\DDC3597\DDC3597Root; +use Doctrine\Tests\OrmFunctionalTestCase; /** * @group DDC-1955 * @group 5570 * @group 6174 */ -class EntityListenersOnMergeTest extends \Doctrine\Tests\OrmFunctionalTestCase +class EntityListenersOnMergeTest extends OrmFunctionalTestCase { - /** - * @var \Doctrine\Tests\Models\Company\CompanyContractListener + * @var CompanyContractListener */ private $listener; @@ -26,34 +27,32 @@ class EntityListenersOnMergeTest extends \Doctrine\Tests\OrmFunctionalTestCase $this->useModelSet('company'); parent::setUp(); - $this->_schemaTool->createSchema( - [ - $this->_em->getClassMetadata(DDC3597Root::class), - $this->_em->getClassMetadata(DDC3597Media::class), - $this->_em->getClassMetadata(DDC3597Image::class), - ] - ); + $this->_schemaTool->createSchema([ + $this->_em->getClassMetadata(DDC3597Root::class), + $this->_em->getClassMetadata(DDC3597Media::class), + $this->_em->getClassMetadata(DDC3597Image::class), + ]); $this->listener = $this->_em->getConfiguration() ->getEntityListenerResolver() - ->resolve('Doctrine\Tests\Models\Company\CompanyContractListener'); + ->resolve(CompanyContractListener::class); } protected function tearDown() { parent::tearDown(); - $this->_schemaTool->dropSchema( - [ - $this->_em->getClassMetadata(DDC3597Root::class), - $this->_em->getClassMetadata(DDC3597Media::class), - $this->_em->getClassMetadata(DDC3597Image::class), - ] - ); + + $this->_schemaTool->dropSchema([ + $this->_em->getClassMetadata(DDC3597Root::class), + $this->_em->getClassMetadata(DDC3597Media::class), + $this->_em->getClassMetadata(DDC3597Image::class), + ]); } public function testMergeNewEntityLifecyleEventsModificationsShouldBeKept() { $imageEntity = new DDC3597Image('foobar'); + $imageEntity->setFormat('JPG'); $imageEntity->setSize(123); $imageEntity->getDimension()->setWidth(300); @@ -68,6 +67,7 @@ class EntityListenersOnMergeTest extends \Doctrine\Tests\OrmFunctionalTestCase public function testPrePersistListenersShouldBeFiredWithCorrectEntityData() { $fix = new CompanyFixContract(); + $fix->setFixPrice(2000); $this->listener->prePersistCalls = []; @@ -79,15 +79,8 @@ class EntityListenersOnMergeTest extends \Doctrine\Tests\OrmFunctionalTestCase $this->assertSame($fix, $this->listener->prePersistCalls[0][0]); - $this->assertInstanceOf( - 'Doctrine\Tests\Models\Company\CompanyFixContract', - $this->listener->prePersistCalls[0][0] - ); - - $this->assertInstanceOf( - 'Doctrine\ORM\Event\LifecycleEventArgs', - $this->listener->prePersistCalls[0][1] - ); + $this->assertInstanceOf(CompanyFixContract::class, $this->listener->prePersistCalls[0][0]); + $this->assertInstanceOf(LifecycleEventArgs::class, $this->listener->prePersistCalls[0][1]); $this->assertArrayHasKey('fixPrice', $this->listener->snapshots[CompanyContractListener::PRE_PERSIST][0]); $this->assertEquals( @@ -95,4 +88,4 @@ class EntityListenersOnMergeTest extends \Doctrine\Tests\OrmFunctionalTestCase $this->listener->snapshots[CompanyContractListener::PRE_PERSIST][0]['fixPrice'] ); } -} \ No newline at end of file +} From 81186105b687636d1a5fed34c2e3817757ec1485 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Sun, 18 Dec 2016 15:37:49 +0100 Subject: [PATCH 14/21] #6174 #5570 started moving tests around `prePersist` event subscriber triggering on `UnitOfWork` into the `UnitOfWorkTest` --- tests/Doctrine/Tests/ORM/UnitOfWorkTest.php | 119 +++++++++++++++++++- 1 file changed, 113 insertions(+), 6 deletions(-) diff --git a/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php b/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php index 306fceaba..693da447a 100644 --- a/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php +++ b/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php @@ -3,8 +3,12 @@ namespace Doctrine\Tests\ORM; use Doctrine\Common\Collections\ArrayCollection; +use Doctrine\Common\EventManager; +use Doctrine\Common\EventSubscriber; use Doctrine\Common\NotifyPropertyChanged; +use Doctrine\Common\Persistence\Event\LifecycleEventArgs; use Doctrine\Common\PropertyChangedListener; +use Doctrine\ORM\Events; use Doctrine\ORM\Mapping\ClassMetadata; use Doctrine\ORM\UnitOfWork; use Doctrine\Tests\Mocks\ConnectionMock; @@ -47,18 +51,22 @@ class UnitOfWorkTest extends \Doctrine\Tests\OrmTestCase */ private $_emMock; - protected function setUp() { + /** + * @var EventManager|\PHPUnit_Framework_MockObject_MockObject + */ + private $eventManager; + + protected function setUp() + { parent::setUp(); - $this->_connectionMock = new ConnectionMock(array(), new DriverMock()); - $this->_emMock = EntityManagerMock::create($this->_connectionMock); + $this->_connectionMock = new ConnectionMock([], new DriverMock()); + $this->eventManager = $this->getMockBuilder(EventManager::class)->getMock(); + $this->_emMock = EntityManagerMock::create($this->_connectionMock, null, $this->eventManager); // SUT $this->_unitOfWork = new UnitOfWorkMock($this->_emMock); $this->_emMock->setUnitOfWork($this->_unitOfWork); } - protected function tearDown() { - } - public function testRegisterRemovedOnNewEntityIsIgnored() { $user = new ForumUser(); @@ -392,6 +400,45 @@ class UnitOfWorkTest extends \Doctrine\Tests\OrmTestCase self::assertSame([], $this->_unitOfWork->getOriginalEntityData($newUser), 'No original data was stored'); } + + public function testMergeWithNewEntityWillPersistItAndTriggerPrePersistListenersWithMergedEntityData() + { + $entity = new EntityWithListenerPopulatedField(); + + $generatedFieldValue = $entity->generatedField; + + $this + ->eventManager + ->expects(self::any()) + ->method('hasListeners') + ->willReturnCallback(function ($eventName) { + return $eventName === Events::prePersist; + }); + $this + ->eventManager + ->expects(self::once()) + ->method('dispatchEvent') + ->with( + self::anything(), + self::callback(function (LifecycleEventArgs $args) use ($entity, $generatedFieldValue) { + /* @var $object EntityWithListenerPopulatedField */ + $object = $args->getObject(); + + self::assertInstanceOf(EntityWithListenerPopulatedField::class, $object); + self::assertNotSame($entity, $object); + self::assertSame($generatedFieldValue, $object->generatedField); + + return true; + }) + ); + + /* @var $object EntityWithListenerPopulatedField */ + $object = $this->_unitOfWork->merge($entity); + + self::assertNotSame($object, $entity); + self::assertInstanceOf(EntityWithListenerPopulatedField::class, $object); + self::assertSame($object->generatedField, $entity->generatedField); + } } /** @@ -498,3 +545,63 @@ class VersionedAssignedIdentifierEntity */ public $version; } + +/** @Entity */ +class EntityWithStringIdentifier +{ + /** + * @Id @Column(type="string") + * + * @var string|null + */ + public $id; +} + +/** @Entity */ +class EntityWithBooleanIdentifier +{ + /** + * @Id @Column(type="boolean") + * + * @var bool|null + */ + public $id; +} + +/** @Entity */ +class EntityWithCompositeStringIdentifier +{ + /** + * @Id @Column(type="string") + * + * @var string|null + */ + public $id1; + + /** + * @Id @Column(type="string") + * + * @var string|null + */ + public $id2; +} + +/** @Entity */ +class EntityWithListenerPopulatedField +{ + const MAX_GENERATED_FIELD_VALUE = 10000; + + /** @Id @Column(type="string") */ + public $id; + + /** + * @Column(type="integer") + */ + public $generatedField; + + public function __construct() + { + $this->id = uniqid('id', true); + $this->generatedField = mt_rand(0, self::MAX_GENERATED_FIELD_VALUE); + } +} From 8d4bc0638dee807cddf851970877535f0beb76e5 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Sun, 18 Dec 2016 15:43:29 +0100 Subject: [PATCH 15/21] #6174 #5570 `prePersist` listeners should never be called when entities are merged, but are already in the UoW --- 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 693da447a..23c04374d 100644 --- a/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php +++ b/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php @@ -439,6 +439,39 @@ class UnitOfWorkTest extends \Doctrine\Tests\OrmTestCase self::assertInstanceOf(EntityWithListenerPopulatedField::class, $object); self::assertSame($object->generatedField, $entity->generatedField); } + + public function testMergeWithExistingEntityWillNotPersistItNorTriggerPrePersistListeners() + { + $persistedEntity = new EntityWithListenerPopulatedField(); + $mergedEntity = new EntityWithListenerPopulatedField(); + + $mergedEntity->id = $persistedEntity->id; + $mergedEntity->generatedField = mt_rand( + $persistedEntity->generatedField + 1, + $persistedEntity->generatedField + 1000 + ); + + $this + ->eventManager + ->expects(self::any()) + ->method('hasListeners') + ->willReturnCallback(function ($eventName) { + return $eventName === Events::prePersist; + }); + $this->eventManager->expects(self::never())->method('dispatchEvent'); + + $this->_unitOfWork->registerManaged( + $persistedEntity, + ['id' => $persistedEntity->id], + ['generatedField' => $persistedEntity->generatedField] + ); + + /* @var $merged EntityWithListenerPopulatedField */ + $merged = $this->_unitOfWork->merge($mergedEntity); + + self::assertSame($merged, $persistedEntity); + self::assertSame($persistedEntity->generatedField, $mergedEntity->generatedField); + } } /** From 67724eb7ae1dd73a7b5f9f37dfcf135fcefe992f Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Sun, 18 Dec 2016 15:44:48 +0100 Subject: [PATCH 16/21] #6174 #5570 adding group annotations to newly introduced test --- tests/Doctrine/Tests/ORM/UnitOfWorkTest.php | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php b/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php index 23c04374d..0aa3e86bb 100644 --- a/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php +++ b/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php @@ -401,6 +401,11 @@ class UnitOfWorkTest extends \Doctrine\Tests\OrmTestCase self::assertSame([], $this->_unitOfWork->getOriginalEntityData($newUser), 'No original data was stored'); } + /** + * @group DDC-1955 + * @group 5570 + * @group 6174 + */ public function testMergeWithNewEntityWillPersistItAndTriggerPrePersistListenersWithMergedEntityData() { $entity = new EntityWithListenerPopulatedField(); @@ -440,6 +445,11 @@ class UnitOfWorkTest extends \Doctrine\Tests\OrmTestCase self::assertSame($object->generatedField, $entity->generatedField); } + /** + * @group DDC-1955 + * @group 5570 + * @group 6174 + */ public function testMergeWithExistingEntityWillNotPersistItNorTriggerPrePersistListeners() { $persistedEntity = new EntityWithListenerPopulatedField(); From e43f5304eff48301b6b4fcd0448da8f5efd42c6e Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Sun, 18 Dec 2016 15:45:03 +0100 Subject: [PATCH 17/21] #6174 #5570 removed unused test class --- .../Functional/EntityListenersOnMergeTest.php | 91 ------------------- 1 file changed, 91 deletions(-) delete mode 100644 tests/Doctrine/Tests/ORM/Functional/EntityListenersOnMergeTest.php diff --git a/tests/Doctrine/Tests/ORM/Functional/EntityListenersOnMergeTest.php b/tests/Doctrine/Tests/ORM/Functional/EntityListenersOnMergeTest.php deleted file mode 100644 index 6c38d9326..000000000 --- a/tests/Doctrine/Tests/ORM/Functional/EntityListenersOnMergeTest.php +++ /dev/null @@ -1,91 +0,0 @@ -useModelSet('company'); - parent::setUp(); - - $this->_schemaTool->createSchema([ - $this->_em->getClassMetadata(DDC3597Root::class), - $this->_em->getClassMetadata(DDC3597Media::class), - $this->_em->getClassMetadata(DDC3597Image::class), - ]); - - $this->listener = $this->_em->getConfiguration() - ->getEntityListenerResolver() - ->resolve(CompanyContractListener::class); - } - - protected function tearDown() - { - parent::tearDown(); - - $this->_schemaTool->dropSchema([ - $this->_em->getClassMetadata(DDC3597Root::class), - $this->_em->getClassMetadata(DDC3597Media::class), - $this->_em->getClassMetadata(DDC3597Image::class), - ]); - } - - public function testMergeNewEntityLifecyleEventsModificationsShouldBeKept() - { - $imageEntity = new DDC3597Image('foobar'); - - $imageEntity->setFormat('JPG'); - $imageEntity->setSize(123); - $imageEntity->getDimension()->setWidth(300); - $imageEntity->getDimension()->setHeight(500); - - $imageEntity = $this->_em->merge($imageEntity); - - $this->assertNotNull($imageEntity->getCreatedAt()); - $this->assertNotNull($imageEntity->getUpdatedAt()); - } - - public function testPrePersistListenersShouldBeFiredWithCorrectEntityData() - { - $fix = new CompanyFixContract(); - - $fix->setFixPrice(2000); - - $this->listener->prePersistCalls = []; - - $fix = $this->_em->merge($fix); - $this->_em->flush(); - - $this->assertCount(1, $this->listener->prePersistCalls); - - $this->assertSame($fix, $this->listener->prePersistCalls[0][0]); - - $this->assertInstanceOf(CompanyFixContract::class, $this->listener->prePersistCalls[0][0]); - $this->assertInstanceOf(LifecycleEventArgs::class, $this->listener->prePersistCalls[0][1]); - - $this->assertArrayHasKey('fixPrice', $this->listener->snapshots[CompanyContractListener::PRE_PERSIST][0]); - $this->assertEquals( - $fix->getFixPrice(), - $this->listener->snapshots[CompanyContractListener::PRE_PERSIST][0]['fixPrice'] - ); - } -} From 39ce6f96a08d9ae23cf0a9fe88ddaec9f7f24829 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Sun, 18 Dec 2016 15:46:34 +0100 Subject: [PATCH 18/21] #6174 #5570 renamed entity for better fitting the use-cases it's in --- tests/Doctrine/Tests/ORM/UnitOfWorkTest.php | 22 ++++++++++----------- 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php b/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php index 0aa3e86bb..fca72fc71 100644 --- a/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php +++ b/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php @@ -408,7 +408,7 @@ class UnitOfWorkTest extends \Doctrine\Tests\OrmTestCase */ public function testMergeWithNewEntityWillPersistItAndTriggerPrePersistListenersWithMergedEntityData() { - $entity = new EntityWithListenerPopulatedField(); + $entity = new EntityWithRandomlyGeneratedField(); $generatedFieldValue = $entity->generatedField; @@ -426,10 +426,10 @@ class UnitOfWorkTest extends \Doctrine\Tests\OrmTestCase ->with( self::anything(), self::callback(function (LifecycleEventArgs $args) use ($entity, $generatedFieldValue) { - /* @var $object EntityWithListenerPopulatedField */ + /* @var $object EntityWithRandomlyGeneratedField */ $object = $args->getObject(); - self::assertInstanceOf(EntityWithListenerPopulatedField::class, $object); + self::assertInstanceOf(EntityWithRandomlyGeneratedField::class, $object); self::assertNotSame($entity, $object); self::assertSame($generatedFieldValue, $object->generatedField); @@ -437,11 +437,11 @@ class UnitOfWorkTest extends \Doctrine\Tests\OrmTestCase }) ); - /* @var $object EntityWithListenerPopulatedField */ + /* @var $object EntityWithRandomlyGeneratedField */ $object = $this->_unitOfWork->merge($entity); self::assertNotSame($object, $entity); - self::assertInstanceOf(EntityWithListenerPopulatedField::class, $object); + self::assertInstanceOf(EntityWithRandomlyGeneratedField::class, $object); self::assertSame($object->generatedField, $entity->generatedField); } @@ -452,8 +452,8 @@ class UnitOfWorkTest extends \Doctrine\Tests\OrmTestCase */ public function testMergeWithExistingEntityWillNotPersistItNorTriggerPrePersistListeners() { - $persistedEntity = new EntityWithListenerPopulatedField(); - $mergedEntity = new EntityWithListenerPopulatedField(); + $persistedEntity = new EntityWithRandomlyGeneratedField(); + $mergedEntity = new EntityWithRandomlyGeneratedField(); $mergedEntity->id = $persistedEntity->id; $mergedEntity->generatedField = mt_rand( @@ -476,7 +476,7 @@ class UnitOfWorkTest extends \Doctrine\Tests\OrmTestCase ['generatedField' => $persistedEntity->generatedField] ); - /* @var $merged EntityWithListenerPopulatedField */ + /* @var $merged EntityWithRandomlyGeneratedField */ $merged = $this->_unitOfWork->merge($mergedEntity); self::assertSame($merged, $persistedEntity); @@ -630,10 +630,8 @@ class EntityWithCompositeStringIdentifier } /** @Entity */ -class EntityWithListenerPopulatedField +class EntityWithRandomlyGeneratedField { - const MAX_GENERATED_FIELD_VALUE = 10000; - /** @Id @Column(type="string") */ public $id; @@ -645,6 +643,6 @@ class EntityWithListenerPopulatedField public function __construct() { $this->id = uniqid('id', true); - $this->generatedField = mt_rand(0, self::MAX_GENERATED_FIELD_VALUE); + $this->generatedField = mt_rand(0, 100000); } } From 3645a9c44d9206424b80586adc8c0feeff3a6f5b Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Sun, 18 Dec 2016 15:46:49 +0100 Subject: [PATCH 19/21] #6174 #5570 removed unused imports --- tests/Doctrine/Tests/ORM/UnitOfWorkTest.php | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php b/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php index fca72fc71..b847d82bd 100644 --- a/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php +++ b/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php @@ -4,7 +4,6 @@ namespace Doctrine\Tests\ORM; use Doctrine\Common\Collections\ArrayCollection; use Doctrine\Common\EventManager; -use Doctrine\Common\EventSubscriber; use Doctrine\Common\NotifyPropertyChanged; use Doctrine\Common\Persistence\Event\LifecycleEventArgs; use Doctrine\Common\PropertyChangedListener; From b0ede40f47b8616649e1824c4e89ccd7affe33e9 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Sun, 18 Dec 2016 15:48:10 +0100 Subject: [PATCH 20/21] #6174 #5570 removed modifications applied to the `CompanyContractListener`, since `UnitOfWorkTest` now completely encapsulates the scenarios being covered --- .../Company/CompanyContractListener.php | 24 ------------------- 1 file changed, 24 deletions(-) diff --git a/tests/Doctrine/Tests/Models/Company/CompanyContractListener.php b/tests/Doctrine/Tests/Models/Company/CompanyContractListener.php index ad4133d50..23714f329 100644 --- a/tests/Doctrine/Tests/Models/Company/CompanyContractListener.php +++ b/tests/Doctrine/Tests/Models/Company/CompanyContractListener.php @@ -4,8 +4,6 @@ namespace Doctrine\Tests\Models\Company; class CompanyContractListener { - const PRE_PERSIST = 0; - public $postPersistCalls; public $prePersistCalls; @@ -19,8 +17,6 @@ class CompanyContractListener public $postLoadCalls; - public $snapshots = []; - /** * @PostPersist */ @@ -34,7 +30,6 @@ class CompanyContractListener */ public function prePersistHandler(CompanyContract $contract) { - $this->snapshots[self::PRE_PERSIST][] = $this->takeSnapshot($contract); $this->prePersistCalls[] = func_get_args(); } @@ -85,23 +80,4 @@ class CompanyContractListener { $this->postLoadCalls[] = func_get_args(); } - - public function takeSnapshot(CompanyContract $contract) - { - $snapshot = []; - - foreach ((new \ReflectionClass($contract))->getProperties() as $property) { - $property->setAccessible(true); - - $value = $property->getValue($contract); - - if (is_object($value) || is_array($value)) { - continue; - } - - $snapshot[$property->getName()] = $property->getValue($contract); - } - - return $snapshot; - } } From d52dbe62ace7453d06909953d5980f7a54ba15ad Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Sun, 18 Dec 2016 16:24:42 +0100 Subject: [PATCH 21/21] #6174 #5570 switching `::class` to string constants for PHP 5.4 compat (still supported in ORM 2.5.x) --- tests/Doctrine/Tests/ORM/UnitOfWorkTest.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php b/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php index b847d82bd..f49fe489a 100644 --- a/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php +++ b/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php @@ -59,7 +59,7 @@ class UnitOfWorkTest extends \Doctrine\Tests\OrmTestCase { parent::setUp(); $this->_connectionMock = new ConnectionMock([], new DriverMock()); - $this->eventManager = $this->getMockBuilder(EventManager::class)->getMock(); + $this->eventManager = $this->getMockBuilder('Doctrine\Common\EventManager')->getMock(); $this->_emMock = EntityManagerMock::create($this->_connectionMock, null, $this->eventManager); // SUT $this->_unitOfWork = new UnitOfWorkMock($this->_emMock); @@ -428,7 +428,7 @@ class UnitOfWorkTest extends \Doctrine\Tests\OrmTestCase /* @var $object EntityWithRandomlyGeneratedField */ $object = $args->getObject(); - self::assertInstanceOf(EntityWithRandomlyGeneratedField::class, $object); + self::assertInstanceOf('Doctrine\Tests\ORM\EntityWithRandomlyGeneratedField', $object); self::assertNotSame($entity, $object); self::assertSame($generatedFieldValue, $object->generatedField); @@ -440,7 +440,7 @@ class UnitOfWorkTest extends \Doctrine\Tests\OrmTestCase $object = $this->_unitOfWork->merge($entity); self::assertNotSame($object, $entity); - self::assertInstanceOf(EntityWithRandomlyGeneratedField::class, $object); + self::assertInstanceOf('Doctrine\Tests\ORM\EntityWithRandomlyGeneratedField', $object); self::assertSame($object->generatedField, $entity->generatedField); }