Fix association handling when there is a MappedSuperclass in the middle of an inheritance hierarchy

[mpdude]

This fixes two closely related bugs.

1. When inheriting a to-one association from a mapped superclass, update the sourceEntity class name to the current class only when the association is actually declared in the mapped superclass.
2. Reject association types that are not allowed on mapped superclasses only when they are actually declared in a mapped superclass, not when inherited from parent classes.

Background

Currently, when a many-to-one association is inherited from a MappedSuperclass, mapping information will be updated so that the association has the current (inheriting) class as the source entity.

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

Lines 384 to 393 in 2138cc9

	private function addInheritedRelations(ClassMetadata $subClass, ClassMetadata $parentClass): void
	{
	foreach ($parentClass->associationMappings as $field => $mapping) {
	if ($parentClass->isMappedSuperclass) {
	if ($mapping['type'] & ClassMetadata::TO_MANY && ! $mapping['isOwningSide']) {
	throw MappingException::illegalToManyAssociationOnMappedSuperclass($parentClass->name, $field);
	}
	$mapping['sourceEntity'] = $subClass->name;
	}

This was added in 7dc8ef1 for DDC-671.

The reason for this is that a mapped superclass is not an entity itself and has no table.

So, in the database, associations can only be from the inheriting entities' tables towards the referred-to target. This is also the reason for the limitation that only to-one associations may be added in mapped superclasses, since for those the database foreign key can be placed on the table(s) of the inheriting entities (and there may be more than one child class).

Issue with the current code

Neither the decision to update the sourceEntity nor the validation check should be based on $parent->isMappedSuperclass.

This only works in the simple case where the class hierarchy is Mapped Superclass → Entity.

The check is wrong when we have an inheritance hierarchy set up and the class hierarchy is Base Entity → Mapped Superclass → Child Entity.

Bug 1: The association should keep the root entity as the source. After all, in a JTI, the root table will contain the foreign key, and we need to base joins on that table when traversing FROM LeafClass l JOIN l.target.

Bug 2: Do not reject the to-many association declared in the base class. It is ok to have the reverse (owning) side point back to the base entity, as it would be if there were no mapped superclasses at all. The mapped superclass does not declare, add or otherwise interfere with the to-many association at all.

Suggested fix

Base the decision to change the sourceEntity on $mapping['inherited'] being set. This field points to the topmost parent entity class in the ancestry tree where the relationship mapping appears for the first time.

When it is not set, the current class is the first entity class in the hierarchy with that association. Since we are inheriting the relation, it must have been added in a mapped superclass above, but was not yet present in the nearest parent entity class.

In that case, it may only be a to-one association and the source entity needs to be updated.

(See #10396 for a clarification of the semantics of inherited.)

Example

Here is a simplified example of the class hierarchy.

See the two tests added for more details – one is for checking the correct usage of a to-one association against/with the base class in JTI. The other is to test that a to-many association on the base class is not rejected.

I am sure that there are other tests that (still) cover the update of sourceEntity is happening.

/**
 * @Entity
 */
class AssociationTarget
{
    /**
     * @Column(type="integer")
     * @Id
     * @GeneratedValue
     */
    public $id;
}

/**
 * @Entity
 * @InheritanceType("JOINED")
 * @DiscriminatorColumn(name="discriminator", type="string")
 * @DiscriminatorMap({"1" = "BaseClass", "2" = "LeafClass"})
 */
class BaseClass
{
    /**
     * @Column(type="integer")
     * @Id
     * @GeneratedValue
     */
    public $id;

    /**
     * @ManyToOne(targetEntity="AssociationTarget")
     */
    public $target;
}

/**
 * @MappedSuperclass
 */
class MediumSuperclass extends BaseClass
{
}

/**
 * @Entity
 */
class LeafClass extends MediumSuperclass
{
}

When querying FROM LeafClass l, it should be possible to JOIN l.target. This currently leads to an SQL error because the SQL join will be made via LeafClass.target_id instead of BaseClass.target_id. LeafClass is considered the sourceEntity for the association – which is wrong–, and so the foreign key field is expected to be in the LeafClass table (using JTI here).

Fixes #5998, fixes #7825.

Updated:

I have removed the abstract entity class, since it is not relevant for the issue and took the discussion off course. Also, the discriminator map now contains all classes.

Updated 2:

Added the second variant of the bug, namely that a to-many association would wrongly be rejected in the same situation.

[mpdude]

It seems #5998 describes the same bug. #7825 might include the attempt to fix it, although it's a bigger (abandoned?) PR and seems to address additional things.

[greg0ire]

Please add something to sum #7825 (comment) up in the docs, that kind of knowledge of what is supported and what isn't could come in handy to both users and maintainers.

[greg0ire]

I don't think 2.7 is still maintained, but https://github.com/doctrine/orm/blob/2.8.x/.doctrine-project.json says it is. Maybe @beberlei or @SenseException will know?

[mpdude]

@greg0ire Regarding additional documentation on MappedSuperclass: I have often missed that myself 👍. Maybe someone who knows better than me could give me some starting pointers...?

Is a @MappedSuperclass declaration always necessary when a class contains fields or associations that need to be persisted, but when that class is not an @Entity?

I sometimes had the impression that things also work without the declaration?

Also, what about @Entity classes that are abstract?

[greg0ire]

Maybe someone who knows better than me could give me some starting pointers...?

Sure, but it won't be me I'm afraid 😅

[SenseException]

For ORM it should the current version that is supported. In that case it's 2.8. I'm not sure if there are exceptions for 2.7, like security issues. I think this should target 2.8.x. I can see confusion what the repository "says" and what the documentation shows, because the docs still label it as upcoming. This will be handled in a different PR.

@mpdude @Entity shouldn't be used in an abstract because Entities can't be abstract itself, while a MappedSuperclass can. There's probably a way to make things work in a way, but this might result in problems on ORM updates in the future.

From the docs: https://www.doctrine-project.org/projects/doctrine-orm/en/2.8/reference/inheritance-mapping.html

Typically, the purpose of such a mapped superclass is to define state and mapping information that is common to multiple entity classes.

If there are entities with similar mapping, the MappedSuperclass can contain these common fields. There's not much extended info about other possible use cases and I'm not sure if there are some that are considered as a intentional feature. The sentence

Mapped superclasses ... can appear in the middle of an otherwise mapped inheritance hierarchy (through Single Table Inheritance or Class Table Inheritance).

seems to open a lot of possibilities that might not be covered by ORM updates.

[mpdude]

@SenseException I'm afraid I might not fully understand your comment.

Regarding the abstract base @Entity, that might be a leftover from the real life case I came from; I'll check that and think the problem persists if I remove it. 

I will also rebase this PR to 2.8. Will it be backported to 2.7, or what is the exact state of that (security fixes only?).

But more in general, are you suggesting that it is not supported, discouraged or otherwise unsafe to use @MappedSuperclass in hierarchies below @Entity classes?

Updated, see next comment.

[mpdude]

Whether or not the base class from my example is abstract does not make a difference for the issue.

Just as a side note, when the class is not abstract, for obvious reasons I get an error message

[base class] has to be part of the discriminator map of [base class] to be properly mapped in the inheritance hierarchy. Alternatively you can make [base class] an abstract class to avoid this exception from occurring

This seems to confirm that an abstract @Entity was not completely unexpected when that part of the ORM was designed. Also, at https://www.doctrine-project.org/projects/doctrine-orm/en/2.13/reference/architecture.html#entities it says

Entities support inheritance, polymorphic associations, and polymorphic queries. Both abstract and concrete classes can be entities. Entities may extend non-entity classes as well as entity classes, and non-entity classes may extend entity classes.

But as I said, it does not matter here.

Regarding MappedSuperclass in the middle of a class hierarchy, apart from the documentation quoted above there has also been some discussion in #7825. If this approach were completely beyond what ORM was designed for, my guess is it would have been rejected over there already, which apparently wasn't the case.

[andrews05]

Would be great to see this working!
I'm surprised at what a minor change it was to make it work and pass all those tests, considering everything it took to achieve this on the v3 branch.

Just for reference, here's what @guilhermeblanco said in #7825:

Actually we originally designed to have any mix of inheritance together, but for simplicity we killed the support for mixing the JTI and STI together, but was always intended to support JTI + MappedSuperclass and STI + MappedSuperclass.

[mpdude]

@andrews05 Great to have you on board here! You must have spent a lot of time working on #7825, so I think you have some valuable expertise to add. From your point of view, are there any edge cases we should cover as well?

[andrews05]

@mpdude Hm, I would say if it's passing all the tests I added, then that should cover all the edge cases I'm aware of.

[mpdude]

Could anybody give me hints what I could do to get this along? 🙏🏻

[vv12131415]

@mpdude since 2.8 is unmaintained, first thing to do, is to change the branch to 2.9

then you can tag someone who can do review for this part of code

upd: you can also see the history https://github.com/doctrine/orm/commits/2.9.x/lib/Doctrine/ORM/Mapping
and tag those who have most recent commits there

[mpdude]

@beberlei you last worked on this area of the code in 2010 when you did 7dc8ef1. @guilhermeblanco you were involved in the discussion of #7825, which should be fixed by this PR.

Would it be possible for either of you to review this PR? 💚

[mpdude]

Rebased onto 2.11.x. The fix itself is a three-line change, the rest are tests added ;-).

[greg0ire]

@mpdude I removed the usage of the simple annotation loader a while ago. This means the annotations in your tests need to come with corresponding use statements.

[mpdude]

Thank you @greg0ire for the heads-up.

[mpdude]

I'd still like to get this resolved and merged, and I am willing to spend the time to port and this to the current 2.x branch.

@beberlei Do you see a chance to review this and what would you need to make this as easy as possible?

[mpdude]

I have an idea how the check can be written even more clearly based on #10397 and #10398

[mpdude]

@derrabus Would it be possible for you to review this?

If it is too complex, has missing context or similar, let me know and I'll try to make it as easy as possible for you.

[greg0ire]

Thanks @mpdude !

[mpdude]

Thank you all!