Skip to content
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

Merge 2.18.x up into 3.0.x #11145

Merged
merged 19 commits into from
Jan 2, 2024
Merged

Merge 2.18.x up into 3.0.x #11145

merged 19 commits into from
Jan 2, 2024

Conversation

greg0ire
Copy link
Member

@greg0ire greg0ire commented Jan 2, 2024

No description provided.

norkunas and others added 19 commits December 2, 2023 11:32
…ine#7963)

Co-authored-by: flaushi <flaushi@users.noreply.github.com>
This PR changes a detail in the commit order computation for depended-upon entities.

We have a parent-child relationship between two entity classes. The association is parent one-to-many children, with the child entities containing the (owning side) back-reference.

Cascade-persist is not used, so all entities have to be passed to `EntityManager::persist()`.

Before v2.16.0, two child entities C1 and C2 will be inserted in the same order in which they are passed to `persist()`, and that is regardless of whether the parent entity was passed to `persist()` before or after the child entities.

As of v2.16.0, passing the parent entity to `persist()` _after_ the child entities will lead to an insert order that is _reversed_ compared to the order of `persist()` calls.

This PR makes the order consistent in both cases, as it was before v2.16.0.

 #### Cause

When the parent is passed to `persist()` after the children, commit order computation has to re-arrange the entities. The parent must be inserted first since it is referred to by the children.

The implementation of the topological sort from doctrine#10547 processed entities in reverse `persist()` order and unshifted finished nodes to an array to obtain the final result. That leads to dependencies (parent → before c1, parent → before c2) showing up in the result in the reverse order of which they were added.

This PR changes the topological sort to produce a result in the opposite order ("all edges pointing left"), which helps to avoid the duplicate array order reversal.

 #### Discussion

* This PR _does not_ change semantics of the `persist()` so that entities would (under all ciscumstances) be inserted in the order of `persist()` calls.
* It fixes an unnecessary inconsistency between versions before 2.16.0 and after. In particular, it may be surprising that the insert order for the child entities depends on whether another referred-to entity (the parent) was added before or after them.
* _Both_ orders (c1 before or after c2) are technically and logically correct with regard to the agreement that `commit()` is free to arrange entities in a way that allows for efficient insertion into the database.

Fixes doctrine#11058.
Avoid an inconsistency in topological sort result order
…licate

Mention in the limitations that private field names cannot be reused
Spotted while trying to merge doctrine#11076
(among other things) up into 3.0.x. On that branch, it is no longer
possible for an entity to extend another entity without specifying an
inheritance mapping type.

I think the goal of that inheritance was just to reuse the identifier
anyway, so let's just duplicate the identifier declaration instead.
@@ -118,7 +121,7 @@ public function validateClass(ClassMetadata $class): array
}

// PHP 7.4 introduces the ability to type properties, so we can't validate them in previous versions
if (PHP_VERSION_ID >= 70400) {
if (PHP_VERSION_ID >= 70400 && $this->validatePropertyTypes) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will create another PR after this one is merged to remove the PHP_VERSION_ID here and in other places.

@greg0ire greg0ire merged commit 7361d3e into doctrine:3.0.x Jan 2, 2024
65 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants