From 9caef624896c1f7ac181038f899e02aa18dd8154 Mon Sep 17 00:00:00 2001 From: Mathieu De Zutter Date: Wed, 5 Nov 2014 07:08:17 +0100 Subject: [PATCH 1/4] Test case for merging entities with associations to identical entities. --- .../Functional/MergeSharedEntitiesTest.php | 125 ++++++++++++++++++ 1 file changed, 125 insertions(+) create mode 100644 tests/Doctrine/Tests/ORM/Functional/MergeSharedEntitiesTest.php diff --git a/tests/Doctrine/Tests/ORM/Functional/MergeSharedEntitiesTest.php b/tests/Doctrine/Tests/ORM/Functional/MergeSharedEntitiesTest.php new file mode 100644 index 000000000..e367e5277 --- /dev/null +++ b/tests/Doctrine/Tests/ORM/Functional/MergeSharedEntitiesTest.php @@ -0,0 +1,125 @@ +_schemaTool->createSchema( + array( + $this->_em->getClassMetadata(__NAMESPACE__ . '\MSEFile'), + $this->_em->getClassMetadata(__NAMESPACE__ . '\MSEPicture'), + ) + ); + } catch (ToolsException $ignored) { + } + } + + public function testMergeSharedNewEntities() + { + + /** @var MSEPicture $picture */ + $file = new MSEFile; + + $picture = new MSEPicture; + $picture->file = $file; + $picture->otherFile = $file; + + $em = $this->_em; + + $picture = $em->merge($picture); + + $this->assertEquals($picture->file, $picture->otherFile, "Identical entities must remain identical"); + } + + public function testMergeSharedManagedEntities() + { + + /** @var MSEPicture $picture */ + $file = new MSEFile; + + $picture = new MSEPicture; + $picture->file = $file; + $picture->otherFile = $file; + + $em = $this->_em; + $em->persist($file); + $em->flush(); + $em->clear(); + + $picture = $em->merge($picture); + + $this->assertEquals($picture->file, $picture->otherFile, "Identical entities must remain identical"); + } + + public function testMergeSharedManagedEntitiesSerialize() + { + + /** @var MSEPicture $picture */ + $file = new MSEFile; + + $picture = new MSEPicture; + $picture->file = $file; + $picture->otherFile = $file; + + $serializedPicture = serialize($picture); + + $em = $this->_em; + $em->persist($file); + $em->flush(); + $em->clear(); + + $picture = unserialize($serializedPicture); + $picture = $em->merge($picture); + + $this->assertEquals($picture->file, $picture->otherFile, "Identical entities must remain identical"); + } + +} + +/** + * @Entity + */ +class MSEPicture +{ + /** + * @Column(name="picture_id", type="integer") + * @Id @GeneratedValue + */ + public $pictureId; + + /** + * @ManyToOne(targetEntity="MSEFile", cascade={"persist", "merge"}) + * @JoinColumn(name="file_id", referencedColumnName="file_id") + */ + public $file; + + /** + * @ManyToOne(targetEntity="MSEFile", cascade={"persist", "merge"}) + * @JoinColumn(name="other_file_id", referencedColumnName="file_id") + */ + public $otherFile; +} + +/** + * @Entity + */ +class MSEFile +{ + /** + * @Column(name="file_id", type="integer") + * @Id + * @GeneratedValue(strategy="AUTO") + */ + public $fileId; + +} From 2ead9e23aba10fa7cd67cdab18dafdfb9d83c863 Mon Sep 17 00:00:00 2001 From: Mathieu De Zutter Date: Wed, 5 Nov 2014 21:13:13 +0100 Subject: [PATCH 2/4] Fix merging of entities with associations to identical entities. Without this patch, when an entity that refers multiple times to the same associated entity gets merged, the second references becomes null. The main issue is that even though doMerge returns a managed copy, that value is not used while cascading the merge. These identicial entities are already detected through the visitor map, but they are ignored. There should be some refactoring so cascadeMerge calls a function that checks if the parent must be updated, based on the return value of its call to doMerge. However, this patch tries to impact the code as little as possible, and only introduces a new function to avoid duplicate code. The secondary issue arises when using inverted associations. In that case, it is possible that an entity to be merged is already merged, so the the visitor map is looked up by the hash of a managed copy instead of the original entity. This means that in this case the visitor map entries should also be set to the entity, instead of being set to 'true'. --- lib/Doctrine/ORM/UnitOfWork.php | 58 ++++++++++++++++++++++----------- 1 file changed, 39 insertions(+), 19 deletions(-) diff --git a/lib/Doctrine/ORM/UnitOfWork.php b/lib/Doctrine/ORM/UnitOfWork.php index 699926f43..45d11c7a4 100644 --- a/lib/Doctrine/ORM/UnitOfWork.php +++ b/lib/Doctrine/ORM/UnitOfWork.php @@ -1782,10 +1782,14 @@ class UnitOfWork implements PropertyChangedListener $oid = spl_object_hash($entity); if (isset($visited[$oid])) { - return $visited[$oid]; // Prevent infinite recursion - } + $managedCopy = $visited[$oid]; - $visited[$oid] = $entity; // mark visited + if ($prevManagedCopy !== null) { + $this->updateAssociationWithMergedEntity($entity, $prevManagedCopy, $assoc, $managedCopy); + } + + return $managedCopy; + } $class = $this->em->getClassMetadata(get_class($entity)); @@ -1855,6 +1859,8 @@ class UnitOfWork implements PropertyChangedListener } } + $visited[$oid] = $managedCopy; // mark visited + // Merge state of $entity into existing (managed) entity foreach ($class->reflClass->getProperties() as $prop) { $name = $prop->name; @@ -1898,9 +1904,9 @@ class UnitOfWork implements PropertyChangedListener $managedCol = $prop->getValue($managedCopy); if (!$managedCol) { $managedCol = new PersistentCollection($this->em, - $this->em->getClassMetadata($assoc2['targetEntity']), - new ArrayCollection - ); + $this->em->getClassMetadata($assoc2['targetEntity']), + new ArrayCollection + ); $managedCol->setOwner($managedCopy, $assoc2); $prop->setValue($managedCopy, $managedCol); $this->originalEntityData[$oid][$name] = $managedCol; @@ -1933,28 +1939,42 @@ class UnitOfWork implements PropertyChangedListener } if ($prevManagedCopy !== null) { - $assocField = $assoc['fieldName']; - $prevClass = $this->em->getClassMetadata(get_class($prevManagedCopy)); - - if ($assoc['type'] & ClassMetadata::TO_ONE) { - $prevClass->reflFields[$assocField]->setValue($prevManagedCopy, $managedCopy); - } else { - $prevClass->reflFields[$assocField]->getValue($prevManagedCopy)->add($managedCopy); - - if ($assoc['type'] == ClassMetadata::ONE_TO_MANY) { - $class->reflFields[$assoc['mappedBy']]->setValue($managedCopy, $prevManagedCopy); - } - } + $this->updateAssociationWithMergedEntity($entity, $prevManagedCopy, $assoc, $managedCopy); } // Mark the managed copy visited as well - $visited[spl_object_hash($managedCopy)] = true; + $visited[spl_object_hash($managedCopy)] = $managedCopy; $this->cascadeMerge($entity, $managedCopy, $visited); return $managedCopy; } + /** + * @param object $entity + * @param object $prevManagedCopy + * @param array $assoc + * @param object $managedCopy + */ + private function updateAssociationWithMergedEntity($entity, $prevManagedCopy, array $assoc, $managedCopy) + { + $assocField = $assoc['fieldName']; + $prevClass = $this->em->getClassMetadata(get_class($prevManagedCopy)); + + if ($assoc['type'] & ClassMetadata::TO_ONE) { + $prevClass->reflFields[$assocField]->setValue($prevManagedCopy, $managedCopy); + return; + } + + $value = $prevClass->reflFields[$assocField]->getValue($prevManagedCopy); + $value[] = $managedCopy; + + if ($assoc['type'] == ClassMetadata::ONE_TO_MANY) { + $class = $this->em->getClassMetadata(get_class($entity)); + $class->reflFields[$assoc['mappedBy']]->setValue($managedCopy, $prevManagedCopy); + } + } + /** * Detaches an entity from the persistence management. It's persistence will * no longer be managed by Doctrine. From 511893e18290385a7a0bc92407043ffd94374858 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Tue, 11 Nov 2014 12:27:57 +0100 Subject: [PATCH 3/4] #1173 - applying CS fixes on top of the patch --- lib/Doctrine/ORM/UnitOfWork.php | 30 ++++++++++++++++++------------ 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/lib/Doctrine/ORM/UnitOfWork.php b/lib/Doctrine/ORM/UnitOfWork.php index 45d11c7a4..c0e681bce 100644 --- a/lib/Doctrine/ORM/UnitOfWork.php +++ b/lib/Doctrine/ORM/UnitOfWork.php @@ -1785,7 +1785,7 @@ class UnitOfWork implements PropertyChangedListener $managedCopy = $visited[$oid]; if ($prevManagedCopy !== null) { - $this->updateAssociationWithMergedEntity($entity, $prevManagedCopy, $assoc, $managedCopy); + $this->updateAssociationWithMergedEntity($entity, $assoc, $prevManagedCopy, $managedCopy); } return $managedCopy; @@ -1939,7 +1939,7 @@ class UnitOfWork implements PropertyChangedListener } if ($prevManagedCopy !== null) { - $this->updateAssociationWithMergedEntity($entity, $prevManagedCopy, $assoc, $managedCopy); + $this->updateAssociationWithMergedEntity($entity, $assoc, $prevManagedCopy, $managedCopy); } // Mark the managed copy visited as well @@ -1951,27 +1951,33 @@ class UnitOfWork implements PropertyChangedListener } /** + * Sets/adds associated managed copies into the previous entity's association field + * * @param object $entity - * @param object $prevManagedCopy - * @param array $assoc + * @param array $association + * @param object $previousManagedCopy * @param object $managedCopy + * + * @return void */ - private function updateAssociationWithMergedEntity($entity, $prevManagedCopy, array $assoc, $managedCopy) + private function updateAssociationWithMergedEntity($entity, array $association, $previousManagedCopy, $managedCopy) { - $assocField = $assoc['fieldName']; - $prevClass = $this->em->getClassMetadata(get_class($prevManagedCopy)); + $assocField = $association['fieldName']; + $prevClass = $this->em->getClassMetadata(get_class($previousManagedCopy)); + + if ($association['type'] & ClassMetadata::TO_ONE) { + $prevClass->reflFields[$assocField]->setValue($previousManagedCopy, $managedCopy); - if ($assoc['type'] & ClassMetadata::TO_ONE) { - $prevClass->reflFields[$assocField]->setValue($prevManagedCopy, $managedCopy); return; } - $value = $prevClass->reflFields[$assocField]->getValue($prevManagedCopy); + $value = $prevClass->reflFields[$assocField]->getValue($previousManagedCopy); $value[] = $managedCopy; - if ($assoc['type'] == ClassMetadata::ONE_TO_MANY) { + if ($association['type'] == ClassMetadata::ONE_TO_MANY) { $class = $this->em->getClassMetadata(get_class($entity)); - $class->reflFields[$assoc['mappedBy']]->setValue($managedCopy, $prevManagedCopy); + + $class->reflFields[$association['mappedBy']]->setValue($managedCopy, $previousManagedCopy); } } From 2888791e5cccf122e01d5d5f53b18e3802420dde Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Tue, 11 Nov 2014 12:37:16 +0100 Subject: [PATCH 4/4] #1173 - test CS fixes, reduced clutter code, made method names more explicit --- .../Functional/MergeSharedEntitiesTest.php | 128 ++++++++---------- 1 file changed, 58 insertions(+), 70 deletions(-) diff --git a/tests/Doctrine/Tests/ORM/Functional/MergeSharedEntitiesTest.php b/tests/Doctrine/Tests/ORM/Functional/MergeSharedEntitiesTest.php index e367e5277..3dae34b67 100644 --- a/tests/Doctrine/Tests/ORM/Functional/MergeSharedEntitiesTest.php +++ b/tests/Doctrine/Tests/ORM/Functional/MergeSharedEntitiesTest.php @@ -1,125 +1,113 @@ . + */ namespace Doctrine\Tests\ORM\Functional; - use Doctrine\ORM\Tools\ToolsException; +use Doctrine\Tests\OrmFunctionalTestCase; -class MergeSharedEntitiesTest extends \Doctrine\Tests\OrmFunctionalTestCase +class MergeSharedEntitiesTest extends OrmFunctionalTestCase { - + /** + * {@inheritDoc} + */ protected function setUp() { parent::setUp(); try { - $this->_schemaTool->createSchema( - array( - $this->_em->getClassMetadata(__NAMESPACE__ . '\MSEFile'), - $this->_em->getClassMetadata(__NAMESPACE__ . '\MSEPicture'), - ) - ); + $this->_schemaTool->createSchema(array( + $this->_em->getClassMetadata(__NAMESPACE__ . '\MSEFile'), + $this->_em->getClassMetadata(__NAMESPACE__ . '\MSEPicture'), + )); } catch (ToolsException $ignored) { } } public function testMergeSharedNewEntities() { - - /** @var MSEPicture $picture */ - $file = new MSEFile; - + $file = new MSEFile; $picture = new MSEPicture; - $picture->file = $file; + + $picture->file = $file; $picture->otherFile = $file; - $em = $this->_em; + $picture = $this->_em->merge($picture); - $picture = $em->merge($picture); - - $this->assertEquals($picture->file, $picture->otherFile, "Identical entities must remain identical"); + $this->assertEquals($picture->file, $picture->otherFile, 'Identical entities must remain identical'); } public function testMergeSharedManagedEntities() { - - /** @var MSEPicture $picture */ - $file = new MSEFile; - + $file = new MSEFile; $picture = new MSEPicture; - $picture->file = $file; + + $picture->file = $file; $picture->otherFile = $file; - $em = $this->_em; - $em->persist($file); - $em->flush(); - $em->clear(); + $this->_em->persist($file); + $this->_em->persist($picture); + $this->_em->flush(); + $this->_em->clear(); - $picture = $em->merge($picture); + $picture = $this->_em->merge($picture); - $this->assertEquals($picture->file, $picture->otherFile, "Identical entities must remain identical"); + $this->assertEquals($picture->file, $picture->otherFile, 'Identical entities must remain identical'); } - public function testMergeSharedManagedEntitiesSerialize() + public function testMergeSharedDetachedSerializedEntities() { - - /** @var MSEPicture $picture */ - $file = new MSEFile; - + $file = new MSEFile; $picture = new MSEPicture; - $picture->file = $file; + + $picture->file = $file; $picture->otherFile = $file; $serializedPicture = serialize($picture); - $em = $this->_em; - $em->persist($file); - $em->flush(); - $em->clear(); + $this->_em->persist($file); + $this->_em->persist($picture); + $this->_em->flush(); + $this->_em->clear(); - $picture = unserialize($serializedPicture); - $picture = $em->merge($picture); + $picture = $this->_em->merge(unserialize($serializedPicture)); - $this->assertEquals($picture->file, $picture->otherFile, "Identical entities must remain identical"); + $this->assertEquals($picture->file, $picture->otherFile, 'Identical entities must remain identical'); } - } -/** - * @Entity - */ +/** @Entity */ class MSEPicture { - /** - * @Column(name="picture_id", type="integer") - * @Id @GeneratedValue - */ - public $pictureId; + /** @Column(type="integer") @Id @GeneratedValue */ + public $id; - /** - * @ManyToOne(targetEntity="MSEFile", cascade={"persist", "merge"}) - * @JoinColumn(name="file_id", referencedColumnName="file_id") - */ + /** @ManyToOne(targetEntity="MSEFile", cascade={"merge"}) */ public $file; - /** - * @ManyToOne(targetEntity="MSEFile", cascade={"persist", "merge"}) - * @JoinColumn(name="other_file_id", referencedColumnName="file_id") - */ + /** @ManyToOne(targetEntity="MSEFile", cascade={"merge"}) */ public $otherFile; } -/** - * @Entity - */ +/** @Entity */ class MSEFile { - /** - * @Column(name="file_id", type="integer") - * @Id - * @GeneratedValue(strategy="AUTO") - */ - public $fileId; - + /** @Column(type="integer") @Id @GeneratedValue(strategy="AUTO") */ + public $id; }