diff --git a/lib/Doctrine/ORM/Persisters/OneToManyPersister.php b/lib/Doctrine/ORM/Persisters/OneToManyPersister.php index cf39436d485..b1ff59cdc9b 100644 --- a/lib/Doctrine/ORM/Persisters/OneToManyPersister.php +++ b/lib/Doctrine/ORM/Persisters/OneToManyPersister.php @@ -19,7 +19,6 @@ namespace Doctrine\ORM\Persisters; -use Doctrine\Common\Proxy\Proxy; use Doctrine\ORM\PersistentCollection; use Doctrine\ORM\UnitOfWork; @@ -223,6 +222,13 @@ public function contains(PersistentCollection $coll, $element) */ public function removeElement(PersistentCollection $coll, $element) { + $mapping = $coll->getMapping(); + + if ( ! $mapping['orphanRemoval']) { + // no-op: this is not the owning side, therefore no operations should be applied + return false; + } + $uow = $this->em->getUnitOfWork(); // shortcut for new entities @@ -238,19 +244,10 @@ public function removeElement(PersistentCollection $coll, $element) return false; } - $mapping = $coll->getMapping(); - $persister = $this->uow->getEntityPersister($mapping['targetEntity']); - $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); + $this + ->uow + ->getEntityPersister($mapping['targetEntity']) + ->delete($element); return true; } diff --git a/tests/Doctrine/Tests/Models/Tweet/Tweet.php b/tests/Doctrine/Tests/Models/Tweet/Tweet.php index 8a315df9ef0..9f564e27b25 100644 --- a/tests/Doctrine/Tests/Models/Tweet/Tweet.php +++ b/tests/Doctrine/Tests/Models/Tweet/Tweet.php @@ -18,17 +18,20 @@ class Tweet public $id; /** - * @Column(type="string") + * @Column(type="string", length=140) */ - public $content; + public $content = ''; /** - * @ManyToOne(targetEntity="User", inversedBy="tweets") + * @ManyToOne(targetEntity="User", inversedBy="tweets", cascade={"persist"}, fetch="EXTRA_LAZY") */ public $author; - public function setAuthor(User $user) + /** + * @param User $author + */ + public function setAuthor(User $author) { - $this->author = $user; + $this->author = $author; } } diff --git a/tests/Doctrine/Tests/Models/Tweet/User.php b/tests/Doctrine/Tests/Models/Tweet/User.php index 4722e63176b..68cf137f2b0 100644 --- a/tests/Doctrine/Tests/Models/Tweet/User.php +++ b/tests/Doctrine/Tests/Models/Tweet/User.php @@ -29,9 +29,15 @@ 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) @@ -39,4 +45,10 @@ 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 new file mode 100644 index 00000000000..1eb3e141f03 --- /dev/null +++ b/tests/Doctrine/Tests/Models/Tweet/UserList.php @@ -0,0 +1,29 @@ +useModelSet('tweet'); $this->useModelSet('cms'); + $this->useModelSet('tweet'); + parent::setUp(); $class = $this->_em->getClassMetadata('Doctrine\Tests\Models\CMS\CmsUser'); @@ -366,8 +369,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(); @@ -388,7 +390,7 @@ public function testRemoveElementOneToMany() $user->articles->removeElement($article); - $this->assertEquals($queryCount, $this->getCurrentQueryCount(), "Removing a persisted entity will not cause queries when the owning side doesn't actually change."); + $this->assertEquals($queryCount, $this->getCurrentQueryCount(), "Removing a persisted entity should cause one query to be executed."); $this->assertFalse($user->articles->isInitialized(), "Post-Condition: Collection is not initialized."); // Test One to Many removal with Entity state as managed @@ -658,7 +660,7 @@ private function loadFixture() /** * @group DDC-3343 */ - public function testRemovesManagedElementFromOneToManyExtraLazyCollection() + public function testRemoveManagedElementFromOneToManyExtraLazyCollectionIsNoOp() { list($userId, $tweetId) = $this->loadTweetFixture(); @@ -672,20 +674,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); - $user->tweets->removeElement($this->_em->find(Tweet::CLASSNAME, $tweetId)); + $user->tweets->removeElement($tweet); $this->_em->clear(); @@ -697,7 +700,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' + ); } /** @@ -723,12 +730,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->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->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' + ); } /** @@ -751,4 +835,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/OrmFunctionalTestCase.php b/tests/Doctrine/Tests/OrmFunctionalTestCase.php index ff0f2cb531d..479887b92b0 100644 --- a/tests/Doctrine/Tests/OrmFunctionalTestCase.php +++ b/tests/Doctrine/Tests/OrmFunctionalTestCase.php @@ -166,6 +166,11 @@ abstract class OrmFunctionalTestCase extends OrmTestCase 'Doctrine\Tests\Models\Taxi\Car', 'Doctrine\Tests\Models\Taxi\Driver', ), + 'tweet' => array( + 'Doctrine\Tests\Models\Tweet\User', + 'Doctrine\Tests\Models\Tweet\Tweet', + 'Doctrine\Tests\Models\Tweet\UserList', + ), ); /** @@ -275,6 +280,7 @@ protected function tearDown() } if (isset($this->_usedModelSets['tweet'])) { $conn->executeUpdate('DELETE FROM tweet_tweet'); + $conn->executeUpdate('DELETE FROM tweet_user_list'); $conn->executeUpdate('DELETE FROM tweet_user'); } if (isset($this->_usedModelSets['legacy'])) { @@ -305,6 +311,12 @@ protected function tearDown() $conn->executeUpdate('DELETE FROM taxi_driver'); } + if (isset($this->_usedModelSets['tweet'])) { + $conn->executeUpdate('DELETE FROM tweet_tweet'); + $conn->executeUpdate('DELETE FROM tweet_user_list'); + $conn->executeUpdate('DELETE FROM tweet_user'); + } + $this->_em->clear(); }