Resolving target entity in discriminator map omits fields from subtables

[mpdude]

#1257 introduced the possibility to use interfaces instead of concrete classes also in the DiscriminatorMap. Here's an example config:

/** @DiscriminatorMap({"foo" = "FooInterface", "bar" = "BarConcreteImpl"}) */

The ResolveTargetEntityListener would be used to resolve such interface references to concrete classes at runtime.

The feature is currently broken. Core of the issue seems to be that \Doctrine\ORM\Mapping\ClassMetadataInfo::$subClasses is not populated with the concrete classes. This proably has to do with the resolution taking place too late, although I do not fully understand where this happens.

Example test failure before the change: https://github.com/doctrine/orm/pull/8402/checks?check_run_id=1587762816#step:7:73. The test case failed because the columns necessary for the subclasses are not created, but if they were (in an existing schema), the loaded entities would also lack data for their respective fields.

PR is against 2.7.x where the feature was introduced, since I did not find any documentation on PR policies/target branches.

[mpdude]

Apparently, this might be fixed if the ResolveTargetEntityListener also deals with the DiscriminatorMap during the loadClassMetadata event.

@Ocramius:

You added this code in #1257. My guess is that, apart from the check that all classes are part of the DiscriminatorMap, it is no longer necessary when the ResolveTargetEntityListener deals with interface-to-class substitution: Since the concrete classes are now available much earlier, the discriminatorValue will already be setup correctly (happens in \Doctrine\ORM\Mapping\ClassMetadataInfo::addDiscriminatorMapClass()).

Maybe this method could even be removed altogether and the check added in \Doctrine\ORM\Mapping\ClassMetadataFactory::validateRuntimeMetadata()?

[mpdude]

Note to self: #8378 might be working on the same thing/might be fixed as well by the last change? It's about the SchemaValidator.

@beberlei If from your work on #8378 (fixing #6578) you have additional insight on edge cases related to invalid Discriminator Map definitions, I'd be glad to hear from you!

[beberlei]

I think this is a valid bugfix, because the Events::loadClassMetadata is triggered inside AbstractClassMetadataFactory::loadMetadata and the logic from resolveDiscriminatorValue seems to be replaced correctly by the simple foreach in ResolveTargetEntityListener.

I am wondering though resolveDiscriminatorValue chanes $metadata->discrimintorValue, does this happen elsewhere now?

[beberlei]

@mpdude needs to be rebased on 2.8.x as this is the bugfix target at the moment.

[mpdude]

Rebased to 2.8.x. The failing CS check applies to files not changed in this PR, so I'll ignore it.

@beberlei regarding your comment

I am wondering though resolveDiscriminatorValue chanes $metadata->discrimintorValue, does this happen elsewhere now?

I am not exactly sure what you're out for... The only write access to \Doctrine\ORM\Mapping\ClassMetadataInfo::$discriminatorValue I could find was in \Doctrine\ORM\Mapping\ClassMetadataInfo::addDiscriminatorMapClass().

My guess is that this will be called on every ClassMetadataInfo instance during metadata load, and that it will determine the discriminator value to be used for the underlying class (\Doctrine\ORM\Mapping\ClassMetadataInfo::$name).

Since the resolution of interfaces to concrete classes now happens before that, we should already be looking a the classes (not intefaces anymore) in this place... Not sure, does this answer your question?

[mpdude]

Rebased against 2.8.x again, hoping that this time this will resolve CS issues 🤞🏻

[mpdude]

@beberlei I've rebased this onto 2.9.x.

Given that you already had a look at this and basically agreed, could you please advise me what changes, clarifications or additional tests you'd like to see to make it a 👍?

[mpdude]

How can I make PHPStan/Psalm happy with regards to \Doctrine\ORM\Mapping\ClassMetadataFactory::validateRuntimeMetadata()?

The second argument $parent is hinted as ClassMetadataInterface, but I need to access $parent->discriminatorMap, which is not possible on an interface.

Changing it to ClassMetadata (as it is for the first $class argument) seems to be not easily possible because that in turn cascades to other, central places like \Doctrine\Persistence\Mapping\AbstractClassMetadataFactory::doLoadMetadata().

[greg0ire]

An easy way to fix this should be to use assert($parent instanceof ClassMetadata) if you are sure that's the case, and to create a Github issue to fix the introduced technical debt by handling that cascade you describe.

[mpdude]

@greg0ire Thank you for the hint! I guess it makes only sense to open the other PR once this PR here is merged, right?

[greg0ire]

@greg0ire Thank you for the hint! I guess it makes only sense to open the other PR once this PR here is merged, right?

I think both issues and PR are independent, feel free to work on it now if you want to.

[greg0ire]

Thanks @mpdude ! I took the liberty of squashing your commits, as most of them were amending the main one.

[mpdude]

ORM 2.8 is end of life and this will not be backported, right?

[greg0ire]

Correct 👍