Add runtime validation that the discriminator map contains all entity classes, also abstract ones?

[mpdude]

A general decision needs to be made here.

This PR adds a runtime metadata check, requiring all entity classes – including abstract intermediate ones – to be listed in the Discriminator Map (DM).

Why?

#8378 (with a fix in #8903) added this requirement to the SchemaValidator tool.

At least #8736 and #9095 pointed out this was a policy change. #9096 briefly removed the requirement for listing abstract entities, but #9142 opposed and put the check back in.

If we think that it is necessary to have all classes in the DM to ensure correct ORM behaviour, then this should not only be checked by the SchemaValidator tool, but instead deserves a runtime check in the ClassMetadataFactory.

Reasons for this requirement

The pro arguments are somewhat sketchy.

• @beberlei in Mapping fails for abstract class in DiscriminatorMap in 2.9 #8736 (comment):

Having all classes in the discriminator map is important for some operations.

• @olsavmic in Doctrine ORM 2.9: MappedSuperclass in entity hierarchy #8771 (comment)

I even found a case [in the schema generation tool] when the behaviour is unexpected if the abstract class in the middle of hierarchy is not present

A failing test for that was provided in #9145 as POC, #10387 might be a fix.

• In Three level table class inheritance not loading second level data due to wrong query generation #8127, @lsrzj reported an issue with JTI, where fields from abstract intermediate entity classes are not loaded when that class is not present in the DM.

That problem goes away when the abstract class is included in the DM (shown in #10388), but it may well be a real issue in its own right.

Reasons against the requirement

• Since no instances of abstract entities can be created, the corresponding value from the DM is never actually used. Users would be required to put in a dummy discriminator value.

• Doctrine could/should be able to figure out abstract intermediate entity classes by itself, given that all relevant leaf classes are listed in the DM.

• Archaeological research as follows

Runtime validation of DM requirements was already in place, with abstract classes being explicitly exempted.

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

Lines 278 to 283 in c5e4e41

	if (
	! $class->reflClass->isAbstract()
	&& ! in_array($class->name, $class->discriminatorMap, true)
	) {
	throw MappingException::mappedClassNotPartOfDiscriminatorMap($class->name, $class->rootEntityName);
	}

That goes back to 5ff44b5 where DM checking was initially added (corresponding issues #1809, #1810).

The exemption for abstract classes was added briefly afterwards in a0a81db.

Until today, the exception thrown for an incomplete DM explicitly mentions abstract entity classes as an option:

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

Lines 761 to 768 in c5e4e41

	public static function mappedClassNotPartOfDiscriminatorMap($className, $rootClassName)
	{
	return new self(
	"Entity '" . $className . "' has to be part of the discriminator map of '" . $rootClassName . "' " .
	"to be properly mapped in the inheritance hierarchy. Alternatively you can make '" . $className . "' an abstract class " .
	'to avoid this exception from occurring.'
	);
	}

Consequences

Merging

• Will cause breakage for users with "misconfigured" DMs. They may complain, pointing to our own exception suggesting abstract entity classes in the past.
• Even some of our own tests break due to the discouraged pattern being used
• Will require extra DM entries that are meaningless to users; maybe schema updates are necessary? (DM values as ENUMs and the like?)
• Will solve or at least mask some issues – they could be rejected as invalid
• Will improve consistency between the Schema Validator tool and runtime checks
• Slight documentation updates should be made to explain the need

Not merging

• Committing ourselves to not requiring abstract entities being in the DM
• Slight documentation updates should be made to mention the possibility of abstract entities in inheritance trees
• Relax the SchemaValidator check (Fix SchemaValidator with abstract child class in discriminator map #9096)
• Deal with issues like Make sure the schema tool does not miss columns from abstract entity classes in a STI tree #10387, Three level table class inheritance not loading second level data due to wrong query generation #8127 and probably others
• Ideally, understand their root cause and work on that. Hypothesis: When working with ClassMetadataInfo::$subclasses, it is not obvious what may/may not be included there.

To-Do

• Review if it works correctly (does not object) when loading a mapped superclass that lives between two entity classes (check should not trigger for mapped superclasses in general?)
• Review if it plays nicely with the feature from Resolve target entity also in discriminator map (allows interfaces and custom names in discriminator map) #1257, where interface names in the DM are converted to classes
• When the Event Listener from Resolve target entity also in discriminator map (allows interfaces and custom names in discriminator map) #1257 substitutes an interface name with an entity class, what happens if that class has another entity class above it that is not declared in the DM? Will we ever notice?

👇🏻 What do you think about this – Merge (:+1:) or do not merge (:-1:)?

[mpdude]

This extra check is needed here to catch inheritance roots missing in the DM.

[mpdude]

@beberlei I know you are short on time, but since you authored the core feature a decade ago, your assessment would be highly valued.