Skip to content

Commit 8e9d01f

Browse files
mpdudempdude
committed
Fix @SequenceGeneratorDefinition inheritance, take 2
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 #
1 parent 609647a commit 8e9d01f

File tree

5 files changed

+135
-8
lines changed

5 files changed

+135
-8
lines changed

‎lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -113,7 +113,7 @@ protected function doLoadMetadata($class, $parent, $rootEntityFound, array $nonS
113113
if ($parent) {
114114
$class->setInheritanceType($parent->inheritanceType);
115115
$class->setDiscriminatorColumn($parent->discriminatorColumn);
116-
$this->inheritIdGeneratorMapping($class, $parent);
116+
$class->setIdGeneratorType($parent->generatorType);
117117
$this->addInheritedFields($class, $parent);
118118
$this->addInheritedRelations($class, $parent);
119119
$this->addInheritedEmbeddedClasses($class, $parent);
@@ -141,8 +141,9 @@ protected function doLoadMetadata($class, $parent, $rootEntityFound, array $nonS
141141
throw MappingException::reflectionFailure($class->getName(), $e);
142142
}
143143

144-
// Complete id generator mapping when the generator was declared/added in this class
145-
if ($class->identifier && (! $parent || ! $parent->identifier)) {
144+
if ($parent && ($rootEntityFound || ($parent->sequenceGeneratorDefinition && ! isset($parent->sequenceGeneratorDefinition['implicit'])))) {
145+
$this->inheritIdGeneratorMapping($class, $parent);
146+
} else {
146147
$this->completeIdGeneratorMapping($class);
147148
}
148149

@@ -677,6 +678,7 @@ private function completeIdGeneratorMapping(ClassMetadataInfo $class): void
677678
'sequenceName' => $this->truncateSequenceName($sequenceName),
678679
'allocationSize' => 1,
679680
'initialValue' => 1,
681+
'implicit' => true,
680682
];
681683

682684
if ($quoted) {

‎lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -721,7 +721,7 @@ class ClassMetadataInfo implements ClassMetadata
721721
* </code>
722722
*
723723
* @var array<string, mixed>|null
724-
* @psalm-var array{sequenceName: string, allocationSize: string, initialValue: string, quoted?: mixed}|null
724+
* @psalm-var array{sequenceName: string, allocationSize: string, initialValue: string, quoted?: mixed, implicit?: bool}|null
725725
* @todo Merge with tableGeneratorDefinition into generic generatorDefinition
726726
*/
727727
public $sequenceGeneratorDefinition;
@@ -3433,7 +3433,7 @@ public function setCustomGeneratorDefinition(array $definition)
34333433
* )
34343434
* </code>
34353435
*
3436-
* @psalm-param array{sequenceName?: string, allocationSize?: int|string, initialValue?: int|string, quoted?: mixed} $definition
3436+
* @psalm-param array{sequenceName?: string, allocationSize?: int|string, initialValue?: int|string, quoted?: mixed, implicit?: bool} $definition
34373437
*
34383438
* @return void
34393439
*

‎phpstan-baseline.neon

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -536,12 +536,12 @@ parameters:
536536
path: lib/Doctrine/ORM/Tools/Console/Command/MappingDescribeCommand.php
537537

538538
-
539-
message: "#^Offset 'allocationSize' on array\\{sequenceName\\: string, allocationSize\\: string, initialValue\\: string, quoted\\?\\: mixed\\} in isset\\(\\) always exists and is not nullable\\.$#"
539+
message: "#^Offset 'allocationSize' on array\\{sequenceName\\: string, allocationSize\\: string, initialValue\\: string, quoted\\?\\: mixed, implicit\\?\\: bool\\} in isset\\(\\) always exists and is not nullable\\.$#"
540540
count: 1
541541
path: lib/Doctrine/ORM/Tools/EntityGenerator.php
542542

543543
-
544-
message: "#^Offset 'initialValue' on array\\{sequenceName\\: string, allocationSize\\: string, initialValue\\: string, quoted\\?\\: mixed\\} in isset\\(\\) always exists and is not nullable\\.$#"
544+
message: "#^Offset 'initialValue' on array\\{sequenceName\\: string, allocationSize\\: string, initialValue\\: string, quoted\\?\\: mixed, implicit\\?\\: bool\\} in isset\\(\\) always exists and is not nullable\\.$#"
545545
count: 1
546546
path: lib/Doctrine/ORM/Tools/EntityGenerator.php
547547

@@ -551,7 +551,7 @@ parameters:
551551
path: lib/Doctrine/ORM/Tools/EntityGenerator.php
552552

553553
-
554-
message: "#^Offset 'sequenceName' on array\\{sequenceName\\: string, allocationSize\\: string, initialValue\\: string, quoted\\?\\: mixed\\} in isset\\(\\) always exists and is not nullable\\.$#"
554+
message: "#^Offset 'sequenceName' on array\\{sequenceName\\: string, allocationSize\\: string, initialValue\\: string, quoted\\?\\: mixed, implicit\\?\\: bool\\} in isset\\(\\) always exists and is not nullable\\.$#"
555555
count: 1
556556
path: lib/Doctrine/ORM/Tools/EntityGenerator.php
557557

‎tests/Doctrine/Tests/ORM/Functional/Ticket/GH10927Test.php

Lines changed: 122 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,122 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace Doctrine\Tests\ORM\Functional\Ticket;
6+
7+
use Doctrine\DBAL\Platforms\PostgreSQLPlatform;
8+
use Doctrine\ORM\Mapping as ORM;
9+
use Doctrine\Tests\OrmFunctionalTestCase;
10+
11+
/**
12+
* @group GH-10927
13+
*/
14+
class GH10927Test extends OrmFunctionalTestCase
15+
{
16+
protected function setUp(): void
17+
{
18+
parent::setUp();
19+
20+
$platform = $this->_em->getConnection()->getDatabasePlatform();
21+
if (! $platform instanceof PostgreSQLPlatform) {
22+
self::markTestSkipped('The ' . self::class . ' requires the use of postgresql.');
23+
}
24+
25+
$this->setUpEntitySchema([
26+
GH10927RootMappedSuperclass::class,
27+
GH10927InheritedMappedSuperclass::class,
28+
GH10927EntityA::class,
29+
GH10927EntityB::class,
30+
GH10927EntityC::class,
31+
]);
32+
}
33+
34+
public function testSequenceGeneratorDefinitionForRootMappedSuperclass(): void
35+
{
36+
$metadata = $this->_em->getClassMetadata(GH10927RootMappedSuperclass::class);
37+
38+
self::assertNull($metadata->sequenceGeneratorDefinition);
39+
}
40+
41+
public function testSequenceGeneratorDefinitionForEntityA(): void
42+
{
43+
$metadata = $this->_em->getClassMetadata(GH10927EntityA::class);
44+
45+
self::assertSame('GH10927EntityA_id_seq', $metadata->sequenceGeneratorDefinition['sequenceName']);
46+
}
47+
48+
public function testSequenceGeneratorDefinitionForInheritedMappedSuperclass(): void
49+
{
50+
$metadata = $this->_em->getClassMetadata(GH10927InheritedMappedSuperclass::class);
51+
52+
self::assertSame('GH10927InheritedMappedSuperclass_id_seq', $metadata->sequenceGeneratorDefinition['sequenceName']);
53+
}
54+
55+
public function testSequenceGeneratorDefinitionForEntityB(): void
56+
{
57+
$metadata = $this->_em->getClassMetadata(GH10927EntityB::class);
58+
59+
self::assertSame('GH10927EntityB_id_seq', $metadata->sequenceGeneratorDefinition['sequenceName']);
60+
}
61+
62+
public function testSequenceGeneratorDefinitionForEntityC(): void
63+
{
64+
$metadata = $this->_em->getClassMetadata(GH10927EntityC::class);
65+
66+
self::assertSame('GH10927EntityB_id_seq', $metadata->sequenceGeneratorDefinition['sequenceName']);
67+
}
68+
}
69+
70+
/**
71+
* @ORM\MappedSuperclass()
72+
*/
73+
class GH10927RootMappedSuperclass
74+
{
75+
}
76+
77+
/**
78+
* @ORM\Entity()
79+
*/
80+
class GH10927EntityA extends GH10927RootMappedSuperclass
81+
{
82+
/**
83+
* @ORM\Id
84+
* @ORM\GeneratedValue(strategy="SEQUENCE")
85+
* @ORM\Column(type="integer")
86+
*
87+
* @var int|null
88+
*/
89+
private $id = null;
90+
}
91+
92+
/**
93+
* @ORM\MappedSuperclass()
94+
*/
95+
class GH10927InheritedMappedSuperclass extends GH10927RootMappedSuperclass
96+
{
97+
/**
98+
* @ORM\Id
99+
* @ORM\GeneratedValue(strategy="SEQUENCE")
100+
* @ORM\Column(type="integer")
101+
*
102+
* @var int|null
103+
*/
104+
private $id = null;
105+
}
106+
107+
/**
108+
* @ORM\Entity()
109+
* @ORM\InheritanceType("JOINED")
110+
* @ORM\DiscriminatorColumn(name="discr", type="string")
111+
* @ORM\DiscriminatorMap({"B" = "GH10927EntityB", "C" = "GH10927EntityC"})
112+
*/
113+
class GH10927EntityB extends GH10927InheritedMappedSuperclass
114+
{
115+
}
116+
117+
/**
118+
* @ORM\Entity()
119+
*/
120+
class GH10927EntityC extends GH10927EntityB
121+
{
122+
}

‎tests/Doctrine/Tests/ORM/Mapping/BasicInheritanceMappingTest.php

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -163,6 +163,7 @@ public function testMappedSuperclassWithId(): void
163163
/**
164164
* @group DDC-1156
165165
* @group DDC-1218
166+
* @group GH-10927
166167
*/
167168
public function testGeneratedValueFromMappedSuperclass(): void
168169
{
@@ -179,6 +180,7 @@ public function testGeneratedValueFromMappedSuperclass(): void
179180
/**
180181
* @group DDC-1156
181182
* @group DDC-1218
183+
* @group GH-10927
182184
*/
183185
public function testSequenceDefinitionInHierarchyWithSandwichMappedSuperclass(): void
184186
{
@@ -195,6 +197,7 @@ public function testSequenceDefinitionInHierarchyWithSandwichMappedSuperclass():
195197
/**
196198
* @group DDC-1156
197199
* @group DDC-1218
200+
* @group GH-10927
198201
*/
199202
public function testMultipleMappedSuperclasses(): void
200203
{

0 commit comments

Comments
 (0)