Ignore subclasses without discriminatorValue when generating discriminator column condition SQL

[DemoniacDeath]

After commit 4e8e3ef when \Doctrine\ORM\Query\SqlWalker generates dicsriminator column condition SQL (method \Doctrine\ORM\Query\SqlWalker::generateDiscriminatorColumnConditionSQL) it adds an empty string to the list of possible values if the inheritance hierarchy contains a non-root abstract class.

When the discriminator column is implemented with a custom type in PostgreSQL (equivalent of Enum) the query fails because the type cannot have a value of an empty string. It boils down to the fact that \Doctrine\ORM\Mapping\ClassMetadataInfo::$subClasses contains an abstract class and in its Metadata the value of \Doctrine\ORM\Mapping\ClassMetadataInfo::$discriminatorValue is null.

Previous behavior

In version 2.14.1 \Doctrine\ORM\Mapping\ClassMetadataInfo::$subClasses does not contain an abstract class.

Fixes #11199, fixes #11177, fixes #10846.

[mpdude]

I have a suggestion how I'd like to change your PR here:

DemoniacDeath#1

If you like, you should be able to merge that PR into your own branch and it should be updated here.

Basically, I'd prefer to not check for null discriminator values, but instead check the fact that a class is abstract – like it is done when building or validating the discriminator map.

Additionally, I discovered the edge case that you might end up with no possible class at all.

[mpdude]

LGTM!

[mpdude]

Maintainers: We probably should squash-merge this, keeping the text from the initial comment as the commit message.

[mpdude]

Does this also fix #11177?

[mpdude]

#10846 is probably also the same

[plantas]

This PR fixes #11177 too 👍

Rethinking this, not sure if the isAbstract check is really the best way to go.

[mpdude]

• What if the class metadata is taken from the cache - may that cause reflection not to be initialized, and may that lead to wrong results?
• When metadata comes from the cache, does the reflection-based check trigger the autoloader/class load that we might want to avoid?

-> Looking at the discriminator value might be a better way to go.

On the other hand, there may be users that put abstract classes into the Discriminator Map, which was a recommendation for some time... would it make sense to skip those classes? Probably we can neglect the performance difference.

Are there any other reasons why the discriminator value might be null, can the Discriminator Map be mis-configured in a stupid way?

[mpdude]

I was unable to get null values into the DM through configuration, it always ended up as the 0 key or a 'null' string. So, null seems to be a safe indication that the class is not in the map.

The map is checked at load time that all intermediate, non-abstract entity classes have been declared, so this seems to be safe.

[mpdude]

#10643 is a very similar fix in another place… does this show additional refactorings would be appropriate?

#11213

[mpdude]

@DemoniacDeath congratulations, you’re now a Doctrine ORM contributor. Keep it up!

🚀

[Legion112]

Big shout out to the contributors 🙇‍♂️