From d1c8d378cfa253489e0fd0623dc8a06708e34af9 Mon Sep 17 00:00:00 2001 From: bilouwan Date: Thu, 15 Dec 2016 12:19:56 +0100 Subject: [PATCH 01/20] 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 493d39f5dffb51d9614bf312fc60ee0ee9cf5c9c Mon Sep 17 00:00:00 2001 From: bilouwan Date: Thu, 15 Dec 2016 12:49:11 +0100 Subject: [PATCH 02/20] 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 00a41e6df..f175d47e9 100644 --- a/lib/Doctrine/ORM/UnitOfWork.php +++ b/lib/Doctrine/ORM/UnitOfWork.php @@ -1816,6 +1816,7 @@ class UnitOfWork implements PropertyChangedListener if ( ! $id) { $managedCopy = $this->newInstance($class); + $this->mergeEntityStateIntoManagedCopy($entity, $managedCopy); $this->persistNew($class, $managedCopy); } else { $flatId = ($class->containsForeignIdentifier) @@ -1847,30 +1848,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); @@ -1889,6 +1876,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 * @@ -3338,6 +3338,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 7f4de25a268e74521c8f674a9065b90111f762e2 Mon Sep 17 00:00:00 2001 From: bilouwan Date: Thu, 15 Dec 2016 13:03:53 +0100 Subject: [PATCH 03/20] 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 1be226cf63943a02e9cf4ea1079fe7fbfb05e4ec Mon Sep 17 00:00:00 2001 From: bilouwan Date: Thu, 15 Dec 2016 15:12:29 +0100 Subject: [PATCH 04/20] 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 754f36ef657c064aa8d062077d13656e0976cc12 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Sun, 18 Dec 2016 14:27:35 +0100 Subject: [PATCH 05/20] #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 f175d47e9..56a7ff7b8 100644 --- a/lib/Doctrine/ORM/UnitOfWork.php +++ b/lib/Doctrine/ORM/UnitOfWork.php @@ -1876,7 +1876,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 81d44d4d6e1de53976fce8508245975b55d5c5c2 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Sun, 18 Dec 2016 14:32:31 +0100 Subject: [PATCH 06/20] #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 56a7ff7b8..55a53c3cd 100644 --- a/lib/Doctrine/ORM/UnitOfWork.php +++ b/lib/Doctrine/ORM/UnitOfWork.php @@ -1784,7 +1784,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, array $assoc = []) { @@ -1876,6 +1876,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 3ce262a61a329187d8d939f9fddbe0e6020b0fb7 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Sun, 18 Dec 2016 14:36:39 +0100 Subject: [PATCH 07/20] #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 55a53c3cd..485f29d01 100644 --- a/lib/Doctrine/ORM/UnitOfWork.php +++ b/lib/Doctrine/ORM/UnitOfWork.php @@ -1887,16 +1887,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 85e2dc8f227d9cc9adbc7013ef8bf8006e252a96 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Sun, 18 Dec 2016 14:38:10 +0100 Subject: [PATCH 08/20] #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 485f29d01..a406c906a 100644 --- a/lib/Doctrine/ORM/UnitOfWork.php +++ b/lib/Doctrine/ORM/UnitOfWork.php @@ -3352,11 +3352,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 ab0e854830f1dcff64e9b8e0160727040519a57e Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Sun, 18 Dec 2016 14:39:46 +0100 Subject: [PATCH 09/20] #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 a406c906a..101d28fb8 100644 --- a/lib/Doctrine/ORM/UnitOfWork.php +++ b/lib/Doctrine/ORM/UnitOfWork.php @@ -1850,7 +1850,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 a22f1650268051d763bc847f6ad0f4b328085839 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Sun, 18 Dec 2016 14:45:08 +0100 Subject: [PATCH 10/20] #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 101d28fb8..c0b0dd8c3 100644 --- a/lib/Doctrine/ORM/UnitOfWork.php +++ b/lib/Doctrine/ORM/UnitOfWork.php @@ -1857,7 +1857,6 @@ class UnitOfWork implements PropertyChangedListener } $visited[$oid] = $managedCopy; // mark visited - // $this->mergeEntityStateIntoManagedCopy($entity, $managedCopy); if ($class->isChangeTrackingDeferredExplicit()) { $this->scheduleForDirtyCheck($entity); From 0c2edcd08aa5890cdfa3f9f28d65c5bbddc17734 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Sun, 18 Dec 2016 14:47:16 +0100 Subject: [PATCH 11/20] #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 00c67ba2db5d2ef9e02e76b16aae80e81ef1f5cb Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Sun, 18 Dec 2016 14:47:55 +0100 Subject: [PATCH 12/20] #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 9582ffc982f8cae74462bb089758faef0e9789d6 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Sun, 18 Dec 2016 14:53:54 +0100 Subject: [PATCH 13/20] #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 30cd2d172bb4a56251312de2bbf4970b1e241d32 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Sun, 18 Dec 2016 15:37:49 +0100 Subject: [PATCH 14/20] #6174 #5570 started moving tests around `prePersist` event subscriber triggering on `UnitOfWork` into the `UnitOfWorkTest` --- tests/Doctrine/Tests/ORM/UnitOfWorkTest.php | 77 +++++++++++++++++++-- 1 file changed, 72 insertions(+), 5 deletions(-) diff --git a/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php b/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php index 72382549e..b91e74e9f 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\ORMInvalidArgumentException; use Doctrine\ORM\UnitOfWork; @@ -48,18 +52,22 @@ class UnitOfWorkTest extends OrmTestCase */ private $_emMock; - protected function setUp() { + /** + * @var EventManager|\PHPUnit_Framework_MockObject_MockObject + */ + private $eventManager; + + protected function setUp() + { parent::setUp(); $this->_connectionMock = new ConnectionMock([], new DriverMock()); - $this->_emMock = EntityManagerMock::create($this->_connectionMock); + $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(); @@ -488,6 +496,45 @@ class UnitOfWorkTest extends 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); + } } /** @@ -634,3 +681,23 @@ class EntityWithCompositeStringIdentifier */ 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 f4595d3a2f35d9e189d976a16cac5067227b829f Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Sun, 18 Dec 2016 15:43:29 +0100 Subject: [PATCH 15/20] #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 b91e74e9f..4feaa88a2 100644 --- a/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php +++ b/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php @@ -535,6 +535,39 @@ class UnitOfWorkTest extends 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 262d13a0476cb9354071dda5af193a1d5eecd4ce Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Sun, 18 Dec 2016 15:44:48 +0100 Subject: [PATCH 16/20] #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 4feaa88a2..57c842fd7 100644 --- a/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php +++ b/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php @@ -497,6 +497,11 @@ class UnitOfWorkTest extends 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(); @@ -536,6 +541,11 @@ class UnitOfWorkTest extends OrmTestCase self::assertSame($object->generatedField, $entity->generatedField); } + /** + * @group DDC-1955 + * @group 5570 + * @group 6174 + */ public function testMergeWithExistingEntityWillNotPersistItNorTriggerPrePersistListeners() { $persistedEntity = new EntityWithListenerPopulatedField(); From f39f1a2e11edcf1e90e8c2192f62f7d560be7864 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Sun, 18 Dec 2016 15:45:03 +0100 Subject: [PATCH 17/20] #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 018a5db08f2eb37dfcc07c770213d72d5bc0de46 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Sun, 18 Dec 2016 15:46:34 +0100 Subject: [PATCH 18/20] #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 57c842fd7..238f4ce45 100644 --- a/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php +++ b/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php @@ -504,7 +504,7 @@ class UnitOfWorkTest extends OrmTestCase */ public function testMergeWithNewEntityWillPersistItAndTriggerPrePersistListenersWithMergedEntityData() { - $entity = new EntityWithListenerPopulatedField(); + $entity = new EntityWithRandomlyGeneratedField(); $generatedFieldValue = $entity->generatedField; @@ -522,10 +522,10 @@ class UnitOfWorkTest extends 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); @@ -533,11 +533,11 @@ class UnitOfWorkTest extends 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); } @@ -548,8 +548,8 @@ class UnitOfWorkTest extends OrmTestCase */ public function testMergeWithExistingEntityWillNotPersistItNorTriggerPrePersistListeners() { - $persistedEntity = new EntityWithListenerPopulatedField(); - $mergedEntity = new EntityWithListenerPopulatedField(); + $persistedEntity = new EntityWithRandomlyGeneratedField(); + $mergedEntity = new EntityWithRandomlyGeneratedField(); $mergedEntity->id = $persistedEntity->id; $mergedEntity->generatedField = mt_rand( @@ -572,7 +572,7 @@ class UnitOfWorkTest extends OrmTestCase ['generatedField' => $persistedEntity->generatedField] ); - /* @var $merged EntityWithListenerPopulatedField */ + /* @var $merged EntityWithRandomlyGeneratedField */ $merged = $this->_unitOfWork->merge($mergedEntity); self::assertSame($merged, $persistedEntity); @@ -726,10 +726,8 @@ class EntityWithCompositeStringIdentifier } /** @Entity */ -class EntityWithListenerPopulatedField +class EntityWithRandomlyGeneratedField { - const MAX_GENERATED_FIELD_VALUE = 10000; - /** @Id @Column(type="string") */ public $id; @@ -741,6 +739,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 cfd595b699dde4ebe7ca4b15ed1a16b568bf5f75 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Sun, 18 Dec 2016 15:46:49 +0100 Subject: [PATCH 19/20] #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 238f4ce45..90c621d60 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 21a5d8ca1b59096fce2afe3ccd2a11fa2165fca2 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Sun, 18 Dec 2016 15:48:10 +0100 Subject: [PATCH 20/20] #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; - } }