-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Fix @SequenceGeneratorDefinition
inheritance, take 1
#11050
Merged
Merged
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
#10927 reported that #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 #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 #10455 break this? When I implemented #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 #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 #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 #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 #10455 to indicate that the new driver mode also affects sequence generator definitions. The `GH1927Test` 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.
a239626
to
e3a360d
Compare
This was referenced Nov 6, 2023
Closed
mpdude
added a commit
to mpdude/doctrine2
that referenced
this pull request
Nov 7, 2023
This is an alternative implementation to doctrine#11050. The difference is that with this PR here, once `@SequenceGeneratorDefinition` is used in an inheritance tree of entities and/or mapped superclasses, this definition (and in particular, the sequence name) will be inherited by all child entity/mapped superclasses. Before doctrine#10455, the rules how `@SequenceGeneratorDefinition` is inherited from parent entity/mapped superclasses were as follows: * 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. But, this has to be considered together with the quirky mapping driver behaviour that was deprecated in doctrine#10455: The mapping driver would not report public and protected fields from mapped superclasses, so these were virtually "pushed down" to the next entity classes. That means `@SequenceGeneratorDefinition` on mapped superclasses would, in fact, be effective as-is for inheriting entity classes. This is what was covered by the tests in `BasicInheritanceMappingTest` that I marked with `@group doctrineGH-10927`. My guess is that this PR will make it possible to opt-in to `reportFieldsWhereDeclared` (see doctrine#10455) and still get the same behaviour for mapped superclasses using `@SequenceGeneratorDefinition` as before. But maybe I don't see the full picture and all edge cases, so 👀 requested. 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.
mpdude
added a commit
to mpdude/doctrine2
that referenced
this pull request
Nov 7, 2023
This is an alternative implementation to doctrine#11050. The difference is that with this PR here, once `@SequenceGeneratorDefinition` is used in an inheritance tree of entities and/or mapped superclasses, this definition (and in particular, the sequence name) will be inherited by all child entity/mapped superclasses. Before doctrine#10455, the rules how `@SequenceGeneratorDefinition` is inherited from parent entity/mapped superclasses were as follows: * 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. But, this has to be considered together with the quirky mapping driver behaviour that was deprecated in doctrine#10455: The mapping driver would not report public and protected fields from mapped superclasses, so these were virtually "pushed down" to the next entity classes. That means `@SequenceGeneratorDefinition` on mapped superclasses would, in fact, be effective as-is for inheriting entity classes. This is what was covered by the tests in `BasicInheritanceMappingTest` that I marked with `@group doctrineGH-10927`. My guess is that this PR will make it possible to opt-in to `reportFieldsWhereDeclared` (see doctrine#10455) and still get the same behaviour for mapped superclasses using `@SequenceGeneratorDefinition` as before. But maybe I don't see the full picture and all edge cases, so 👀 requested. 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. # Please enter the commit message for your changes. Lines starting # with '#' will be kept; you may remove them yourself if you want to. # An empty message aborts the commit. # # Date: Mon Nov 6 23:01:25 2023 +0100 # # On branch fix-10927-take2 # Your branch is up to date with 'mpdude/fix-10927-take2'. # # Changes to be committed: # modified: lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php # modified: lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php # modified: phpstan-baseline.neon # new file: tests/Doctrine/Tests/ORM/Functional/Ticket/GH10927Test.php # modified: tests/Doctrine/Tests/ORM/Mapping/BasicInheritanceMappingTest.php # # Untracked files: # phpunit.xml # tests/Doctrine/Tests/ORM/Functional/Ticket/GH11040Test.php #
@beberlei you basically agreed to this, right? |
beberlei
approved these changes
Jan 12, 2024
Is it possible that this adjustment changed the way of autoincrement integer ids? Got problems with my fixtures in this version (my snapshottests failing after every generation). If a use 2.17.2 it works well only in 2.17.3 i got random id data. I use mariadb! |
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.
#10927 reported that #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 #10455 when entity inheritance comes into play:@GeneratedValue
) from their parent classes@SequenceGeneratorDefinition
, however, is not generally inheritedWhy did #10455 break this?
When I implemented #10455, I was mislead by two tests
BasicInheritanceMappingTest::testGeneratedValueFromMappedSuperclass
andBasicInheritanceMappingTest::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 #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 #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 #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 #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 the2.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 #10927. There is another implementation with slightly different inheritance semantics in #11052, in case the fix is not good enough and we'd need to review the topic later on.