Document the meanings of 'inherited' and 'declared' in field mapping information #10396
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
document-inherited-declared-meaning
branch
from
1f45d80 to
f72f6b1
Compare
greg0ire
reviewed
Jan 13, 2023
Co-authored-by: Grégoire Paris <[email protected]>
greg0ire
approved these changes
Jan 14, 2023
SenseException
approved these changes
Jan 14, 2023
mpdude
added a commit
to mpdude/doctrine2
that referenced
this pull request
Jan 17, 2023
1. When inheriting a to-one association from a mapped superclass, update the `sourceEntity` class name to the current class only when the association is actually _declared_ in the mapped superclass. 2. Reject association types that are not allowed on mapped superclasses only when they are actually _declared_ in a mapped superclass, not when inherited from parent classes. Currently, when a many-to-one association is inherited from a `MappedSuperclass`, mapping information will be updated so that the association has the current (inheriting) class as the source entity. https://github.com/doctrine/orm/blob/2138cc93834cfae9cd3f86c991fa051a3129b693/lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php#L384-L393 This was added in 7dc8ef1 for [DDC-671](doctrine#5181). The reason for this is that a mapped superclass is not an entity itself and has no table. So, in the database, associations can only be from the inheriting entities' tables towards the referred-to target. This is also the reason for the limitation that only to-one associations may be added in mapped superclasses, since for those the database foreign key can be placed on the table(s) of the inheriting entities (and there may be more than one child class). Neither the decision to update the `sourceEntity` nor the validation check should be based on `$parent->isMappedSuperclass`. This only works in the simple case where the class hierarchy is `Mapped Superclass → Entity`. The check is wrong when we have an inheritance hierarchy set up and the class hierarchy is `Base Entity → Mapped Superclass → Child Entity`. Bug 1: The association should keep the root entity as the source. After all, in a JTI, the root table will contain the foreign key, and we need to base joins on that table when traversing `FROM LeafClass l JOIN l.target`. Bug 2: Do not reject the to-many association declared in the base class. It is ok to have the reverse (owning) side point back to the base entity, as it would be if there were no mapped superclasses at all. The mapped superclass does not declare, add or otherwise interfere with the to-many association at all. Base the decision to change the `sourceEntity` on `$mapping['inherited']` being set. This field points to the topmost _parent entity_ class in the ancestry tree where the relationship mapping appears for the first time. When it is not set, the current class is the first _entity_ class in the hierarchy with that association. Since we are inheriting the relation, it must have been added in a mapped superclass above, but was not yet present in the nearest parent entity class. In that case, it may only be a to-one association and the source entity needs to be updated. (See doctrine#10396 for a clarification of the semantics of `inherited`.) Here is a simplified example of the class hierarchy. See the two tests added for more details – one is for checking the correct usage of a to-one association against/with the base class in JTI. The other is to test that a to-many association on the base class is not rejected. I am sure that there are other tests that (still) cover the update of `sourceEntity` is happening. ```php /** * @entity */ class AssociationTarget { /** * @column(type="integer") * @id * @GeneratedValue */ public $id; } /** * @entity * @InheritanceType("JOINED") * @DiscriminatorColumn(name="discriminator", type="string") * @DiscriminatorMap({"1" = "BaseClass", "2" = "LeafClass"}) */ class BaseClass { /** * @column(type="integer") * @id * @GeneratedValue */ public $id; /** * @manytoone(targetEntity="AssociationTarget") */ public $target; } /** * @MappedSuperclass */ class MediumSuperclass extends BaseClass { } /** * @entity */ class LeafClass extends MediumSuperclass { } ``` When querying `FROM LeafClass l`, it should be possible to `JOIN l.target`. This currently leads to an SQL error because the SQL join will be made via `LeafClass.target_id` instead of `BaseClass.target_id`. `LeafClass` is considered the `sourceEntity` for the association – which is wrong–, and so the foreign key field is expected to be in the `LeafClass` table (using JTI here). Fixes doctrine#5998, fixes doctrine#7825. I have removed the abstract entity class, since it is not relevant for the issue and took the discussion off course. Also, the discriminator map now contains all classes. Added the second variant of the bug, namely that a to-many association would wrongly be rejected in the same situation.
mpdude
added a commit
to mpdude/doctrine2
that referenced
this pull request
Jan 17, 2023
…le of an inheritance hierarchy This fixes two closely related bugs. 1. When inheriting a to-one association from a mapped superclass, update the `sourceEntity` class name to the current class only when the association is actually _declared_ in the mapped superclass. 2. Reject association types that are not allowed on mapped superclasses only when they are actually _declared_ in a mapped superclass, not when inherited from parent classes. Currently, when a many-to-one association is inherited from a `MappedSuperclass`, mapping information will be updated so that the association has the current (inheriting) class as the source entity. https://github.com/doctrine/orm/blob/2138cc93834cfae9cd3f86c991fa051a3129b693/lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php#L384-L393 This was added in 7dc8ef1 for [DDC-671](doctrine#5181). The reason for this is that a mapped superclass is not an entity itself and has no table. So, in the database, associations can only be from the inheriting entities' tables towards the referred-to target. This is also the reason for the limitation that only to-one associations may be added in mapped superclasses, since for those the database foreign key can be placed on the table(s) of the inheriting entities (and there may be more than one child class). Neither the decision to update the `sourceEntity` nor the validation check should be based on `$parent->isMappedSuperclass`. This only works in the simple case where the class hierarchy is `Mapped Superclass → Entity`. The check is wrong when we have an inheritance hierarchy set up and the class hierarchy is `Base Entity → Mapped Superclass → Child Entity`. Bug 1: The association should keep the root entity as the source. After all, in a JTI, the root table will contain the foreign key, and we need to base joins on that table when traversing `FROM LeafClass l JOIN l.target`. Bug 2: Do not reject the to-many association declared in the base class. It is ok to have the reverse (owning) side point back to the base entity, as it would be if there were no mapped superclasses at all. The mapped superclass does not declare, add or otherwise interfere with the to-many association at all. Base the decision to change the `sourceEntity` on `$mapping['inherited']` being set. This field points to the topmost _parent entity_ class in the ancestry tree where the relationship mapping appears for the first time. When it is not set, the current class is the first _entity_ class in the hierarchy with that association. Since we are inheriting the relation, it must have been added in a mapped superclass above, but was not yet present in the nearest parent entity class. In that case, it may only be a to-one association and the source entity needs to be updated. (See doctrine#10396 for a clarification of the semantics of `inherited`.) Here is a simplified example of the class hierarchy. See the two tests added for more details – one is for checking the correct usage of a to-one association against/with the base class in JTI. The other is to test that a to-many association on the base class is not rejected. I am sure that there are other tests that (still) cover the update of `sourceEntity` is happening. ```php /** * @entity */ class AssociationTarget { /** * @column(type="integer") * @id * @GeneratedValue */ public $id; } /** * @entity * @InheritanceType("JOINED") * @DiscriminatorColumn(name="discriminator", type="string") * @DiscriminatorMap({"1" = "BaseClass", "2" = "LeafClass"}) */ class BaseClass { /** * @column(type="integer") * @id * @GeneratedValue */ public $id; /** * @manytoone(targetEntity="AssociationTarget") */ public $target; } /** * @MappedSuperclass */ class MediumSuperclass extends BaseClass { } /** * @entity */ class LeafClass extends MediumSuperclass { } ``` When querying `FROM LeafClass l`, it should be possible to `JOIN l.target`. This currently leads to an SQL error because the SQL join will be made via `LeafClass.target_id` instead of `BaseClass.target_id`. `LeafClass` is considered the `sourceEntity` for the association – which is wrong–, and so the foreign key field is expected to be in the `LeafClass` table (using JTI here). Fixes doctrine#5998, fixes doctrine#7825. I have removed the abstract entity class, since it is not relevant for the issue and took the discussion off course. Also, the discriminator map now contains all classes. Added the second variant of the bug, namely that a to-many association would wrongly be rejected in the same situation.
mpdude
added a commit
to mpdude/doctrine2
that referenced
this pull request
Jan 17, 2023
…le of an inheritance hierarchy This fixes two closely related bugs. 1. When inheriting a to-one association from a mapped superclass, update the `sourceEntity` class name to the current class only when the association is actually _declared_ in the mapped superclass. 2. Reject association types that are not allowed on mapped superclasses only when they are actually _declared_ in a mapped superclass, not when inherited from parent classes. Currently, when a many-to-one association is inherited from a `MappedSuperclass`, mapping information will be updated so that the association has the current (inheriting) class as the source entity. https://github.com/doctrine/orm/blob/2138cc93834cfae9cd3f86c991fa051a3129b693/lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php#L384-L393 This was added in 7dc8ef1 for [DDC-671](doctrine#5181). The reason for this is that a mapped superclass is not an entity itself and has no table. So, in the database, associations can only be from the inheriting entities' tables towards the referred-to target. This is also the reason for the limitation that only to-one associations may be added in mapped superclasses, since for those the database foreign key can be placed on the table(s) of the inheriting entities (and there may be more than one child class). Neither the decision to update the `sourceEntity` nor the validation check should be based on `$parent->isMappedSuperclass`. This only works in the simple case where the class hierarchy is `Mapped Superclass → Entity`. The check is wrong when we have an inheritance hierarchy set up and the class hierarchy is `Base Entity → Mapped Superclass → Child Entity`. Bug 1: The association should keep the root entity as the source. After all, in a JTI, the root table will contain the foreign key, and we need to base joins on that table when traversing `FROM LeafClass l JOIN l.target`. Bug 2: Do not reject the to-many association declared in the base class. It is ok to have the reverse (owning) side point back to the base entity, as it would be if there were no mapped superclasses at all. The mapped superclass does not declare, add or otherwise interfere with the to-many association at all. Base the decision to change the `sourceEntity` on `$mapping['inherited']` being set. This field points to the topmost _parent entity_ class in the ancestry tree where the relationship mapping appears for the first time. When it is not set, the current class is the first _entity_ class in the hierarchy with that association. Since we are inheriting the relation, it must have been added in a mapped superclass above, but was not yet present in the nearest parent entity class. In that case, it may only be a to-one association and the source entity needs to be updated. (See doctrine#10396 for a clarification of the semantics of `inherited`.) Here is a simplified example of the class hierarchy. See the two tests added for more details – one is for checking the correct usage of a to-one association against/with the base class in JTI. The other is to test that a to-many association on the base class is not rejected. I am sure that there are other tests that (still) cover the update of `sourceEntity` is happening. ```php /** * @entity */ class AssociationTarget { /** * @column(type="integer") * @id * @GeneratedValue */ public $id; } /** * @entity * @InheritanceType("JOINED") * @DiscriminatorColumn(name="discriminator", type="string") * @DiscriminatorMap({"1" = "BaseClass", "2" = "LeafClass"}) */ class BaseClass { /** * @column(type="integer") * @id * @GeneratedValue */ public $id; /** * @manytoone(targetEntity="AssociationTarget") */ public $target; } /** * @MappedSuperclass */ class MediumSuperclass extends BaseClass { } /** * @entity */ class LeafClass extends MediumSuperclass { } ``` When querying `FROM LeafClass l`, it should be possible to `JOIN l.target`. This currently leads to an SQL error because the SQL join will be made via `LeafClass.target_id` instead of `BaseClass.target_id`. `LeafClass` is considered the `sourceEntity` for the association – which is wrong–, and so the foreign key field is expected to be in the `LeafClass` table (using JTI here). Fixes doctrine#5998, fixes doctrine#7825. I have removed the abstract entity class, since it is not relevant for the issue and took the discussion off course. Also, the discriminator map now contains all classes. Added the second variant of the bug, namely that a to-many association would wrongly be rejected in the same situation.
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.
While trying to understand and debug inheritance-related issues, I figured I have no clear idea what
inheritedanddeclaredactually means.This PR tries to document my findings, hopefully in a place where others might look for it.