BUG: Calling PersistenceManager->persistAll() with scheduled ValueObjects twice breaks Unit Of Work

[fcool]

Is there an existing issue for this?

• [X] I have searched the existing issues

Current Behavior

I'm not yet sure what the "minimal" set of actions triggers the behvior. So the complete story for now is:

1. Create new ValueObjects as related objects in a Doctrine lifecycle event (prePersist)
2. Call persistAll() as usual
3. Create more objects, creating new instances of the theoretically same ValueObject as before
4. Call persistAll() as usual
5. System crashes with

A managed+dirty entity Acme\Package\Domain\Model\ValueObject@OID can not be scheduled for insertion.

Expected Behavior

No crash

Steps To Reproduce

see above

Environment

- Flow: 7.3.0 
- PHP: 7.4.30

Anything else?

Probably the error has been introduced with this two commits. Reverting to the behavior to "as before" but with spl_object_id instead of spl_object_hash solves the problem. So probably it exists sind 6.3.13

https://github.com/neos/flow-development-collection/commit/2e914c06eb9b68335357d3f974a6f907deaba912 https://github.com/neos/flow-development-collection/commit/50d3ff3c47591125c17e6ee237c919acfbb0a5ac

[albe]

Interesting, that implies that in this case calling scheduleForDelete is not the same as removing the VO from the doctrine internal insert map. See https://github.com/doctrine/orm/blob/2.12.x/lib/Doctrine/ORM/UnitOfWork.php#L1478-L1504 and https://github.com/neos/flow-development-collection/blob/7.3/Neos.Flow/Classes/Persistence/Doctrine/ObjectValidationAndDeDuplicationListener.php#L86-L106 for reference. The only thing I see that might make a difference is the $this->removeFromIdentityMap($entity); call. Could that be the cause?

In case we can't solve this with a proper code path calling a doctrine method, I would suggest fixing this by doing:

foreach ($entityInsertions as $oid => $entity) {
   ...
   unset($entityInsertions[$oid]);
}

That way we at least still do not hardcode the knowledge about how doctrine internally indexes it's entities.

[fcool]

Owww! Nice idea Alex! I'll try this out. Currently I'm trying to write a functional test covering this situation, which is - especially for regressions - always a good thing to have, I guess ;)

And yes: My suspicion was this removeFromIdentityMap, too. The ValueObject entity gets managed, but from the ORM view never persisted, so its "dirty".

What I simply not understand, why it only crashes in the SECOND persistAll but Doctrine has quite a bit of complexity there.

[fcool]

I managed to write a minimal Test-Case :)

    public function testValueObjectDeduplication()
    {
        for ($i = 0; $i < 2; $i++) {
            $testEntity = new TestEntity();
            $testEntity->setRelatedValueObject(new TestValueObject('deduplicate'));
            $this->persistenceManager->add($testEntity);
            $this->persistenceManager->persistAll();
            $this->persistenceManager->update($testEntity);
            $this->persistenceManager->persistAll();
        }
        $query = new Query(TestEntity::class);
        self::assertEquals(2, $query->count());
    }

It crashes on $this->persistenceManager->update($testEntity); with: Doctrine\ORM\ORMInvalidArgumentException: A managed+dirty entity Neos\Flow\Tests\Functional\Persistence\Fixtures\TestValueObject@2660 can not be scheduled for insertion.

What do you think, should I add this test to our test suite? The TestValueObject and TestEntity are reused from other PersistenceTests

[fcool]

And your proposed solution indeed works.

[fcool]

And just made the tests again against the 6.3-dev - Branch. And yes, assumption was true, error exists since 6.3.13

[kitsunet]

interesting read, thanks both for digging into this!