From 31892fb4a8d3116f05da4e91f48865393c957030 Mon Sep 17 00:00:00 2001 From: romanb Date: Thu, 2 Jul 2009 11:48:44 +0000 Subject: [PATCH] [2.0] Fixed cascading issue (#2307). Fixed many-many object hydration issue. --- .../ORM/Internal/Hydration/ObjectHydrator.php | 20 ++- lib/Doctrine/ORM/PersistentCollection.php | 134 +++++++----------- lib/Doctrine/ORM/UnitOfWork.php | 14 +- .../Doctrine/Tests/Models/CMS/CmsArticle.php | 4 + .../Tests/Models/CMS/CmsPhonenumber.php | 8 +- tests/Doctrine/Tests/Models/CMS/CmsUser.php | 8 +- .../Models/ECommerce/ECommerceFeature.php | 5 +- .../AbstractManyToManyAssociationTestCase.php | 3 +- ...ManyToManyBidirectionalAssociationTest.php | 27 ++-- ...anyToManyUnidirectionalAssociationTest.php | 12 +- .../Doctrine/Tests/OrmFunctionalTestCase.php | 1 + 11 files changed, 111 insertions(+), 125 deletions(-) diff --git a/lib/Doctrine/ORM/Internal/Hydration/ObjectHydrator.php b/lib/Doctrine/ORM/Internal/Hydration/ObjectHydrator.php index 2716e5397..356f4f445 100644 --- a/lib/Doctrine/ORM/Internal/Hydration/ObjectHydrator.php +++ b/lib/Doctrine/ORM/Internal/Hydration/ObjectHydrator.php @@ -123,7 +123,6 @@ class ObjectHydrator extends AbstractHydrator // Take snapshots from all initialized collections foreach ($this->_collections as $coll) { $coll->takeSnapshot(); - $coll->setHydrationFlag(false); } // Clean up @@ -344,7 +343,7 @@ class ObjectHydrator extends AbstractHydrator if ( ! $relation->isOneToOne()) { if (isset($nonemptyComponents[$dqlAlias])) { if ( ! isset($this->_initializedRelations[spl_object_hash($baseElement)][$relationAlias])) { - $this->initRelatedCollection($baseElement, $relationAlias)->setHydrationFlag(true); + $this->initRelatedCollection($baseElement, $relationAlias); } $path = $parent . '.' . $dqlAlias; @@ -357,10 +356,17 @@ class ObjectHydrator extends AbstractHydrator // If it's a bi-directional many-to-many, also initialize the reverse collection. if ($relation->isManyToMany()) { if ($relation->isOwningSide && isset($this->_ce[$entityName]->inverseMappings[$relationAlias])) { - $this->initRelatedCollection($element, $this->_ce[$entityName] - ->inverseMappings[$relationAlias]->sourceFieldName); + $inverseFieldName = $this->_ce[$entityName]->inverseMappings[$relationAlias]->sourceFieldName; + // Only initialize reverse collection if it is not yet initialized. + if ( ! isset($this->_initializedRelations[spl_object_hash($element)][$inverseFieldName])) { + $this->initRelatedCollection($element, $this->_ce[$entityName] + ->inverseMappings[$relationAlias]->sourceFieldName); + } } else if ($relation->mappedByFieldName) { - $this->initRelatedCollection($element, $relation->mappedByFieldName); + // Only initialize reverse collection if it is not yet initialized. + if ( ! isset($this->_initializedRelations[spl_object_hash($element)][$relation->mappedByFieldName])) { + $this->initRelatedCollection($element, $relation->mappedByFieldName); + } } } @@ -371,12 +377,12 @@ class ObjectHydrator extends AbstractHydrator $this->_ce[$parentClass] ->reflFields[$relationAlias] ->getValue($baseElement) - ->set($indexValue, $element); + ->hydrateSet($indexValue, $element); } else { $this->_ce[$parentClass] ->reflFields[$relationAlias] ->getValue($baseElement) - ->add($element); + ->hydrateAdd($element); } $this->_identifierMap[$path][$id[$parent]][$id[$dqlAlias]] = $this->getLastKey( $this->_ce[$parentClass] diff --git a/lib/Doctrine/ORM/PersistentCollection.php b/lib/Doctrine/ORM/PersistentCollection.php index ef12cdd72..4f8af76dc 100644 --- a/lib/Doctrine/ORM/PersistentCollection.php +++ b/lib/Doctrine/ORM/PersistentCollection.php @@ -26,7 +26,7 @@ use Doctrine\ORM\Mapping\AssociationMapping; /** * A PersistentCollection represents a collection of elements that have persistent state. - * + * * Collections of entities represent only the associations (links) to those entities. * That means, if the collection is part of a many-many mapping and you remove * entities from the collection, only the links in the relation table are removed (on flush). @@ -43,7 +43,7 @@ use Doctrine\ORM\Mapping\AssociationMapping; * @author Roman Borschel */ final class PersistentCollection extends \Doctrine\Common\Collections\Collection -{ +{ /** * The base type of the collection. * @@ -61,7 +61,7 @@ final class PersistentCollection extends \Doctrine\Common\Collections\Collection /** * The entity that owns this collection. - * + * * @var object */ private $_owner; @@ -80,14 +80,14 @@ final class PersistentCollection extends \Doctrine\Common\Collections\Collection * @var string */ private $_keyField; - + /** * The EntityManager that manages the persistence of the collection. * * @var Doctrine\ORM\EntityManager */ private $_em; - + /** * The name of the field on the target entities that points to the owner * of the collection. This is only set if the association is bi-directional. @@ -95,14 +95,6 @@ final class PersistentCollection extends \Doctrine\Common\Collections\Collection * @var string */ private $_backRefFieldName; - - /** - * Hydration flag. - * - * @var boolean - * @see setHydrationFlag() - */ - private $_hydrationFlag = false; /** * The class descriptor of the owning entity. @@ -116,7 +108,7 @@ final class PersistentCollection extends \Doctrine\Common\Collections\Collection * @var boolean */ private $_isDirty = false; - + /** Whether the collection has already been initialized. */ private $_initialized = false; @@ -151,7 +143,7 @@ final class PersistentCollection extends \Doctrine\Common\Collections\Collection { return $this->_keyField; } - + /** * INTERNAL: * Sets the collection owner. Used (only?) during hydration. @@ -170,8 +162,7 @@ final class PersistentCollection extends \Doctrine\Common\Collections\Collection $targetClass = $this->_em->getClassMetadata($assoc->targetEntityName); if (isset($targetClass->inverseMappings[$assoc->sourceFieldName])) { // Bi-directional - $this->_backRefFieldName = $targetClass->inverseMappings[$assoc->sourceFieldName] - ->sourceFieldName; + $this->_backRefFieldName = $targetClass->inverseMappings[$assoc->sourceFieldName]->sourceFieldName; } } } @@ -208,7 +199,7 @@ final class PersistentCollection extends \Doctrine\Common\Collections\Collection { //TODO: delete entity if shouldDeleteOrphans /*if ($this->_association->isOneToMany() && $this->_association->shouldDeleteOrphans()) { - $this->_em->delete($removed); + $this->_em->delete($removed); }*/ $removed = parent::remove($key); if ($removed) { @@ -220,7 +211,7 @@ final class PersistentCollection extends \Doctrine\Common\Collections\Collection /** * When the collection is a Map this is like put(key,value)/add(key,value). * When the collection is a List this is like add(position,value). - * + * * @param integer $key * @param mixed $value * @override @@ -228,55 +219,57 @@ final class PersistentCollection extends \Doctrine\Common\Collections\Collection public function set($key, $value) { parent::set($key, $value); - if ( ! $this->_hydrationFlag) { - $this->_changed(); - } + $this->_changed(); } /** * Adds an element to the collection. - * + * * @param mixed $value - * @param string $key + * @param string $key * @return boolean Always TRUE. * @override */ public function add($value) { parent::add($value); - - if ($this->_hydrationFlag) { - if ($this->_backRefFieldName) { - // Set back reference to owner - if ($this->_association->isOneToMany()) { - $this->_typeClass->getReflectionProperty($this->_backRefFieldName) - ->setValue($value, $this->_owner); - } else { - // ManyToMany - $this->_typeClass->getReflectionProperty($this->_backRefFieldName) - ->getValue($value)->add($this->_owner); - } - } - } else { - $this->_changed(); - } - + $this->_changed(); return true; } + + /** + * INTERNAL: + * Adds an element to a collection during hydration. + * + * @param mixed $value The element to add. + */ + public function hydrateAdd($value) + { + parent::add($value); + if ($this->_backRefFieldName) { + // Set back reference to owner + if ($this->_association->isOneToMany()) { + // OneToMany + $this->_typeClass->getReflectionProperty($this->_backRefFieldName) + ->setValue($value, $this->_owner); + } else { + // ManyToMany + $this->_typeClass->getReflectionProperty($this->_backRefFieldName) + ->getValue($value)->add($this->_owner); + } + } + } /** - * Adds all elements of the other collection to this collection. + * INTERNAL: + * Sets a keyed element in the collection during hydration. * - * @param object $otherCollection - * @todo Impl - * @override + * @param mixed $key The key to set. + * $param mixed $value The element to set. */ - public function addAll($otherCollection) + public function hydrateSet($key, $value) { - parent::addAll($otherCollection); - //... - //TODO: Register collection as dirty with the UoW if necessary - //$this->_changed(); + parent::set($key, $value); } /** @@ -285,7 +278,7 @@ final class PersistentCollection extends \Doctrine\Common\Collections\Collection */ public function contains($element) { - + if ( ! $this->_initialized) { //TODO: Probably need to hit the database here...? //return $this->_checkElementExistence($element); @@ -306,31 +299,12 @@ final class PersistentCollection extends \Doctrine\Common\Collections\Collection private function _checkElementExistence($element) { - + } private function _initialize() { - - } - - /** - * INTERNAL: - * Sets a flag that indicates whether the collection is currently being hydrated. - * - * If the flag is set to TRUE, this has the following consequences: - * - * 1) During hydration, bidirectional associations are completed automatically - * by setting the back reference. - * 2) During hydration no change notifications are reported to the UnitOfWork. - * That means add() etc. do not cause the collection to be scheduled - * for an update. - * - * @param boolean $bool - */ - public function setHydrationFlag($bool) - { - $this->_hydrationFlag = $bool; + } /** @@ -376,24 +350,24 @@ final class PersistentCollection extends \Doctrine\Common\Collections\Collection /** * Compares two records. To be used on _snapshot diffs using array_udiff. - * + * * @return integer */ private function _compareRecords($a, $b) { return $a === $b ? 0 : 1; } - + /** * INTERNAL: Gets the association mapping of the collection. - * + * * @return Doctrine\ORM\Mapping\AssociationMapping */ public function getMapping() { return $this->_association; } - + /** * Clears the collection. */ @@ -402,14 +376,14 @@ final class PersistentCollection extends \Doctrine\Common\Collections\Collection //TODO: Register collection as dirty with the UoW if necessary //TODO: If oneToMany() && shouldDeleteOrphan() delete entities /*if ($this->_association->isOneToMany() && $this->_association->shouldDeleteOrphans()) { - foreach ($this->_data as $entity) { - $this->_em->delete($entity); - } + foreach ($this->_data as $entity) { + $this->_em->delete($entity); + } }*/ parent::clear(); $this->_changed(); } - + private function _changed() { $this->_isDirty = true; diff --git a/lib/Doctrine/ORM/UnitOfWork.php b/lib/Doctrine/ORM/UnitOfWork.php index 4ed02e960..2aa95e6a8 100644 --- a/lib/Doctrine/ORM/UnitOfWork.php +++ b/lib/Doctrine/ORM/UnitOfWork.php @@ -1148,11 +1148,11 @@ class UnitOfWork implements PropertyChangedListener } $relatedEntities = $class->reflFields[$assocMapping->getSourceFieldName()] ->getValue($entity); - if (($relatedEntities instanceof Collection) && count($relatedEntities) > 0) { + if ($relatedEntities instanceof Collection) { foreach ($relatedEntities as $relatedEntity) { $this->_doMerge($relatedEntity, $visited, $managedCopy, $assocMapping); } - } else if (is_object($relatedEntities)) { + } else if ($relatedEntities !== null) { $this->_doMerge($relatedEntities, $visited, $managedCopy, $assocMapping); } } @@ -1173,12 +1173,11 @@ class UnitOfWork implements PropertyChangedListener continue; } $relatedEntities = $class->reflFields[$assocMapping->sourceFieldName]->getValue($entity); - if (($relatedEntities instanceof Collection || is_array($relatedEntities)) - && count($relatedEntities) > 0) { + if (($relatedEntities instanceof Collection || is_array($relatedEntities))) { foreach ($relatedEntities as $relatedEntity) { $this->_doSave($relatedEntity, $visited, $insertNow); } - } else if (is_object($relatedEntities)) { + } else if ($relatedEntities !== null) { $this->_doSave($relatedEntities, $visited, $insertNow); } } @@ -1199,12 +1198,11 @@ class UnitOfWork implements PropertyChangedListener } $relatedEntities = $class->reflFields[$assocMapping->sourceFieldName] ->getValue($entity); - if ($relatedEntities instanceof Collection || is_array($relatedEntities) - && count($relatedEntities) > 0) { + if ($relatedEntities instanceof Collection || is_array($relatedEntities)) { foreach ($relatedEntities as $relatedEntity) { $this->_doDelete($relatedEntity, $visited); } - } else if (is_object($relatedEntities)) { + } else if ($relatedEntities !== null) { $this->_doDelete($relatedEntities, $visited); } } diff --git a/tests/Doctrine/Tests/Models/CMS/CmsArticle.php b/tests/Doctrine/Tests/Models/CMS/CmsArticle.php index 719f4b3d9..2d0eb3eb7 100644 --- a/tests/Doctrine/Tests/Models/CMS/CmsArticle.php +++ b/tests/Doctrine/Tests/Models/CMS/CmsArticle.php @@ -31,4 +31,8 @@ class CmsArticle * @OneToMany(targetEntity="CmsComment", mappedBy="article") */ public $comments; + + public function setAuthor(CmsUser $author) { + $this->user = $author; + } } diff --git a/tests/Doctrine/Tests/Models/CMS/CmsPhonenumber.php b/tests/Doctrine/Tests/Models/CMS/CmsPhonenumber.php index 1c1a11a82..39fd7b8f3 100644 --- a/tests/Doctrine/Tests/Models/CMS/CmsPhonenumber.php +++ b/tests/Doctrine/Tests/Models/CMS/CmsPhonenumber.php @@ -9,8 +9,7 @@ namespace Doctrine\Tests\Models\CMS; class CmsPhonenumber { /** - * @Column(type="string", length=50) - * @Id + * @Id @Column(type="string", length=50) */ public $phonenumber; /** @@ -21,6 +20,9 @@ class CmsPhonenumber public function setUser(CmsUser $user) { $this->user = $user; - $user->addPhonenumber($this); + } + + public function getUser() { + return $this->user; } } diff --git a/tests/Doctrine/Tests/Models/CMS/CmsUser.php b/tests/Doctrine/Tests/Models/CMS/CmsUser.php index 50ab5daee..7faa4d4a2 100644 --- a/tests/Doctrine/Tests/Models/CMS/CmsUser.php +++ b/tests/Doctrine/Tests/Models/CMS/CmsUser.php @@ -68,9 +68,7 @@ class CmsUser */ public function addPhonenumber(CmsPhonenumber $phone) { $this->phonenumbers[] = $phone; - if ($phone->user !== $this) { - $phone->user = $this; - } + $phone->setUser($this); } public function getPhonenumbers() { @@ -79,9 +77,7 @@ class CmsUser public function addArticle(CmsArticle $article) { $this->articles[] = $article; - if ($article->user !== $this) { - $article->user = $this; - } + $article->setAuthor($this); } public function addGroup(CmsGroup $group) { diff --git a/tests/Doctrine/Tests/Models/ECommerce/ECommerceFeature.php b/tests/Doctrine/Tests/Models/ECommerce/ECommerceFeature.php index ecdd9ff93..901e7c1e9 100644 --- a/tests/Doctrine/Tests/Models/ECommerce/ECommerceFeature.php +++ b/tests/Doctrine/Tests/Models/ECommerce/ECommerceFeature.php @@ -42,10 +42,7 @@ class ECommerceFeature } public function setProduct(ECommerceProduct $product) { - if ($this->product !== $product) { - $this->product = $product; - $product->addFeature($this); - } + $this->product = $product; } public function removeProduct() { diff --git a/tests/Doctrine/Tests/ORM/Functional/AbstractManyToManyAssociationTestCase.php b/tests/Doctrine/Tests/ORM/Functional/AbstractManyToManyAssociationTestCase.php index 8a175e50f..f0e240a62 100644 --- a/tests/Doctrine/Tests/ORM/Functional/AbstractManyToManyAssociationTestCase.php +++ b/tests/Doctrine/Tests/ORM/Functional/AbstractManyToManyAssociationTestCase.php @@ -32,7 +32,8 @@ class AbstractManyToManyAssociationTestCase extends \Doctrine\Tests\OrmFunctiona FROM {$this->_table} WHERE {$this->_firstField}=? AND {$this->_secondField}=?", - array($firstId, $secondId))); + array($firstId, $secondId)) + ->fetchAll()); } public function assertCollectionEquals(Collection $first, Collection $second) diff --git a/tests/Doctrine/Tests/ORM/Functional/ManyToManyBidirectionalAssociationTest.php b/tests/Doctrine/Tests/ORM/Functional/ManyToManyBidirectionalAssociationTest.php index 0ac35af1a..1f48be18d 100644 --- a/tests/Doctrine/Tests/ORM/Functional/ManyToManyBidirectionalAssociationTest.php +++ b/tests/Doctrine/Tests/ORM/Functional/ManyToManyBidirectionalAssociationTest.php @@ -14,9 +14,9 @@ require_once __DIR__ . '/../../TestInit.php'; */ class ManyToManyBidirectionalAssociationTest extends AbstractManyToManyAssociationTestCase { - protected $_firstField = 'cart_id'; - protected $_secondField = 'product_id'; - protected $_table = 'ecommerce_carts_products'; + protected $_firstField = 'product_id'; + protected $_secondField = 'category_id'; + protected $_table = 'ecommerce_products_categories'; private $firstProduct; private $secondProduct; private $firstCategory; @@ -77,12 +77,23 @@ class ManyToManyBidirectionalAssociationTest extends AbstractManyToManyAssociati { $this->_createLoadingFixture(); list ($firstProduct, $secondProduct) = $this->_findProducts(); - $categories = $firstProduct->getCategories(); - $products = $categories[0]->getProducts(); + + $this->assertEquals(2, count($firstProduct->getCategories())); + $this->assertEquals(2, count($secondProduct->getCategories())); + + $categories = $firstProduct->getCategories(); + $firstCategoryProducts = $categories[0]->getProducts(); + $secondCategoryProducts = $categories[1]->getProducts(); + + $this->assertEquals(2, count($firstCategoryProducts)); + $this->assertEquals(2, count($secondCategoryProducts)); - $this->assertTrue($products[0] instanceof ECommerceProduct); - $this->assertTrue($products[1] instanceof ECommerceProduct); - $this->assertCollectionEquals($products, $categories[1]->getProducts()); + $this->assertTrue($firstCategoryProducts[0] instanceof ECommerceProduct); + $this->assertTrue($firstCategoryProducts[1] instanceof ECommerceProduct); + $this->assertTrue($secondCategoryProducts[0] instanceof ECommerceProduct); + $this->assertTrue($secondCategoryProducts[1] instanceof ECommerceProduct); + + $this->assertCollectionEquals($firstCategoryProducts, $secondCategoryProducts); } protected function _createLoadingFixture() diff --git a/tests/Doctrine/Tests/ORM/Functional/ManyToManyUnidirectionalAssociationTest.php b/tests/Doctrine/Tests/ORM/Functional/ManyToManyUnidirectionalAssociationTest.php index cd2cf7f9c..054679c31 100644 --- a/tests/Doctrine/Tests/ORM/Functional/ManyToManyUnidirectionalAssociationTest.php +++ b/tests/Doctrine/Tests/ORM/Functional/ManyToManyUnidirectionalAssociationTest.php @@ -41,10 +41,8 @@ class ManyToManyUnidirectionalAssociationTest extends AbstractManyToManyAssociat $this->_em->save($this->firstCart); $this->_em->flush(); - $this->assertForeignKeysContain($this->firstCart->getId(), - $this->firstProduct->getId()); - $this->assertForeignKeysContain($this->firstCart->getId(), - $this->secondProduct->getId()); + $this->assertForeignKeysContain($this->firstCart->getId(), $this->firstProduct->getId()); + $this->assertForeignKeysContain($this->firstCart->getId(), $this->secondProduct->getId()); } public function testRemovesAManyToManyAssociation() @@ -56,10 +54,8 @@ class ManyToManyUnidirectionalAssociationTest extends AbstractManyToManyAssociat $this->_em->flush(); - $this->assertForeignKeysNotContain($this->firstCart->getId(), - $this->firstProduct->getId()); - $this->assertForeignKeysContain($this->firstCart->getId(), - $this->secondProduct->getId()); + $this->assertForeignKeysNotContain($this->firstCart->getId(), $this->firstProduct->getId()); + $this->assertForeignKeysContain($this->firstCart->getId(), $this->secondProduct->getId()); } public function testEagerLoad() diff --git a/tests/Doctrine/Tests/OrmFunctionalTestCase.php b/tests/Doctrine/Tests/OrmFunctionalTestCase.php index 9357c978c..f419b3019 100644 --- a/tests/Doctrine/Tests/OrmFunctionalTestCase.php +++ b/tests/Doctrine/Tests/OrmFunctionalTestCase.php @@ -83,6 +83,7 @@ class OrmFunctionalTestCase extends OrmTestCase $conn->exec('DELETE FROM ecommerce_shippings'); $conn->exec('DELETE FROM ecommerce_features'); $conn->exec('DELETE FROM ecommerce_categories'); + $conn->exec('DELETE FROM ecommerce_products_categories'); } if (isset($this->_usedModelSets['company'])) { $conn->exec('DELETE FROM company_persons_friends');