[2.0][DDC-21] Fixed.

lib/Doctrine/ORM/Internal/Hydration/ObjectHydrator.php

@@ -31,6 +31,7 @@ use Doctrine\ORM\PersistentCollection,
  *
  * @author Roman Borschel <roman@code-factory.org>
  * @since 2.0
+ * @internal Highly performance-sensitive code.
  */
 class ObjectHydrator extends AbstractHydrator
 {
@@ -49,7 +50,10 @@ class ObjectHydrator extends AbstractHydrator
     private $_fetchedAssociations;
     private $_rootAliases = array();
     private $_initializedCollections = array();
+    private $_existingCollections = array();
     private $_proxyFactory;
+    //private $_createdEntities;
+    
 
     /** @override */
     protected function _prepare()
@@ -57,9 +61,7 @@ class ObjectHydrator extends AbstractHydrator
         $this->_allowPartialObjects = $this->_em->getConfiguration()->getAllowPartialObjects()
                 || isset($this->_hints[Query::HINT_FORCE_PARTIAL_LOAD]);
         
-        if ( ! $this->_allowPartialObjects) {
-            $this->_proxyFactory = $this->_em->getProxyFactory();
-        }
+        $this->_proxyFactory = $this->_em->getProxyFactory();
         
         $this->_identifierMap =
         $this->_resultPointers =
@@ -81,7 +83,7 @@ class ObjectHydrator extends AbstractHydrator
             // collection stubs or proxies and where not.
             if (isset($this->_rsm->relationMap[$dqlAlias])) {
                 $targetClassName = $this->_rsm->aliasMap[$this->_rsm->parentAliasMap[$dqlAlias]];
-                $targetClass = $this->_em->getClassMetadata($targetClassName);
+                $targetClass = $this->_getClassMetadata($targetClassName);
                 $this->_ce[$targetClassName] = $targetClass;
                 $assoc = $targetClass->associationMappings[$this->_rsm->relationMap[$dqlAlias]];
                 $this->_fetchedAssociations[$assoc->sourceEntityName][$assoc->sourceFieldName] = true;
@@ -99,22 +101,20 @@ class ObjectHydrator extends AbstractHydrator
     
     /**
      * {@inheritdoc}
-     * 
-     * @override
      */
-    protected function _cleanup()
+    /*@override*/ protected function _cleanup()
     {
         parent::_cleanup();
         $this->_identifierMap =
+        $this->_initializedCollections =
+        $this->_existingCollections =
         $this->_resultPointers = array();
     }
 
     /**
      * {@inheritdoc}
-     *
-     * @override
      */
-    protected function _hydrateAll()
+    /*@override*/ protected function _hydrateAll()
     {
         $result = array();
         $cache = array();
@@ -122,11 +122,10 @@ class ObjectHydrator extends AbstractHydrator
             $this->_hydrateRow($data, $cache, $result);
         }
 
-        // Take snapshots from all initialized collections
+        // Take snapshots from all newly initialized collections
         foreach ($this->_initializedCollections as $coll) {
             $coll->takeSnapshot();
         }
-        $this->_initializedCollections = array();
 
         return $result;
     }
@@ -155,17 +154,20 @@ class ObjectHydrator extends AbstractHydrator
                 $value
             );
             $value->setOwner($entity, $relation);
-        } else {
-            // Is already PersistentCollection.
+            $class->reflFields[$name]->setValue($entity, $value);
+            $this->_uow->setOriginalEntityProperty($oid, $name, $value);
+            $this->_initializedCollections[$oid . $name] = $value;
+        } else if (isset($this->_hints[Query::HINT_REFRESH])) {
+            // Is already PersistentCollection, but REFRESH
             $value->clear();
             $value->setDirty(false);
             $value->setInitialized(true);
+            $this->_initializedCollections[$oid . $name] = $value;
+        } else {
+            // Is already PersistentCollection, and DONT REFRESH
+            $this->_existingCollections[$oid . $name] = $value;
         }
         
