Fix single table inheritance with intermediate abstract class(es) #10643
Conversation
|
|
||
| foreach ($this->class->subClasses as $subclassName) { | ||
| $values[] = $this->conn->quote($discrValues[$subclassName]); | ||
| array_unshift($values, $this->conn->quote($this->class->discriminatorValue)); | ||
| } |
There was a problem hiding this comment.
A different approach would be extending of ClassMetadata(Info) with something in the lines of discriminatorSubClasses. This would cause more work and refactoring, though.
There was a problem hiding this comment.
As the author of #10411 I can approve this.
Before #10411, all subclasses had to be declared in the discriminator map, or you'd get strange errors elsewhere.
Since #10411, abstract entity classes may be omitted in the discriminator map, and they'd be filled into \Doctrine\ORM\Mapping\ClassMetadataInfo::$subClasses automatically. That, in turn, means there may be subClasses without corresponding discriminator map entries.
We cannot look at the discriminator map only, since it contains all classes from the inheritance tree. In this situation here, we are only looking for those at or below the current class, so we need to intersect both subClasses and the discriminatorMap.
|
I'm not quite sure, that all subclasses had to be in the discriminator map before #10411 when the documentation only included concrete implementations itself, e.g. in [1]. On the other hand the annotation documentation is a bit vague [2]
Though this doesn't address abstract classes, it only refers to database backed entities, which IMHO shouldn't be abstract anyway. [1] https://www.doctrine-project.org/projects/doctrine-orm/en/2.12/cookbook/decorator-pattern.html |
|
Having all classes in the DM was a recommendation given (eg at #8736 (comment)) to work around some quirks when not all subclasses were in the #10411 was a change to fill in the missing subclasses automatically, so that only non-abstract entity classes need to be listed in the DM. |
|
Ah, thanks for the explanation and clarification. We probably just had luck for all these years then 🙈 |
|
@greg0ire can you help us to get this along? Changes are reasonable from my POV. Do you see anything that should be addressed? |
|
Argh! The target branch was wrong. Let me create a merge up PR |
We need to make sure to query the discriminator map only with contained classes, otherwise we end up trying to access it with e.g. abstract sub-classes of the current class, which are not to be contained in the discriminator map. This is caused by the introduction of auto-discovering of missing sub-classes in #10411 which also added abstract classes to ClassMetadata::subClasses.
Fixes #10625
Test based on PR #10411