From d1c8d378cfa253489e0fd0623dc8a06708e34af9 Mon Sep 17 00:00:00 2001 From: bilouwan Date: Thu, 15 Dec 2016 12:19:56 +0100 Subject: [PATCH 1/4] 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 00000000000..a3575a821e8 --- /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 2/4] 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 00a41e6dffc..f175d47e96b 100644 --- a/lib/Doctrine/ORM/UnitOfWork.php +++ b/lib/Doctrine/ORM/UnitOfWork.php @@ -1816,6 +1816,7 @@ private function doMerge($entity, array &$visited, $prevManagedCopy = null, arra if ( ! $id) { $managedCopy = $this->newInstance($class); + $this->mergeEntityStateIntoManagedCopy($entity, $managedCopy); $this->persistNew($class, $managedCopy); } else { $flatId = ($class->containsForeignIdentifier) @@ -1847,30 +1848,16 @@ private function doMerge($entity, array &$visited, $prevManagedCopy = null, arra $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 @@ private function doMerge($entity, array &$visited, $prevManagedCopy = null, arra 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 @@ private function isIdentifierEquals($entity1, $entity2) */ 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 3/4] 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 61c1d4719f2..c5862887835 100644 --- a/tests/Doctrine/Tests/Models/Company/CompanyContractListener.php +++ b/tests/Doctrine/Tests/Models/Company/CompanyContractListener.php @@ -4,19 +4,23 @@ 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 @@ public function postPersistHandler(CompanyContract $contract) */ public function prePersistHandler(CompanyContract $contract) { + $this->snapshots[self::PRE_PERSIST][] = $this->takeSnapshot($contract); $this->prePersistCalls[] = func_get_args(); } @@ -81,4 +86,20 @@ public function postLoadHandler(CompanyContract $contract) $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 a3575a821e8..5354c0a30aa 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 @@ protected function setUp() $this->_em->getClassMetadata(DDC3597Image::class), ] ); + + $this->listener = $this->_em->getConfiguration() + ->getEntityListenerResolver() + ->resolve('Doctrine\Tests\Models\Company\CompanyContractListener'); } protected function tearDown() @@ -47,4 +62,35 @@ public function testMergeNewEntityLifecyleEventsModificationsShouldBeKept() $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 4/4] 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 5354c0a30aa..6123fe99770 100644 --- a/tests/Doctrine/Tests/ORM/Functional/EntityListenersOnMergeTest.php +++ b/tests/Doctrine/Tests/ORM/Functional/EntityListenersOnMergeTest.php @@ -63,7 +63,7 @@ public function testMergeNewEntityLifecyleEventsModificationsShouldBeKept() $this->assertNotNull($imageEntity->getUpdatedAt()); } - public function testPrePersistListeners() + public function testPrePersistListenersShouldBeFiredWithCorrectEntityData() { $fix = new CompanyFixContract(); $fix->setFixPrice(2000);