From d674f1923de96c69dbac82c0359711bb85823608 Mon Sep 17 00:00:00 2001 From: romanb Date: Thu, 23 Jul 2009 09:52:16 +0000 Subject: [PATCH] [2.0] Fixed #2373. Some small perf. improvements for UnitOfWork. --- .../ORM/Mapping/ClassMetadataFactory.php | 2 +- .../ORM/Mapping/Driver/AnnotationDriver.php | 2 + .../Mapping/Driver/DoctrineAnnotations.php | 2 + lib/Doctrine/ORM/Mapping/OneToManyMapping.php | 6 +- lib/Doctrine/ORM/Mapping/OneToOneMapping.php | 6 +- lib/Doctrine/ORM/PersistentCollection.php | 8 +- lib/Doctrine/ORM/UnitOfWork.php | 105 ++++++++++++------ tests/Doctrine/Tests/Models/CMS/CmsUser.php | 2 +- .../ORM/Functional/BasicFunctionalTest.php | 9 +- 9 files changed, 91 insertions(+), 51 deletions(-) diff --git a/lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php b/lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php index fdb70a3b3..95d73da91 100644 --- a/lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php +++ b/lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php @@ -91,7 +91,7 @@ class ClassMetadataFactory /** * Returns the metadata object for a class. * - * @param string $className The name of the class. + * @param string $className The name of the class. * @return Doctrine\ORM\Mapping\ClassMetadata */ public function getMetadataFor($className) diff --git a/lib/Doctrine/ORM/Mapping/Driver/AnnotationDriver.php b/lib/Doctrine/ORM/Mapping/Driver/AnnotationDriver.php index 8cd291aa8..f88fe7591 100644 --- a/lib/Doctrine/ORM/Mapping/Driver/AnnotationDriver.php +++ b/lib/Doctrine/ORM/Mapping/Driver/AnnotationDriver.php @@ -184,11 +184,13 @@ class AnnotationDriver implements Driver $mapping['joinColumns'] = $joinColumns; $mapping['mappedBy'] = $oneToOneAnnot->mappedBy; $mapping['cascade'] = $oneToOneAnnot->cascade; + $mapping['orphanRemoval'] = $oneToOneAnnot->orphanRemoval; $metadata->mapOneToOne($mapping); } else if ($oneToManyAnnot = $this->_reader->getPropertyAnnotation($property, 'Doctrine\ORM\Mapping\OneToMany')) { $mapping['mappedBy'] = $oneToManyAnnot->mappedBy; $mapping['targetEntity'] = $oneToManyAnnot->targetEntity; $mapping['cascade'] = $oneToManyAnnot->cascade; + $mapping['orphanRemoval'] = $oneToManyAnnot->orphanRemoval; $metadata->mapOneToMany($mapping); } else if ($manyToOneAnnot = $this->_reader->getPropertyAnnotation($property, 'Doctrine\ORM\Mapping\ManyToOne')) { $mapping['joinColumns'] = $joinColumns; diff --git a/lib/Doctrine/ORM/Mapping/Driver/DoctrineAnnotations.php b/lib/Doctrine/ORM/Mapping/Driver/DoctrineAnnotations.php index fc9693c3e..1f0e11952 100644 --- a/lib/Doctrine/ORM/Mapping/Driver/DoctrineAnnotations.php +++ b/lib/Doctrine/ORM/Mapping/Driver/DoctrineAnnotations.php @@ -65,12 +65,14 @@ final class OneToOne extends \Doctrine\Common\Annotations\Annotation { public $cascade; public $fetch; public $optional; + public $orphanRemoval = false; } final class OneToMany extends \Doctrine\Common\Annotations\Annotation { public $mappedBy; public $targetEntity; public $cascade; public $fetch; + public $orphanRemoval = false; } final class ManyToOne extends \Doctrine\Common\Annotations\Annotation { public $targetEntity; diff --git a/lib/Doctrine/ORM/Mapping/OneToManyMapping.php b/lib/Doctrine/ORM/Mapping/OneToManyMapping.php index bec1dc2cd..d50877c6c 100644 --- a/lib/Doctrine/ORM/Mapping/OneToManyMapping.php +++ b/lib/Doctrine/ORM/Mapping/OneToManyMapping.php @@ -43,7 +43,7 @@ namespace Doctrine\ORM\Mapping; class OneToManyMapping extends AssociationMapping { /** Whether to delete orphaned elements (removed from the collection) */ - public $deleteOrphans = false; + public $orphanRemoval = false; /** FUTURE: The key column mapping, if any. The key column holds the keys of the Collection. */ //public $keyColumn; @@ -73,8 +73,8 @@ class OneToManyMapping extends AssociationMapping throw MappingException::oneToManyRequiresMappedBy($mapping['fieldName']); } - $this->deleteOrphans = isset($mapping['deleteOrphans']) ? - (bool)$mapping['deleteOrphans'] : false; + $this->orphanRemoval = isset($mapping['orphanRemoval']) ? + (bool) $mapping['orphanRemoval'] : false; } /** diff --git a/lib/Doctrine/ORM/Mapping/OneToOneMapping.php b/lib/Doctrine/ORM/Mapping/OneToOneMapping.php index bffa50af2..592a284e5 100644 --- a/lib/Doctrine/ORM/Mapping/OneToOneMapping.php +++ b/lib/Doctrine/ORM/Mapping/OneToOneMapping.php @@ -58,7 +58,7 @@ class OneToOneMapping extends AssociationMapping * * @var boolean */ - public $deleteOrphans = false; + public $orphanRemoval = false; /** * The join column definitions. @@ -107,8 +107,8 @@ class OneToOneMapping extends AssociationMapping $this->targetToSourceKeyColumns = array_flip($this->sourceToTargetKeyColumns); } - $this->deleteOrphans = isset($mapping['deleteOrphans']) ? - (bool)$mapping['deleteOrphans'] : false; + $this->orphanRemoval = isset($mapping['orphanRemoval']) ? + (bool) $mapping['orphanRemoval'] : false; return $mapping; } diff --git a/lib/Doctrine/ORM/PersistentCollection.php b/lib/Doctrine/ORM/PersistentCollection.php index 2b9a7ef6e..caaa9243b 100644 --- a/lib/Doctrine/ORM/PersistentCollection.php +++ b/lib/Doctrine/ORM/PersistentCollection.php @@ -197,14 +197,14 @@ final class PersistentCollection extends \Doctrine\Common\Collections\Collection */ public function remove($key) { - //TODO: delete entity if shouldDeleteOrphans - /*if ($this->_association->isOneToMany() && $this->_association->shouldDeleteOrphans) { - $this->_em->remove($removed); - }*/ $removed = parent::remove($key); if ($removed) { $this->_changed(); + if ($this->_association->isOneToMany() && $this->_association->orphanRemoval) { + $this->_em->getUnitOfWork()->scheduleOrphanRemoval($removed); + } } + return $removed; } diff --git a/lib/Doctrine/ORM/UnitOfWork.php b/lib/Doctrine/ORM/UnitOfWork.php index f493d543c..4624dd2cb 100644 --- a/lib/Doctrine/ORM/UnitOfWork.php +++ b/lib/Doctrine/ORM/UnitOfWork.php @@ -230,6 +230,13 @@ class UnitOfWork implements PropertyChangedListener * @var EventManager */ private $_evm; + + /** + * Orphaned entities scheduled for removal. + * + * @var array + */ + private $_orphanRemovals = array(); /** * Initializes a new UnitOfWork instance, bound to the given EntityManager. @@ -254,16 +261,23 @@ class UnitOfWork implements PropertyChangedListener // This populates _entityUpdates and _collectionUpdates. $this->computeChangeSets(); - if (empty($this->_entityInsertions) && - empty($this->_entityDeletions) && - empty($this->_entityUpdates) && - empty($this->_collectionUpdates) && - empty($this->_collectionDeletions)) { + if ( ! ($this->_entityInsertions || + $this->_entityDeletions || + $this->_entityUpdates || + $this->_collectionUpdates || + $this->_collectionDeletions || + $this->_orphanRemovals)) { return; // Nothing to do. } // Now we need a commit order to maintain referential integrity $commitOrder = $this->_getCommitOrder(); + + if ($this->_orphanRemovals) { + foreach ($this->_orphanRemovals as $orphan) { + $this->remove($orphan); + } + } $conn = $this->_em->getConnection(); try { @@ -317,17 +331,18 @@ class UnitOfWork implements PropertyChangedListener } // Clear up - $this->_entityInsertions = array(); - $this->_entityUpdates = array(); - $this->_entityDeletions = array(); - $this->_extraUpdates = array(); - $this->_entityChangeSets = array(); - $this->_collectionUpdates = array(); - $this->_collectionDeletions = array(); - $this->_visitedCollections = array(); + $this->_entityInsertions = + $this->_entityUpdates = + $this->_entityDeletions = + $this->_extraUpdates = + $this->_entityChangeSets = + $this->_collectionUpdates = + $this->_collectionDeletions = + $this->_visitedCollections = + $this->_orphanRemovals = array(); } - protected function _executeExtraUpdates() + private function _executeExtraUpdates() { foreach ($this->_extraUpdates as $oid => $update) { list ($entity, $changeset) = $update; @@ -363,11 +378,8 @@ class UnitOfWork implements PropertyChangedListener */ public function computeChangeSets() { - $entitySet = $this->_identityMap; - $entityInsertions = $this->_entityInsertions; - // Compute changes for INSERTed entities first. This must always happen. - foreach ($entityInsertions as $entity) { + foreach ($this->_entityInsertions as $entity) { $class = $this->_em->getClassMetadata(get_class($entity)); $this->_computeEntityChanges($class, $entity); // Look for changes in associations of the entity @@ -380,7 +392,7 @@ class UnitOfWork implements PropertyChangedListener } // Compute changes for other MANAGED entities. Change tracking policies take effect here. - foreach ($entitySet as $className => $entities) { + foreach ($this->_identityMap as $className => $entities) { $class = $this->_em->getClassMetadata($className); // Skip class if change tracking happens through notification @@ -493,8 +505,13 @@ class UnitOfWork implements PropertyChangedListener if (isset($changeSet[$propName])) { if (isset($class->associationMappings[$propName])) { $assoc = $class->associationMappings[$propName]; - if ($assoc->isOneToOne() && $assoc->isOwningSide) { - $entityIsDirty = true; + if ($assoc->isOneToOne()) { + if ($assoc->isOwningSide) { + $entityIsDirty = true; + } + if ($actualValue === null && $assoc->orphanRemoval) { + $this->scheduleOrphanRemoval($orgValue); + } } else if ($orgValue instanceof PersistentCollection) { // A PersistentCollection was de-referenced, so delete it. if ( ! in_array($orgValue, $this->_collectionDeletions, true)) { @@ -1478,19 +1495,39 @@ class UnitOfWork implements PropertyChangedListener $this->_collectionDeletions = array(); //$this->_collectionCreations = array(); $this->_collectionUpdates = array(); + //$this->_orphanRemovals = array(); $this->_commitOrderCalculator->clear(); } + + /** + * INTERNAL: + * Schedules an orphaned entity for removal. The remove() operation will be + * invoked on that entity at the beginning of the next commit of this + * UnitOfWork. + * + * @param object $entity + */ + public function scheduleOrphanRemoval($entity) + { + $this->_orphanRemovals[spl_object_hash($entity)] = $entity; + } - public function scheduleCollectionUpdate(PersistentCollection $coll) + /*public function scheduleCollectionUpdate(PersistentCollection $coll) { $this->_collectionUpdates[] = $coll; - } + }*/ - public function isCollectionScheduledForUpdate(PersistentCollection $coll) + /*public function isCollectionScheduledForUpdate(PersistentCollection $coll) { //... - } - + }*/ + + /** + * INTERNAL: + * Schedules a complete collection for removal when this UnitOfWork commits. + * + * @param PersistentCollection $coll + */ public function scheduleCollectionDeletion(PersistentCollection $coll) { //TODO: if $coll is already scheduled for recreation ... what to do? @@ -1503,15 +1540,15 @@ class UnitOfWork implements PropertyChangedListener //... } - public function scheduleCollectionRecreation(PersistentCollection $coll) + /*public function scheduleCollectionRecreation(PersistentCollection $coll) { $this->_collectionRecreations[] = $coll; - } + }*/ - public function isCollectionScheduledForRecreation(PersistentCollection $coll) + /*public function isCollectionScheduledForRecreation(PersistentCollection $coll) { //... - } + }*/ /** * INTERNAL: @@ -1519,8 +1556,8 @@ class UnitOfWork implements PropertyChangedListener * * @param string $className The name of the entity class. * @param array $data The data for the entity. - * @return object - * @internal Performance-sensitive method. Run the performance test suites when + * @return object The created entity instance. + * @internal Highly performance-sensitive method. Run the performance test suites when * making modifications. */ public function createEntity($className, array $data, $hints = array()) @@ -1783,7 +1820,7 @@ class UnitOfWork implements PropertyChangedListener */ public function propertyChanged($entity, $propertyName, $oldValue, $newValue) { - //if ($this->getEntityState($entity) == self::STATE_MANAGED) { + if ($this->getEntityState($entity) == self::STATE_MANAGED) { $oid = spl_object_hash($entity); $class = $this->_em->getClassMetadata(get_class($entity)); @@ -1802,6 +1839,6 @@ class UnitOfWork implements PropertyChangedListener } else { $this->_entityUpdates[$oid] = $entity; } - //} + } } } \ No newline at end of file diff --git a/tests/Doctrine/Tests/Models/CMS/CmsUser.php b/tests/Doctrine/Tests/Models/CMS/CmsUser.php index 7e65683c3..6f103740a 100644 --- a/tests/Doctrine/Tests/Models/CMS/CmsUser.php +++ b/tests/Doctrine/Tests/Models/CMS/CmsUser.php @@ -26,7 +26,7 @@ class CmsUser */ public $name; /** - * @OneToMany(targetEntity="CmsPhonenumber", mappedBy="user", cascade={"save", "delete"}) + * @OneToMany(targetEntity="CmsPhonenumber", mappedBy="user", cascade={"save", "delete"}, orphanRemoval=true) */ public $phonenumbers; /** diff --git a/tests/Doctrine/Tests/ORM/Functional/BasicFunctionalTest.php b/tests/Doctrine/Tests/ORM/Functional/BasicFunctionalTest.php index 19444040d..5530c909f 100644 --- a/tests/Doctrine/Tests/ORM/Functional/BasicFunctionalTest.php +++ b/tests/Doctrine/Tests/ORM/Functional/BasicFunctionalTest.php @@ -184,8 +184,7 @@ class BasicFunctionalTest extends \Doctrine\Tests\OrmFunctionalTestCase $this->assertEquals(0, $count); } - /* NOT YET IMPLEMENTED - public function testOneToManyOrphanDelete() + public function testOneToManyOrphanRemoval() { $user = new CmsUser; $user->name = 'Guilherme'; @@ -203,15 +202,15 @@ class BasicFunctionalTest extends \Doctrine\Tests\OrmFunctionalTestCase $this->_em->flush(); $user->getPhonenumbers()->remove(0); + $this->assertEquals(2, count($user->getPhonenumbers())); $this->_em->flush(); - // Check that the links in the association table have been deleted + // Check that there are just 2 phonenumbers left $count = $this->_em->getConnection()->execute("SELECT COUNT(*) FROM cms_phonenumbers", array())->fetchColumn(); $this->assertEquals(2, $count); // only 2 remaining - - }*/ + } public function testBasicQuery() {