diff --git a/lib/Doctrine/ORM/PersistentCollection.php b/lib/Doctrine/ORM/PersistentCollection.php index 2210c3c31..4ea56f55b 100644 --- a/lib/Doctrine/ORM/PersistentCollection.php +++ b/lib/Doctrine/ORM/PersistentCollection.php @@ -692,21 +692,39 @@ final class PersistentCollection extends AbstractLazyCollection implements Selec protected function doInitialize() { // Has NEW objects added through add(). Remember them. - $newObjects = []; + $newlyAddedDirtyObjects = []; if ($this->isDirty) { - $newObjects = $this->collection->toArray(); + $newlyAddedDirtyObjects = $this->collection->toArray(); } $this->collection->clear(); $this->em->getUnitOfWork()->loadCollection($this); $this->takeSnapshot(); - // Reattach NEW objects added through add(), if any. - if ($newObjects) { - foreach ($newObjects as $obj) { - $this->collection->add($obj); - } + if ($newlyAddedDirtyObjects) { + $this->restoreNewObjectsInDirtyCollection($newlyAddedDirtyObjects); + } + } + + /** + * @param object[] $newObjects + * + * Note: the only reason why this entire looping/complexity is performed via `spl_object_hash` + * is because we want to prevent using `array_udiff()`, which is likely to cause very + * high overhead (complexity of O(n^2)). `array_diff_key()` performs the operation in + * core, which is faster than using a callback for comparisons + */ + private function restoreNewObjectsInDirtyCollection(array $newObjects) : void + { + $loadedObjects = $this->collection->toArray(); + $newObjectsByOid = \array_combine(\array_map('spl_object_hash', $newObjects), $newObjects); + $loadedObjectsByOid = \array_combine(\array_map('spl_object_hash', $loadedObjects), $loadedObjects); + $newObjectsThatWereNotLoaded = \array_diff_key($newObjectsByOid, $loadedObjectsByOid); + + if ($newObjectsThatWereNotLoaded) { + // Reattach NEW objects added through add(), if any. + \array_walk($newObjectsThatWereNotLoaded, [$this->collection, 'add']); $this->isDirty = true; } diff --git a/tests/Doctrine/Tests/ORM/PersistentCollectionTest.php b/tests/Doctrine/Tests/ORM/PersistentCollectionTest.php index 0e1c20f51..38be94522 100644 --- a/tests/Doctrine/Tests/ORM/PersistentCollectionTest.php +++ b/tests/Doctrine/Tests/ORM/PersistentCollectionTest.php @@ -4,6 +4,7 @@ namespace Doctrine\Tests\ORM; use Doctrine\Common\Collections\ArrayCollection; use Doctrine\ORM\PersistentCollection; +use Doctrine\ORM\UnitOfWork; use Doctrine\Tests\Mocks\ConnectionMock; use Doctrine\Tests\Mocks\DriverMock; use Doctrine\Tests\Mocks\EntityManagerMock; @@ -24,7 +25,7 @@ class PersistentCollectionTest extends OrmTestCase protected $collection; /** - * @var \Doctrine\ORM\EntityManagerInterface + * @var EntityManagerMock */ private $_emMock; @@ -33,6 +34,8 @@ class PersistentCollectionTest extends OrmTestCase parent::setUp(); $this->_emMock = EntityManagerMock::create(new ConnectionMock([], new DriverMock())); + + $this->setUpPersistentCollection(); } /** @@ -59,7 +62,6 @@ class PersistentCollectionTest extends OrmTestCase */ public function testCurrentInitializesCollection() { - $this->setUpPersistentCollection(); $this->collection->current(); $this->assertTrue($this->collection->isInitialized()); } @@ -69,7 +71,6 @@ class PersistentCollectionTest extends OrmTestCase */ public function testKeyInitializesCollection() { - $this->setUpPersistentCollection(); $this->collection->key(); $this->assertTrue($this->collection->isInitialized()); } @@ -79,7 +80,6 @@ class PersistentCollectionTest extends OrmTestCase */ public function testNextInitializesCollection() { - $this->setUpPersistentCollection(); $this->collection->next(); $this->assertTrue($this->collection->isInitialized()); } @@ -89,8 +89,6 @@ class PersistentCollectionTest extends OrmTestCase */ public function testNonObjects() { - $this->setUpPersistentCollection(); - $this->assertEmpty($this->collection); $this->collection->add("dummy"); @@ -113,12 +111,12 @@ class PersistentCollectionTest extends OrmTestCase */ public function testRemovingElementsAlsoRemovesKeys() { - $this->setUpPersistentCollection(); + $dummy = new \stdClass(); - $this->collection->add('dummy'); + $this->collection->add($dummy); $this->assertEquals([0], array_keys($this->collection->toArray())); - $this->collection->removeElement('dummy'); + $this->collection->removeElement($dummy); $this->assertEquals([], array_keys($this->collection->toArray())); } @@ -127,9 +125,7 @@ class PersistentCollectionTest extends OrmTestCase */ public function testClearWillAlsoClearKeys() { - $this->setUpPersistentCollection(); - - $this->collection->add('dummy'); + $this->collection->add(new \stdClass()); $this->collection->clear(); $this->assertEquals([], array_keys($this->collection->toArray())); } @@ -139,12 +135,133 @@ class PersistentCollectionTest extends OrmTestCase */ public function testClearWillAlsoResetKeyPositions() { - $this->setUpPersistentCollection(); + $dummy = new \stdClass(); - $this->collection->add('dummy'); - $this->collection->removeElement('dummy'); + $this->collection->add($dummy); + $this->collection->removeElement($dummy); $this->collection->clear(); - $this->collection->add('dummy'); + $this->collection->add($dummy); $this->assertEquals([0], array_keys($this->collection->toArray())); } + + /** + * @group 6613 + * @group 6614 + * @group 6616 + */ + public function testWillKeepNewItemsInDirtyCollectionAfterInitialization() : void + { + /* @var $unitOfWork UnitOfWork|\PHPUnit_Framework_MockObject_MockObject */ + $unitOfWork = $this->createMock(UnitOfWork::class); + + $this->_emMock->setUnitOfWork($unitOfWork); + + $newElement = new \stdClass(); + $persistedElement = new \stdClass(); + + $this->collection->add($newElement); + + self::assertFalse($this->collection->isInitialized()); + self::assertTrue($this->collection->isDirty()); + + $unitOfWork + ->expects(self::once()) + ->method('loadCollection') + ->with($this->collection) + ->willReturnCallback(function (PersistentCollection $persistentCollection) use ($persistedElement) : void { + $persistentCollection->unwrap()->add($persistedElement); + }); + + $this->collection->initialize(); + + self::assertSame([$persistedElement, $newElement], $this->collection->toArray()); + self::assertTrue($this->collection->isInitialized()); + self::assertTrue($this->collection->isDirty()); + } + + /** + * @group 6613 + * @group 6614 + * @group 6616 + */ + public function testWillDeDuplicateNewItemsThatWerePreviouslyPersistedInDirtyCollectionAfterInitialization() : void + { + /* @var $unitOfWork UnitOfWork|\PHPUnit_Framework_MockObject_MockObject */ + $unitOfWork = $this->createMock(UnitOfWork::class); + + $this->_emMock->setUnitOfWork($unitOfWork); + + $newElement = new \stdClass(); + $newElementThatIsAlsoPersisted = new \stdClass(); + $persistedElement = new \stdClass(); + + $this->collection->add($newElementThatIsAlsoPersisted); + $this->collection->add($newElement); + + self::assertFalse($this->collection->isInitialized()); + self::assertTrue($this->collection->isDirty()); + + $unitOfWork + ->expects(self::once()) + ->method('loadCollection') + ->with($this->collection) + ->willReturnCallback(function (PersistentCollection $persistentCollection) use ( + $persistedElement, + $newElementThatIsAlsoPersisted + ) : void { + $persistentCollection->unwrap()->add($newElementThatIsAlsoPersisted); + $persistentCollection->unwrap()->add($persistedElement); + }); + + $this->collection->initialize(); + + self::assertSame( + [$newElementThatIsAlsoPersisted, $persistedElement, $newElement], + $this->collection->toArray() + ); + self::assertTrue($this->collection->isInitialized()); + self::assertTrue($this->collection->isDirty()); + } + + /** + * @group 6613 + * @group 6614 + * @group 6616 + */ + public function testWillNotMarkCollectionAsDirtyAfterInitializationIfNoElementsWereAdded() : void + { + /* @var $unitOfWork UnitOfWork|\PHPUnit_Framework_MockObject_MockObject */ + $unitOfWork = $this->createMock(UnitOfWork::class); + + $this->_emMock->setUnitOfWork($unitOfWork); + + $newElementThatIsAlsoPersisted = new \stdClass(); + $persistedElement = new \stdClass(); + + $this->collection->add($newElementThatIsAlsoPersisted); + + self::assertFalse($this->collection->isInitialized()); + self::assertTrue($this->collection->isDirty()); + + $unitOfWork + ->expects(self::once()) + ->method('loadCollection') + ->with($this->collection) + ->willReturnCallback(function (PersistentCollection $persistentCollection) use ( + $persistedElement, + $newElementThatIsAlsoPersisted + ) : void { + $persistentCollection->unwrap()->add($newElementThatIsAlsoPersisted); + $persistentCollection->unwrap()->add($persistedElement); + }); + + $this->collection->initialize(); + + self::assertSame( + [$newElementThatIsAlsoPersisted, $persistedElement], + $this->collection->toArray() + ); + self::assertTrue($this->collection->isInitialized()); + self::assertFalse($this->collection->isDirty()); + } }