fix: compute changes on lately added entities for insertions

[gseidel]

In the uow the function computeChangeSets will computeScheduleInsertsChangeSets first to get all changes for new insert entities. For freshly added entities during a compute over a cascade persist, a single computeSet will be called, see:

orm/src/UnitOfWork.php

Lines 997 to 998 in fe48a6d

	$this->persistNew($targetClass, $entry);
	$this->computeChangeSet($targetClass, $entry);

But a persist hook may call a persist on further entities. This entities are dangling in the entityInsertions but never computed. This lead to sql errors later on.

This PR will check entityInsertions that aren't computed yet at the end of computeChangeSets to avoid sql errors.

[gseidel]

@greg0ire Maybe you have time to review this, it's familar with my previous PR #11835

[beberlei]

This is a bad idea:

• for one it does not handle this recursively, because the second call could again create new entities and they would need to be handled.
• it will iterate over the list of entities twice, costing performance.

The documentation on prePersist states this restriction:

Doctrine will not recognize changes made to relations in a prePersist event. This includes modifications to collections such as additions, removals or replacement.

Also the fix in the previous PR seems to reimplement cascade persist, why not use cascade persist on the $object->unmapped association?

[gseidel]

Thanks @beberlei for your fast review. Let me try to address your concerns.

Also the fix in the previous PR seems to reimplement cascade persist, why not use cascade persist on the $object->unmapped association?

The $object->unmapped doesn't have a build in doctrine association. I try to add my own custom association as an extension, so indeed, I want to reimplement a cascade persist and found this bug and the previous one. I think it should be possible to add own associations and therefor this fix may help others as well.

for one it does not handle this recursively, because the second call could again create new entities and they would need to be handled.

I might be wrong, but also the first computeScheduleInsertsChangeSets doesn't handle recursion as well. I guess it's not necessary, because at this point the persist function already found all entities for insertions recursively. So I think that would be the same after computing the changes for managed entities. All entites are found and there will be no more prePersist hook, that can be called. But while computing changes for managed entities, prePersist hook may be triggered and thats the problem here, that new entities added beyond build in associations are not computed anymore.

But as I say, maybe I miss out something. Do you have an example in mind where new entities could be added during computeScheduleInsertsChangeSets?

it will iterate over the list of entities twice, costing performance.

I see that as well. It could be avoided by just calling computeScheduleInsertsChangeSets once after computing managed entities. There is a comment Compute changes for INSERTed entities first. This must always happen.. But if I remove the the first call, there is no breaking changes in the tests. I'm trying to figure out some side effects. I only see that a persist hook don't have access to change sets, but on this point change sets may not be completed at all.

Doctrine will not recognize changes made to relations in a prePersist event. This includes modifications to collections such as additions, removals or replacement.

I don't have a strong argument for this, but I'm not trying to make changes on the entity itself, just found new entites to persist.