Merge pull request #1281 from Ocramius/hotfix/#1169-extra-lazy-one-to…

…-many-should-not-delete-referenced-entities

Hotfix/#1169 extra lazy one to many should not delete referenced entities

lib/Doctrine/ORM/Cache/Persister/Collection/AbstractCollectionPersister.php

@@ -23,6 +23,7 @@
 use Doctrine\Common\Collections\Criteria;
 use Doctrine\ORM\Cache\EntityCacheKey;
 use Doctrine\ORM\Cache\CollectionCacheKey;
+use Doctrine\ORM\Cache\Persister\Entity\CachedEntityPersister;
 use Doctrine\ORM\Persisters\Collection\CollectionPersister;
 use Doctrine\ORM\PersistentCollection;
 use Doctrine\ORM\EntityManagerInterface;
@@ -162,13 +163,13 @@ public function loadCollectionCache(PersistentCollection $collection, Collection
      */
     public function storeCollectionCache(CollectionCacheKey $key, $elements)
     {
+        /* @var $targetPersister CachedEntityPersister */
         $targetPersister    = $this->uow->getEntityPersister($this->targetEntity->rootEntityName);
         $targetRegion       = $targetPersister->getCacheRegion();
         $targetHydrator     = $targetPersister->getEntityHydrator();
         $entry              = $this->hydrator->buildCacheEntry($this->targetEntity, $key, $elements);
 
         foreach ($entry->identifiers as $index => $entityKey) {
-
             if ($targetRegion->contains($entityKey)) {
                 continue;
             }
@@ -238,15 +239,13 @@ public function get(PersistentCollection $collection, $index)
      */
     public function removeElement(PersistentCollection $collection, $element)
     {
-        return $this->persister->removeElement($collection, $element);
-    }
+        if ($persisterResult = $this->persister->removeElement($collection, $element)) {
+            $this->evictCollectionCache($collection);
+            $this->evictElementCache($this->sourceEntity->rootEntityName, $collection->getOwner());
+            $this->evictElementCache($this->targetEntity->rootEntityName, $element);
+        }
 
-    /**
-     * {@inheritdoc}
-     */
-    public function removeKey(PersistentCollection $collection, $key)
-    {
-        return $this->persister->removeKey($collection, $key);
+        return $persisterResult;
     }
 
     /**
@@ -264,4 +263,42 @@ public function loadCriteria(PersistentCollection $collection, Criteria $criteri
     {
         return $this->persister->loadCriteria($collection, $criteria);
     }
+
+    /**
+     * Clears cache entries related to the current collection
+     *
+     * @param PersistentCollection $collection
+     */
+    protected function evictCollectionCache(PersistentCollection $collection)
+    {
+        $key = new CollectionCacheKey(
+            $this->sourceEntity->rootEntityName,
+            $this->association['fieldName'],
+            $this->uow->getEntityIdentifier($collection->getOwner())
+        );
+
+        $this->region->evict($key);
+
+        if ($this->cacheLogger) {
+            $this->cacheLogger->collectionCachePut($this->regionName, $key);
+        }
+    }
+
+    /**
+     * @param string $targetEntity
+     * @param object $element
+     */
+    protected function evictElementCache($targetEntity, $element)
+    {
+        /* @var $targetPersister CachedEntityPersister */
+        $targetPersister = $this->uow->getEntityPersister($targetEntity);
+        $targetRegion    = $targetPersister->getCacheRegion();
+        $key             = new EntityCacheKey($targetEntity, $this->uow->getEntityIdentifier($element));
+
+        $targetRegion->evict($key);
+
+        if ($this->cacheLogger) {
+            $this->cacheLogger->entityCachePut($targetRegion->getName(), $key);
+        }
+    }
 }

lib/Doctrine/ORM/Persisters/Collection/OneToManyPersister.php

@@ -20,6 +20,7 @@
 namespace Doctrine\ORM\Persisters\Collection;
 
 use Doctrine\Common\Collections\Criteria;
+use Doctrine\Common\Proxy\Proxy;
 use Doctrine\ORM\PersistentCollection;
 
 /**
@@ -163,7 +164,20 @@ public function removeElement(PersistentCollection $collection, $element)
         $mapping   = $collection->getMapping();
         $persister = $this->uow->getEntityPersister($mapping['targetEntity']);
 
-        return $persister->delete($element);
+        $targetMetadata = $this->em->getClassMetadata($mapping['targetEntity']);
+
+        if ($element instanceof Proxy && ! $element->__isInitialized()) {
+            $element->__load();
+        }
+
+        // clearing owning side value
+        $targetMetadata->reflFields[$mapping['mappedBy']]->setValue($element, null);
+
+        $this->uow->computeChangeSet($targetMetadata, $element);
+
+        $persister->update($element);
+
+        return true;
     }
 
     /**

tests/Doctrine/Tests/Models/Tweet/Tweet.php

@@ -8,6 +8,8 @@
  */
 class Tweet
 {
+    const CLASSNAME = __CLASS__;
+
     /**
      * @Id
      * @GeneratedValue

tests/Doctrine/Tests/Models/Tweet/User.php

@@ -10,6 +10,8 @@
  */
 class User
 {
+    const CLASSNAME = __CLASS__;
+
     /**
      * @Id
      * @GeneratedValue

tests/Doctrine/Tests/ORM/Cache/Persister/Collection/AbstractCollectionPersisterTest.php

@@ -240,22 +240,6 @@ public function testInvokeRemoveElement()
         $this->assertFalse($persister->removeElement($collection, $element));
     }
 
-    public function testInvokeRemoveKey()
-    {
-        $entity     = new State("Foo");
-        $persister  = $this->createPersisterDefault();
-        $collection = $this->createCollection($entity);
-
-        $this->em->getUnitOfWork()->registerManaged($entity, array('id'=>1), array('id'=>1, 'name'=>'Foo'));
-
-        $this->collectionPersister->expects($this->once())
-            ->method('removeKey')
-            ->with($this->equalTo($collection), $this->equalTo(0))
-            ->will($this->returnValue(false));
-
-        $this->assertFalse($persister->removeKey($collection, 0));
-    }
-
     public function testInvokeGet()
     {
         $entity     = new State("Foo");

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

@@ -5,13 +5,16 @@
 use Doctrine\ORM\Mapping\ClassMetadataInfo;
 use Doctrine\Tests\Models\DDC2504\DDC2504ChildClass;
 use Doctrine\Tests\Models\DDC2504\DDC2504OtherClass;
+use Doctrine\Tests\Models\Tweet\Tweet;
+use Doctrine\Tests\Models\Tweet\User;
+use Doctrine\Tests\OrmFunctionalTestCase;
 
 /**
  * Description of ExtraLazyCollectionTest
  *
  * @author beberlei
  */
-class ExtraLazyCollectionTest extends \Doctrine\Tests\OrmFunctionalTestCase
+class ExtraLazyCollectionTest extends OrmFunctionalTestCase
 {
     private $userId;
     private $userId2;
@@ -27,6 +30,7 @@ class ExtraLazyCollectionTest extends \Doctrine\Tests\OrmFunctionalTestCase
 
     public function setUp()
     {
+        $this->useModelSet('tweet');
         $this->useModelSet('cms');
         $this->useModelSet('ddc2504');
         parent::setUp();
@@ -480,7 +484,8 @@ public function testRemoveElementOneToMany()
         $user->articles->removeElement($article);
 
         $this->assertFalse($user->articles->isInitialized(), "Post-Condition: Collection is not initialized.");
-        $this->assertEquals($queryCount + 1, $this->getCurrentQueryCount());
+        // NOTE: +2 queries because CmsArticle is a versioned entity, and that needs to be handled accordingly
+        $this->assertEquals($queryCount + 2, $this->getCurrentQueryCount());
 
         // Test One to Many removal with Entity state as new
         $article = new \Doctrine\Tests\Models\CMS\CmsArticle();
@@ -501,7 +506,7 @@ public function testRemoveElementOneToMany()
 
         $user->articles->removeElement($article);
 
-        $this->assertEquals($queryCount + 1, $this->getCurrentQueryCount(), "Removing a persisted entity should cause one query to be executed.");
+        $this->assertEquals($queryCount, $this->getCurrentQueryCount(), "Removing a persisted entity will not cause queries when the owning side doesn't actually change.");
         $this->assertFalse($user->articles->isInitialized(), "Post-Condition: Collection is not initialized.");
 
         // Test One to Many removal with Entity state as managed
@@ -532,17 +537,10 @@ public function testRemovalOfManagedElementFromOneToManyJoinedInheritanceCollect
 
         $this->assertFalse($otherClass->childClasses->isInitialized(), 'Collection is not initialized.');
 
-        $expectedQueryCount = $queryCount + 1;
-
-        if (! $this->_em->getConnection()->getDatabasePlatform()->supportsForeignKeyConstraints()) {
-            // the ORM emulates cascades in a JTI if the underlying platform does not support them
-            $expectedQueryCount = $queryCount + 2;
-        };
-
         $this->assertEquals(
-            $expectedQueryCount,
+            $queryCount + 1,
             $this->getCurrentQueryCount(),
-            'One removal per table in the JTI has been executed'
+            'The owning side of the association is updated'
         );
 
         $this->assertFalse($otherClass->childClasses->contains($childClass));
@@ -601,17 +599,10 @@ public function testRemovalOfNewManagedElementFromOneToManyJoinedInheritanceColl
 
         $otherClass->childClasses->removeElement($childClass);
 
-        $expectedQueryCount = $queryCount + 1;
-
-        if (! $this->_em->getConnection()->getDatabasePlatform()->supportsForeignKeyConstraints()) {
-            // the ORM emulates cascades in a JTI if the underlying platform does not support them
-            $expectedQueryCount = $queryCount + 2;
-        };
-
         $this->assertEquals(
-            $expectedQueryCount,
+            $queryCount,
             $this->getCurrentQueryCount(),
-            'Removing a persisted entity should cause two queries to be executed.'
+            'No queries are executed, as the owning side of the association is not actually updated.'
         );
         $this->assertFalse($otherClass->childClasses->isInitialized(), 'Collection is not initialized.');
     }
@@ -1040,4 +1031,103 @@ private function loadFixture()
         $this->phonenumber = $phonenumber1->phonenumber;
 
     }
+
+    /**
+     * @group DDC-3343
+     */
+    public function testRemovesManagedElementFromOneToManyExtraLazyCollection()
+    {
+        list($userId, $tweetId) = $this->loadTweetFixture();
+
+        /* @var $user User */
+        $user = $this->_em->find(User::CLASSNAME, $userId);
+
+        $user->tweets->removeElement($this->_em->find(Tweet::CLASSNAME, $tweetId));
+
+        $this->_em->clear();
+
+        /* @var $user User */
+        $user = $this->_em->find(User::CLASSNAME, $userId);
+
+        $this->assertCount(0, $user->tweets);
+    }
+
+    /**
+     * @group DDC-3343
+     */
+    public function testRemovesManagedElementFromOneToManyExtraLazyCollectionWithoutDeletingTheTargetEntityEntry()
+    {
+        list($userId, $tweetId) = $this->loadTweetFixture();
+
+        /* @var $user User */
+        $user  = $this->_em->find(User::CLASSNAME, $userId);
+
+        $e = $this->_em->find(Tweet::CLASSNAME, $tweetId);
+
+        $user->tweets->removeElement($e);
+
+        $this->_em->clear();
+
+        /* @var $tweet Tweet */
+        $tweet = $this->_em->find(Tweet::CLASSNAME, $tweetId);
+        $this->assertInstanceOf(
+            Tweet::CLASSNAME,
+            $tweet,
+            'Even though the collection is extra lazy, the tweet should not have been deleted'
+        );
+
+        $this->assertNull($tweet->author, 'Tweet author link has been removed');
+    }
+
+    /**
+     * @group DDC-3343
+     */
+    public function testRemovingManagedLazyProxyFromExtraLazyOneToManyDoesRemoveTheAssociationButNotTheEntity()
+    {
+        list($userId, $tweetId) = $this->loadTweetFixture();
+
+        /* @var $user User */
+        $user  = $this->_em->find(User::CLASSNAME, $userId);
+        $tweet = $this->_em->getReference(Tweet::CLASSNAME, $tweetId);
+
+        $user->tweets->removeElement($this->_em->getReference(Tweet::CLASSNAME, $tweetId));
+
+        $this->_em->clear();
+
+        /* @var $tweet Tweet */
+        $tweet = $this->_em->find(Tweet::CLASSNAME, $tweet->id);
+        $this->assertInstanceOf(
+            Tweet::CLASSNAME,
+            $tweet,
+            'Even though the collection is extra lazy, the tweet should not have been deleted'
+        );
+
+        $this->assertNull($tweet->author);
+
+        /* @var $user User */
+        $user = $this->_em->find(User::CLASSNAME, $userId);
+
+        $this->assertCount(0, $user->tweets);
+    }
+
+    /**
+     * @return int[] ordered tuple: user id and tweet id
+     */
+    private function loadTweetFixture()
+    {
+        $user  = new User();
+        $tweet = new Tweet();
+
+        $user->name     = 'ocramius';
+        $tweet->content = 'The cat is on the table';
+
+        $user->addTweet($tweet);
+
+        $this->_em->persist($user);
+        $this->_em->persist($tweet);
+        $this->_em->flush();
+        $this->_em->clear();
+
+        return array($user->id, $tweet->id);
+    }
 }

tests/Doctrine/Tests/ORM/Functional/ValueConversionType/OneToManyExtraLazyTest.php

@@ -2,7 +2,6 @@
 
 namespace Doctrine\Tests\ORM\Functional\ValueConversionType;
 
-use Doctrine\DBAL\Types\Type as DBALType;
 use Doctrine\Tests\Models\ValueConversionType as Entity;
 use Doctrine\Tests\OrmFunctionalTestCase;