-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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.
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
#6613 #6614 correct handling of new elements added to an initialized (yet dirty) collection #6616
#6613 #6614 correct handling of new elements added to an initialized (yet dirty) collection #6616
Conversation
…g identifiers to reduce moving parts
…ng`), segregating phone creation away
…are restored in the dirty state of the collection
… `PersistentCollection`
… `PersistentCollection`
$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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clever...I'm totally stealing this one.
{ | ||
$this->id = uniqid('phone', true); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs trailing newline 😛
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll probably get rid of the test once I convert it to a unit test
|
||
$this->isDirty = true; | ||
} | ||
$this->isDirty = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The collection shouldn't be marked as dirty if no items were added after initializing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It actually should: it was loaded, but it was never flushed. I also had a previous assertion for that, see 1fc7f81
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assume you add the following code after 1fc7f81#diff-c32ade803f1349ec4da7da8644879ef6R65:
$this->_em->flush();
self::assertFalse($phones->isInitialized());
self::assertFalse($phones->isDirty());
$phones->add($item2);
self::assertFalse($phones->isInitialized());
self::assertTrue($phones->isDirty());
$phones->initialize();
self::assertTrue($phones->isInitialized());
self::assertFalse($phones->isDirty());
After the initialize, the only item that was added to the uninitialized collection was already present after the collection was initialized. Thus, after initializing, the collection is not dirty.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That element does not exist in the DB though, so the collection does indeed need to go through persistence at the next flush()
call.
If the collection wasn't dirty after this initialization, the object association would be completely lost (other tests break too if I do that, btw).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you were to flush after adding $item2
, then re-add $item2
to the (now uninitialized) collection. Once the collection is initialized, there will be no element to be added in restoreNewObjectsInDirtyCollection
, but the collection will be marked as dirty, even though it wasn't modified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alcaeus re-read the test carefully:
- add (collection dirty, uninitialized)
- flush (collection clean, uninitialized)
- add (collection dirty, uninitialized)
- initialize (collection dirty, since we added and we didn't flush)
- flush (collection clean, initialized)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alcaeus I converted these tests into more isolated PersistentCollectionTest
scenarios: please check them.
Also, should we drop this integration test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Ocramius new tests look good. I believe we can drop the integration test.
I've also pushed testWillNotMarkCollectionAsDirtyAfterInitializationIfNoElementsWereAdded
to show the issue I was mentioning above: after initializing, there are no new elements in the collection, but it is still marked as dirty. Feel free updating either the test or the collection logic to make it pass.
@@ -0,0 +1,107 @@ | |||
<?php |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This entire test should probably be ported to PersistentCollectionTest
and stripped of any UnitOfWork
interactions
|
||
if ($this->isDirty) { | ||
$newObjects = $this->collection->toArray(); | ||
$newlyAddedDirtyObjects = $this->collection->toArray(); | ||
} | ||
|
||
$this->collection->clear(); |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
private function restoreNewObjectsInDirtyCollection(array $newObjects) | ||
{ | ||
$loadedObjects = $this->collection->toArray(); | ||
$newObjectsByOid = array_combine(array_map('spl_object_hash', $newObjects), $newObjects); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These variables are redundant
…erifies basic dirty collection initialization semantics
…that are also coming from initialization data de-duplicates new and persisted items
… if all new items are contained in the initialized ones
…toreNewObjectsInDirtyCollection` implementation
…erifies basic dirty collection initialization semantics
…that are also coming from initialization data de-duplicates new and persisted items
… if all new items are contained in the initialized ones
…toreNewObjectsInDirtyCollection` implementation
Fixes #6613
Fixes #6614
This is a very bad bug. The fix introduces some ovehead, but the scenario is usually not that common, so it should be fine.
Ideally, we should keep
new
anddirty
states completely separate, but that's not possible without completely changing also the base class (Doctrine\Common\Collections\AbstractLazyCollection
)