Fix inherited embeddables and nesting after AnnotationDriver change #8006 #8036
Conversation
malarzm
force-pushed
the
gh-8031-extending-embeddable
branch
from
526553e
to
f3402cd
Compare
| @@ -277,6 +277,8 @@ public function loadMetadataForClass($className, ClassMetadata $metadata) | |||
| /* @var $property \ReflectionProperty */ | |||
| foreach ($class->getProperties() as $property) { | |||
| if ($metadata->isMappedSuperclass && ! $property->isPrivate() | |||
There was a problem hiding this comment.
I believe the time has come to move the whole expression of this if condition into a function :-)
| @@ -401,7 +401,7 @@ private function getShortName($className) | |||
| private function addInheritedFields(ClassMetadata $subClass, ClassMetadata $parentClass) | |||
| { | |||
| foreach ($parentClass->fieldMappings as $mapping) { | |||
| if ( ! isset($mapping['inherited']) && ! $parentClass->isMappedSuperclass) { | |||
| if ( ! isset($mapping['inherited']) && ! $parentClass->isMappedSuperclass && ! $parentClass->isEmbeddedClass) { | |||
There was a problem hiding this comment.
Couldn't you use the isEntity method here? This function is used during mapping construction time, so its not critical here to avoid too many function calls.
beberlei
changed the title
|
It seems like these changes are treating embeddable objects as mapped superclasses... can we also make sure we don't add support to associations for embeddable? This would be quite odd design-wise since they're value objects. |
|
@lcobucci I am afraid adding relationships to embeddables was possible before or not? If it was possible before I don't think we should prevent adding associations though, since Doctrine doesn't prescribe a paradigm such as DDD, we don't really have value objects here but embeddables. |
|
It wasn't possible before (as far as I remember) |
|
I confirm using references wasn't possible as I was really sad when I found out ;) |
|
If you want to expand the concept of an embeddable, that's fine. I'd just suggest to avoid, as much as, doing that on a patch release. We'd also have to make sure that DQL works properly, btw. |
I agree, that'd be a new feature anyway
Is this about current patch or still references? |
|
References 😁 |
|
There isn't a specific error on 2.7.0 with associations on embedded objects though. They are just not stored. The embeddable has the association, but its not copied over to the Entity. The same is true of 2.7.2 with this patch. So there is nothing we need to change. |
|
Added this issue to track improvement to schema validator to disallow this: #8052 |
…change doctrine#8006 (doctrine#8036)" This reverts commit a9b6b72.
Changing
AnnotationDriverto mark embeddables as "entities" in #8006 caused a bunch of bugs with inheritance and nested embeddables that are fixed by this PR.