Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Regression: abstract entities in the middle of a hierarchy need to be part of discriminator map #9095

Closed
BenMorel opened this issue Oct 5, 2021 · 5 comments · Fixed by #9096 or #10420
Labels

Comments

@BenMorel
Copy link
Contributor

BenMorel commented Oct 5, 2021

BC Break Report

Q A
BC Break yes
Version 2.9.x, 2.10.x

Summary

As mentioned in #8771 (comment), there is a regression that affects both 2.9 and 2.10:

If an entity hierarchy has an abstract entity class in the middle of the hierarchy, Doctrine complains that it must be part of the discriminator map. Example:

  • abstract class A
    • abstract class B
      • class C

bin/console doctrine:schema:validate reports:

[FAIL] The entity-class B mapping is invalid:

  • Entity class 'B' is part of inheritance hierarchy, but is not mapped in the root entity 'A' discriminator map. All subclasses must be listed in the discriminator map.

This doesn't make sense, as there cannot be an entry in the database for an abstract entity class.

This was working fine up to Doctrine 2.8.

The quick fix for those affected is to add "fake" entries to the discriminator map for abstract child entities, that do not have an equivalent in the database ENUM; which is rather ugly.

@derrabus derrabus added the Bug label Oct 5, 2021
@derrabus
Copy link
Member

derrabus commented Oct 5, 2021

Do you want to work on a fix?

@BenMorel
Copy link
Contributor Author

BenMorel commented Oct 5, 2021

@derrabus Yes! I opened a PR in #9096, building on top of #8903.

I'm dubious as to whether it makes sense to have both ! $class->reflClass->isAbstract() and ! $class->isMappedSuperclass now though: does it make sense to have a non-abstract @MappedSuperclass? Isn't it enough to check if the class is abstract?

@derrabus
Copy link
Member

derrabus commented Oct 5, 2021

Looks good to me. 🙂

@BenMorel
Copy link
Contributor Author

Be careful that this has been reverted in #9262.

@mpdude
Copy link
Contributor

mpdude commented Jan 17, 2023

#10420 suggests to fix this again; it should now be possible due to #10411.

@greg0ire greg0ire reopened this Jan 17, 2023
greg0ire pushed a commit to mpdude/doctrine2 that referenced this issue Feb 5, 2023
Since doctrine#10411 has been merged, no more need to specify all intermediate
abstract entity classes.

Thus, we can relax the schema validator check as requested in doctrine#9095. The
reasons given in doctrine#9142 no longer apply.

Fixes doctrine#9095
Closes doctrine#9096
greg0ire pushed a commit to mpdude/doctrine2 that referenced this issue Feb 5, 2023
Since doctrine#10411 has been merged, no more need to specify all intermediate
abstract entity classes.

Thus, we can relax the schema validator check as requested in doctrine#9095. The
reasons given in doctrine#9142 no longer apply.

Fixes doctrine#9095
Closes doctrine#9096
greg0ire pushed a commit to mpdude/doctrine2 that referenced this issue Feb 5, 2023
Since doctrine#10411 has been merged, no more need to specify all intermediate
abstract entity classes.

Thus, we can relax the schema validator check as requested in doctrine#9095. The
reasons given in doctrine#9142 no longer apply.

Fixes doctrine#9095
greg0ire added a commit that referenced this issue Feb 6, 2023
derrabus added a commit to derrabus/orm that referenced this issue Feb 7, 2023
* 2.14.x:
  Remove calls to assertObjectHasAttribute() (doctrine#10502)
  Remove calls to withConsecutive() (doctrine#10501)
  Use recognized array key
  Fix doctrine#9095 by re-applying doctrine#9096
  Use linebreaks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants