[2.0] Fixed issue with self-referential one-to-many associations not being...

[2.0] Fixed issue with self-referential one-to-many associations not being persisted correctly when IDENTITY key generation was used. Included now passing OneToManySelfReferentialTest.

lib/Doctrine/ORM/Persisters/StandardEntityPersister.php

@@ -179,8 +179,10 @@ class StandardEntityPersister
     {
         $updateData = array();
         $this->_prepareData($entity, $updateData);
-        $id = array_combine($this->_class->getIdentifierFieldNames(),
-        $this->_em->getUnitOfWork()->getEntityIdentifier($entity));
+        $id = array_combine(
+            $this->_class->getIdentifierFieldNames(),
+            $this->_em->getUnitOfWork()->getEntityIdentifier($entity)
+        );
         $tableName = $this->_class->primaryTable['name'];
 
         if ($this->_evm->hasListeners(Events::preUpdate)) {
@@ -239,9 +241,10 @@ class StandardEntityPersister
     }
 
     /**
-     * Prepares the data changeset of an entity for database insertion.
-     * The array that is passed as the second parameter is filled with
-     * <columnName> => <value> pairs, grouped by table name, during this preparation.
+     * Prepares the data changeset of an entity for database insertion (INSERT/UPDATE).
+     * 
+     * During this preparation the array that is passed as the second parameter is filled with
+     * <columnName> => <value> pairs, grouped by table name.
      *
      * Example:
      * <code>
@@ -256,7 +259,7 @@ class StandardEntityPersister
      *
      * @param object $entity
      * @param array $result The reference to the data array.
-     * @param boolean $isInsert
+     * @param boolean $isInsert Whether the preparation is for an INSERT (or UPDATE, if FALSE).
      */
     protected function _prepareData($entity, array &$result, $isInsert = false)
     {
@@ -276,30 +279,36 @@ class StandardEntityPersister
                     continue;
                 }
 
-                // Special case: One-one self-referencing of the same class.
-                if ($newVal !== null && $assocMapping->sourceEntityName == $assocMapping->targetEntityName) {
+                // Special case: One-one self-referencing of the same class with IDENTITY type key generation.
+                if ($this->_class->isIdGeneratorIdentity() && $newVal !== null &&
+                        $assocMapping->sourceEntityName == $assocMapping->targetEntityName) {
                     $oid = spl_object_hash($newVal);
                     $isScheduledForInsert = $uow->isRegisteredNew($newVal);
                     if (isset($this->_queuedInserts[$oid]) || $isScheduledForInsert) {
                         // The associated entity $newVal is not yet persisted, so we must
-                        // set $newVal = null, in order to insert a null value and update later.
+                        // set $newVal = null, in order to insert a null value and schedule an
+                        // extra update on the UnitOfWork.
+                        $uow->scheduleExtraUpdate($entity, array(
+                            $field => array(null, $newVal)
+                        ));
                         $newVal = null;
                     } else if ($isInsert && ! $isScheduledForInsert && $uow->getEntityState($newVal) == UnitOfWork::STATE_MANAGED) {
-                        // $newVal is already fully persisted
-                        // Clear changeset of $newVal, so that only the identifier is updated.
-                        // Not sure this is really rock-solid here but it seems to work.
-                        $uow->clearEntityChangeSet($oid);
-                        $uow->propertyChanged($newVal, $field, $entity, $entity);
+                        // $newVal is already fully persisted.
+                        // Schedule an extra update for it, so that the foreign key(s) are properly set.
+                        $uow->scheduleExtraUpdate($newVal, array(
+                            $field => array(null, $entity)
+                        ));
                     }
                 }
                 
                 foreach ($assocMapping->sourceToTargetKeyColumns as $sourceColumn => $targetColumn) {
-                    $otherClass = $this->_em->getClassMetadata($assocMapping->targetEntityName);
                     if ($newVal === null) {
                         $result[$this->getOwningTable($field)][$sourceColumn] = null;
                     } else {
+                        $otherClass = $this->_em->getClassMetadata($assocMapping->targetEntityName);
                         $result[$this->getOwningTable($field)][$sourceColumn] =
-                        $otherClass->reflFields[$otherClass->fieldNames[$targetColumn]]->getValue($newVal);
+                                $otherClass->reflFields[$otherClass->fieldNames[$targetColumn]]
+                                ->getValue($newVal);
                     }
                 }
             } else if ($newVal === null) {

lib/Doctrine/ORM/UnitOfWork.php

@@ -136,6 +136,8 @@ class UnitOfWork implements PropertyChangedListener
      */
     private $_entityUpdates = array();
 
+    private $_extraUpdates = array();
+
     /**
      * A list of all pending entity deletions.
      *
@@ -245,6 +247,10 @@ class UnitOfWork implements PropertyChangedListener
             foreach ($commitOrder as $class) {
                 $this->_executeUpdates($class);
             }
+            // Extra updates that were requested by persisters.
+            if ($this->_extraUpdates) {
+                $this->_executeExtraUpdates();
+            }
 
             // Collection deletions (deletions of complete collections)
             foreach ($this->_collectionDeletions as $collectionToDelete) {
@@ -278,12 +284,22 @@ class UnitOfWork implements PropertyChangedListener
         $this->_entityInsertions = array();
         $this->_entityUpdates = array();
         $this->_entityDeletions = array();
+        $this->_extraUpdates = array();
         $this->_entityChangeSets = array();
         $this->_collectionUpdates = array();
         $this->_collectionDeletions = array();
         $this->_visitedCollections = array();
     }
 
+    protected function _executeExtraUpdates()
+    {
+        foreach ($this->_extraUpdates as $oid => $update) {
+            list ($entity, $changeset) = $update;
+            $this->_entityChangeSets[$oid] = $changeset;
+            $this->getEntityPersister(get_class($entity))->update($entity);
+        }
+    }
+
     /**
      * Gets the changeset for an entity.
      *
@@ -399,10 +415,8 @@ class UnitOfWork implements PropertyChangedListener
                 $actualData[$name] = $refProp->getValue($entity);
             }
 
-            if ($class->isCollectionValuedAssociation($name)
-                    && $actualData[$name] !== null
-                    && ! ($actualData[$name] instanceof PersistentCollection)
-                ) {
+            if ($class->isCollectionValuedAssociation($name) && $actualData[$name] !== null
+                    && ! ($actualData[$name] instanceof PersistentCollection)) {
                 // If $actualData[$name] is Collection then unwrap the array
                 if ($actualData[$name] instanceof Collection) {
                     $actualData[$name] = $actualData[$name]->unwrap();
@@ -609,7 +623,8 @@ class UnitOfWork implements PropertyChangedListener
             $entityChangeSet = array_merge(
                     $this->_entityInsertions,
                     $this->_entityUpdates,
-                    $this->_entityDeletions);
+                    $this->_entityDeletions
+                    );
         }
 
         // TODO: We can cache computed commit orders in the metadata cache!
@@ -715,6 +730,11 @@ class UnitOfWork implements PropertyChangedListener
         }
     }
 
+    public function scheduleExtraUpdate($entity, array $changeset)
+    {
+        $this->_extraUpdates[spl_object_hash($entity)] = array($entity, $changeset);
+    }
+
     /**
      * Checks whether an entity is registered as dirty in the unit of work.
      * Note: Is not very useful currently as dirty entities are only registered
@@ -919,9 +939,7 @@ class UnitOfWork implements PropertyChangedListener
             return false;
         }
 
-        return isset($this->_identityMap
-                [$classMetadata->rootEntityName]
-                [$idHash]);
+        return isset($this->_identityMap[$classMetadata->rootEntityName][$idHash]);
     }
 
     /**
@@ -954,6 +972,11 @@ class UnitOfWork implements PropertyChangedListener
             foreach ($commitOrder as $class) {
                 $this->_executeInserts($class);
             }
+            // Extra updates that were requested by persisters.
+            if ($this->_extraUpdates) {
+                $this->_executeExtraUpdates();
+                $this->_extraUpdates = array();
+            }
             // remove them from _entityInsertions and _entityChangeSets
             $this->_entityInsertions = array_diff_key($this->_entityInsertions, $insertNow);
             $this->_entityChangeSets = array_diff_key($this->_entityChangeSets, $insertNow);
@@ -1092,7 +1115,7 @@ class UnitOfWork implements PropertyChangedListener
         $id = $class->getIdentifierValues($entity);
 
         if ( ! $id) {
-            throw new InvalidArgumentException('New entity passed to merge().');
+            throw new \InvalidArgumentException('New entity passed to merge().');
         }
 
         $managedCopy = $this->tryGetById($id, $class->rootEntityName);

tests/Doctrine/Tests/Models/ECommerce/ECommerceCart.php

@@ -2,6 +2,8 @@
 
 namespace Doctrine\Tests\Models\ECommerce;
 
+use Doctrine\Common\Collections\Collection;
+
 /**
  * ECommerceCart
  * Represents a typical cart of a shopping application.
@@ -40,7 +42,7 @@ class ECommerceCart
 
     public function __construct()
     {
-        $this->products = new \Doctrine\Common\Collections\Collection;
+        $this->products = new Collection;
     }
     
     public function getId() {

tests/Doctrine/Tests/Models/ECommerce/ECommerceCategory.php

@@ -2,6 +2,8 @@
 
 namespace Doctrine\Tests\Models\ECommerce;
 
+use Doctrine\Common\Collections\Collection;
+
 /**
  * ECommerceCategory
  * Represents a tag applied on particular products.
@@ -42,8 +44,8 @@ class ECommerceCategory
 
     public function __construct()
     {
-        $this->products = new \Doctrine\Common\Collections\Collection();
-        $this->children = new \Doctrine\Common\Collections\Collection();
+        $this->products = new Collection();
+        $this->children = new Collection();
     }
 
     public function getId()

tests/Doctrine/Tests/Models/ECommerce/ECommerceProduct.php

@@ -2,6 +2,8 @@
 
 namespace Doctrine\Tests\Models\ECommerce;
 
+use Doctrine\Common\Collections\Collection;
+
 /**
  * ECommerceProduct
  * Represents a type of product of a shopping application.
@@ -45,8 +47,8 @@ class ECommerceProduct
 
     public function __construct()
     {
-        $this->features = new \Doctrine\Common\Collections\Collection;
-        $this->categories = new \Doctrine\Common\Collections\Collection;
+        $this->features = new Collection;
+        $this->categories = new Collection;
     }
 
     public function getId()

tests/Doctrine/Tests/Models/ECommerce/ECommerceShipping.php

@@ -13,8 +13,7 @@ namespace Doctrine\Tests\Models\ECommerce;
 class ECommerceShipping
 {
     /**
-     * @Column(type="integer")
-     * @Id
+     * @Id @Column(type="integer")
      * @GeneratedValue(strategy="AUTO")
      */
     private $id;

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

@@ -38,7 +38,9 @@ class AbstractManyToManyAssociationTestCase extends \Doctrine\Tests\OrmFunctiona
 
     public function assertCollectionEquals(Collection $first, Collection $second)
     {
-        if (count($first) != count($second)) {
+        return $first->forAll(function($k, $e) use($second) { return $second->contains($e); });
+        
+        /*if (count($first) != count($second)) {
             return false;
         }
         foreach ($first as $element) {
@@ -46,6 +48,6 @@ class AbstractManyToManyAssociationTestCase extends \Doctrine\Tests\OrmFunctiona
                 return false;
             }
         }
-        return true;
+        return true;*/
     }
 }

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

@@ -31,6 +31,7 @@ class AllTests
         $suite->addTestSuite('Doctrine\Tests\ORM\Functional\OneToManyBidirectionalAssociationTest');
         $suite->addTestSuite('Doctrine\Tests\ORM\Functional\ManyToManyUnidirectionalAssociationTest');
         $suite->addTestSuite('Doctrine\Tests\ORM\Functional\ManyToManyBidirectionalAssociationTest');
+        $suite->addTestSuite('Doctrine\Tests\ORM\Functional\OneToManySelfReferentialAssociationTest');
 
         return $suite;
     }

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

@@ -32,6 +32,8 @@ class OneToManySelfReferentialAssociationTest extends \Doctrine\Tests\OrmFunctio
         $this->parent->addChild($this->secondChild);
         $this->_em->save($this->parent);
         
+        $this->_em->flush();
+        
         $this->assertForeignKeyIs($this->parent->getId(), $this->firstChild);
         $this->assertForeignKeyIs($this->parent->getId(), $this->secondChild);
     }
