Merge pull request doctrine#1288 from Ocramius/hotfix/doctrine#1169-e…

…xtra-lazy-one-to-many-is-noop-when-not-doing-orphan-removal Hotfix - doctrine#1169 - extra lazy one to many must be no-op when not doing orphan removal
FabioBatSilva · Jan 28, 2015 · 3ed73b4 · 3ed73b4
2 parents ffaffa0 + e76b20b
commit 3ed73b4
Show file tree

Hide file tree

Showing 8 changed files with 206 additions and 38 deletions.
diff --git a/lib/Doctrine/ORM/Cache/Persister/Entity/NonStrictReadWriteCachedEntityPersister.php b/lib/Doctrine/ORM/Cache/Persister/Entity/NonStrictReadWriteCachedEntityPersister.php
@@ -78,9 +78,13 @@ public function afterTransactionRolledBack()
      */
     public function delete($entity)
     {
-        $this->persister->delete($entity);
+        $key = new EntityCacheKey($this->class->rootEntityName, $this->uow->getEntityIdentifier($entity));
 
-        $this->queuedCache['delete'][] = new EntityCacheKey($this->class->rootEntityName, $this->uow->getEntityIdentifier($entity));
+        if ($this->persister->delete($entity)) {
+            $this->region->evict($key);
+        }
+
+        $this->queuedCache['delete'][] = $key;
     }
 
     /**

diff --git a/lib/Doctrine/ORM/Cache/Persister/Entity/ReadWriteCachedEntityPersister.php b/lib/Doctrine/ORM/Cache/Persister/Entity/ReadWriteCachedEntityPersister.php
@@ -103,7 +103,9 @@ public function delete($entity)
         $key   = new EntityCacheKey($this->class->rootEntityName, $this->uow->getEntityIdentifier($entity));
         $lock  = $this->region->lock($key);
 
-        $this->persister->delete($entity);
+        if ($this->persister->delete($entity)) {
+            $this->region->evict($key);
+        }
 
         if ($lock === null) {
             return;

diff --git a/lib/Doctrine/ORM/Persisters/Collection/OneToManyPersister.php b/lib/Doctrine/ORM/Persisters/Collection/OneToManyPersister.php
@@ -157,27 +157,21 @@ public function contains(PersistentCollection $collection, $element)
      */
     public function removeElement(PersistentCollection $collection, $element)
     {
-        if ( ! $this->isValidEntityState($element)) {
+        $mapping = $collection->getMapping();
+
+        if ( ! $mapping['orphanRemoval']) {
+            // no-op: this is not the owning side, therefore no operations should be applied
             return false;
         }
 
-        $mapping   = $collection->getMapping();
-        $persister = $this->uow->getEntityPersister($mapping['targetEntity']);
-
-        $targetMetadata = $this->em->getClassMetadata($mapping['targetEntity']);
-
-        if ($element instanceof Proxy && ! $element->__isInitialized()) {
-            $element->__load();
+        if ( ! $this->isValidEntityState($element)) {
+            return false;
         }
 
-        // clearing owning side value
-        $targetMetadata->reflFields[$mapping['mappedBy']]->setValue($element, null);
-
-        $this->uow->computeChangeSet($targetMetadata, $element);
-
-        $persister->update($element);
-
-        return true;
+        return $this
+            ->uow
+            ->getEntityPersister($mapping['targetEntity'])
+            ->delete($element);
     }
 
     /**

diff --git a/tests/Doctrine/Tests/Models/Tweet/User.php b/tests/Doctrine/Tests/Models/Tweet/User.php
@@ -29,14 +29,26 @@ class User
      */
     public $tweets;
 
+    /**
+     * @OneToMany(targetEntity="UserList", mappedBy="owner", fetch="EXTRA_LAZY", orphanRemoval=true)
+     */
+    public $userLists;
+
     public function __construct()
     {
-        $this->tweets = new ArrayCollection();
+        $this->tweets    = new ArrayCollection();
+        $this->userLists = new ArrayCollection();
     }
 
     public function addTweet(Tweet $tweet)
     {
         $tweet->setAuthor($this);
         $this->tweets->add($tweet);
     }
+
+    public function addUserList(UserList $userList)
+    {
+        $userList->owner = $this;
+        $this->userLists->add($userList);
+    }
 }
diff --git a/tests/Doctrine/Tests/Models/Tweet/UserList.php b/tests/Doctrine/Tests/Models/Tweet/UserList.php
@@ -0,0 +1,29 @@
+<?php
+
+namespace Doctrine\Tests\Models\Tweet;
+
+/**
+ * @Entity
+ * @Table(name="tweet_user_list")
+ */
+class UserList
+{
+    const CLASSNAME = __CLASS__;
+
+    /**
+     * @Id
+     * @GeneratedValue
+     * @Column(type="integer")
+     */
+    public $id;
+
+    /**
+     * @Column(type="string")
+     */
+    public $listName;
+
+    /**
+     * @ManyToOne(targetEntity="User", inversedBy="userLists")
+     */
+    public $owner;
+}
diff --git a/tests/Doctrine/Tests/ORM/Functional/ExtraLazyCollectionTest.php b/tests/Doctrine/Tests/ORM/Functional/ExtraLazyCollectionTest.php
@@ -7,6 +7,7 @@
 use Doctrine\Tests\Models\DDC2504\DDC2504OtherClass;
 use Doctrine\Tests\Models\Tweet\Tweet;
 use Doctrine\Tests\Models\Tweet\User;
+use Doctrine\Tests\Models\Tweet\UserList;
 use Doctrine\Tests\OrmFunctionalTestCase;
 
 /**
@@ -484,8 +485,7 @@ public function testRemoveElementOneToMany()
         $user->articles->removeElement($article);
 
         $this->assertFalse($user->articles->isInitialized(), "Post-Condition: Collection is not initialized.");
-        // NOTE: +2 queries because CmsArticle is a versioned entity, and that needs to be handled accordingly
-        $this->assertEquals($queryCount + 2, $this->getCurrentQueryCount());
+        $this->assertEquals($queryCount, $this->getCurrentQueryCount());
 
         // Test One to Many removal with Entity state as new
         $article = new \Doctrine\Tests\Models\CMS\CmsArticle();
@@ -528,29 +528,45 @@ public function testRemoveElementOneToMany()
      */
     public function testRemovalOfManagedElementFromOneToManyJoinedInheritanceCollectionDoesNotInitializeIt()
     {
+        /* @var $otherClass DDC2504OtherClass */
         $otherClass = $this->_em->find(DDC2504OtherClass::CLASSNAME, $this->ddc2504OtherClassId);
+        /* @var $childClass DDC2504ChildClass */
         $childClass = $this->_em->find(DDC2504ChildClass::CLASSNAME, $this->ddc2504ChildClassId);
 
         $queryCount = $this->getCurrentQueryCount();
 
         $otherClass->childClasses->removeElement($childClass);
+        $childClass->other = null; // updating owning side
 
         $this->assertFalse($otherClass->childClasses->isInitialized(), 'Collection is not initialized.');
 
         $this->assertEquals(
-            $queryCount + 1,
+            $queryCount,
             $this->getCurrentQueryCount(),
-            'The owning side of the association is updated'
+            'No queries have been executed'
         );
 
-        $this->assertFalse($otherClass->childClasses->contains($childClass));
+        $this->assertTrue(
+            $otherClass->childClasses->contains($childClass),
+            'Collection item still not updated (needs flushing)'
+        );
+
+        $this->_em->flush();
+
+        $this->assertFalse(
+            $otherClass->childClasses->contains($childClass),
+            'Referenced item was removed in the transaction'
+        );
+
+        $this->assertFalse($otherClass->childClasses->isInitialized(), 'Collection is not initialized.');
     }
 
     /**
      * @group DDC-2504
      */
     public function testRemovalOfNonManagedElementFromOneToManyJoinedInheritanceCollectionDoesNotInitializeIt()
     {
+        /* @var $otherClass DDC2504OtherClass */
         $otherClass = $this->_em->find(DDC2504OtherClass::CLASSNAME, $this->ddc2504OtherClassId);
         $queryCount = $this->getCurrentQueryCount();
 
@@ -568,6 +584,7 @@ public function testRemovalOfNonManagedElementFromOneToManyJoinedInheritanceColl
      */
     public function testRemovalOfNewElementFromOneToManyJoinedInheritanceCollectionDoesNotInitializeIt()
     {
+        /* @var $otherClass DDC2504OtherClass */
         $otherClass = $this->_em->find(DDC2504OtherClass::CLASSNAME, $this->ddc2504OtherClassId);
         $childClass = new DDC2504ChildClass();
 
@@ -1035,7 +1052,7 @@ private function loadFixture()
     /**
      * @group DDC-3343
      */
-    public function testRemovesManagedElementFromOneToManyExtraLazyCollection()
+    public function testRemoveManagedElementFromOneToManyExtraLazyCollectionIsNoOp()
     {
         list($userId, $tweetId) = $this->loadTweetFixture();
 
@@ -1049,22 +1066,21 @@ public function testRemovesManagedElementFromOneToManyExtraLazyCollection()
         /* @var $user User */
         $user = $this->_em->find(User::CLASSNAME, $userId);
 
-        $this->assertCount(0, $user->tweets);
+        $this->assertCount(1, $user->tweets, 'Element was not removed - need to update the owning side first');
     }
 
     /**
      * @group DDC-3343
      */
-    public function testRemovesManagedElementFromOneToManyExtraLazyCollectionWithoutDeletingTheTargetEntityEntry()
+    public function testRemoveManagedElementFromOneToManyExtraLazyCollectionWithoutDeletingTheTargetEntityEntryIsNoOp()
     {
         list($userId, $tweetId) = $this->loadTweetFixture();
 
         /* @var $user User */
         $user  = $this->_em->find(User::CLASSNAME, $userId);
+        $tweet = $this->_em->find(Tweet::CLASSNAME, $tweetId);
 
-        $e = $this->_em->find(Tweet::CLASSNAME, $tweetId);
-
-        $user->tweets->removeElement($e);
+        $user->tweets->removeElement($tweet);
 
         $this->_em->clear();
 
@@ -1076,7 +1092,11 @@ public function testRemovesManagedElementFromOneToManyExtraLazyCollectionWithout
             'Even though the collection is extra lazy, the tweet should not have been deleted'
         );
 
-        $this->assertNull($tweet->author, 'Tweet author link has been removed');
+        $this->assertInstanceOf(
+            User::CLASSNAME,
+            $tweet->author,
+            'Tweet author link has not been removed - need to update the owning side first'
+        );
     }
 
     /**
@@ -1102,12 +1122,89 @@ public function testRemovingManagedLazyProxyFromExtraLazyOneToManyDoesRemoveTheA
             'Even though the collection is extra lazy, the tweet should not have been deleted'
         );
 
-        $this->assertNull($tweet->author);
+        $this->assertInstanceOf(User::CLASSNAME, $tweet->author);
+
+        /* @var $user User */
+        $user = $this->_em->find(User::CLASSNAME, $userId);
+
+        $this->assertCount(1, $user->tweets, 'Element was not removed - need to update the owning side first');
+    }
+
+    /**
+     * @group DDC-3343
+     */
+    public function testRemoveOrphanedManagedElementFromOneToManyExtraLazyCollection()
+    {
+        list($userId, $userListId) = $this->loadUserListFixture();
+
+        /* @var $user User */
+        $user = $this->_em->find(User::CLASSNAME, $userId);
+
+        $user->userLists->removeElement($this->_em->find(UserList::CLASSNAME, $userListId));
+
+        $this->_em->clear();
 
         /* @var $user User */
         $user = $this->_em->find(User::CLASSNAME, $userId);
 
-        $this->assertCount(0, $user->tweets);
+        $this->assertCount(0, $user->userLists, 'Element was removed from association due to orphan removal');
+        $this->assertNull(
+            $this->_em->find(UserList::CLASSNAME, $userListId),
+            'Element was deleted due to orphan removal'
+        );
+    }
+
+    /**
+     * @group DDC-3343
+     */
+    public function testRemoveOrphanedUnManagedElementFromOneToManyExtraLazyCollection()
+    {
+        list($userId, $userListId) = $this->loadUserListFixture();
+
+        /* @var $user User */
+        $user = $this->_em->find(User::CLASSNAME, $userId);
+
+        $user->userLists->removeElement(new UserList());
+
+        $this->_em->clear();
+
+        /* @var $userList UserList */
+        $userList = $this->_em->find(UserList::CLASSNAME, $userListId);
+        $this->assertInstanceOf(
+            UserList::CLASSNAME,
+            $userList,
+            'Even though the collection is extra lazy + orphan removal, the user list should not have been deleted'
+        );
+
+        $this->assertInstanceOf(
+            User::CLASSNAME,
+            $userList->owner,
+            'User list to owner link has not been removed'
+        );
+    }
+
+    /**
+     * @group DDC-3343
+     */
+    public function testRemoveOrphanedManagedLazyProxyFromExtraLazyOneToMany()
+    {
+        list($userId, $userListId) = $this->loadUserListFixture();
+
+        /* @var $user User */
+        $user = $this->_em->find(User::CLASSNAME, $userId);
+
+        $user->userLists->removeElement($this->_em->getReference(UserList::CLASSNAME, $userListId));
+
+        $this->_em->clear();
+
+        /* @var $user User */
+        $user = $this->_em->find(User::CLASSNAME, $userId);
+
+        $this->assertCount(0, $user->userLists, 'Element was removed from association due to orphan removal');
+        $this->assertNull(
+            $this->_em->find(UserList::CLASSNAME, $userListId),
+            'Element was deleted due to orphan removal'
+        );
     }
 
     /**
@@ -1130,4 +1227,25 @@ private function loadTweetFixture()
 
         return array($user->id, $tweet->id);
     }
+
+    /**
+     * @return int[] ordered tuple: user id and user list id
+     */
+    private function loadUserListFixture()
+    {
+        $user     = new User();
+        $userList = new UserList();
+
+        $user->name     = 'ocramius';
+        $userList->listName = 'PHP Developers to follow closely';
+
+        $user->addUserList($userList);
+
+        $this->_em->persist($user);
+        $this->_em->persist($userList);
+        $this->_em->flush();
+        $this->_em->clear();
+
+        return array($user->id, $userList->id);
+    }
 }
diff --git a/tests/Doctrine/Tests/ORM/Functional/SecondLevelCacheTest.php b/tests/Doctrine/Tests/ORM/Functional/SecondLevelCacheTest.php
@@ -296,10 +296,12 @@ public function testPostRemoveFailure()
 
         $this->_em->clear();
 
-        $this->assertTrue($this->cache->containsEntity(Country::CLASSNAME, $countryId));
+        $this->assertFalse(
+            $this->cache->containsEntity(Country::CLASSNAME, $countryId),
+            'Removal attempts should clear the cache entry corresponding to the entity'
+        );
 
-        $country = $this->_em->find(Country::CLASSNAME, $countryId);
-        $this->assertInstanceOf(Country::CLASSNAME, $country);
+        $this->assertInstanceOf(Country::CLASSNAME, $this->_em->find(Country::CLASSNAME, $countryId));
     }
 
     public function testCachedNewEntityExists()