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

Make "targetEntity must not be a mapped superclass" a lazy check #10554

Merged

Conversation

mpdude
Copy link
Contributor

@mpdude mpdude commented Mar 1, 2023

#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 targetEntity of an association.

To find out whether a class is a mapped superclass, the \Doctrine\ORM\Mapping\ClassMetadataFactory::peekIfIsMappedSuperclass method 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::peekIfIsMappedSuperclass employs.

As I explained in #10552 (comment), we cannot use the full ClassMetadataFactory itself to obtain information about the targetEntity while 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.

@mpdude mpdude changed the title mapped superclass resolve to many lazy check Make "targetEntity must not be a mapped superclass" a lazy check Mar 1, 2023
Copy link
Contributor

@alexander-schranz alexander-schranz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👍

Copy link

@BreyndotEchse BreyndotEchse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MappingException gone and all tests passed!
ty @mpdude

@mpdude
Copy link
Contributor Author

mpdude commented Mar 2, 2023

Doctrine core team reviewers:

Please confirm that using a SchemaValidator based check only instead of a runtime check is a valid approach. That's what the schema validator is made for, right? Not all problems can easily be spotted at runtime.

@mpdude
Copy link
Contributor Author

mpdude commented Mar 23, 2023

@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 greg0ire requested review from derrabus and beberlei March 23, 2023 13:57
@greg0ire greg0ire added this to the 2.15.0 milestone Mar 24, 2023
@greg0ire greg0ire added the Bug label Mar 24, 2023
@greg0ire greg0ire merged commit aec3556 into doctrine:2.15.x Mar 24, 2023
@greg0ire
Copy link
Member

Thanks @mpdude !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants