Improved support for Mapped Superclasses #7825

andrews05 · 2019-09-22T23:12:34Z

This PR makes improvements to support Mapped Superclasses in a number of ways that didn't work previously.

Support for version property declared in mapped superclass
Better support for to-one relationships declared in mapped superclass
Support for mapped superclasses both above and within JTI/STI hierarchies

lcobucci · 2019-09-23T05:37:46Z

@andrews05 thanks for sending this patch. Since this is a bug fix it must be sent to 2.6 instead of master and we need a functional test showing the problem and this fixes it.

andrews05 · 2019-09-23T06:03:25Z

@lcobucci I did look at v2.6 but the code looked quite different in some places so I wasn't sure if I could fix it the same way. (I also wasn't sure if it would really be classified as bug, it's perhaps more a matter of supporting a certain combination of features that wasn't previously supported).

I could try a v2.6 patch as well if you like though. And I will work on putting together a test...

Ocramius · 2019-09-23T08:48:23Z

@andrews05 can you maybe open a separate patch against 2.6 to just highlight the bug? A failing test case would suffice.

lcobucci · 2019-09-23T08:52:06Z

@Ocramius @andrews05 brings a fair point, though.

I think that the bug relates to even allowing to mix a mapped superclass in an STI/JTI and that should trigger a schema validation error. We didn't design for this use case and I don't believe we should support it.

What do you think about it @guilhermeblanco?

andrews05 · 2019-09-24T00:23:13Z

I've added a test. I had to also change ClassMetadata::isInheritedProperty to check for MappedSuperclasses in order for schema creation to work.
The test is currently only testing the case where the root table extends a mappedsuperclass. There are some issues still remaining if a mappedsuperclass exists between the root table and a joined subclass.
Much of the problem revolves around the isInheritedProperty/getDeclaringClass methods. It might be helpful to have another method like ClassMetadata::getTableClass($fieldName) which would return the class of the table containing the given column.

@lcobucci MappedSuperclass with STI already works fine, as far as I can tell. (With the exception of schema creation which is fixed with my latest commit.)

lcobucci · 2019-09-24T05:36:58Z

@lcobucci MappedSuperclass with STI already works fine, as far as I can tell. (With the exception of schema creation which is fixed with my latest commit.)

Can you please provide a test for that too? If we're going to support this scenario it can't break ever again.

andrews05 · 2019-09-25T05:07:16Z

@lcobucci I've updated the test to include STI as well, and also fixed the case where a mapped superclass exists between the root table and a joined subclass.

andrews05 · 2019-09-25T22:34:43Z

I've fixed up coding style and resolved a couple of failing tests. One failure still remains though:
In BasicInheritanceMappingTest it is asserting that a property declared in a mappedsuperclass returns true for isInheritedProperty(). This seems odd to me because as I best I can tell, all other uses of isInheritedProperty are only interested in table inheritance. Any advice?

SenseException · 2019-09-29T19:05:56Z

lib/Doctrine/ORM/Mapping/ClassMetadata.php

+        }
+
+        $parentClass = $this;
+        while (($parentClass = $parentClass->getParent()) !== $declaringClass) {


You override $parentClass here while above you set it to $this. Should it be $this->getParent() to be more readable or is there a reason for this choice? Something similar can be found in ClassMetadata::getRootClassName().

This is just a means of iterating up the hierarchy. The same method is used a number of times already in JoinedSubclassPersister.php.

guilhermeblanco · 2019-11-15T05:09:12Z

@Ocramius @andrews05 brings a fair point, though.

I think that the bug relates to even allowing to mix a mapped superclass in an STI/JTI and that should trigger a schema validation error. We didn't design for this use case and I don't believe we should support it.

What do you think about it @guilhermeblanco?

Actually we originally designed to have any mix of inheritance together, but for simplicity we killed the support for mixing the JTI and STI together, but was always intended to support JTI + MappedSuperclass and STI + MappedSuperclass.

guilhermeblanco · 2019-11-15T05:11:12Z

I feel that most of the parent skipping would be gone once we break the ClassMetadata into more specialized units... I am still curious if there's a better way to go through check of isInheritedProperty and make the test suite pass somehow... maybe check against the getDeclaringClass() ClassMetadata table presence? I don't know yet... would have to dig more to answer this one.

andrews05 · 2019-11-15T06:10:14Z

@guilhermeblanco I guess the question is, what is the correct behaviour of isInheritedProperty()? As best I can tell, all existing uses of this method are wanting to know if the property belongs to the current table or not. Since any properties declared in a mapped superclass actually do belong to the table of the subclass, these will likely break if the method returns true for a mapped superclass. Is the failing test perhaps actually invalid?

guilhermeblanco · 2019-11-15T06:17:04Z

@andrews05 maybe a good alternative would be to start differentiating between inheritedProperty (tracking classes) and inheritedColumn (tracking tables).

andrews05 · 2019-11-15T07:40:57Z

@guilhermeblanco Just to prove a point: Currently, if you declare a to-one relationship in a mapped superclass and then try to find a child class (without STI/JTI) using the relationship as a condition, it will error. This is fixed by the change to isInheritedProperty here. (I could submit a test case but I don't think it's particularly relevant to this PR).

Anyway, your suggestion does seem sensible. I could implement that new method if you like and change all the existing calls to use it. Although the old method would then only exist for the sake of that one test.

andrews05 · 2019-11-15T21:59:00Z

So I'll admit this has turned out to be somewhat more complicated than I initially thought when I first submitted this PR. I've added more tests for relationships and version properties and fixed issues with them via a new method ClassMetadata::getPropertyTableClass().
I've also added the isInheritedColumn() method as @guilhermeblanco suggested, so the BasicInheritanceMappingTest now passes.

guilhermeblanco · 2019-11-18T03:25:49Z

@andrews05 Wow, that is impressive work! Thanks a lot for the work on this. I'm feeling you earned some good internals knowledge while building this patch!
I think I might be able to assume this from now on. I see a few things I could change that would make this patch much easier, so I might just go ahead and do it and then incorporate your PR together. I'll keep this PR open, and add to my todo list to have this supported earlier. Keep in mind I'm working on lazy data types for now, but will soon get back to this.

In case this is something really critical for your team's operation, please let me know and I can try something out.

andrews05 · 2019-11-18T03:40:54Z

Thanks! Yeah, I've definitely been learning a lot more than I intended to. Quite happy to leave it to the pros now :)
Some other thoughts I had that might make things easier:

A getParentTableClass() method, like getParent() but would skip mapped superclasses.
Making use of $property->getTableName().

No hurry for this, take your time!

mpdude · 2021-01-07T08:00:16Z

I think I've just stumbled across #5998 mentioned in the initial comment, and created #8415 to fix it.

Everyone involved in this discussion here seems to have a deeper understanding of how things work together internally, so I'd really appreciate your reviews over at #8415 to see if it suffices, misses edge cases or could help to get this one here along as well?

Co-authored-by: Andrew <andrew@digerati.co.nz>

mpdude · 2021-01-07T08:30:50Z

@andrews05 I've cherry-picked your functional test over at #8415 and it passes.

Co-authored-by: Andrew <andrew@digerati.co.nz>

@id

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.

@id

…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.

@id

…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.

andrews05 added 4 commits September 23, 2019 11:04

Skip MappedSuperclass during getJoinSql

896f3b3

Fix inserting with MappedSuperclass

95fcabb

Fix selecting with MappedSuperclass

b8bb1f7

Fix deleting & locking with MappedSuperclass

3f37638

andrews05 changed the title ~~Skip MappedSuperclass during getJoinSql~~ Fix using JOINED inheritance with MappedSuperclass Sep 23, 2019

lcobucci added Bug Missing Tests labels Sep 23, 2019

andrews05 added 2 commits September 24, 2019 11:56

Add Test

83e0903

Fix creating schema with MappedSuperclass

1f16ff6

andrews05 added 3 commits September 25, 2019 16:16

Also test STI

56d496b

Fix getRootClassName for mapped superclasses

4bd3d08

Update test with multiple mapped superclasses

5388c77

andrews05 added 5 commits September 26, 2019 08:22

Fix CS

5c35053

Fix getInsertColumnList

c771e07

Use isRootEntity to check parentColumns

cdd1d17

Improve isInheritedProperty

7b97b32

Test a relationship

e0787a2

SenseException reviewed Sep 29, 2019

View reviewed changes

Fix SqlWalker

1e4ae08

guilhermeblanco added this to the 3.0 milestone Nov 15, 2019

guilhermeblanco added Requires merge/review WIP labels Nov 15, 2019

andrews05 added 4 commits November 15, 2019 21:26

Test sqlwalker

cf6f117

Don't create inherited relations on child table

9710221

Merge branch 'master' into andrews05-mappedsuperclass

9ff8ebe

Fixes for rels & version

2197f24

andrews05 changed the title ~~Fix using JOINED inheritance with MappedSuperclass~~ Improved support for Mapped Superclasses Nov 15, 2019

mpdude mentioned this pull request Jan 7, 2021

Fix association handling when there is a MappedSuperclass in the middle of an inheritance hierarchy #8415

Merged

mpdude added a commit to mpdude/doctrine2 that referenced this pull request Jan 7, 2021

Cherry-pick GH5998Test from doctrine#7825

ea2ad89

Co-authored-by: Andrew <andrew@digerati.co.nz>

mpdude added a commit to mpdude/doctrine2 that referenced this pull request Jan 7, 2021

Cherry-pick GH5998Test from doctrine#7825

4414090

Co-authored-by: Andrew <andrew@digerati.co.nz>

mpdude added a commit to mpdude/doctrine2 that referenced this pull request Jan 7, 2021

Cherry-pick GH5998Test from doctrine#7825

5c0d4bf

Co-authored-by: Andrew <andrew@digerati.co.nz>

mpdude added a commit to mpdude/doctrine2 that referenced this pull request Jan 11, 2021

Cherry-pick GH5998Test from doctrine#7825

3727150

Co-authored-by: Andrew <andrew@digerati.co.nz>

mpdude added a commit to mpdude/doctrine2 that referenced this pull request Jan 11, 2021

Cherry-pick GH5998Test from doctrine#7825

d0f1891

Co-authored-by: Andrew <andrew@digerati.co.nz>

mpdude added a commit to mpdude/doctrine2 that referenced this pull request Feb 9, 2021

Cherry-pick GH5998Test from doctrine#7825

719fb73

Co-authored-by: Andrew <andrew@digerati.co.nz>

Base automatically changed from master to old-prototype-3.x February 23, 2021 08:19

mpdude added a commit to mpdude/doctrine2 that referenced this pull request Jun 7, 2021

Cherry-pick GH5998Test from doctrine#7825

bcb2e42

Co-authored-by: Andrew <andrew@digerati.co.nz>

greg0ire mentioned this pull request Jun 17, 2021

Doctrine ORM 2.9: MappedSuperclass in entity hierarchy #8771

Closed

olsavmic mentioned this pull request Aug 9, 2021

SchemaValidator: Fix mapped superclass missing in discriminator map #8903

Merged

mpdude added a commit to mpdude/doctrine2 that referenced this pull request Feb 17, 2022

Cherry-pick GH5998Test from doctrine#7825

b2f2736

Co-authored-by: Andrew <andrew@digerati.co.nz>

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved support for Mapped Superclasses #7825

Improved support for Mapped Superclasses #7825

andrews05 commented Sep 22, 2019 •

edited

Loading

lcobucci commented Sep 23, 2019

andrews05 commented Sep 23, 2019

Ocramius commented Sep 23, 2019

lcobucci commented Sep 23, 2019

andrews05 commented Sep 24, 2019

lcobucci commented Sep 24, 2019

andrews05 commented Sep 25, 2019

andrews05 commented Sep 25, 2019

SenseException Sep 29, 2019

andrews05 Oct 1, 2019

guilhermeblanco commented Nov 15, 2019

guilhermeblanco commented Nov 15, 2019

andrews05 commented Nov 15, 2019

guilhermeblanco commented Nov 15, 2019

andrews05 commented Nov 15, 2019

andrews05 commented Nov 15, 2019

guilhermeblanco commented Nov 18, 2019

andrews05 commented Nov 18, 2019 •

edited

Loading

mpdude commented Jan 7, 2021

mpdude commented Jan 7, 2021 •

edited

Loading

Improved support for Mapped Superclasses #7825

Improved support for Mapped Superclasses #7825

Conversation

andrews05 commented Sep 22, 2019 • edited Loading

lcobucci commented Sep 23, 2019

andrews05 commented Sep 23, 2019

Ocramius commented Sep 23, 2019

lcobucci commented Sep 23, 2019

andrews05 commented Sep 24, 2019

lcobucci commented Sep 24, 2019

andrews05 commented Sep 25, 2019

andrews05 commented Sep 25, 2019

SenseException Sep 29, 2019

Choose a reason for hiding this comment

andrews05 Oct 1, 2019

Choose a reason for hiding this comment

guilhermeblanco commented Nov 15, 2019

guilhermeblanco commented Nov 15, 2019

andrews05 commented Nov 15, 2019

guilhermeblanco commented Nov 15, 2019

andrews05 commented Nov 15, 2019

andrews05 commented Nov 15, 2019

guilhermeblanco commented Nov 18, 2019

andrews05 commented Nov 18, 2019 • edited Loading

mpdude commented Jan 7, 2021

mpdude commented Jan 7, 2021 • edited Loading

andrews05 commented Sep 22, 2019 •

edited

Loading

andrews05 commented Nov 18, 2019 •

edited

Loading

mpdude commented Jan 7, 2021 •

edited

Loading