Skip to content

Commit 8e9d01f

Browse files
mpdudempdude
committed
Fix @SequenceGeneratorDefinition inheritance, take 2
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 #
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)