Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

2.14.2: Entity Inheritance (at least STI) with intermediate abstract class is broken #10625

Closed
htto opened this issue Apr 13, 2023 · 1 comment · Fixed by #10643
Closed

2.14.2: Entity Inheritance (at least STI) with intermediate abstract class is broken #10625

htto opened this issue Apr 13, 2023 · 1 comment · Fixed by #10643

Comments

@htto
Copy link

htto commented Apr 13, 2023

BC Break Report

The Single Table Inheritance Entity Inheritance is defined in your documentation with

#[DiscriminatorMap] declares the possible values for the discriminator column and maps them to class names in the hierarchy. This discriminator map has to declare all non-abstract entity classes that exist in that particular inheritance hierarchy. That includes the root as well as any intermediate entity classes, given they are not abstract.

but will fail on EntityManager::find() with an intermediate abstract entity class:

Undefined index: "DoctrineOrmTest\Intermediate"

vendor/doctrine/orm/lib/Doctrine/ORM/Persisters/Entity/SingleTablePersister.php:157
vendor/doctrine/orm/lib/Doctrine/ORM/Persisters/Entity/SingleTablePersister.php:128
vendor/doctrine/orm/lib/Doctrine/ORM/Persisters/Entity/BasicEntityPersister.php:1092
vendor/doctrine/orm/lib/Doctrine/ORM/Persisters/Entity/BasicEntityPersister.php:748
vendor/doctrine/orm/lib/Doctrine/ORM/Persisters/Entity/BasicEntityPersister.php:768
vendor/doctrine/orm/lib/Doctrine/ORM/EntityManager.php:521
vendor/doctrine/orm/lib/Doctrine/ORM/Decorator/EntityManagerDecorator.php:205

as the code iterates over all subclasses (abstract and non-abstract) and expects them in the discriminator map.

Q A
BC Break yes
Version 2.14.2

Summary

Apparently the intermediate abstract class seems to be expected in the discriminator map, even though the documentation states otherwise.

Not quite sure if the ClassMetadataInfo::subClasses iteration should directly access the discriminator values discrValues in SingleTablePersister::getSelectConditionDiscriminatorValueSQL()

        $discrValues = array_flip($this->class->discriminatorMap);

        foreach ($this->class->subClasses as $subclassName) {
            $values[] = $this->conn->quote($discrValues[$subclassName]);
        }

as subClasses is documented with having also abstract classes

     * READ-ONLY: For classes in inheritance mapping hierarchies, this field contains the names of all                  
     * <em>entity</em> subclasses of this class. These may also be abstract classes. 

where those are not to be expected in the discriminator map.

Previous behavior

It worked ™️

Current behavior

Access violation on runtime, but not on validation.

How to reproduce

Assume the following example of entities:

namespace DoctrineOrmTest;

use Doctrine\ORM\Mapping as ORM;

/**
 * @ORM\InheritanceType("SINGLE_TABLE")
 * @ORM\DiscriminatorMap({"1" = "DoctrineOrmTest\Concrete"})
 * @ORM\Entity()
 */
abstract class Base
{
    /**
     * @ORM\Column
     * @ORM\Id
     * @ORM\GeneratedValue(strategy="IDENTITY")
     */
    private int $id;
}
namespace DoctrineOrmTest;

use Doctrine\ORM\Mapping as ORM;

/**
 * @ORM\Entity()
 */
abstract class Intermediate extends Base
{
}
namespace DoctrineOrmTest;

use Doctrine\ORM\Mapping as ORM;

/**
 * @ORM\Entity()
 */
class Concrete extends Intermediate
{
}

plus some boilerplate:

composer.json

{
    "require": {
        "doctrine/annotations": "^2.0",
        "doctrine/orm": "^2.14.1",
        "symfony/cache": "^5.4"
    },
    "autoload": {
        "psr-4": {
            "DoctrineOrmTest\\": "src"
        }
    }
}

bootstrap.php

require_once(__DIR__ . '/vendor/autoload.php');