@@ -39,6 +41,7 @@ class OneToManySelfReferentialAssociationTest extends \Doctrine\Tests\OrmFunctio
     public function testSavesAnEmptyCollection()
     {
         $this->_em->save($this->parent);
+        $this->_em->flush();
 
         $this->assertEquals(0, count($this->parent->getChildren()));
     }
@@ -46,6 +49,7 @@ class OneToManySelfReferentialAssociationTest extends \Doctrine\Tests\OrmFunctio
     public function testDoesNotSaveAnInverseSideSet() {
         $this->parent->brokenAddChild($this->firstChild);
         $this->_em->save($this->parent);
+        $this->_em->flush();
         
         $this->assertForeignKeyIs(null, $this->firstChild);
     }
@@ -80,10 +84,10 @@ class OneToManySelfReferentialAssociationTest extends \Doctrine\Tests\OrmFunctio
         
         $this->assertTrue($children[0] instanceof ECommerceCategory);
         $this->assertSame($parent, $children[0]->getParent());
-        $this->assertTrue(strstr($children[0]->getName(), ' books'));
+        $this->assertEquals(' books', strstr($children[0]->getName(), ' books'));
         $this->assertTrue($children[1] instanceof ECommerceCategory);
         $this->assertSame($parent, $children[1]->getParent());
-        $this->assertTrue(strstr($children[1]->getName(), ' books'));
+        $this->assertEquals(' books', strstr($children[1]->getName(), ' books'));
     }
     
     /* TODO: not yet implemented