mirror of
https://github.com/doctrine/orm.git
synced 2026-03-23 22:42:18 +01:00
Enforcing Abstract Entity in DiscriminatorMap is important for some operations #6862
Reference in New Issue
Block a user
Delete Branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Originally created by @olsavmic on GitHub (Oct 21, 2021).
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
@Entityclasses, including those that are non-instantiable (abstract) is important for some operation.@beberlei already mentioned it here https://github.com/doctrine/orm/issues/8736#issuecomment-853044536
I also mentioned this in the original issue (https://github.com/doctrine/orm/issues/8771#issuecomment-893266118) regarding this change.
I'll try to prepare a convincing test case.
Current behaviour
Abstract class marked with
@Entityannotation does not have to be part of DiscriminatorMap (according to SchemaValidator)Expected behavior
Only
@MappedSuperclassdoes not have to be part of the SchemaValidator, missing@Entityis dangerous in some cases@derrabus commented on GitHub (Oct 21, 2021):
cc @BenMorel
@BenMorel commented on GitHub (Oct 22, 2021):
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 commented on GitHub (Oct 22, 2021):
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 commented on GitHub (Oct 29, 2021):
We should perform some archeology to find out why we enforced this with 2.9
@olsavmic commented on GitHub (Oct 29, 2021):
@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 commented on GitHub (Oct 29, 2021):
@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 commented on GitHub (Oct 30, 2021):
@BenMorel the problem is "all abstract classes" are not known when root entity is loaded
@olsavmic commented on GitHub (Dec 4, 2021):
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 commented on GitHub (Jan 13, 2023):
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).