Enforcing Abstract Entity in DiscriminatorMap is important for some operations

[olsavmic]

Bug Report

Summary

Recent fix introduced in 2.10.2 9096: Fix SchemaValidator with abstract child class in discriminator map is a regression to the SchemaValidator as having all @Entity classes, including those that are non-instantiable (abstract) is important for some operation.

@beberlei already mentioned it here #8736 (comment)

I also mentioned this in the original issue (#8771 (comment)) regarding this change.

I'll try to prepare a convincing test case.

Q	A
BC Break	no
Version	2.10.2

Current behaviour

Abstract class marked with @Entity annotation does not have to be part of DiscriminatorMap (according to SchemaValidator)

Expected behavior

Only @MappedSuperclass does not have to be part of the SchemaValidator, missing @Entity is dangerous in some cases

[derrabus]

cc @BenMorel

[BenMorel]

Looking forward to the test case, I'd like to fix the need for abstract classes in the discriminator map rather than reintroducing it. AFAIK, it always worked fine without in Doctrine ORM <= 2.8, so requiring it now looks like a regression.

[olsavmic]

I added a test case demonstrating the one issue I know of which is caused by missing abstract middle class in the $classMetadata->subClasses (look at the PHPUnit output).

I would also like to find a way around having abstract middle class in the DiscriminatorMap YET I'd prefer to keep the validator catching this very dangerous case UNTIL we have a working solution.

I'm not sure if there are any other problems related to this except the one demonstrated.

[beberlei]

We should perform some archeology to find out why we enforced this with 2.9

[olsavmic]

@beberlei I'd say it's obvious why the check was added -> to prevent issues like the one demonstrated in the test case.

Therefore I think the removal of the check is a regression as it allows for hidden problems that were there even BEFORE v2.9

We should find out a solution for the root case but that's rather a feature request and should be discussed separately.

As I already mentioned, we encountered exactly this issue in our production code on v2.7 and the check added in 2.9 prevents the issue from occurring again.

[BenMorel]

@olsavmic Interesting, thanks, I didn’t know that this issue was already present in 2.7, I thought that this was a regression introduced in 2.9.

@beberlei as a quick fix, would it make sense to automatically add a dummy entry in the discriminator map for all abstract classes?

[beberlei]

@BenMorel the problem is "all abstract classes" are not known when root entity is loaded

[olsavmic]

Since we found no simple and preferable solution, can we revert the change and update documentation accordingly (explaining that even abstract classes must be part of discriminator map and why)?

@beberlei @derrabus I'll prepare the PR if you agree with me :)

[mpdude]

Join #10389 for a discussion of whether "all abstract entities must be declared" shall be the new policy and also be enforced by a runtime check (not only in the Schema Tool).