-        $class->reflFields[$name]->setValue($entity, $value);
-        $this->_uow->setOriginalEntityProperty($oid, $name, $value);
-        $this->_initializedCollections[$oid . $name] = $value;
-        
         return $value;
     }
     
@@ -187,6 +189,7 @@ class ObjectHydrator extends AbstractHydrator
         
         $entity = $this->_uow->createEntity($className, $data, $this->_hints);
 
+        //FIXME: If $entity comes from the identity map there is no need to do this!
         // Properly initialize any unfetched associations, if partial objects are not allowed.
         if ( ! $this->_allowPartialObjects) {
             $oid = spl_object_hash($entity);
@@ -232,6 +235,23 @@ class ObjectHydrator extends AbstractHydrator
         return $entity;
     }
     
+    private function _getEntityFromIdentityMap($className, array $data)
+    {
+        $class = $this->_ce[$className];
+
+        if ($class->isIdentifierComposite) {
+            $idHash = '';
+            foreach ($class->identifier as $fieldName) {
+                $idHash .= $data[$fieldName] . ' ';
+            }
+            $idHash = rtrim($idHash);
+        } else {
+            $idHash = $data[$class->identifier[0]];
+        }
+
+        return $this->_uow->tryGetByIdHash($idHash, $class->rootEntityName);
+    }
+    
     /**
      * Gets a ClassMetadata instance from the local cache.
      * If the instance is not yet in the local cache, it is loaded into the
@@ -250,10 +270,8 @@ class ObjectHydrator extends AbstractHydrator
 
     /**
      * {@inheritdoc}
-     * 
-     * @override
      */
-    protected function _hydrateRow(array &$data, array &$cache, array &$result)
+    /*@override*/ protected function _hydrateRow(array &$data, array &$cache, array &$result)
     {
         // Initialize
         $id = $this->_idTemplate; // initialize the id-memory
@@ -266,7 +284,7 @@ class ObjectHydrator extends AbstractHydrator
             unset($rowData['scalars']);
         }
 
