Skip to content

Commit

Permalink
Fix regression with an edge case for the `CMF::peekIfIsMappedSupercla…
Browse files Browse the repository at this point in the history
…ss()` method

[This comment](doctrine#10473 (comment)) hints to a case where the `ClassMetadataFactory::peekIfIsMappedSuperclass()` method introduced in doctrine#10411 causes a failure.

`CMF::peekIfIsMappedSuperclass()` has to perform improvised metadata loading in a situation where the CMF is currently loading a class. So, we cannot use the full/real `ClassMetadataFactory` mechanisms, since it would require a re-entry for a subclass of the current class, causing an infinite loop (loads parent classes first, and that's what we're currently doing).

The problem is that the improvised call to `$driver->loadMetadataForClass()` cannot provide a pre-filled `ClassMetadata` instance populated with all parent class fields and associations. But, when attribute or association overrides are used, a check is made to see if the overridden field/association actually exists, and this information is missing in that situation.

This PR suggests to override the methods to get around this. In fact, we do not care about all these details, we only want to ask the driver if the class is a mapped superclass or not.

A much better fix would be to have a dedicated method on the driver to ask it just that particular question (also better performance-wise). But I do not see how we could get that done in a BC way – ideas? 💡

A few things that need to come together to make the bug surface:

* Load an entity declaring an inheritance tree
* There must be a mapped superclass in the inheritance tree to provide the field that shall be overriden
* An entity class must inherit from the mapped superclass and override the field
* That entity class must be an abstract, intermediate class not be declared in the discriminator map so we can "discover" it
* The overriden property must be private so the mapping drivers (using reflection) do not see it when looking at the overriding entity class.
  • Loading branch information
mpdude committed Mar 8, 2023
1 parent 69543ba commit 72a8f3e
Show file tree
Hide file tree
Showing 4 changed files with 110 additions and 1 deletion.
35 changes: 35 additions & 0 deletions lib/Doctrine/ORM/Internal/OverrideDisregardingClassMetadata.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
<?php

declare(strict_types=1);

namespace Doctrine\ORM\Internal;

use Doctrine\ORM\Mapping\ClassMetadata;

/**
* @internal
*
* @psalm-internal Doctrine\ORM\Mapping
* @template T of object
* @template-extends ClassMetadata<T>
*/
class OverrideDisregardingClassMetadata extends ClassMetadata
{
/**
* @param string $fieldName
* @psalm-param array<string, mixed> $overrideMapping
*/
public function setAttributeOverride($fieldName, array $overrideMapping): void
{
// override to ignore consistency checks not relevant in this use case
}

/**
* @param string $fieldName
* @psalm-param array<string, mixed> $overrideMapping
*/
public function setAssociationOverride($fieldName, array $overrideMapping): void
{
// override to ignore consistency checks not relevant in this use case
}
}
8 changes: 7 additions & 1 deletion lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
use Doctrine\ORM\Id\IdentityGenerator;
use Doctrine\ORM\Id\SequenceGenerator;
use Doctrine\ORM\Id\UuidGenerator;
use Doctrine\ORM\Internal\OverrideDisregardingClassMetadata;
use Doctrine\ORM\Mapping\Exception\CannotGenerateIds;
use Doctrine\ORM\Mapping\Exception\InvalidCustomGenerator;
use Doctrine\ORM\Mapping\Exception\UnknownGeneratorType;
Expand Down Expand Up @@ -386,8 +387,13 @@ private function findAbstractEntityClassesNotListedInDiscriminatorMap(ClassMetad
/** @param class-string $className */
private function peekIfIsMappedSuperclass(string $className): bool
{
// Code formatting like this necessary for Psalm, https://github.com/vimeo/psalm/issues/7702
$class = new OverrideDisregardingClassMetadata(
$className,
$this->em->getConfiguration()->getNamingStrategy(),
$this->em->getConfiguration()->getTypedFieldMapper()
);
$reflService = $this->getReflectionService();
$class = $this->newClassMetadataInstance($className);
$this->initializeReflection($class, $reflService);

$this->driver->loadMetadataForClass($className, $class);
Expand Down
1 change: 1 addition & 0 deletions psalm.xml
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,7 @@
</errorLevel>
<errorLevel type="suppress">
<directory name="lib/Doctrine/ORM/Query/AST" />
<file name="lib/Doctrine/ORM/Internal/OverrideDisregardingClassMetadata.php" />
</errorLevel>
</PropertyNotSetInConstructor>
<RedundantCastGivenDocblockType>
Expand Down
67 changes: 67 additions & 0 deletions tests/Doctrine/Tests/ORM/Functional/Ticket/GH10557Test.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
<?php

declare(strict_types=1);

namespace Doctrine\Tests\ORM\Functional\Ticket;

use Doctrine\ORM\Mapping as ORM;
use Doctrine\Tests\OrmTestCase;

final class GH10557Test extends OrmTestCase
{
public function testGH10557(): void
{
$this->expectNotToPerformAssertions();

$entityManager = $this->getTestEntityManager();
$entityManager->getClassMetadata(GH10557Base::class);
}
}

/**
* @ORM\Entity
* @ORM\InheritanceType("SINGLE_TABLE")
* @ORM\DiscriminatorColumn(name="discr", type="string")
* @ORM\DiscriminatorMap({"base" = "GH10557Base", "leaf" = "GH10557Leaf"})
*/
class GH10557Base
{
/**
* @ORM\Id
* @ORM\Column(type="integer")
* @ORM\GeneratedValue
*
* @var int
*/
public $id;
}

/**
* @ORM\MappedSuperclass
*/
abstract class GH10557MappedSuperclass extends GH10557Base
{
/**
* @ORM\Column(type="string")
*
* @var string
*/
private $test;
}

/**
* @ORM\Entity
* @ORM\AttributeOverrides(
* @ORM\AttributeOverride(name="test", column=@ORM\Column(name="test_override"))
* )
*/
abstract class GH10557OverrideEntity extends GH10557MappedSuperclass
{
}

/**
* @ORM\Entity
*/
class GH10557Leaf extends GH10557OverrideEntity
{
}

0 comments on commit 72a8f3e

Please sign in to comment.