Cannot persist new entity if primary key contains a foreign key and the referenced object is in state new and its id is not assigned #7003
Conversation
…s and the target entity is new. This prevents a throw in UnitOfWork#addToIdentityMap because some fields are null.
lib/Doctrine/ORM/UnitOfWork.php
Outdated
| @@ -1033,12 +1044,16 @@ private function executeInserts($class) | |||
| $persister = $this->getEntityPersister($className); | |||
| $invoke = $this->listenersInvoker->getSubscribedSystems($class, Events::postPersist); | |||
|
|
|||
| $theseInsertions = []; | |||
There was a problem hiding this comment.
$theseInsertions is a bad name here
lib/Doctrine/ORM/UnitOfWork.php
Outdated
| //add it now | ||
| $identifier = []; | ||
| $idFields = $class->getIdentifierFieldNames(); | ||
| foreach ($idFields as $idField) { |
There was a problem hiding this comment.
This block seems to detect whether an identifier is incomplete. Can it be moved to a private method?
lib/Doctrine/ORM/UnitOfWork.php
Outdated
| @@ -1067,6 +1082,34 @@ private function executeInserts($class) | |||
|
|
|||
| $this->addToIdentityMap($entity); | |||
| } | |||
| } else { | |||
| foreach ($theseInsertions as $oid => $entity) { | |||
There was a problem hiding this comment.
If the commit order calculator is working correctly, then this should simply be pushed down in the execution path, no?
|
Hi @Ocramius, This is actually not related to #7006. Here, the code path starts with
The commit order calculator is not involved here, it is just basic tree walking. Actually the commit order calculator does the job, otherwise the second part of the patch (in |
|
Thanks for clarifying that!
…On 26 Jan 2018 15:21, "Nicolas FRANÇOIS" ***@***.***> wrote:
Hi @Ocramius <https://github.com/ocramius>,
I'll make the requested changes.
This is actually not related to #7006
<#7006>. Here, the code path
starts with UnitOfWork#persist
and what happens is:
- set identifier if possible
- schedule for insert
- add to identity map if identifier has been set
- cascade persist for all associations
The commit order calculator is not involved here, it is just basic tree
walking.
Actually the commit order calculator does the job, otherwise the second
part of the patch (in executeInserts) would not work : the referenced
entity is correctly inserted first, and we can finally have a complete
identifier for the referencing entity.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#7003 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAJakG13poHjtKGkmsYtnLNgPDQYvur7ks5tOd9agaJpZM4RpenL>
.
|
|
Is this PR ready to be merged ? |
|
It is IMHO, but I'm not the maintainer. @Ocramius, is it OK for you ? |
|
Actually I think this is breaking a lot of projects as it's not possible anymore after upgrading to 2.6 to persist any entity related to a new one in the same batch. Can we get this merged or at least a feedback to get it merged asap? |
|
I'm simply busy with other work stuff ATM - don't expect any deep involved OSS work from me for a while, sorry. |
|
Can we get at least another maintainer have a look at this? It's a really high priority bug to fix |
|
The code provided here doesn't follow our coding standards and we already had a test that reproduces this issue in #6701 (with a simpler set of entities too). I'll try to combine everything and adjust stuff to make this part of |
|
This is a huge news, thank you for your time! |
As the id of the referenced entity is generated, when
UnitOfWork#persistNewcomputes the primary key, it may contain somenullvalues.Because of 1cb8d79 ,
UnitOfWork#addToIdentityMapdoes not allow this anymore, which is a good thing.We need to delay the insertion into
identityMapandentityIdentifiersafter the actual insert to ensure that we have a complete primary key, as we do with post-insert ID generators.This kinda worked in 2.5, but in the end
identityMapandentityIdentifierscontained wrong values anyway.