Fix @SequenceGeneratorDefinition inheritance, take 2
#11052
Closed
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
mpdude
changed the title
mpdude
force-pushed
the
fix-10927-take2
branch
from
e87126c to
6b4698e
Compare
mpdude
changed the title
@SequenceGeneratorDefinition inheritance, take 2
mpdude
force-pushed
the
fix-10927-take2
branch
from
6b4698e to
ef1b344
Compare
mpdude
force-pushed
the
fix-10927-take2
branch
from
ef1b344 to
8098da2
Compare
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 #
mpdude
force-pushed
the
fix-10927-take2
branch
from
8098da2 to
8e9d01f
Compare
|
Thanks for working on this @mpdude We use Postgres and gedmo/doctrine-extensions and we have pinned doctrine/orm to Can you describe what are the next possible steps to get this PR or the take 1 merged? |
|
@beberlei maybe you have a preference which of these two PRs is better? We can also chat/Slack call and I could try to explain the differences |
mpdude
added a commit
that referenced
this pull request
Jan 12, 2024
#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 `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 #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.
|
We've agreed to use #11050 for now. |
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.
This is an alternative implementation to #11050. The difference is that with this PR here, once
@SequenceGeneratorDefinitionis 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 #10455, the rules how
@SequenceGeneratorDefinitionis inherited from parent entity/mapped superclasses were as follows:@GeneratedValue) from their parent classes@SequenceGeneratorDefinition, however, is not generally inheritedBut, this has to be considered together with the quirky mapping driver behaviour that was deprecated in #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
@SequenceGeneratorDefinitionon mapped superclasses would, in fact, be effective as-is for inheriting entity classes. This is what was covered by the tests inBasicInheritanceMappingTestthat I marked with@group GH-10927.My guess is that this PR will make it possible to opt-in to
reportFieldsWhereDeclared(see #10455) and still get the same behaviour for mapped superclasses using@SequenceGeneratorDefinitionas before. But maybe I don't see the full picture and all edge cases, so 👀 requested.The
GH10927Testtest case validates the sequence names generated in a few cases. In fact, I wrote this test against the2.15.xbranch to make sure we get results that are consistent with the previous behaviour.