Allow to-many associations on mapped superclasses w/ ResolveTargetEntityListener #10473
Conversation
This file contains 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
mpdude
force-pushed
the
mapped-superclass-resolve-to-many
branch
2 times, most recently
from
3118dd6 to
b7f47e3
Compare
mpdude
commented
Jan 27, 2023
| { | ||
| foreach ($class->getAssociationMappings() as $mapping) { | ||
| $targetEntity = $mapping['targetEntity']; | ||
| if ($this->driver->isTransient($targetEntity) || $this->peekIfIsMappedSuperclass($targetEntity)) { |
There was a problem hiding this comment.
Is this correct, or am I missing a case where different drivers might be used for entities from different modules or similar?
There is just one ClassMetadataFactory for all entities (within the boundaries of a particular entity manager), and it has a single driver, right?
mpdude
force-pushed
the
mapped-superclass-resolve-to-many
branch
2 times, most recently
from
f411932 to
dc3677d
Compare
mpdude
force-pushed
the
mapped-superclass-resolve-to-many
branch
2 times, most recently
from
64d6122 to
7020f2e
Compare
|
@greg0ire Any ideas where the Psalm errors might come from? |
|
DBAL 3.6.0, published 20 hours ago? |
…ityListener Allow to-many associations to be used on mapped superclasses when the owning (inverse) side does not refer back to the mapped superclass, thanks to `ResolveTargetEntityListener`. #### Current situation The [documentation states](https://www.doctrine-project.org/projects/doctrine-orm/en/latest/reference/inheritance-mapping.html): > No database table will be created for a mapped superclass itself > [...] persistent relationships defined by a mapped superclass must be unidirectional (with an owning side only). This means that One-To-Many associations are not possible on a mapped superclass at all. That's a though limitation. ~Obviously~ ~apparently~ Probably the limitation comes from the fact that in a to-many association the "many" side has to hold a foreign key. Since the mapped superclass does not have a database table (it's not an entity), no such backreference can be established. Currently, to-many associations trigger an exception as soon as they are seen on a mapped superclass: https://github.com/doctrine/orm/blob/d6c0031d44f04e04bbc0cd57a3ed7e05c7ea8b40/lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php#L459-L461 #### `ResolveTargetEntityListener` The `ResolveTargetEntityListener` can be used to substitute interface or class names in mapping configuration at runtime, during the metadata load phase. When this gimmick is used to replace _all_ references to the mapped superclass with an entity class in time, it should be possible to have to-many associations on the inheriting entity classes. #### Suggested solution Instead of rejecting to-many associations on mapped superclasses right away, validate that at the end of the day (after the `loadClassMetadata` event has been processed) no association may target at a non-entity class. That includes mapped superclasses as well as transient classes. #### Motivating example Consider a library that comes with a `User` base class. This class is `abstract` and has to be subclassed/filled when the library is used. By making this a mapped superclass, library users have the freedom to either have a simple user entity class or a user class hierarchy, but we do not impose any requirements on them. (NB we also don't want to have a root entity in the library, because that would have to declare the entire class hierarchy, including library users' classes.) The actual user class to be used will be configured through the `ResolveTargetEntityListener`. The library also includes a `SocialMediaAccount` entity. A `User` can have multiple of these accounts, and we want to be able to navigate the accounts from the user side. To make the example even more fancy, there is a self-referencing association on the `User`: A `User` has been created by another user, and holds a collection of all other `User`s it created. The test case contained in this PR contains this example and validates that all association mappings look just as if the final user class had been written as an entity directly, without the superclass. #### Potential review talking points - Am I missing other reasons why to-many is not feasible? - We now reject association mappings with `targetEntity`s that are not entities; relevant BC break? (IMHO: no.) #### Review tip Review commit by commit, not all files at once. The last commit adds a lot of entity declarations that were previously missed in tests and now raised exceptions; that's a lot of clutter in the PR.
…ities, but never declared Now that we validate association targets, that's an error.
mpdude
force-pushed
the
mapped-superclass-resolve-to-many
branch
from
7020f2e to
072c403
Compare
greg0ire
previously approved these changes
Feb 8, 2023
SenseException
previously approved these changes
Feb 23, 2023
|
Super happy to have two approvals already 🤩 and a bit afraid to lose them again when I push a markdown/rst fix 🤪 |
|
@greg0ire / @SenseException Fixed the .rst glitch, please re-approve. |
SenseException
approved these changes
Feb 27, 2023
greg0ire
approved these changes
Mar 1, 2023
|
🎉 🍾 Awesome, thanks! |
|
#10455 is the logical next step ;-) |
|
Causes an exception when using overrides:
|
mpdude
added a commit
to mpdude/doctrine2
that referenced
this pull request
Mar 1, 2023
mpdude
added a commit
to mpdude/doctrine2
that referenced
this pull request
Mar 1, 2023
|
@BreyndotEchse and @alexander-schranz Could you please try and report back in #10554 if that solves the problems this caused for you? |
mpdude
added a commit
to mpdude/doctrine2
that referenced
this pull request
Mar 2, 2023
…ss()` method [This comment](doctrine#10473 (comment)) hints to a case where the `ClassMetadataFactory::peekIfIsMappedSuperclass()` method introduced in doctrine#10411 causes a failure. `CMF::peekIfIsMappedSuperclass()` has to perform improvised metadata loading in a situation where the CMF is currently loading a class. So, we cannot use the full/real `ClassMetadataFactory` mechanisms, since it would require a re-entry for a subclass of the current class, causing an infinite loop (loads parent classes first, and that's what we're currently doing). The problem is that the improvised call to `$driver->loadMetadataForClass()` cannot provide a pre-filled `ClassMetadata` instance populated with all parent class fields and associations. But, when attribute or association overrides are used, a check is made to see if the overridden field/association actually exists, and this information is missing in that situation. This PR suggests to override the methods to get around this. In fact, we do not care about all these details, we only want to ask the driver if the class is a mapped superclass or not. A much better fix would be to have a dedicated method on the driver to ask it just that particular question (also better performance-wise). But I do not see how we could get that done in a BC way – ideas? 💡 A few things that need to come together to make the bug surface: * Load an entity declaring an inheritance tree * There must be a mapped superclass in the inheritance tree to provide the field that shall be overriden * An entity class must inherit from the mapped superclass and override the field * That entity class must be an abstract, intermediate class not be declared in the discriminator map so we can "discover" it * The overriden property must be private so the mapping drivers (using reflection) do not see it when looking at the overriding entity class.
mpdude
added a commit
to mpdude/doctrine2
that referenced
this pull request
Mar 8, 2023
…ss()` method [This comment](doctrine#10473 (comment)) hints to a case where the `ClassMetadataFactory::peekIfIsMappedSuperclass()` method introduced in doctrine#10411 causes a failure. `CMF::peekIfIsMappedSuperclass()` has to perform improvised metadata loading in a situation where the CMF is currently loading a class. So, we cannot use the full/real `ClassMetadataFactory` mechanisms, since it would require a re-entry for a subclass of the current class, causing an infinite loop (loads parent classes first, and that's what we're currently doing). The problem is that the improvised call to `$driver->loadMetadataForClass()` cannot provide a pre-filled `ClassMetadata` instance populated with all parent class fields and associations. But, when attribute or association overrides are used, a check is made to see if the overridden field/association actually exists, and this information is missing in that situation. This PR suggests to override the methods to get around this. In fact, we do not care about all these details, we only want to ask the driver if the class is a mapped superclass or not. A much better fix would be to have a dedicated method on the driver to ask it just that particular question (also better performance-wise). But I do not see how we could get that done in a BC way – ideas? 💡 A few things that need to come together to make the bug surface: * Load an entity declaring an inheritance tree * There must be a mapped superclass in the inheritance tree to provide the field that shall be overriden * An entity class must inherit from the mapped superclass and override the field * That entity class must be an abstract, intermediate class not be declared in the discriminator map so we can "discover" it * The overriden property must be private so the mapping drivers (using reflection) do not see it when looking at the overriding entity class.
mpdude
added a commit
to mpdude/doctrine2
that referenced
this pull request
Mar 8, 2023
…ss()` method [This comment](doctrine#10473 (comment)) hints to a case where the `ClassMetadataFactory::peekIfIsMappedSuperclass()` method introduced in doctrine#10411 causes a failure. `CMF::peekIfIsMappedSuperclass()` has to perform improvised metadata loading in a situation where the CMF is currently loading a class. So, we cannot use the full/real `ClassMetadataFactory` mechanisms, since it would require a re-entry for a subclass of the current class, causing an infinite loop (loads parent classes first, and that's what we're currently doing). The problem is that the improvised call to `$driver->loadMetadataForClass()` cannot provide a pre-filled `ClassMetadata` instance populated with all parent class fields and associations. But, when attribute or association overrides are used, a check is made to see if the overridden field/association actually exists, and this information is missing in that situation. This PR suggests to override the methods to get around this. In fact, we do not care about all these details, we only want to ask the driver if the class is a mapped superclass or not. A much better fix would be to have a dedicated method on the driver to ask it just that particular question (also better performance-wise). But I do not see how we could get that done in a BC way – ideas? 💡 A few things that need to come together to make the bug surface: * Load an entity declaring an inheritance tree * There must be a mapped superclass in the inheritance tree to provide the field that shall be overriden * An entity class must inherit from the mapped superclass and override the field * That entity class must be an abstract, intermediate class not be declared in the discriminator map so we can "discover" it * The overriden property must be private so the mapping drivers (using reflection) do not see it when looking at the overriding entity class.
mpdude
added a commit
to mpdude/doctrine2
that referenced
this pull request
Mar 8, 2023
…lassMetadataFactory::peekIfIsMappedSuperclass()` method introduced in doctrine#10411 causes a failure. `CMF::peekIfIsMappedSuperclass()` has to perform improvised metadata loading in a situation where the CMF is currently loading a class. So, we cannot use the full/real `ClassMetadataFactory` mechanisms, since it would require a re-entry for a subclass of the current class, causing an infinite loop (loads parent classes first, and that's what we're currently doing). The problem is that the improvised call to `$driver->loadMetadataForClass()` cannot provide a pre-filled `ClassMetadata` instance populated with all parent class fields and associations. But, when attribute or association overrides are used, a check is made to see if the overridden field/association actually exists, and this information is missing in that situation. This PR suggests to override the methods to get around this. In fact, we do not care about all these details, we only want to ask the driver if the class is a mapped superclass or not. A much better fix would be to have a dedicated method on the driver to ask it just that particular question (also better performance-wise). But I do not see how we could get that done in a BC way – ideas? 💡 A few things that need to come together to make the bug surface: * Load an entity declaring an inheritance tree * There must be a mapped superclass in the inheritance tree to provide the field that shall be overriden * An entity class must inherit from the mapped superclass and override the field * That entity class must be an abstract, intermediate class not be declared in the discriminator map so we can "discover" it * The overriden property must be private so the mapping drivers (using reflection) do not see it when looking at the overriding entity class.
mpdude
added a commit
to mpdude/doctrine2
that referenced
this pull request
Mar 8, 2023
…lassMetadataFactory::peekIfIsMappedSuperclass()` method introduced in doctrine#10411 causes a failure. `CMF::peekIfIsMappedSuperclass()` has to perform improvised metadata loading in a situation where the CMF is currently loading a class. So, we cannot use the full/real `ClassMetadataFactory` mechanisms, since it would require a re-entry for a subclass of the current class, causing an infinite loop (loads parent classes first, and that's what we're currently doing). The problem is that the improvised call to `$driver->loadMetadataForClass()` cannot provide a pre-filled `ClassMetadata` instance populated with all parent class fields and associations. But, when attribute or association overrides are used, a check is made to see if the overridden field/association actually exists, and this information is missing in that situation. This PR suggests to override the methods to get around this. In fact, we do not care about all these details, we only want to ask the driver if the class is a mapped superclass or not. A much better fix would be to have a dedicated method on the driver to ask it just that particular question (also better performance-wise). But I do not see how we could get that done in a BC way – ideas? 💡 A few things that need to come together to make the bug surface: * Load an entity declaring an inheritance tree * There must be a mapped superclass in the inheritance tree to provide the field that shall be overriden * An entity class must inherit from the mapped superclass and override the field * That entity class must be an abstract, intermediate class not be declared in the discriminator map so we can "discover" it * The overriden property must be private so the mapping drivers (using reflection) do not see it when looking at the overriding entity class.
mpdude
added a commit
to mpdude/doctrine2
that referenced
this pull request
May 5, 2023
…ses where they are declared This PR will make the annotations and attribute mapping drivers more consistently report mapping configuration for the classes where it is declared. This is necessary to be able to catch mis-configurations in `ClassMetadataFactory`. Fixes doctrine#10417, closes doctrine#10449, closes doctrine#10450, closes doctrine#10454. #### Current situation The annotations mapping driver has the following condition to skip properties that are reported by the PHP reflection API: https://github.com/doctrine/orm/blob/69c7791ba256d947ddb1aafe5f2439ab31704937/lib/Doctrine/ORM/Mapping/Driver/AnnotationDriver.php#L345-L357 This code has been there basically unchanged since the initial 2.0 release. The same condition can be found in the attribute driver, probably it has been copied when attributes were added. I _think_ what the driver tries to do here is to deal with the fact that Reflection will also report `public`/`protected` properties inherited from parent classes. This is supported by the observation (see doctrine#5744) that e. g. YAML and XML drivers do not contain this logic. The conditions are not precise enough for edge cases. They lead to some fields and configuration values not even being reported by the driver. Only since the fields would be "discovered" again when reflecting on subclasses, they eventually end up in class metadata structures for the subclasses. In one case of inherited ID generator mappings, the `ClassMetadataFactory` would also rely on this behaviour. A few of the bugs that can result from this are demonstrated in doctrine#10449, doctrine#10450 and doctrine#10454. #### Suggested solution In order to find a more reliable way of separating properties that are merely reported again in subclasses from those that are actual re-declarations, use the information already available in `ClassMetadata`. In particular, `declared` tells us in which non-transient class a "field" was first seen. Make the mapping driver skip only those properties for which we already know that they have been declared in parent classes, and skip them only when the observed declaring class matches the expectation. For all other properties, report them to `ClassMetadataFactory` and let that deal with consistency checking/error handling. doctrine#10449, doctrine#10450 and doctrine#10454 are merged into this PR to show that they all pass now. #### Soft deprecation strategy When users re-declare (overwrite) mapped properties inherited from mapped superclasses and/or other entities, the new behaviour may cause their mapping configuration to be rejected as invalid. This applies only to configurations that were never allowed as per the documentation. For some cases, we missed the opportunity to reject the configuration with an exception early on. In other cases, we had the exception-throwing code in place, but due to the driver's behaviour, it was never reached. To avoid throwing new/surprising exceptions (even for misconfigurations) during a minor version upgrade, the new driver mode is opt-in. Users will have to set the `$reportFieldsWhereDeclared` constructor parameters to `true` for the `AnnotationDriver` and/or `AttributesDriver`. Unless they do so, a deprecation warning will be raised. In 3.0, the "new" mode will become the default. The constructor parameter can be deprecated (as of ORM 3.1, probably) and is a no-op. We should follow up in other places (DoctrineBundle, ... – what else?) to make this driver parameter an easy-to-change configuration setting. #### Relationship to doctrine#10473 Update: doctrine#10473 was merged ~In doctrine#10473, an example is given where code that currently works will break because the CMF will now see and reject an association mapping that it missed before. I think that the example given is valid, and that the check which will suddenly become effective is, in fact, too strict. So, my recommendation is to treat both PRs as closely related, even though both are valid and can be reviewed in their own right.~
This was referenced May 5, 2023
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Allow to-many associations to be used on mapped superclasses when the owning (inverse) side does not refer back to the mapped superclass, thanks to
ResolveTargetEntityListener.Current situation
The documentation states:
That's a though limitation.
ObviouslyapparentlyProbably the limitation comes from the fact that in a to-many association the "many" side has to hold a foreign key. Since the mapped superclass does not have a database table (it's not an entity), no such backreference can be established.Currently, to-many associations trigger an exception as soon as they are seen on a mapped superclass:
orm/lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php
Lines 459 to 461 in d6c0031
ResolveTargetEntityListenerThe
ResolveTargetEntityListenercan be used to substitute interface or class names in mapping configuration at runtime, during the metadata load phase.When this gimmick is used to replace all references to the mapped superclass with an entity class in time, it should be possible to have to-many associations on the inheriting entity classes.
Suggested solution
Instead of rejecting to-many associations on mapped superclasses right away, validate that at the end of the day (after the
loadClassMetadataevent has been processed) no association may target at a non-entity class. That includes mapped superclasses as well as transient classes.Motivating example
Consider a library that comes with a
Userbase class. This class isabstractand has to be subclassed/filled when the library is used.By making this a mapped superclass, library users have the freedom to either have a simple user entity class or a user class hierarchy, but we do not impose any requirements on them. (NB we also don't want to have a root entity in the library, because that would have to declare the entire class hierarchy, including library users' classes.)
The actual user class to be used will be configured through the
ResolveTargetEntityListener.The library also includes a
SocialMediaAccountentity. AUsercan have multiple of these accounts, and we want to be able to navigate the accounts from the user side.To make the example even more fancy, there is a self-referencing association on the
User: AUserhas been created by another user, and holds a collection of all otherUsers it created.The test case contained in this PR contains this example and validates that all association mappings look just as if the final user class had been written as an entity directly, without the superclass.
Potential review talking points
targetEntitys that are not entities; relevant BC break? (IMHO: no.)Review tip
Review commit by commit, not all files at once. The last commit adds a lot of entity declarations that were previously missed in tests and now raised exceptions; that's a lot of clutter in the PR.
Relationship to #10455
This PR here does not make any changes to the inner workings of the ORM (!).
In fact, the example given can be run with current versions/releases of the ORM with no issues. That is possible because the one config validation rule that would stop it is in fact not effective, since the annotations/attribute mapping drivers do not report the potentially problematic association mapping in the first place.
This would change with #10455. Once that is merged, the to-many association would be dismissed right away and break the example.
So we need this PR here to keep examples like the one given working with #10455, by finding a more precise rule why/when to reject invalid configurations.
Other: This closes #10398, since the check no longer exists.