Skip to content

Commit f939045

Browse files
mpdudempdude
committed
Fix condition when ID generator setting shall have defaults added
We need to fill in the defaults only for the class where the ID definition occurs for the first (only) time.
1 parent 1a23665 commit f939045

File tree

8 files changed

+94
-76
lines changed

8 files changed

+94
-76
lines changed

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

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -138,7 +138,10 @@ protected function doLoadMetadata($class, $parent, $rootEntityFound, array $nonS
138138
throw MappingException::reflectionFailure($class->getName(), $e);
139139
}
140140

141-
$this->completeIdGeneratorMapping($class);
141+
// Complete id generator mapping when the generator was declared/added in this class
142+
if ($class->identifier && (! $parent || ! $parent->identifier)) {
143+
$this->completeIdGeneratorMapping($class);
144+
}
142145

143146
if (! $class->isMappedSuperclass) {
144147
if ($rootEntityFound && $class->isInheritanceTypeNone()) {

‎lib/Doctrine/ORM/Mapping/Driver/AnnotationDriver.php‎

Lines changed: 2 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@
3434
class AnnotationDriver extends CompatibilityAnnotationDriver
3535
{
3636
use ColocatedMappingDriver;
37+
use ReflectionBasedDriver;
3738

3839
/**
3940
* The annotation reader.
@@ -344,37 +345,7 @@ public function loadMetadataForClass($className, PersistenceClassMetadata $metad
344345

345346
// Evaluate annotations on properties/fields
346347
foreach ($class->getProperties() as $property) {
347-
// Handle the case that reflection may report properties inherited from parent classes.
348-
// When we know about the fields already (inheritance has been anticipanted in ClassMetadataFactory),
349-
// skip them.
350-
// The declaring classes may mismatch when there are private properties: The same property name may be
351-
// reported multiple times, but since it is private, it is in fact multiple (different) properties in
352-
// different classes. In that case, report the property as an individual field. (ClassMetadataFactory will
353-
// probably fail in that case, though.)
354-
355-
$declaringClass = $property->getDeclaringClass()->getName();
356-
357-
if (
358-
isset($metadata->fieldMappings[$property->name])
359-
&& isset($metadata->fieldMappings[$property->name]['declared'])
360-
&& $metadata->fieldMappings[$property->name]['declared'] === $declaringClass
361-
) {
362-
continue;
363-
}
364-
365-
if (
366-
isset($metadata->associationMappings[$property->name])
367-
&& isset($metadata->associationMappings[$property->name]['declared'])
368-
&& $metadata->associationMappings[$property->name]['declared'] === $declaringClass
369-
) {
370-
continue;
371-
}
372-
373-
if (
374-
isset($metadata->embeddedClasses[$property->name])
375-
&& isset($metadata->embeddedClasses[$property->name]['declared'])
376-
&& $metadata->embeddedClasses[$property->name]['declared'] === $declaringClass
377-
) {
348+
if ($this->isRepeatedPropertyDeclaration($property, $metadata)) {
378349
continue;
379350
}
380351

‎lib/Doctrine/ORM/Mapping/Driver/AttributeDriver.php‎

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@
3030
class AttributeDriver extends CompatibilityAnnotationDriver
3131
{
3232
use ColocatedMappingDriver;
33+
use ReflectionBasedDriver;
3334

3435
private const ENTITY_ATTRIBUTE_CLASSES = [
3536
Mapping\Entity::class => 1,
@@ -294,15 +295,8 @@ public function loadMetadataForClass($className, PersistenceClassMetadata $metad
294295

295296
foreach ($reflectionClass->getProperties() as $property) {
296297
assert($property instanceof ReflectionProperty);
297-
if (
298-
$metadata->isMappedSuperclass && ! $property->isPrivate()
299-
||
300-
$metadata->isInheritedField($property->name)
301-
||
302-
$metadata->isInheritedAssociation($property->name)
303-
||
304-
$metadata->isInheritedEmbeddedClass($property->name)
305-
) {
298+
299+
if ($this->isRepeatedPropertyDeclaration($property, $metadata)) {
306300
continue;
307301
}
308302

‎lib/Doctrine/ORM/Mapping/Driver/ReflectionBasedDriver.php‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace Doctrine\ORM\Mapping\Driver;
6+
7+
use Doctrine\ORM\Mapping\ClassMetadata;
8+
use ReflectionProperty;
9+
10+
/** @internal */
11+
trait ReflectionBasedDriver
12+
{
13+
/**
14+
* Helps to deal with the case that reflection may report properties inherited from parent classes.
15+
* When we know about the fields already (inheritance has been anticipated in ClassMetadataFactory),
16+
* the driver must skip them.
17+
*
18+
* The declaring classes may mismatch when there are private properties: The same property name may be
19+
* reported multiple times, but since it is private, it is in fact multiple (different) properties in
20+
* different classes. In that case, report the property as an individual field. (ClassMetadataFactory will
21+
* probably fail in that case, though.)
22+
*/
23+
private function isRepeatedPropertyDeclaration(ReflectionProperty $property, ClassMetadata $metadata): bool
24+
{
25+
$declaringClass = $property->getDeclaringClass()->getName();
26+
27+
if (
28+
isset($metadata->fieldMappings[$property->name])
29+
&& isset($metadata->fieldMappings[$property->name]['declared'])
30+
&& $metadata->fieldMappings[$property->name]['declared'] === $declaringClass
31+
) {
32+
return true;
33+
}
34+
35+
if (
36+
isset($metadata->associationMappings[$property->name])
37+
&& isset($metadata->associationMappings[$property->name]['declared'])
38+
&& $metadata->associationMappings[$property->name]['declared'] === $declaringClass
39+
) {
40+
return true;
41+
}
42+
43+
return isset($metadata->embeddedClasses[$property->name])
44+
&& isset($metadata->embeddedClasses[$property->name]['declared'])
45+
&& $metadata->embeddedClasses[$property->name]['declared'] === $declaringClass;
46+
}
47+
}

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

Lines changed: 7 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -8,35 +8,22 @@
88
use Doctrine\ORM\Mapping as ORM;
99
use Doctrine\ORM\Mapping\MappingException;
1010
use Doctrine\Tests\OrmTestCase;
11-
12-
use function get_parent_class;
13-
use function method_exists;
11+
use Doctrine\Tests\PHPUnitCompatibility\ExceptionMatching;
1412

1513
class GH10449Test extends OrmTestCase
1614
{
15+
use ExceptionMatching;
16+
1717
public function testToManyAssociationOnMappedSuperclassShallBeRejected(): void
1818
{
19-
$em = $this->getTestEntityManager();
20-
$classes = [GH10449MappedSuperclass::class, GH10449Entity::class, GH10449ToManyAssociationTarget::class];
19+
$em = $this->getTestEntityManager();
2120

2221
$this->expectException(MappingException::class);
2322
$this->expectExceptionMessageMatches('/illegal to put an inverse side one-to-many or many-to-many association on mapped superclass/');
2423

25-
foreach ($classes as $class) {
26-
$cm = $em->getClassMetadata($class);
27-
}
28-
}
29-
30-
/**
31-
* Override for BC with PHPUnit <8
32-
*/
33-
public function expectExceptionMessageMatches(string $regularExpression): void
34-
{
35-
if (method_exists(get_parent_class($this), 'expectExceptionMessageMatches')) {
36-
parent::expectExceptionMessageMatches($regularExpression);
37-
} else {
38-
parent::expectExceptionMessageRegExp($regularExpression);
39-
}
24+
// Currently the ClassMetadataFactory performs the check only when loading the subclasses, this might change with
25+
// https://github.com/doctrine/orm/pull/10398
26+
$em->getClassMetadata(GH10449Entity::class);
4027
}
4128
}
4229

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

Lines changed: 3 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -7,10 +7,13 @@
77
use Doctrine\ORM\Mapping as ORM;
88
use Doctrine\ORM\Mapping\MappingException;
99
use Doctrine\Tests\OrmTestCase;
10+
use Doctrine\Tests\PHPUnitCompatibility\ExceptionMatching;
1011
use Generator;
1112

1213
class GH10454Test extends OrmTestCase
1314
{
15+
use ExceptionMatching;
16+
1417
/**
1518
* @param class-string $className
1619
*
@@ -31,18 +34,6 @@ public function classesThatOverrideFieldNames(): Generator
3134
yield 'Entity class that redeclares a protected field inherited from a base entity' => [GH10454EntityChildProtected::class];
3235
yield 'Entity class that redeclares a protected field inherited from a mapped superclass' => [GH10454MappedSuperclassChildProtected::class];
3336
}
34-
35-
/**
36-
* Override for BC with PHPUnit <8
37-
*/
38-
public function expectExceptionMessageMatches(string $regularExpression): void
39-
{
40-
if (method_exists(get_parent_class($this), 'expectExceptionMessageMatches')) {
41-
parent::expectExceptionMessageMatches($regularExpression);
42-
} else {
43-
parent::expectExceptionMessageRegExp($regularExpression);
44-
}
45-
}
4637
}
4738

4839
/**

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

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -48,14 +48,15 @@ private function classTests($className): void
4848
$this->_em->persist($child->rel);
4949
$this->_em->flush();
5050
$this->_em->clear();
51+
$id = $child->id;
5152

5253
// Test find by rel
5354
$child = $this->_em->getRepository($className)->findOneBy(['rel' => $child->rel]);
5455
self::assertNotNull($child);
5556
$this->_em->clear();
5657

5758
// Test query by id with fetch join
58-
$child = $this->_em->createQuery('SELECT t, r FROM ' . $className . ' t JOIN t.rel r WHERE t.id = 1')->getOneOrNullResult();
59+
$child = $this->_em->createQuery('SELECT t, r FROM ' . $className . ' t JOIN t.rel r WHERE t.id = ?0')->setParameter(0, $id)->getOneOrNullResult();
5960
self::assertNotNull($child);
6061

6162
// Test lock and update
@@ -65,14 +66,15 @@ private function classTests($className): void
6566
$child->status = 0;
6667
});
6768
$this->_em->clear();
68-
$child = $this->_em->getRepository($className)->find(1);
69+
$child = $this->_em->getRepository($className)->find($id);
70+
self::assertNotNull($child);
6971
self::assertEquals($child->firstName, 'Bob');
7072
self::assertEquals($child->status, 0);
7173

7274
// Test delete
7375
$this->_em->remove($child);
7476
$this->_em->flush();
75-
$child = $this->_em->getRepository($className)->find(1);
77+
$child = $this->_em->getRepository($className)->find($id);
7678
self::assertNull($child);
7779
}
7880
}

‎tests/Doctrine/Tests/PHPUnitCompatibility/ExceptionMatching.php‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace Doctrine\Tests\PHPUnitCompatibility;
6+
7+
use function get_parent_class;
8+
use function method_exists;
9+
10+
trait ExceptionMatching
11+
{
12+
/**
13+
* Override for BC with PHPUnit <8
14+
*/
15+
public function expectExceptionMessageMatches(string $regularExpression): void
16+
{
17+
if (method_exists(get_parent_class($this), 'expectExceptionMessageMatches')) {
18+
parent::expectExceptionMessageMatches($regularExpression);
19+
} else {
20+
parent::expectExceptionMessageRegExp($regularExpression);
21+
}
22+
}
23+
}

0 commit comments

Comments
 (0)