Merge 2.18.x up into 3.0.x #11166
Merged
Merge 2.18.x up into 3.0.x #11166
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This test show that eager collections are broken when used in conjuction with iterating over a result.
…ry collections (doctrine#11146) ... plus minor tweaks.
…n-iterable Do not defer eager collection loading when in iteration context
…ion (doctrine#10913) In order to resolve doctrine#10348, some changes were included in doctrine#10547 to improve the computed _delete_ order for entities. One assumption was that foreign key references with `ON DELETE SET NULL` or `... CASCADE` need not need to be taken into consideration when planning the deletion order, since the RDBMS would unset or cascade-delete such associations by itself when necessary. Only associations that do _not_ use RDBMS-level cascade handling would be sequenced, to make sure the referring entity is deleted before the referred-to one. This assumption is wrong for `ON DELETE CASCADE`. The following examples give reasons why we need to also consider such associations, and in addition, we need to be able to deal with cycles formed by them. In the following diagrams, `odc` means `ON DELETE CASCADE`, and `ref` is a regular foreign key with no extra `ON DELETE` semantics. ```mermaid graph LR; C-->|ref| B; B-->|odc| A; ``` In this example, C must be removed before B and A. If we ignore the B->A dependency in the delete order computation, the result may not to be correct. ACB is not a working solution. ```mermaid graph LR; A-->|odc| B; B-->|odc| A; C-->|ref| B; ``` This is the situation in doctrine#10912. We have to deal with a cycle in the graph. C must be removed before A as well as B. If we ignore the B->A dependency (e.g. because we set it to "optional" to get away with the cycle), we might end up with an incorrect order ACB. ```mermaid graph LR; A-->|odc| B; B-->|odc| A; A-->|ref| C; C-->|ref| B; ``` This example has no possible remove order. But, if we treat `odc` edges as optional, A -> C -> B would wrongly be deemed suitable. ```mermaid graph LR; A-->|ref| B; B-->|odc| C; C-->|odc| B; D-->|ref| C; ``` Here, we must first remove A and D in any order; then, B and C in any order. If we treat one of the `odc` edges as optional, we might find the invalid solutions ABDC or DCAB. #### Solution implemented in this PR First, build a graph with a node for every to-be-removed entity, and edges for `ON DELETE CASCADE` associations between those entities. Then, use [Tarjan's algorithm](https://en.wikipedia.org/wiki/Tarjan%27s_strongly_connected_components_algorithm) to find strongly connected components (SCCs) in this graph. The significance of SCCs is that whenever we remove one of the entities in a SCC from the database (no matter which one), the DBMS will immediately remove _all_ the other entities of that group as well. For every SCC, pick one (arbitrary) entity from the group to represent all entities of that group. Then, build a second graph. Again we have nodes for all entities that are to be removed. This time, we insert edges for all regular (foreign key) associations and those with `ON DELETE CASCADE`. `ON DELETE SET NULL` can be left out. The edges are not added between the entities themselves, but between the entities representing the respective SCCs. Also, for all non-trivial SCCs (those containing more than a single entity), add dependency edges to indicate that all entities of the SCC shall be processed _after_ the entity representing the group. This is to make sure we do not remove a SCC inadvertedly by removing one of its entities too early. Run a topological sort on the second graph to get the actual delete order. Cycles in this second graph are a problem, there is no delete order. Fixes doctrine#10912.
doctrine#10927 reported that doctrine#10455 broke the way how the default `@SequenceGeneratorDefinition` is created and inherited by subclasses for ID columns using `@GeneratedValue(strategy="SEQUENCE")`. First, I had to understand how `@SequenceGeneratorDefinition` has been handled before doctrine#10455 when entity inheritance comes into play: * Entity and mapped superclasses inherit the ID generator type (as given by `@GeneratedValue`) from their parent classes * `@SequenceGeneratorDefinition`, however, is not generally inherited * ... instead, a default sequence generator definition is created for every class when no explicit configuration is given. In this case, sequence names are based on the current class' table name. * Once a root entity has been identified, all subclasses inherit its sequence generator definition unchanged. #### Why did doctrine#10455 break this? When I implemented doctrine#10455, I was mislead by two tests `BasicInheritanceMappingTest::testGeneratedValueFromMappedSuperclass` and `BasicInheritanceMappingTest::testMultipleMappedSuperclasses`. These tests check the sequence generator definition that is inherited by an entity class from a mapped superclass, either directly or through an additional (intermediate) mapped superclass. The tests expect the sequence generator definition on the entity _to be the same_ as on the base mapped superclass. The reason why the tests worked before was the quirky behaviour of the annotation and attribute drivers that doctrine#10455 was aiming at: The drivers did not report the `@SequenceGeneratorDefinition` on the base mapped superclass where it was actually defined. Instead, they reported this `@SequenceGeneratorDefinition` for the entity class only. This means the inheritance rules stated above did not take effect, since the ID field with the sequence generator was virtually pushed down to the entity class. In doctrine#10455, I did not realize that these failing tests had to do with the quirky and changed mapping driver behaviour. Instead, I tried to "fix" the inheritance rules by passing along the sequence generator definition unchanged once the ID column had been defined. #### Consequences of the change suggested here This PR reverts the changes made to `@SequenceGeneratorDefinition` inheritance behaviour that were done in doctrine#10455. This means that with the new "report fields where declared" driver mode (which is active in our functional tests) we can not expect the sequence generator definition to be inherited from mapped superclasses. The two test cases from `BasicInheritanceMappingTest` are removed. I will leave a notice in doctrine#10455 to indicate that the new driver mode also affects sequence generator definitions. The `GH10927Test` test case validates the sequence names generated in a few cases. In fact, I wrote this test against the `2.15.x` branch to make sure we get results that are consistent with the previous behaviour. This also means `@SequenceGeneratorDefinition` on mapped superclasses is pointless: The mapped superclass does not make use of the definition itself (it has no table), and the setting is never inherited to child classes. Fixes doctrine#10927. There is another implementation with slightly different inheritance semantics in doctrine#11052, in case the fix is not good enough and we'd need to review the topic later on.
doctrine#11135) When using `AttributeOverride` to override mapping information inherited from a parent class (a mapped superclass), make sure to keep information about where the field was originally declared. This is important for `private` fields: Without the correct `declared` information, it will lead to errors when cached mapping information is loaded, reflection wakes up and looks for the private field in the wrong class.
The "any" tags inside the definition for mapped superclasses and embeddables duplicate what is already done for entities. The other removed "any" tags are also redundant, as they duplicate what's already done inside the grandparent "choice" tag. Starting with version libxml 2.12, such redundant tags cause errors about the content model not being "determinist". Fixes doctrine#11117
Remove redundant tags
…o-2.18.x_slYTN7ur
…2.18.x_slYTN7ur Merge release 2.17.3 into 2.18.x
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
…ble-loading-test Use foreach on iterable to prevent table locks during tests
mpdude
approved these changes
Jan 18, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.