Improved entity state detection.

lib/Doctrine/ORM/Mapping/ClassMetadata.php

@@ -143,7 +143,11 @@ class ClassMetadata extends ClassMetadataInfo
             }
             return $id;
         } else {
-            return array($this->identifier[0] => $this->reflFields[$this->identifier[0]]->getValue($entity));
+            $value = $this->reflFields[$this->identifier[0]]->getValue($entity);
+            if ($value !== null) {
+                return array($this->identifier[0] => $value);
+            }
+            return array();
         }
     }
 

lib/Doctrine/ORM/PersistentCollection.php

@@ -32,12 +32,11 @@ use Doctrine\ORM\Mapping\AssociationMapping,
  * Similarly, if you remove entities from a collection that is part of a one-many
  * mapping this will only result in the nulling out of the foreign keys on flush.
  *
- * @license   http://www.opensource.org/licenses/lgpl-license.php LGPL
  * @since     2.0
- * @version   $Revision: 4930 $
  * @author    Konsta Vesterinen <kvesteri@cc.hut.fi>
  * @author    Roman Borschel <roman@code-factory.org>
  * @author    Giorgio Sironi <piccoloprincipeazzurro@gmail.com>
+ * @todo Design for inheritance to allow custom implementations?
  */
 final class PersistentCollection implements Collection
 {

lib/Doctrine/ORM/Persisters/BasicEntityPersister.php

@@ -57,9 +57,9 @@ use PDO,
  *
  *   - {@link load} : Loads (the state of) a single, managed entity.
  *   - {@link loadAll} : Loads multiple, managed entities.
- *   - {@link loadOneToOneEntity} : Loads a one/many-to-one association (lazy-loading).
+ *   - {@link loadOneToOneEntity} : Loads a one/many-to-one entity association (lazy-loading).
- *   - {@link loadOneToManyCollection} : Loads a one-to-many association (lazy-loading).
+ *   - {@link loadOneToManyCollection} : Loads a one-to-many entity association (lazy-loading).
- *   - {@link loadManyToManyCollection} : Loads a many-to-many association (lazy-loading).
+ *   - {@link loadManyToManyCollection} : Loads a many-to-many entity association (lazy-loading).
  *
  * The BasicEntityPersister implementation provides the default behavior for
  * persisting and querying entities that are mapped to a single database table.
@@ -712,7 +712,7 @@ class BasicEntityPersister
      * Creates or fills a single entity object from an SQL result.
      * 
      * @param $result The SQL result.
-     * @param object $entity The entity object to fill.
+     * @param object $entity The entity object to fill, if any.
      * @param array $hints Hints for entity creation.
      * @return object The filled and managed entity object or NULL, if the SQL result is empty.
      */
@@ -1119,6 +1119,22 @@ class BasicEntityPersister
         $stmt->closeCursor();
     }
 
+    /**
+     * Checks whether the given managed entity exists in the database.
+     *
+     * @param object $entity
+     * @return boolean TRUE if the entity exists in the database, FALSE otherwise.
+     */
+    public function exists($entity)
+    {
+        $criteria = $this->_class->getIdentifierValues($entity);
+        $sql = 'SELECT 1 FROM ' . $this->_class->getQuotedTableName($this->_platform)
+                . ' ' . $this->_getSQLTableAlias($this->_class->name)
+                . ' WHERE ' . $this->_getSelectConditionSQL($criteria);
+
+        return (bool) $this->_conn->fetchColumn($sql, array_values($criteria));
+    }
+
     //TODO
     /*protected function _getOneToOneEagerFetchSQL()
     {

lib/Doctrine/ORM/Query/SqlWalker.php

@@ -497,7 +497,6 @@ class SqlWalker implements TreeWalker
 
                     $sql .= reset($assoc->targetToSourceKeyColumns);
                 } else {
-                    // 2- Inverse side: NOT (YET?) SUPPORTED
                     throw QueryException::associationPathInverseSideNotSupported();
                 }
                 break;

lib/Doctrine/ORM/UnitOfWork.php

@@ -556,7 +556,7 @@ class UnitOfWork implements PropertyChangedListener
             return; // "Persistence by reachability" only if persist cascade specified
         }
         
-        // Look through the entities, and in any of their associations, for transient
+        // Look through the entities, and in any of their associations, for transient (new)
         // enities, recursively. ("Persistence by reachability")
         if ($assoc->isOneToOne()) {
             if ($value instanceof Proxy && ! $value->__isInitialized__) {
@@ -569,42 +569,45 @@ class UnitOfWork implements PropertyChangedListener
         
         $targetClass = $this->_em->getClassMetadata($assoc->targetEntityName);
         foreach ($value as $entry) {
-            $state = $this->getEntityState($entry, self::STATE_NEW);
+            $state = $this->getEntityState($entry);
             $oid = spl_object_hash($entry);
             if ($state == self::STATE_NEW) {
-                if (isset($targetClass->lifecycleCallbacks[Events::prePersist])) {
+                $this->persistNew($targetClass, $entry);
-                    $targetClass->invokeLifecycleCallbacks(Events::prePersist, $entry);
-                }
-                if ($this->_evm->hasListeners(Events::prePersist)) {
-                    $this->_evm->dispatchEvent(Events::prePersist, new LifecycleEventArgs($entry, $this->_em));
-                }
-                
-                // Get identifier, if possible (not post-insert)
-                $idGen = $targetClass->idGenerator;
-                if ( ! $idGen->isPostInsertGenerator()) {
-                    $idValue = $idGen->generate($this->_em, $entry);
-                    if ( ! $idGen instanceof \Doctrine\ORM\Id\AssignedGenerator) {
-                        $this->_entityIdentifiers[$oid] = array($targetClass->identifier[0] => $idValue);
-                        $targetClass->getSingleIdReflectionProperty()->setValue($entry, $idValue);
-                    } else {
-                        $this->_entityIdentifiers[$oid] = $idValue;
-                    }
-                    $this->addToIdentityMap($entry);
-                }
-                $this->_entityStates[$oid] = self::STATE_MANAGED;
-
-                // NEW entities are INSERTed within the current unit of work.
-                $this->_entityInsertions[$oid] = $entry;
-
                 $this->computeChangeSet($targetClass, $entry);
-                
             } else if ($state == self::STATE_REMOVED) {
-                throw ORMException::removedEntityInCollectionDetected($entity, $assoc);
+                throw ORMException::removedEntityInCollectionDetected($entry, $assoc);
+            } else if ($state == self::STATE_DETACHED) {
+                throw new \InvalidArgumentException("Detached entity in association during cascading a persist operation.");
             }
             // MANAGED associated entities are already taken into account
             // during changeset calculation anyway, since they are in the identity map.
         }
     }
+
+    private function persistNew($class, $entity)
+    {
+        $oid = spl_object_hash($entity);
+        if (isset($class->lifecycleCallbacks[Events::prePersist])) {
+            $class->invokeLifecycleCallbacks(Events::prePersist, $entity);
+        }
+        if ($this->_evm->hasListeners(Events::prePersist)) {
+            $this->_evm->dispatchEvent(Events::prePersist, new LifecycleEventArgs($entity, $this->_em));
+        }
+
+        $idGen = $class->idGenerator;
+        if ( ! $idGen->isPostInsertGenerator()) {
+            $idValue = $idGen->generate($this->_em, $entity);
+            if ( ! $idGen instanceof \Doctrine\ORM\Id\AssignedGenerator) {
+                $this->_entityIdentifiers[$oid] = array($class->identifier[0] => $idValue);
+                $class->setIdentifierValues($entity, $idValue);
+            } else {
+                $this->_entityIdentifiers[$oid] = $idValue;
+            }
+        }
+        $this->_entityStates[$oid] = self::STATE_MANAGED;
+
+        $this->scheduleForInsert($entity);
+    }
     
     /**
      * INTERNAL:
@@ -1031,35 +1034,54 @@ class UnitOfWork implements PropertyChangedListener
     }
 
     /**
-     * Gets the state of an entity within the current unit of work.
+     * Gets the state of an entity with regard to the current unit of work.
-     * 
-     * NOTE: This method sees entities that are not MANAGED or REMOVED and have a
-     *       populated identifier, whether it is generated or manually assigned, as
-     *       DETACHED. This can be incorrect for manually assigned identifiers.
      *
      * @param object $entity
-     * @param integer $assume The state to assume if the state is not yet known. This is usually
+     * @param integer $assume The state to assume if the state is not yet known (not MANAGED or REMOVED).
-     *                        used to avoid costly state lookups, in the worst case with a database
+     *                        This parameter can be set to improve performance of entity state detection
-     *                        lookup.
+     *                        by potentially avoiding a database lookup if the distinction between NEW and DETACHED
+     *                        is either known or does not matter for the caller of the method.
      * @return int The entity state.
      */
     public function getEntityState($entity, $assume = null)
     {
         $oid = spl_object_hash($entity);
         if ( ! isset($this->_entityStates[$oid])) {
-            // State can only be NEW or DETACHED, because MANAGED/REMOVED states are immediately
+            // State can only be NEW or DETACHED, because MANAGED/REMOVED states are known.
-            // set by the UnitOfWork directly. We treat all entities that have a populated
+            // Note that you can not remember the NEW or DETACHED state in _entityStates since
-            // identifier as DETACHED and all others as NEW. This is not really correct for
+            // the UoW does not hold references to such objects and the object hash can be reused.
-            // manually assigned identifiers but in that case we would need to hit the database
+            // More generally because the state may "change" between NEW/DETACHED without the UoW being aware of it.
-            // and we would like to avoid that.
             if ($assume === null) {
-                if ($this->_em->getClassMetadata(get_class($entity))->getIdentifierValues($entity)) {
+                $class = $this->_em->getClassMetadata(get_class($entity));
-                    $this->_entityStates[$oid] = self::STATE_DETACHED;
+                $id = $class->getIdentifierValues($entity);
+                if ( ! $id) {
+                    return self::STATE_NEW;
+                } else if ($class->isIdentifierNatural()) {
+                    // Check for a version field, if available, to avoid a db lookup.
+                    if ($class->isVersioned) {
+                        if ($class->getFieldValue($entity, $class->versionField)) {
+                            return self::STATE_DETACHED;
+                        } else {
+                            return self::STATE_NEW;
+                        }
+                    } else {
+                        // Last try before db lookup: check the identity map.
+                        if ($this->tryGetById($id, $class->rootEntityName)) {
+                            return self::STATE_DETACHED;
+                        } else {
+                            // db lookup
+                            if ($this->getEntityPersister(get_class($entity))->exists($entity)) {
+                                return self::STATE_DETACHED;
+                            } else {
+                                return self::STATE_NEW;
+                            }
+                        }
+                    }
                 } else {
-                    $this->_entityStates[$oid] = self::STATE_NEW;
+                    return self::STATE_DETACHED;
                 }
             } else {
-                $this->_entityStates[$oid] = $assume;
+                return $assume;
             }
         }
         return $this->_entityStates[$oid];
@@ -1169,12 +1191,10 @@ class UnitOfWork implements PropertyChangedListener
     }
 
     /**
-     * Saves an entity as part of the current unit of work.
+     * Persists an entity as part of the current unit of work.
-     * This method is internally called during save() cascades as it tracks
-     * the already visited entities to prevent infinite recursions.
      * 
-     * NOTE: This method always considers entities that are not yet known to
+     * This method is internally called during persist() cascades as it tracks
-     * this UnitOfWork as NEW.
+     * the already visited entities to prevent infinite recursions.
      *
      * @param object $entity The entity to persist.
      * @param array $visited The already visited entities.
@@ -1189,8 +1209,8 @@ class UnitOfWork implements PropertyChangedListener
         $visited[$oid] = $entity; // Mark visited
 
         $class = $this->_em->getClassMetadata(get_class($entity));
-        $entityState = $this->getEntityState($entity, self::STATE_NEW);
+        $entityState = $this->getEntityState($entity);
-        
+
         switch ($entityState) {
             case self::STATE_MANAGED:
                 // Nothing to do, except if policy is "deferred explicit"
@@ -1199,30 +1219,10 @@ class UnitOfWork implements PropertyChangedListener
                 }
                 break;
             case self::STATE_NEW:
-                if (isset($class->lifecycleCallbacks[Events::prePersist])) {
+                $this->persistNew($class, $entity);
-                    $class->invokeLifecycleCallbacks(Events::prePersist, $entity);
-                }
-                if ($this->_evm->hasListeners(Events::prePersist)) {
-                    $this->_evm->dispatchEvent(Events::prePersist, new LifecycleEventArgs($entity, $this->_em));
-                }
-                
-                $idGen = $class->idGenerator;
-                if ( ! $idGen->isPostInsertGenerator()) {
-                    $idValue = $idGen->generate($this->_em, $entity);
-                    if ( ! $idGen instanceof \Doctrine\ORM\Id\AssignedGenerator) {
-                        $this->_entityIdentifiers[$oid] = array($class->identifier[0] => $idValue);
-                        $class->setIdentifierValues($entity, $idValue);
-                    } else {
-                        $this->_entityIdentifiers[$oid] = $idValue;
-                    }
-                }
-                $this->_entityStates[$oid] = self::STATE_MANAGED;
-                
-                $this->scheduleForInsert($entity);
                 break;
             case self::STATE_DETACHED:
-                throw new \InvalidArgumentException(
+                throw new \InvalidArgumentException("Detached entity passed to persist().");
-                        "Behavior of persist() for a detached entity is not yet defined.");
             case self::STATE_REMOVED:
                 // Entity becomes managed again
                 if ($this->isScheduledForDelete($entity)) {
@@ -1235,7 +1235,7 @@ class UnitOfWork implements PropertyChangedListener
             default:
                 throw ORMException::invalidEntityState($entityState);
         }
-        
+
         $this->_cascadePersist($entity, $visited);
     }
 
@@ -1440,7 +1440,6 @@ class UnitOfWork implements PropertyChangedListener
      * 
      * @param object $entity
      * @param array $visited
-     * @internal This method always considers entities with an assigned identifier as DETACHED.
      */
     private function _doDetach($entity, array &$visited)
     {
@@ -1657,7 +1656,7 @@ class UnitOfWork implements PropertyChangedListener
      */
     public function lock($entity, $lockMode, $lockVersion = null)
     {
-        if ($this->getEntityState($entity) != self::STATE_MANAGED) {
+        if ($this->getEntityState($entity, self::STATE_NEW) != self::STATE_MANAGED) {
             throw new \InvalidArgumentException("Entity is not MANAGED.");
         }
         
@@ -2183,4 +2182,47 @@ class UnitOfWork implements PropertyChangedListener
     {
         return $this->_collectionUpdates;
     }
+
+    /**
+     * Specifically tells this UnitOfWork that the given entity should be treated
+     * as NEW. This can be useful in the case of manually assigned identifiers to
+     * avoid a database lookup that is used to tell NEW and DETACHED entities apart.
+     * Use setNew($entity) prior to persist($entity) to avoid the database lookup.
+     *
+     * Note that the UnitOfWork uses the spl_object_hash() of the object to associate the
+     * state with the object. Thus the UnitOfWork does not prevent the object from being
+     * garbage collected which can result in the object hash being reused for other objects.
+     *
+     * @param object $entity The entity to mark as NEW.
+     * @throws InvalidArgumentException If the entity is already known to this UnitOfWork.
+     */
+    public function setNew($entity)
+    {
+        $oid = spl_object_hash($entity);
+        if (isset($this->_entityStates[$oid])) {
+            throw new \InvalidArgumentException("The passed entity must not be already known to this UnitOfWork.");
+        }
+        $this->_entityStates[$oid] = self::STATE_NEW;
+    }
+
+    /**
+     * Specifically tells this UnitOfWork that the given entity should be treated
+     * as DETACHED. This can be useful in the case of manually assigned identifiers to
+     * avoid a database lookup that is used to tell NEW and DETACHED entities apart.
+     *
+     * Note that the UnitOfWork uses the spl_object_hash() of the object to associate the
+     * state with the object. Thus the UnitOfWork does not prevent the object from being
+     * garbage collected which can result in the object hash being reused for other objects.
+     *
+     * @param object $entity The entity to mark as DETACHED.
+     * @throws InvalidArgumentException If the entity is already known to this UnitOfWork.
+     */
+    public function setDetached($entity)
+    {
+        $oid = spl_object_hash($entity);
+        if (isset($this->_entityStates[$oid])) {
+            throw new \InvalidArgumentException("The passed entity must not be already known to this UnitOfWork.");
+        }
+        $this->_entityStates[$oid] = self::STATE_DETACHED;
+    }
 }

tests/Doctrine/Tests/Mocks/EntityPersisterMock.php

@@ -13,6 +13,7 @@ class EntityPersisterMock extends \Doctrine\ORM\Persisters\BasicEntityPersister
     private $_identityColumnValueCounter = 0;
     private $_mockIdGeneratorType;
     private $_postInsertIds = array();
+    private $existsCalled = false;
 
     /**
      * @param <type> $entity
@@ -57,6 +58,11 @@ class EntityPersisterMock extends \Doctrine\ORM\Persisters\BasicEntityPersister
     {
         $this->_updates[] = $entity;
     }
+
+    public function exists($entity)
+    {
+        $this->existsCalled = true;
+    }
     
     public function delete($entity)
     {
@@ -80,9 +86,15 @@ class EntityPersisterMock extends \Doctrine\ORM\Persisters\BasicEntityPersister
     
     public function reset()
     {
+        $this->existsCalled = false;
         $this->_identityColumnValueCounter = 0;
         $this->_inserts = array();
         $this->_updates = array();
         $this->_deletes = array();
     }
+
+    public function isExistsCalled()
+    {
+        return $this->existsCalled;
+    }
 }

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

@@ -86,6 +86,20 @@ class DetachedEntityTest extends \Doctrine\Tests\OrmFunctionalTestCase
         $this->assertTrue($this->_em->contains($phonenumbers[1]));
     }
 
+    /**
+     * @expectedException InvalidArgumentException
+     * @group DDC-203
+     */
+    public function testDetachedEntityWithAssignedIdentityThrowsExceptionOnPersist()
+    {
+        $ph = new CmsPhonenumber();
+        $ph->phonenumber = '12345';
+        $this->_em->persist($ph);
+        $this->_em->flush();
+        $this->_em->clear();
+        $this->_em->persist($ph);
+    }
+
     /**
      * @group DDC-518
      */

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

@@ -185,7 +185,9 @@ class ManyToManyBasicAssociationTest extends \Doctrine\Tests\OrmFunctionalTestCa
 
         /* @var $freshUser CmsUser */
         $freshUser = $this->_em->find('Doctrine\Tests\Models\CMS\CmsUser', $user->getId());
-        $freshUser->addGroup($group);
+        $newGroup = new CmsGroup();
+        $newGroup->setName('12Monkeys');
+        $freshUser->addGroup($newGroup);
 
         $this->assertFalse($freshUser->groups->isInitialized(), "CmsUser::groups Collection has to be uninitialized for this test.");
 

tests/Doctrine/Tests/ORM/UnitOfWorkTest.php

@@ -2,6 +2,7 @@
 
 namespace Doctrine\Tests\ORM;
 
+use Doctrine\ORM\UnitOfWork;
 use Doctrine\Tests\Mocks\ConnectionMock;
 use Doctrine\Tests\Mocks\EntityManagerMock;
 use Doctrine\Tests\Mocks\UnitOfWorkMock;
@@ -147,47 +148,46 @@ class UnitOfWorkTest extends \Doctrine\Tests\OrmTestCase
         $this->assertEquals(array('data' => array('thedata', 'newdata')), $this->_unitOfWork->getEntityChangeSet($entity));
     }
 
-    /*
+    public function testGetEntityStateOnVersionedEntityWithAssignedIdentifier()
-    public function testSavingSingleEntityWithSequenceIdGeneratorSchedulesInsert()
     {
-        //...
+        $persister = new EntityPersisterMock($this->_emMock, $this->_emMock->getClassMetadata("Doctrine\Tests\ORM\VersionedAssignedIdentifierEntity"));
+        $this->_unitOfWork->setEntityPersister('Doctrine\Tests\ORM\VersionedAssignedIdentifierEntity', $persister);
+
+        $e = new VersionedAssignedIdentifierEntity();
+        $e->id = 42;
+        $this->assertEquals(UnitOfWork::STATE_NEW, $this->_unitOfWork->getEntityState($e));
+        $this->assertFalse($persister->isExistsCalled());
     }
-    
+
-    public function testSavingSingleEntityWithTableIdGeneratorSchedulesInsert()
+    public function testGetEntityStateWithAssignedIdentity()
     {
-        //...
+        $persister = new EntityPersisterMock($this->_emMock, $this->_emMock->getClassMetadata("Doctrine\Tests\Models\CMS\CmsPhonenumber"));
+        $this->_unitOfWork->setEntityPersister('Doctrine\Tests\Models\CMS\CmsPhonenumber', $persister);
+
+        $ph = new \Doctrine\Tests\Models\CMS\CmsPhonenumber();
+        $ph->phonenumber = '12345';
+
+        $this->assertEquals(UnitOfWork::STATE_NEW, $this->_unitOfWork->getEntityState($ph));
+        $this->assertTrue($persister->isExistsCalled());
+
+        $persister->reset();
+
+        // setNew should avoid exists() check
+        $this->_unitOfWork->setNew($ph);
+        $this->assertEquals(UnitOfWork::STATE_NEW, $this->_unitOfWork->getEntityState($ph));
+        $this->assertFalse($persister->isExistsCalled());
+
+        $persister->reset();
+
+        // if the entity is already managed the exists() check should also be skipped
+        $this->_unitOfWork->registerManaged($ph, array('phonenumber' => '12345'), array());
+        $this->assertEquals(UnitOfWork::STATE_MANAGED, $this->_unitOfWork->getEntityState($ph));
+        $this->assertFalse($persister->isExistsCalled());
+        $ph2 = new \Doctrine\Tests\Models\CMS\CmsPhonenumber();
+        $ph2->phonenumber = '12345';
+        $this->assertEquals(UnitOfWork::STATE_DETACHED, $this->_unitOfWork->getEntityState($ph2));
+        $this->assertFalse($persister->isExistsCalled());
     }
-    
-    public function testSavingSingleEntityWithSingleNaturalIdForcesInsert()
-    {
-        //...
-    }
-    
-    public function testSavingSingleEntityWithCompositeIdForcesInsert()
-    {
-        //...
-    }
-    
-    public function testSavingEntityGraphWithIdentityColumnsForcesInserts()
-    {
-        //...
-    }
-    
-    public function testSavingEntityGraphWithSequencesDelaysInserts()
-    {
-        //...
-    }
-    
-    public function testSavingEntityGraphWithNaturalIdsForcesInserts()
-    {
-        //...
-    }
-    
-    public function testSavingEntityGraphWithMixedIdGenerationStrategies()
-    {
-        //...
-    }
-    */
 }
 
 /**
@@ -243,4 +243,17 @@ class NotifyChangedEntity implements \Doctrine\Common\NotifyPropertyChanged
             }
         }
     }
+}
+
+/** @Entity */
+class VersionedAssignedIdentifierEntity
+{
+    /**
+     * @Id @Column(type="integer")
+     */
+    public $id;
+    /**
+     * @Version @Column(type="integer")
+     */
+    public $version;
 }