-        // Hydrate the entity data found in the current row.
+        // Hydrate the data found in the current row.
         foreach ($rowData as $dqlAlias => $data) {
             $index = false;
             $entityName = $this->_rsm->aliasMap[$dqlAlias];
@@ -274,31 +292,34 @@ class ObjectHydrator extends AbstractHydrator
             if (isset($this->_rsm->parentAliasMap[$dqlAlias])) {
                 // It's a joined result
                 
-                $parent = $this->_rsm->parentAliasMap[$dqlAlias];
+                $parentAlias = $this->_rsm->parentAliasMap[$dqlAlias];
 
-                // Get a reference to the right object to which the joined result belongs.
-                if ($this->_rsm->isMixed && isset($this->_rootAliases[$parent])) {
+                // Get a reference to the parent object to which the joined element belongs.
+                if ($this->_rsm->isMixed && isset($this->_rootAliases[$parentAlias])) {
                 	$first = reset($this->_resultPointers);
-                    $baseElement = $this->_resultPointers[$parent][key($first)];
-                } else if (isset($this->_resultPointers[$parent])) {
-                    $baseElement = $this->_resultPointers[$parent];
+                    $parentObject = $this->_resultPointers[$parentAlias][key($first)];
+                } else if (isset($this->_resultPointers[$parentAlias])) {
+                    $parentObject = $this->_resultPointers[$parentAlias];
                 } else {
-                    throw HydrationException::parentObjectOfRelationNotFound($dqlAlias, $parent);
+                    // Parent object of relation not found, so skip it.
+                    continue;
                 }
 
-                $parentClass = get_class($baseElement);
-                $oid = spl_object_hash($baseElement);
+                $parentClass = get_class($parentObject);
+                $oid = spl_object_hash($parentObject);
                 $relationField = $this->_rsm->relationMap[$dqlAlias];
                 $relation = $this->_ce[$parentClass]->associationMappings[$relationField];
                 $reflField = $this->_ce[$parentClass]->reflFields[$relationField];
-                $reflFieldValue = $reflField->getValue($baseElement);
                 
                 // Check the type of the relation (many or single-valued)
                 if ( ! $relation->isOneToOne()) {
                     // Collection-valued association
                     if (isset($nonemptyComponents[$dqlAlias])) {
-                        if ( ! isset($this->_initializedCollections[$oid . $relationField])) {
-                            $reflFieldValue = $this->_initRelatedCollection($baseElement, $relationField);
+                        $collKey = $oid . $relationField;
+                        if (isset($this->_initializedCollections[$collKey])) {
+                            $reflFieldValue = $this->_initializedCollections[$collKey];
+                        } else if ( ! isset($this->_existingCollections[$collKey])) {
+                            $reflFieldValue = $this->_initRelatedCollection($parentObject, $relationField);
                         }
                         
                         $indexExists = isset($this->_identifierMap[$dqlAlias][$id[$dqlAlias]]);
@@ -306,66 +327,78 @@ class ObjectHydrator extends AbstractHydrator
                         $indexIsValid = $index !== false ? isset($reflFieldValue[$index]) : false;
                         
                         if ( ! $indexExists || ! $indexIsValid) {
-                            $element = $this->_getEntity($data, $dqlAlias);
-
-                            // 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[$relation->sourceEntityName][$relationField])) {
-                                    $inverseFieldName = $this->_ce[$entityName]->inverseMappings[$relation->sourceEntityName][$relationField]->sourceFieldName;
-                                    // Only initialize reverse collection if it is not yet initialized.
-                                    if ( ! isset($this->_initializedCollections[spl_object_hash($element) . $inverseFieldName])) {
-                                        $this->_initRelatedCollection($element, $inverseFieldName);
-                                    }
-                                } else if ($relation->mappedByFieldName) {
-                                    // Only initialize reverse collection if it is not yet initialized.
-                                    if ( ! isset($this->_initializedCollections[spl_object_hash($element) . $relation->mappedByFieldName])) {
-                                        $this->_initRelatedCollection($element, $relation->mappedByFieldName);
-                                    }
+                            if (isset($this->_existingCollections[$collKey])) {
+                                // Collection exists, only look for $element in identity map.
+                                if ($element = $this->_getEntityFromIdentityMap($entityName, $data)) {
+                                    $this->_resultPointers[$dqlAlias] = $element;
+                                } else {
+                                    unset($this->_resultPointers[$dqlAlias]);
                                 }
-                            }
-
-                            if (isset($this->_rsm->indexByMap[$dqlAlias])) {
-                                $field = $this->_rsm->indexByMap[$dqlAlias];
-                                $indexValue = $this->_ce[$entityName]->reflFields[$field]->getValue($element);
-                                $reflFieldValue->hydrateSet($indexValue, $element);
-                                $this->_identifierMap[$dqlAlias][$id[$dqlAlias]] = $indexValue;
                             } else {
-                                $reflFieldValue->hydrateAdd($element);
-                                $reflFieldValue->last();
-                                $this->_identifierMap[$dqlAlias][$id[$dqlAlias]] = $reflFieldValue->key();
+                                $element = $this->_getEntity($data, $dqlAlias);
+                                
+                                // 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[$relation->sourceEntityName][$relationField])) {
+                                        $inverseFieldName = $this->_ce[$entityName]->inverseMappings[$relation->sourceEntityName][$relationField]->sourceFieldName;
+                                        // Only initialize reverse collection if it is not yet initialized.
+                                        if ( ! isset($this->_initializedCollections[spl_object_hash($element) . $inverseFieldName])) {
+                                            $this->_initRelatedCollection($element, $inverseFieldName);
+                                        }
+                                    } else if ($relation->mappedByFieldName) {
+                                        // Only initialize reverse collection if it is not yet initialized.
+                                        if ( ! isset($this->_initializedCollections[spl_object_hash($element) . $relation->mappedByFieldName])) {
+                                            $this->_initRelatedCollection($element, $relation->mappedByFieldName);
+                                        }
+                                    }
+                                }
+    
+                                if (isset($this->_rsm->indexByMap[$dqlAlias])) {
+                                    $field = $this->_rsm->indexByMap[$dqlAlias];
+                                    $indexValue = $this->_ce[$entityName]->reflFields[$field]->getValue($element);
+                                    $reflFieldValue->hydrateSet($indexValue, $element);
+                                    $this->_identifierMap[$dqlAlias][$id[$dqlAlias]] = $indexValue;
+                                } else {
+                                    $reflFieldValue->hydrateAdd($element);
+                                    $reflFieldValue->last();
+                                    $this->_identifierMap[$dqlAlias][$id[$dqlAlias]] = $reflFieldValue->key();
+                                }
+                                // Update result pointer
+                                $this->_resultPointers[$dqlAlias] = $element;
                             }
+                        } else {
+                            // Update result pointer
+                            $this->_resultPointers[$dqlAlias] = $reflFieldValue[$index];
                         }
-                        
-                        // Update result pointer
-                        $this->_resultPointers[$dqlAlias] = $index === false ? $element : $reflFieldValue[$index];
-                        
-                    } else if ( ! $reflFieldValue) {
+                    } else if ( ! $reflField->getValue($parentObject)) {
                         $coll = new PersistentCollection($this->_em, $this->_ce[$entityName], new ArrayCollection);
-                        $reflField->setValue($baseElement, $coll);
+                        $reflField->setValue($parentObject, $coll);
                         $this->_uow->setOriginalEntityProperty($oid, $relationField, $coll);
                     }
                 } else {
                     // Single-valued association
-                    $reflFieldValue = $reflField->getValue($baseElement);
-                    if ( ! $reflFieldValue /* || doctrine.refresh hint set */) {
+                    $reflFieldValue = $reflField->getValue($parentObject);
+                    if ( ! $reflFieldValue || isset($this->_hints[Query::HINT_REFRESH])) {
                         if (isset($nonemptyComponents[$dqlAlias])) {
                             $element = $this->_getEntity($data, $dqlAlias);
-                            $reflField->setValue($baseElement, $element);
+                            $reflField->setValue($parentObject, $element);
                             $this->_uow->setOriginalEntityProperty($oid, $relationField, $element);
                             $targetClass = $this->_ce[$relation->targetEntityName];
                             if ($relation->isOwningSide) {
                                 // If there is an inverse mapping on the target class its bidirectional
                                 if (isset($targetClass->inverseMappings[$relation->sourceEntityName][$relationField])) {
                                     $sourceProp = $targetClass->inverseMappings[$relation->sourceEntityName][$relationField]->sourceFieldName;
-                                    $targetClass->reflFields[$sourceProp]->setValue($element, $baseElement);
+                                    $targetClass->reflFields[$sourceProp]->setValue($element, $parentObject);
                                 } else if ($this->_ce[$parentClass] === $targetClass && $relation->mappedByFieldName) {
                                     // Special case: bi-directional self-referencing one-one on the same class
-                                    $targetClass->reflFields[$relationField]->setValue($element, $baseElement);
+                                    $targetClass->reflFields[$relationField]->setValue($element, $parentObject);
                                 }
                             } else {
                                 // For sure bidirectional, as there is no inverse side in unidirectional mappings
-                                $targetClass->reflFields[$relation->mappedByFieldName]->setValue($element, $baseElement);
+                                $targetClass->reflFields[$relation->mappedByFieldName]->setValue($element, $parentObject);
                             }
+                            // Update result pointer
+                            $this->_resultPointers[$dqlAlias] = $element;
                         }
                         // else leave $reflFieldValue null for single-valued associations
                     } else {

lib/Doctrine/ORM/Mapping/Driver/AnnotationDriver.php

@@ -273,7 +273,7 @@ class AnnotationDriver implements Driver
                 $mapping['mappedBy'] = $manyToManyAnnot->mappedBy;
                 $mapping['cascade'] = $manyToManyAnnot->cascade;
                 $metadata->mapManyToMany($mapping);
-            }   
+            }
         }
         
         // Evaluate HasLifecycleCallbacks annotation

lib/Doctrine/ORM/UnitOfWork.php

@@ -1080,10 +1080,8 @@ class UnitOfWork implements PropertyChangedListener
      */
     public function tryGetByIdHash($idHash, $rootClassName)
     {
-        if ($this->containsIdHash($idHash, $rootClassName)) {
-            return $this->getByIdHash($idHash, $rootClassName);
-        }
-        return false;
+        return isset($this->_identityMap[$rootClassName][$idHash]) ?
+                $this->_identityMap[$rootClassName][$idHash] : false;
     }
 
     /**
@@ -1656,8 +1654,7 @@ class UnitOfWork implements PropertyChangedListener
      * @param string $className  The name of the entity class.
      * @param array $data  The data for the entity.
      * @return object The created entity instance.
-     * @internal Highly performance-sensitive method. Run the performance test suites when
-     *           making modifications.
+     * @internal Highly performance-sensitive method.
      */
     public function createEntity($className, array $data, $hints = array())
     {
@@ -1703,6 +1700,7 @@ class UnitOfWork implements PropertyChangedListener
             }
         }
         
+        //TODO: These should be invoked later, because associations are not yet loaded here.
         if (isset($class->lifecycleCallbacks[Events::postLoad])) {
             $class->invokeLifecycleCallbacks(Events::postLoad, $entity);
         }

tests/Doctrine/Tests/ORM/Functional/AllTests.php

@@ -40,6 +40,7 @@ class AllTests
         $suite->addTestSuite('Doctrine\Tests\ORM\Functional\StandardEntityPersisterTest');
         $suite->addTestSuite('Doctrine\Tests\ORM\Functional\MappedSuperclassTest');
         $suite->addTestSuite('Doctrine\Tests\ORM\Functional\EntityRepositoryTest');
+        $suite->addTestSuite('Doctrine\Tests\ORM\Functional\IdentityMapTest');
         
         $suite->addTest(Locking\AllTests::suite());
         $suite->addTest(SchemaTool\AllTests::suite());

tests/Doctrine/Tests/ORM/Functional/IdentityMapTest.php

+<?php
+
+namespace Doctrine\Tests\ORM\Functional;
+
+use Doctrine\Tests\Models\CMS\CmsUser,
+    Doctrine\Tests\Models\CMS\CmsAddress,
+    Doctrine\Tests\Models\CMS\CmsPhonenumber,
+    Doctrine\ORM\Query;
+
+require_once __DIR__ . '/../../TestInit.php';
+
+/**
+ * IdentityMapTest
+ * 
+ * Tests correct behavior and usage of the identity map. Local values and associations
+ * that are already fetched always prevail, unless explicitly refreshed.
+ *
+ * @author Roman Borschel <roman@code-factory.org>
+ */
+class IdentityMapTest extends \Doctrine\Tests\OrmFunctionalTestCase
+{
+    protected function setUp() {
+        $this->useModelSet('cms');
+        parent::setUp();
+    }
+
+    public function testSingleValuedAssociationIdentityMapBehavior()
+    {
+        $address = new CmsAddress;
+        $address->country = 'de';
+        $address->zip = '12345';
+        $address->city = 'Berlin';
+        
+        $user1 = new CmsUser;
+        $user1->status = 'dev';
+        $user1->username = 'romanb';
+        $user1->name = 'Roman B.';
+
+        $user2 = new CmsUser;
+        $user2->status = 'dev';
+        $user2->username = 'gblanco';
+        $user2->name = 'Guilherme Blanco';
+        
+        $address->setUser($user1);
+        
+        $this->_em->persist($address);
+        $this->_em->persist($user1);
+        $this->_em->persist($user2);
+        $this->_em->flush();
+        
+        
+        $this->assertSame($user1, $address->user);
+        
+        //external update to CmsAddress
+        $this->_em->getConnection()->executeUpdate('update cms_addresses set user_id = ?', array($user2->getId()));
+        
+        //select
+        $q = $this->_em->createQuery('select a, u from Doctrine\Tests\Models\CMS\CmsAddress a join a.user u');
+        $address2 = $q->getSingleResult();
+        
+        $this->assertSame($address, $address2);
+        
+        // Should still be $user1
+        $this->assertSame($user1, $address2->user);
+        $this->assertTrue($user2->address === null);
+        
+        // But we want to have this external change!
+        // Solution 1: refresh(), broken atm!  
+        //$this->_em->refresh($address2);
+        // Solution 2: Alternatively, a refresh query should work
+        $q = $this->_em->createQuery('select a, u from Doctrine\Tests\Models\CMS\CmsAddress a join a.user u');
+        $q->setHint(Query::HINT_REFRESH, true);
+        $address3 = $q->getSingleResult();
+        
+        $this->assertSame($address, $address3); // should still be the same, always from identity map
+        
+        // Now the association should be "correct", referencing $user2
+        $this->assertSame($user2, $address2->user);
+        $this->assertSame($user2->address, $address2); // check back reference also
+        
+        // Attention! refreshes can result in broken bidirectional associations! this is currently expected!
+        // $user1 still points to $address2!
+        $this->assertSame($user1->address, $address2);
+    }
+    
+    public function testCollectionValuedAssociationIdentityMapBehavior()
+    {
+        $user = new CmsUser;
+        $user->status = 'dev';
+        $user->username = 'romanb';
+        $user->name = 'Roman B.';
+
+        $phone1 = new CmsPhonenumber;
+        $phone1->phonenumber = 123;
+        
+        $phone2 = new CmsPhonenumber;
+        $phone2->phonenumber = 234;
+        
+        $phone3 = new CmsPhonenumber;
+        $phone3->phonenumber = 345;
+        
+        $user->addPhonenumber($phone1);
+        $user->addPhonenumber($phone2);
+        $user->addPhonenumber($phone3);
+        
+        $this->_em->persist($user); // cascaded to phone numbers
+        $this->_em->flush();
+        
+        $this->assertEquals(3, count($user->getPhonenumbers()));
+        
+        //external update to CmsAddress
+        $this->_em->getConnection()->executeUpdate('insert into cms_phonenumbers (phonenumber, user_id) VALUES (?,?)', array(999, $user->getId()));
+        
+        //select
+        $q = $this->_em->createQuery('select u, p from Doctrine\Tests\Models\CMS\CmsUser u join u.phonenumbers p');
+        $user2 = $q->getSingleResult();
+        
+        $this->assertSame($user, $user2);
+        
+        // Should still be the same 3 phonenumbers
+        $this->assertEquals(3, count($user2->getPhonenumbers()));
+        
+        // But we want to have this external change!
+        // Solution 1: refresh().
+        //$this->_em->refresh($user2); broken atm!
+        // Solution 2: Alternatively, a refresh query should work
+        $q = $this->_em->createQuery('select u, p from Doctrine\Tests\Models\CMS\CmsUser u join u.phonenumbers p');
+        $q->setHint(Query::HINT_REFRESH, true);
+        $user3 = $q->getSingleResult();
+        
+        $this->assertSame($user, $user3); // should still be the same, always from identity map
+        
+        // Now the collection should be refreshed with correct count
+        $this->assertEquals(4, count($user3->getPhonenumbers()));
+    }
+}
+

tests/Doctrine/Tests/ORM/Functional/StandardEntityPersisterTest.php

@@ -88,7 +88,7 @@ class StandardEntityPersisterTest extends \Doctrine\Tests\OrmFunctionalTestCase
         // (change from ArrayCollection to PersistentCollection)
         $f3 = new ECommerceFeature();
         $f3->setDescription('XVID');
-        $p->addfeature($f3);
+        $p->addFeature($f3);
         
         // Now we persist the Feature #3
         $this->_em->persist($p);