Fix #7877 #7882
Fix #7877 #7882
Conversation
| } else { | ||
| $identifiers = $this->class->getIdentifier(); | ||
| // Only single-column identifiers are supported | ||
| if (1 === count($identifiers) && isset($entityChangeSet[$identifiers[0]])) { |
There was a problem hiding this comment.
There should be an explicit method to ask if the identifier is single-valued? Otherwise, I'd suggest using the identifier flattener (it's a class lying around in internals)
There was a problem hiding this comment.
@Ocramius I don't know about such method, and I would gladly implement it if you point me in the right direction
There was a problem hiding this comment.
See https://github.com/doctrine/orm/blob/d528f7056886fa2ea114b3179946cc6a5e8d08ee/lib/Doctrine/ORM/Utility/IdentifierFlattener.php - it should produce a hash of the identifier
There was a problem hiding this comment.
@Ocramius actually the identifier flattener returns an array too, not a hash as string.
And I've not found any method to ask if the identifier is single-valued.
tests/Doctrine/Tests/Models/Issue7877/Issue7877ApplicationGenerated.php
Outdated
Show resolved
Hide resolved
|
Please improve your commit message according to the contributing guide. |
sylfabre
force-pushed
the
fix_7877
branch
6 times, most recently
from
2fadbae to
400383f
Compare
|
@greg0ire Which coding standard do you use with PHPCS? I see on Travis that it is checked with So I'm expecting the default one of PHPCS but I don't get the same violations when I run it. I've looked on https://www.doctrine-project.org/contribute/index.html but I cannot find any info about the coding standard. Thanks! |
|
We use https://github.com/doctrine/coding-standard |
sylfabre
force-pushed
the
fix_7877
branch
8 times, most recently
from
1ca3670 to
469d49b
Compare
sylfabre
force-pushed
the
fix_7877
branch
3 times, most recently
from
2e94ca0 to
efc8a15
Compare
|
Documented the SQLite requirement at #10734 |
sylfabre
force-pushed
the
fix_7877
branch
3 times, most recently
from
77ded32 to
c2d12c4
Compare
|
@greg0ire I think we are good to go now. A test was mis-prefixed ( |
|
Is my understanding correct that you want to optimize only for the case that you have a self-referencing entity with application-generated IDs? In that case, avoid scheduling the extra update, since the ID is available at insert time? |
|
The following patch seems to work as well. In general, I prefer dedicated methods like the one you introduced. In this case, I somehow find it easier to see what the optimization is about. WDYT? By the way, do you see any cases apart from self-referencing entities where the extra update might be unnecessary? --- a/lib/Doctrine/ORM/Persisters/Entity/BasicEntityPersister.php
+++ b/lib/Doctrine/ORM/Persisters/Entity/BasicEntityPersister.php
@@ -680,12 +680,16 @@ class BasicEntityPersister implements EntityPersister
$oid = spl_object_id($newVal);
if (isset($this->queuedInserts[$oid]) || $uow->isScheduledForInsert($newVal)) {
- // The associated entity $newVal is not yet persisted, so we must
- // set $newVal = null, in order to insert a null value and schedule an
- // extra update on the UnitOfWork.
- $uow->scheduleExtraUpdate($entity, [$field => [null, $newVal]]);
-
- $newVal = null;
+ // Optimization: Self-referencing entities with application-provided IDs
+ // do not need extra updates
+ if (!($newVal === $entity && $this->class->getIdentifierValues($newVal))) {
+ // The associated entity $newVal is not yet persisted, so we must
+ // set $newVal = null, in order to insert a null value and schedule an
+ // extra update on the UnitOfWork.
+ $uow->scheduleExtraUpdate($entity, [$field => [null, $newVal]]);
+
+ $newVal = null;
+ }
}
}
|
|
Hello @mpdude Thank you for your review You correctly identified the corner case I'm targeting here. I implemented the change you suggested: it is indeed easier to understand 👍 |
|
Following-up on #7877 (comment), this PR still needs some work |
…hen using application-provided IDs The change makes the `BasicEntityPersister` not schedule an extra update in the case of an entity referencing itself and having an application-provided ID (the "NONE" generator strategy). While it looks like a special corner case, the INSERTion of a NULL value plus the extra update require the self-referencing column to be defined as NULLable. This is only an issue for the self-referencing entity case. All other associations work naturally. Fixes doctrine#7877, closes doctrine#7882. Co-authored-by: Sylvain Fabre <sylvain.fabre@assoconnect.com>
|
@mpdude Alright, I'm trusting your judgment on this one as I'm not a Doctrine expert! Are you taking the lead with your other PR? If yes, I may close mine |
…es for association-target entities that use application-provided IDs (the "NONE" generator strategy). For this generator strategy, we may assume that IDs are already present at the time the insert is executed. The benefit is that now the join column can be defined as not-NULLable, since the ORM no longer uses temporary NULL values. However, care must be taken: Extra updates are not only about the question of having IDs for the associated entities available, but also about having those entities in the database already (to satisfy foreign key constaints). The latter will become easier with doctrine#10547, since the UoW will take care of that detail when computing the commit order. But without that detail, we might break use cases that currently work – that's cases where the extra update gives us the extra time to insert the referred-to entity first. Fixes doctrine#7877, closes doctrine#7882. Co-authored-by: Sylvain Fabre <sylvain.fabre@assoconnect.com>
|
As the tests in #10735 show, only considering the self-referencing entity case is not enough. We also have to avoid extra updates for associations between different entities. The extra update is not only about knowing database-provided IDs, but also about making sure that the depended-upon entity is already in the database so we can make a foreign key reference to it. That, in turn, requires that the commit order is good enough to insert depended-upon entities first. The current implementation is not, at least not for references between entities of the same class. In addition to that, the Looking at IDs is not sufficient, since for application-provided IDs, they are set even though the insert is still pending. |
…hen using application-provided IDs This change improves scheduling of extra updates in the `BasicEntityPersister`. Extra updates can be avoided when * the referred-to entity has already been inserted during the current insert batch/transaction * we have a self-referencing entity with application-provided ID values (the `NONE` generator strategy). As a corollary, with this change applications that provide their own IDs can define self-referencing associations as not NULLable. I am considering this a bugfix since the ORM previously executed additional queries that were not strictly necessary, and that required users to work with NULLable columns where conceptually a non-NULLable column would be valid and more expressive. One caveat, though: In the absence of entity-level commit ordering (doctrine#10547), it is not guaranteed that entities with self-references (at the class level) will be inserted in a suitable order. The order depends on the sequence in which the entities were added with `persist()`. Fixes doctrine#7877, closes doctrine#7882. Co-authored-by: Sylvain Fabre <sylvain.fabre@assoconnect.com>
…hen using application-provided IDs This change improves scheduling of extra updates in the `BasicEntityPersister`. Extra updates can be avoided when * the referred-to entity has already been inserted during the current insert batch/transaction * we have a self-referencing entity with application-provided ID values (the `NONE` generator strategy). As a corollary, with this change applications that provide their own IDs can define self-referencing associations as not NULLable. I am considering this a bugfix since the ORM previously executed additional queries that were not strictly necessary, and that required users to work with NULLable columns where conceptually a non-NULLable column would be valid and more expressive. One caveat, though: In the absence of entity-level commit ordering (doctrine#10547), it is not guaranteed that entities with self-references (at the class level) will be inserted in a suitable order. The order depends on the sequence in which the entities were added with `persist()`. Fixes doctrine#7877, closes doctrine#7882. Co-authored-by: Sylvain Fabre <sylvain.fabre@assoconnect.com>
…hen using application-provided IDs This change improves scheduling of extra updates in the `BasicEntityPersister`. Extra updates can be avoided when * the referred-to entity has already been inserted during the current insert batch/transaction * we have a self-referencing entity with application-provided ID values (the `NONE` generator strategy). As a corollary, with this change applications that provide their own IDs can define self-referencing associations as not NULLable. I am considering this a bugfix since the ORM previously executed additional queries that were not strictly necessary, and that required users to work with NULLable columns where conceptually a non-NULLable column would be valid and more expressive. One caveat, though: In the absence of entity-level commit ordering (doctrine#10547), it is not guaranteed that entities with self-references (at the class level) will be inserted in a suitable order. The order depends on the sequence in which the entities were added with `persist()`. Fixes doctrine#7877, closes doctrine#7882. Co-authored-by: Sylvain Fabre <sylvain.fabre@assoconnect.com>
…hen using application-provided IDs This change improves scheduling of extra updates in the `BasicEntityPersister`. Extra updates can be avoided when * the referred-to entity has already been inserted during the current insert batch/transaction * we have a self-referencing entity with application-provided ID values (the `NONE` generator strategy). As a corollary, with this change applications that provide their own IDs can define self-referencing associations as not NULLable. I am considering this a bugfix since the ORM previously executed additional queries that were not strictly necessary, and that required users to work with NULLable columns where conceptually a non-NULLable column would be valid and more expressive. One caveat, though: In the absence of entity-level commit ordering (doctrine#10547), it is not guaranteed that entities with self-references (at the class level) will be inserted in a suitable order. The order depends on the sequence in which the entities were added with `persist()`. Fixes doctrine#7877, closes doctrine#7882. Co-authored-by: Sylvain Fabre <sylvain.fabre@assoconnect.com>
…hen using application-provided IDs This change improves scheduling of extra updates in the `BasicEntityPersister`. Extra updates can be avoided when * the referred-to entity has already been inserted during the current insert batch/transaction * we have a self-referencing entity with application-provided ID values (the `NONE` generator strategy). As a corollary, with this change applications that provide their own IDs can define self-referencing associations as not NULLable. I am considering this a bugfix since the ORM previously executed additional queries that were not strictly necessary, and that required users to work with NULLable columns where conceptually a non-NULLable column would be valid and more expressive. One caveat, though: In the absence of entity-level commit ordering (doctrine#10547), it is not guaranteed that entities with self-references (at the class level) will be inserted in a suitable order. The order depends on the sequence in which the entities were added with `persist()`. Fixes doctrine#7877, closes doctrine#7882. Co-authored-by: Sylvain Fabre <sylvain.fabre@assoconnect.com>
…hen using application-provided IDs (#10735) This change improves scheduling of extra updates in the `BasicEntityPersister`. Extra updates can be avoided when * the referred-to entity has already been inserted during the current insert batch/transaction * we have a self-referencing entity with application-provided ID values (the `NONE` generator strategy). As a corollary, with this change applications that provide their own IDs can define self-referencing associations as not NULLable. I am considering this a bugfix since the ORM previously executed additional queries that were not strictly necessary, and that required users to work with NULLable columns where conceptually a non-NULLable column would be valid and more expressive. One caveat, though: In the absence of entity-level commit ordering (#10547), it is not guaranteed that entities with self-references (at the class level) will be inserted in a suitable order. The order depends on the sequence in which the entities were added with `persist()`. Fixes #7877, closes #7882. Co-authored-by: Sylvain Fabre <sylvain.fabre@assoconnect.com>
|
Abandoned in favor of #10735 |
Fixes #7877