From 8e9d01fcb6680625c36b28ad0ed48fa710d7560a Mon Sep 17 00:00:00 2001 From: Matthias Pigulla Date: Mon, 6 Nov 2023 23:01:25 +0100 Subject: [PATCH] Fix `@SequenceGeneratorDefinition` inheritance, take 2 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is an alternative implementation to #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 #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 #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 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 `@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 # --- .../ORM/Mapping/ClassMetadataFactory.php | 8 +- .../ORM/Mapping/ClassMetadataInfo.php | 4 +- phpstan-baseline.neon | 6 +- .../ORM/Functional/Ticket/GH10927Test.php | 122 ++++++++++++++++++ .../Mapping/BasicInheritanceMappingTest.php | 3 + 5 files changed, 135 insertions(+), 8 deletions(-) create mode 100644 tests/Doctrine/Tests/ORM/Functional/Ticket/GH10927Test.php diff --git a/lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php b/lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php index 333da89feed..668e00b8193 100644 --- a/lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php +++ b/lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php @@ -113,7 +113,7 @@ protected function doLoadMetadata($class, $parent, $rootEntityFound, array $nonS if ($parent) { $class->setInheritanceType($parent->inheritanceType); $class->setDiscriminatorColumn($parent->discriminatorColumn); - $this->inheritIdGeneratorMapping($class, $parent); + $class->setIdGeneratorType($parent->generatorType); $this->addInheritedFields($class, $parent); $this->addInheritedRelations($class, $parent); $this->addInheritedEmbeddedClasses($class, $parent); @@ -141,8 +141,9 @@ protected function doLoadMetadata($class, $parent, $rootEntityFound, array $nonS throw MappingException::reflectionFailure($class->getName(), $e); } - // Complete id generator mapping when the generator was declared/added in this class - if ($class->identifier && (! $parent || ! $parent->identifier)) { + if ($parent && ($rootEntityFound || ($parent->sequenceGeneratorDefinition && ! isset($parent->sequenceGeneratorDefinition['implicit'])))) { + $this->inheritIdGeneratorMapping($class, $parent); + } else { $this->completeIdGeneratorMapping($class); } @@ -677,6 +678,7 @@ private function completeIdGeneratorMapping(ClassMetadataInfo $class): void 'sequenceName' => $this->truncateSequenceName($sequenceName), 'allocationSize' => 1, 'initialValue' => 1, + 'implicit' => true, ]; if ($quoted) { diff --git a/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php b/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php index 9d8f27cd1a8..e7518f2533d 100644 --- a/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php +++ b/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php @@ -721,7 +721,7 @@ class ClassMetadataInfo implements ClassMetadata * * * @var array|null - * @psalm-var array{sequenceName: string, allocationSize: string, initialValue: string, quoted?: mixed}|null + * @psalm-var array{sequenceName: string, allocationSize: string, initialValue: string, quoted?: mixed, implicit?: bool}|null * @todo Merge with tableGeneratorDefinition into generic generatorDefinition */ public $sequenceGeneratorDefinition; @@ -3433,7 +3433,7 @@ public function setCustomGeneratorDefinition(array $definition) * ) * * - * @psalm-param array{sequenceName?: string, allocationSize?: int|string, initialValue?: int|string, quoted?: mixed} $definition + * @psalm-param array{sequenceName?: string, allocationSize?: int|string, initialValue?: int|string, quoted?: mixed, implicit?: bool} $definition * * @return void * diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index f85a4fc74cd..101792c1b0b 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -536,12 +536,12 @@ parameters: path: lib/Doctrine/ORM/Tools/Console/Command/MappingDescribeCommand.php - - message: "#^Offset 'allocationSize' on array\\{sequenceName\\: string, allocationSize\\: string, initialValue\\: string, quoted\\?\\: mixed\\} in isset\\(\\) always exists and is not nullable\\.$#" + message: "#^Offset 'allocationSize' on array\\{sequenceName\\: string, allocationSize\\: string, initialValue\\: string, quoted\\?\\: mixed, implicit\\?\\: bool\\} in isset\\(\\) always exists and is not nullable\\.$#" count: 1 path: lib/Doctrine/ORM/Tools/EntityGenerator.php - - message: "#^Offset 'initialValue' on array\\{sequenceName\\: string, allocationSize\\: string, initialValue\\: string, quoted\\?\\: mixed\\} in isset\\(\\) always exists and is not nullable\\.$#" + message: "#^Offset 'initialValue' on array\\{sequenceName\\: string, allocationSize\\: string, initialValue\\: string, quoted\\?\\: mixed, implicit\\?\\: bool\\} in isset\\(\\) always exists and is not nullable\\.$#" count: 1 path: lib/Doctrine/ORM/Tools/EntityGenerator.php @@ -551,7 +551,7 @@ parameters: path: lib/Doctrine/ORM/Tools/EntityGenerator.php - - message: "#^Offset 'sequenceName' on array\\{sequenceName\\: string, allocationSize\\: string, initialValue\\: string, quoted\\?\\: mixed\\} in isset\\(\\) always exists and is not nullable\\.$#" + message: "#^Offset 'sequenceName' on array\\{sequenceName\\: string, allocationSize\\: string, initialValue\\: string, quoted\\?\\: mixed, implicit\\?\\: bool\\} in isset\\(\\) always exists and is not nullable\\.$#" count: 1 path: lib/Doctrine/ORM/Tools/EntityGenerator.php diff --git a/tests/Doctrine/Tests/ORM/Functional/Ticket/GH10927Test.php b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH10927Test.php new file mode 100644 index 00000000000..5675674e512 --- /dev/null +++ b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH10927Test.php @@ -0,0 +1,122 @@ +_em->getConnection()->getDatabasePlatform(); + if (! $platform instanceof PostgreSQLPlatform) { + self::markTestSkipped('The ' . self::class . ' requires the use of postgresql.'); + } + + $this->setUpEntitySchema([ + GH10927RootMappedSuperclass::class, + GH10927InheritedMappedSuperclass::class, + GH10927EntityA::class, + GH10927EntityB::class, + GH10927EntityC::class, + ]); + } + + public function testSequenceGeneratorDefinitionForRootMappedSuperclass(): void + { + $metadata = $this->_em->getClassMetadata(GH10927RootMappedSuperclass::class); + + self::assertNull($metadata->sequenceGeneratorDefinition); + } + + public function testSequenceGeneratorDefinitionForEntityA(): void + { + $metadata = $this->_em->getClassMetadata(GH10927EntityA::class); + + self::assertSame('GH10927EntityA_id_seq', $metadata->sequenceGeneratorDefinition['sequenceName']); + } + + public function testSequenceGeneratorDefinitionForInheritedMappedSuperclass(): void + { + $metadata = $this->_em->getClassMetadata(GH10927InheritedMappedSuperclass::class); + + self::assertSame('GH10927InheritedMappedSuperclass_id_seq', $metadata->sequenceGeneratorDefinition['sequenceName']); + } + + public function testSequenceGeneratorDefinitionForEntityB(): void + { + $metadata = $this->_em->getClassMetadata(GH10927EntityB::class); + + self::assertSame('GH10927EntityB_id_seq', $metadata->sequenceGeneratorDefinition['sequenceName']); + } + + public function testSequenceGeneratorDefinitionForEntityC(): void + { + $metadata = $this->_em->getClassMetadata(GH10927EntityC::class); + + self::assertSame('GH10927EntityB_id_seq', $metadata->sequenceGeneratorDefinition['sequenceName']); + } +} + +/** + * @ORM\MappedSuperclass() + */ +class GH10927RootMappedSuperclass +{ +} + +/** + * @ORM\Entity() + */ +class GH10927EntityA extends GH10927RootMappedSuperclass +{ + /** + * @ORM\Id + * @ORM\GeneratedValue(strategy="SEQUENCE") + * @ORM\Column(type="integer") + * + * @var int|null + */ + private $id = null; +} + +/** + * @ORM\MappedSuperclass() + */ +class GH10927InheritedMappedSuperclass extends GH10927RootMappedSuperclass +{ + /** + * @ORM\Id + * @ORM\GeneratedValue(strategy="SEQUENCE") + * @ORM\Column(type="integer") + * + * @var int|null + */ + private $id = null; +} + +/** + * @ORM\Entity() + * @ORM\InheritanceType("JOINED") + * @ORM\DiscriminatorColumn(name="discr", type="string") + * @ORM\DiscriminatorMap({"B" = "GH10927EntityB", "C" = "GH10927EntityC"}) + */ +class GH10927EntityB extends GH10927InheritedMappedSuperclass +{ +} + +/** + * @ORM\Entity() + */ +class GH10927EntityC extends GH10927EntityB +{ +} diff --git a/tests/Doctrine/Tests/ORM/Mapping/BasicInheritanceMappingTest.php b/tests/Doctrine/Tests/ORM/Mapping/BasicInheritanceMappingTest.php index f7b2c97b620..9b19153ae1b 100644 --- a/tests/Doctrine/Tests/ORM/Mapping/BasicInheritanceMappingTest.php +++ b/tests/Doctrine/Tests/ORM/Mapping/BasicInheritanceMappingTest.php @@ -163,6 +163,7 @@ public function testMappedSuperclassWithId(): void /** * @group DDC-1156 * @group DDC-1218 + * @group GH-10927 */ public function testGeneratedValueFromMappedSuperclass(): void { @@ -179,6 +180,7 @@ public function testGeneratedValueFromMappedSuperclass(): void /** * @group DDC-1156 * @group DDC-1218 + * @group GH-10927 */ public function testSequenceDefinitionInHierarchyWithSandwichMappedSuperclass(): void { @@ -195,6 +197,7 @@ public function testSequenceDefinitionInHierarchyWithSandwichMappedSuperclass(): /** * @group DDC-1156 * @group DDC-1218 + * @group GH-10927 */ public function testMultipleMappedSuperclasses(): void {