Merge pull request #1300 from Ocramius/hotfix/#1169-DDC-3343-one-to-m…

…any-persister-deletes-only-on-extra-lazy-plus-orphan-removal-2.4

[2.4] #1169 DDC-3343 one-to-omany persister deletes only on EXTRA_LAZY plus orphanRemoval

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;
  }

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;
  }
 }

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);
+ }
 }

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;
+}

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

@@ -5,6 +5,7 @@
 use Doctrine\ORM\Mapping\ClassMetadataInfo;
 use Doctrine\Tests\Models\Tweet\Tweet;
 use Doctrine\Tests\Models\Tweet\User;
+use Doctrine\Tests\Models\Tweet\UserList;
 
 require_once __DIR__ . '/../../TestInit.php';
 
@@ -26,6 +27,8 @@ public function setUp()
  {
  $this->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);
+ }
 }

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();
  }