Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

#6613 #6614 correct handling of new elements added to an initialized (yet dirty) collection #6616

Merged
merged 23 commits into from
Aug 11, 2017
Merged

#6613 #6614 correct handling of new elements added to an initialized (yet dirty) collection #6616

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
874d60d
It added the unit test #6613
Sysaninster Aug 11, 2017
594e60d
#6613 #6614 simplifying entity definition - using auto-assigned strin…
Ocramius Aug 11, 2017
0a1a841
#6613 #6614 better test specification - removing useless assertions
Ocramius Aug 11, 2017
112a402
#6613 #6614 smashing entity definitions into the test
Ocramius Aug 11, 2017
625f792
#6613 #6614 removing dedicated DDC6613 model directory
Ocramius Aug 11, 2017
c195064
#6613 #6614 CS - applying `@group` annotation to the test
Ocramius Aug 11, 2017
eca1d6b
#6613 #6614 rewrote test logic to be less magic-constant-dependent
Ocramius Aug 11, 2017
a4e547b
#6613 #6614 remove superfluous mappings
Ocramius Aug 11, 2017
80f12ed
#6613 #6614 correcting column mapping (was `integer`, should be `stri…
Ocramius Aug 11, 2017
8c4b5a4
#6613 #6614 removing phone/user specifics, using ORM naming for assoc…
Ocramius Aug 11, 2017
d2c9b22
#6613 #6614 removing IDE-generated header
Ocramius Aug 11, 2017
5e2257d
#6613 #6614 adding assertions about collection initialization and dir…
Ocramius Aug 11, 2017
1fc7f81
#6613 #6614 after initialization, the collection should be dirty anyway
Ocramius Aug 11, 2017
d44e6e1
#6613 #6614 ensuring that only newly added items that weren't loaded …
Ocramius Aug 11, 2017
031e79e
#6613 #6614 correcting broken test that isn't using objects against a…
Ocramius Aug 11, 2017
345cf1a
#6613 #6614 correcting broken test that isn't using objects against a…
Ocramius Aug 11, 2017
04a5b12
#6613 #6614 #6616 moved integration test basics to a unit test that v…
Ocramius Aug 11, 2017
93c4064
#6613 #6614 #6616 initializing a dirty collection that has new items …
Ocramius Aug 11, 2017
b064fe3
#6613 #6614 #6616 removing repeated `PersistentCollectionTest` chunks…
Ocramius Aug 11, 2017
1174ec6
Add failing test for dirty flag
alcaeus Aug 11, 2017
a2ca6bb
#6613 #6614 #6616 ensuring that the collection is marked as non-dirty…
Ocramius Aug 11, 2017
3579997
#6613 #6614 #6616 removing DDC6613 test, which was fully ported to un…
Ocramius Aug 11, 2017
004ac51
#6613 #6614 #6616 minor performance optimisations around the new `res…
Ocramius Aug 11, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 27 additions & 7 deletions lib/Doctrine/ORM/PersistentCollection.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -692,21 +692,41 @@ public function unwrap()
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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the issue could be solved by only not calling clear() here.
IIRC, UnitOfWork already handles dirty elements in the collection and does not replace them when hydrating in BasicEntityPersister::loadCollectionFromStatement (which is where all calls go at the end).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not the case: removing the clear() causes this test to fail, as well as another 3:

There were 4 failures:

1) Doctrine\Tests\ORM\Functional\ManyToManyBasicAssociationTest::testRetrieveManyToManyAndAddMore
Failed asserting that 4 matches expected 3.

/home/ocramius/Documents/doctrine/doctrine2/tests/Doctrine/Tests/ORM/Functional/ManyToManyBasicAssociationTest.php:201

2) Doctrine\Tests\ORM\Functional\SecondLevelCacheOneToManyTest::testCacheInitializeCollectionWithNewObjects
Failed asserting that actual size 3 matches expected size 4.

/home/ocramius/Documents/doctrine/doctrine2/tests/Doctrine/Tests/ORM/Functional/SecondLevelCacheOneToManyTest.php:377

3) Doctrine\Tests\ORM\Functional\Ticket\DDC1643Test::testNotCloneAndPassAroundFlush
Failed asserting that 2 matches expected 3.

/home/ocramius/Documents/doctrine/doctrine2/tests/Doctrine/Tests/ORM/Functional/Ticket/DDC1643Test.php:118

4) Doctrine\Tests\ORM\Functional\Ticket\DDC6613Test::testFail
Failed asserting that actual size 3 matches expected size 2.

/home/ocramius/Documents/doctrine/doctrine2/tests/Doctrine/Tests/ORM/Functional/Ticket/DDC6613Test.php:71

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the current solution is a decent one, as it keeps the responsibility of re-attaching the NEW records within the PersistentCollection implementation. Distributing it around multiple objects may be better for performance, but is actually a mess to track on the long term

$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
*
* @return void
*
* 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)
{
$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;
}
Expand Down
150 changes: 134 additions & 16 deletions tests/Doctrine/Tests/ORM/PersistentCollectionTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@
namespace Doctrine\Tests\ORM;

use Doctrine\Common\Collections\ArrayCollection;
use Doctrine\Common\Collections\Collection;
use Doctrine\ORM\PersistentCollection;
use Doctrine\ORM\UnitOfWork;
use Doctrine\Tests\Mocks\ConnectionMock;
use Doctrine\Tests\Mocks\DriverMock;
use Doctrine\Tests\Mocks\EntityManagerMock;
Expand All @@ -24,7 +26,7 @@ class PersistentCollectionTest extends OrmTestCase
protected $collection;

/**
* @var \Doctrine\ORM\EntityManagerInterface
* @var EntityManagerMock
*/
private $_emMock;

Expand All @@ -33,6 +35,8 @@ protected function setUp()
parent::setUp();

$this->_emMock = EntityManagerMock::create(new ConnectionMock([], new DriverMock()));

$this->setUpPersistentCollection();
}

/**
Expand All @@ -59,7 +63,6 @@ public function testCanBePutInLazyLoadingMode()
*/
public function testCurrentInitializesCollection()
{
$this->setUpPersistentCollection();
$this->collection->current();
$this->assertTrue($this->collection->isInitialized());
}
Expand All @@ -69,7 +72,6 @@ public function testCurrentInitializesCollection()
*/
public function testKeyInitializesCollection()
{
$this->setUpPersistentCollection();
$this->collection->key();
$this->assertTrue($this->collection->isInitialized());
}
Expand All @@ -79,7 +81,6 @@ public function testKeyInitializesCollection()
*/
public function testNextInitializesCollection()
{
$this->setUpPersistentCollection();
$this->collection->next();
$this->assertTrue($this->collection->isInitialized());
}
Expand All @@ -89,8 +90,6 @@ public function testNextInitializesCollection()
*/
public function testNonObjects()
{
$this->setUpPersistentCollection();

$this->assertEmpty($this->collection);

$this->collection->add("dummy");
Expand All @@ -113,12 +112,12 @@ public function testNonObjects()
*/
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()));
}

Expand All @@ -127,9 +126,7 @@ public function testRemovingElementsAlsoRemovesKeys()
*/
public function testClearWillAlsoClearKeys()
{
$this->setUpPersistentCollection();

$this->collection->add('dummy');
$this->collection->add(new \stdClass());
$this->collection->clear();
$this->assertEquals([], array_keys($this->collection->toArray()));
}
Expand All @@ -139,12 +136,133 @@ public function testClearWillAlsoClearKeys()
*/
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()
{
/* @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) {
$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()
{
/* @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
) {
$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()
{
/* @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
) {
$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());
}
}