Make "targetEntity must not be a mapped superclass" a lazy check#10554
Merged
greg0ire merged 3 commits intodoctrine:2.15.xfrom Mar 24, 2023
Merged
Conversation
ee30dd4 to
2c40e91
Compare
This was referenced Mar 2, 2023
alexander-schranz
approved these changes
Mar 2, 2023
Contributor
alexander-schranz
left a comment
There was a problem hiding this comment.
I did not have a look at the code as I'm not unfamiliar with internals here but this PR fixes our problems inside @sulu: sulu/sulu#7026
Thank you @mpdude 👍
BreyndotEchse
approved these changes
Mar 2, 2023
BreyndotEchse
left a comment
There was a problem hiding this comment.
MappingException gone and all tests passed!
ty @mpdude
Contributor
Author
|
Doctrine core team reviewers: Please confirm that using a |
Contributor
Author
|
@derrabus, @greg0ire or @SenseException – can we try to get this merged in the near future? I have the impression it would unblock people who are kind enough to try 2.15-dev in their CI pipelines. |
greg0ire
approved these changes
Mar 23, 2023
SenseException
approved these changes
Mar 23, 2023
Member
|
Thanks @mpdude ! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#10473 relaxed the runtime check that mapped superclasses must not use one-to-many associations. It turned the check the other way round, so that no mapped superclass may be used as the
targetEntityof an association.To find out whether a class is a mapped superclass, the
\Doctrine\ORM\Mapping\ClassMetadataFactory::peekIfIsMappedSuperclassmethod was used, which is directly using the mapping driver.#10552 reports a case where mapping configuration is not collected through the driver alone, but also a metadata listener is contributing relevant information. Updating metadata with a listener is currently not supported by the check in that method.
Also, #10473 (comment) has a mapping configuration that cannot be loaded in the improvised way that
\Doctrine\ORM\Mapping\ClassMetadataFactory::peekIfIsMappedSuperclassemploys.As I explained in #10552 (comment), we cannot use the full ClassMetadataFactory itself to obtain information about the
targetEntitywhile we're performing runtime validation for another class currently being loaded.So, I don't see any other way than to make this check a non-runtime (offline?) check in the
SchemaValidator. There, we can first completely load metadata for all classes and then inspect it.Many fixes to the test models that were made in #10473 can be reverted, since these files are no longer scrutinized at runtime, and we probably don't care about the extra validation checks that much.