Skip to content

Commit

Permalink
Deprecate undeclared entity inheritance
Browse files Browse the repository at this point in the history
Inheritance has to be declared as soon as one entity class extends (directly or through middle classes) another one.

This is also pointed out in the opening comment for #8348:

> Entities are not allowed to extend from entities without an inheritence mapping relationship (Single Table or Joined Table inheritance). [...] While Doctrine so far allowed these things, they are fragile and will break on certain scenarios.

Throwing an exception in case of this misconfiguration is nothing we should do light-heartedly, given that it may surprise users in a bugfix or feature release. So, we should start with a deprecation notice  and make this an exception in 3.0. The documentation is updated accordingly at #10429.

Catching missing inheritance declarations early on is important to avoid weird errors further down the road, giving users a clear indication of the root cause.

In case you are affected by this, please understand that although things "previously worked" for you, you have been using the ORM outside of what it was designed to do. That may have worked in simple cases, but may also have caused invalid results (wrong or missing data after hydration?) that possibly went unnoticed in subtle cases.
  • Loading branch information
mpdude committed Jan 23, 2023
1 parent 7f783b5 commit 7d7dc1b
Show file tree
Hide file tree
Showing 8 changed files with 161 additions and 23 deletions.
5 changes: 5 additions & 0 deletions UPGRADE.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,11 @@ now deprecated and will be removed in 3.0.

# Upgrade to 2.14

## Deprecated undeclared entity inheritance

As soon as an entity class inherits from another entity class, inheritance has to
be declared by adding the appropriate configuration for the root entity.

## Deprecated `Doctrine\ORM\Persisters\Exception\UnrecognizedField::byName($field)` method.

Use `Doctrine\ORM\Persisters\Exception\UnrecognizedField::byFullyQualifiedName($className, $field)` instead.
Expand Down
12 changes: 12 additions & 0 deletions lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,18 @@ protected function doLoadMetadata($class, $parent, $rootEntityFound, array $nonS
}

if (! $class->isMappedSuperclass) {
if ($rootEntityFound && $class->isInheritanceTypeNone()) {
Deprecation::trigger(
'doctrine/orm',
'https://github.com/doctrine/orm/pull/10431',
"Entity class '%s' is a subclass of the root entity class '%s', but no inheritance mapping type was declared. This is a misconfiguration and will be an error in Doctrine ORM 3.0.",
$class->name,
end($nonSuperclassParents)
);
// enable this in 3.0
// throw MappingException::missingInheritanceTypeDeclaration(end($nonSuperclassParents), $class->name);
}

foreach ($class->embeddedClasses as $property => $embeddableClass) {
if (isset($embeddableClass['inherited'])) {
continue;
Expand Down
13 changes: 13 additions & 0 deletions lib/Doctrine/ORM/Mapping/MappingException.php
Original file line number Diff line number Diff line change
Expand Up @@ -539,6 +539,19 @@ static function ($a, $b) {
);
}

/**
* @param class-string $rootEntityClass
* @param class-string $childEntityClass
*/
//public static function missingInheritanceTypeDeclaration(string $rootEntityClass, string $childEntityClass): self
//{
// return new self(sprintf(
// "Entity class '%s' is a subclass of the root entity class '%s', but no inheritance mapping type was declared.",
// $childEntityClass,
// $rootEntityClass
// ));
//}

/**
* @param string $className
*
Expand Down
2 changes: 0 additions & 2 deletions tests/Doctrine/Tests/Models/DDC1590/DDC1590Entity.php
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,11 @@

use DateTime;
use Doctrine\ORM\Mapping\Column;
use Doctrine\ORM\Mapping\Entity;
use Doctrine\ORM\Mapping\GeneratedValue;
use Doctrine\ORM\Mapping\Id;
use Doctrine\ORM\Mapping\MappedSuperclass;

/**
* @Entity
* @MappedSuperclass
*/
abstract class DDC1590Entity
Expand Down
6 changes: 6 additions & 0 deletions tests/Doctrine/Tests/Models/DDC2372/DDC2372User.php
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,21 @@
namespace Doctrine\Tests\Models\DDC2372;

use Doctrine\ORM\Mapping\Column;
use Doctrine\ORM\Mapping\DiscriminatorColumn;
use Doctrine\ORM\Mapping\DiscriminatorMap;
use Doctrine\ORM\Mapping\Entity;
use Doctrine\ORM\Mapping\GeneratedValue;
use Doctrine\ORM\Mapping\Id;
use Doctrine\ORM\Mapping\InheritanceType;
use Doctrine\ORM\Mapping\Table;
use Doctrine\Tests\Models\DDC2372\Traits\DDC2372AddressAndAccessors;

/**
* @Entity
* @Table(name="users")
* @InheritanceType("SINGLE_TABLE")
* @DiscriminatorColumn("type")
* @DiscriminatorMap({"user"="DDC2372User", "admin"="DDC2372Admin"})
*/
class DDC2372User
{
Expand Down
6 changes: 3 additions & 3 deletions tests/Doctrine/Tests/Models/DDC5934/DDC5934BaseContract.php
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,13 @@
use Doctrine\Common\Collections\Collection;
use Doctrine\ORM\Mapping\ClassMetadata;
use Doctrine\ORM\Mapping\Column;
use Doctrine\ORM\Mapping\Entity;
use Doctrine\ORM\Mapping\GeneratedValue;
use Doctrine\ORM\Mapping\Id;
use Doctrine\ORM\Mapping\ManyToMany;
use Doctrine\ORM\Mapping\MappedSuperclass;

/** @Entity */
#[Entity]
/** @MappedSuperclass() */
#[MappedSuperclass]
class DDC5934BaseContract
{
/**
Expand Down
18 changes: 0 additions & 18 deletions tests/Doctrine/Tests/Models/Forum/ForumAdministrator.php

This file was deleted.

122 changes: 122 additions & 0 deletions tests/Doctrine/Tests/ORM/Mapping/BasicInheritanceMappingTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

namespace Doctrine\Tests\ORM\Mapping;

use Doctrine\Deprecations\PHPUnit\VerifyDeprecations;
use Doctrine\ORM\EntityRepository;
use Doctrine\ORM\Id\SequenceGenerator as IdSequenceGenerator;
use Doctrine\ORM\Mapping\ClassMetadata;
Expand All @@ -29,13 +30,16 @@
use Doctrine\Tests\Models\DDC869\DDC869Payment;
use Doctrine\Tests\Models\DDC869\DDC869PaymentRepository;
use Doctrine\Tests\OrmTestCase;
use Generator;

use function assert;
use function serialize;
use function unserialize;

class BasicInheritanceMappingTest extends OrmTestCase
{
use VerifyDeprecations;

/** @var ClassMetadataFactory */
private $cmf;

Expand Down Expand Up @@ -218,6 +222,42 @@ public function testMappedSuperclassIndex(): void
self::assertArrayHasKey('IDX_MAPPED1_INDEX', $class->table['uniqueConstraints']);
self::assertArrayHasKey('IDX_MAPPED2_INDEX', $class->table['indexes']);
}

/**
* @dataProvider invalidHierarchyDeclarationClasses
*/
public function testUndeclaredHierarchyRejection(string $rootEntity, string $childClass): void
{
$this->expectDeprecationWithIdentifier('https://github.com/doctrine/orm/pull/10431');

/* on 3.0, use this instead: */
// self::expectException(MappingException::class);
// self::expectExceptionMessage(\sprintf(
// "Entity class '%s' is a subclass of the root entity class '%s', but no inheritance mapping type was declared.",
// $childClass,
// $rootEntity
// ));

$this->cmf->getMetadataFor($childClass);
}

public function invalidHierarchyDeclarationClasses(): Generator
{
yield 'concrete Entity root and child class, direct inheritance'
=> [InvalidEntityRoot::class, InvalidEntityRootChild::class];

yield 'concrete Entity root and abstract child class, direct inheritance'
=> [InvalidEntityRoot::class, InvalidEntityRootAbstractChild::class];

yield 'abstract Entity root and concrete child class, direct inheritance'
=> [InvalidAbstractEntityRoot::class, InvalidAbstractEntityRootChild::class];

yield 'abstract Entity root and abstract child class, direct inheritance'
=> [InvalidAbstractEntityRoot::class, InvalidAbstractEntityRootAbstractChild::class];

yield 'complex example (Entity Root -> Mapped Superclass -> transient class -> Entity)'
=> [InvalidComplexRoot::class, InvalidComplexEntity::class];
}
}

class TransientBaseClass
Expand Down Expand Up @@ -438,3 +478,85 @@ class MediumSuperclassEntity extends MediumSuperclassBase
class SubclassWithRepository extends DDC869Payment
{
}

/**
* @Entity
*
* This class misses the DiscriminatorMap declaration
*/
class InvalidEntityRoot
{
/**
* @Column(type="integer")
* @Id
* @GeneratedValue(strategy="AUTO")
* @var int
*/
public $id;
}

/** @Entity */
class InvalidEntityRootChild extends InvalidEntityRoot
{
}

/** @Entity */
abstract class InvalidEntityRootAbstractChild extends InvalidEntityRoot
{
}

/**
* @Entity
*
* This class misses the DiscriminatorMap declaration
*/
class InvalidAbstractEntityRoot
{
/**
* @Column(type="integer")
* @Id
* @GeneratedValue(strategy="AUTO")
* @var int
*/
public $id;
}

/** @Entity */
class InvalidAbstractEntityRootChild extends InvalidAbstractEntityRoot
{
}

/** @Entity */
abstract class InvalidAbstractEntityRootAbstractChild extends InvalidAbstractEntityRoot
{
}

/**
* @Entity
*
* This class misses the DiscriminatorMap declaration
*/
class InvalidComplexRoot
{
/**
* @Column(type="integer")
* @Id
* @GeneratedValue(strategy="AUTO")
* @var int
*/
public $id;
}

/** @MappedSuperclass */
class InvalidComplexMappedSuperclass extends InvalidComplexRoot
{
}

class InvalidComplexTransientClass extends InvalidComplexMappedSuperclass
{
}

/** @Entity */
class InvalidComplexEntity extends InvalidComplexTransientClass
{
}

0 comments on commit 7d7dc1b

Please sign in to comment.