$config = Doctrine\ORM\ORMSetup::createAnnotationMetadataConfiguration([__DIR__ . '/src'], true);
$connection = Doctrine\DBAL\DriverManager::getConnection(['driver' => 'pdo_sqlite', 'memory' => true,], $config);
return new Doctrine\ORM\EntityManager($connection, $config);

cli-config.php

return Doctrine\ORM\Tools\Console\ConsoleRunner::createHelperSet(require_once(__DIR__ . '/bootstrap.php'));

and test.php

set_error_handler(fn () => die(var_export(func_get_args(), true)));

$em = require_once(__DIR__ . '/bootstrap.php');
(new Doctrine\ORM\Tools\SchemaTool($em))->createSchema([$em->getClassMetadata(DoctrineOrmTest\Base::class)]);
$em->clear();

$em->find(DoctrineOrmTest\Concrete::class, 1);
$em->find(DoctrineOrmTest\Intermediate::class, 1);
$em->find(DoctrineOrmTest\Base::class, 1);

will result in

 $ php test.php 
array (
  0 => 8,
  1 => 'Undefined index: DoctrineOrmTest\\Intermediate',
  2 => 'vendor/doctrine/orm/lib/Doctrine/ORM/Persisters/Entity/SingleTablePersister.php',
  3 => 157,
  4 => 
  array (
    'values' => 
    array (
      0 => '\'1\'',
    ),
    'discrValues' => 
    array (
      'DoctrineOrmTest\\Concrete' => 1,
    ),
    'subclassName' => 'DoctrineOrmTest\\Intermediate',
  ),

while

$ vendor/bin/doctrine orm:info
 Found 3 mapped entities:

 [OK]   DoctrineOrmTest\Base
 [OK]   DoctrineOrmTest\Concrete
 [OK]   DoctrineOrmTest\Intermediate
@htto
Copy link
Author

htto commented Apr 13, 2023

Possibly caused by the auto-filling of sub-classes in ClassMetadataInfo in #10411

mpdude added a commit to mpdude/doctrine2 that referenced this issue Apr 25, 2023
#### Current situation

The implementation of `\Doctrine\ORM\Mapping\ClassMetadataInfo::_validateAndCompleteOneToOneMapping` will consider a field with a one-to-one association to be the owning side also when it configures `@JoinColumn` settings.

#### Suggested change

For a one to one association, a field should be the inverse side when it uses the `mappedBy` attribute, and be the owning side otherwise. The `JoinColumn` may be configured on the owning side only.

This PR adds a deprecation notice when `@JoinColumn` is used on the side of a one-to-one association where `mappedBy` occurs.

In 3.0, this will throw a `MappingException`.
mpdude added a commit to mpdude/doctrine2 that referenced this issue Apr 25, 2023
#### Current situation

The implementation of `\Doctrine\ORM\Mapping\ClassMetadataInfo::_validateAndCompleteOneToOneMapping` will consider a field with a one-to-one association to be the owning side also when it configures `@JoinColumn` settings.

#### Suggested change

For a one to one association, a field should be the inverse side when it uses the `mappedBy` attribute, and be the owning side otherwise. The `JoinColumn` may be configured on the owning side only.

This PR adds a deprecation notice when `@JoinColumn` is used on the side of a one-to-one association where `mappedBy` occurs.

In 3.0, this will throw a `MappingException`.
mpdude added a commit to mpdude/doctrine2 that referenced this issue Apr 25, 2023
#### Current situation

The implementation of `\Doctrine\ORM\Mapping\ClassMetadataInfo::_validateAndCompleteOneToOneMapping` will consider a field with a one-to-one association to be the owning side also when it configures `@JoinColumn` settings.

#### Suggested change

For a one to one association, a field should be the inverse side when it uses the `mappedBy` attribute, and be the owning side otherwise. The `JoinColumn` may be configured on the owning side only.

This PR adds a deprecation notice when `@JoinColumn` is used on the side of a one-to-one association where `mappedBy` occurs.

In 3.0, this will throw a `MappingException`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant