FEATURE: Add support for embedded ValueObjects #426
Conversation
|
@albe, thanks for your PR! By analyzing the annotation information on this pull request, we identified @kdambekalns, @afoeder and @skurfuerst to be potential reviewers |
albe
force-pushed
the
embedded_valueobjects
branch
3 times, most recently
from
0375d33
to
d48df4c
Compare
|
Regarding the work-around commit, see also #439 |
This changeset adds support for embedded ValueObjects by using the Embeddable features of Doctrine ORM 2.5. All ValueObjects can be made embedded by setting the annotation option "embedded=true". Depends on FLOW-260 FLOW-257 #close
Also, embedded Value Objects are not to be considered transient for the AnnotationDriver at all.
albe
force-pushed
the
embedded_valueobjects
branch
from
d48df4c
to
fd528f0
Compare
|
Why that breaks the Embeddables now is beyond me atm. Will investigate, so hopefully we can deliver this feature soon after the 3.3 release |
|
Ok, found the issue, will push once GH allows me to. |
|
Ok, this is ready for final review IMO |
|
Making Value Objects great again. (= |
|
So, hm. A feature for a released branch… I guess it's worth it, since it delivers one of the key features from ORM 2.5. Judging from the ❤️ reaction @kitsunet agrees? |
|
Yes, @albe and me agreed on this beforehand ;) |
| @@ -732,7 +737,13 @@ protected function evaluatePropertyAnnotations(ClassMetadataInfo $metadata) | |||
| break; | |||
| default: | |||
| if (strpos($propertyMetaData['type'], '\\') !== false) { | |||
| if ($this->reflectionService->isClassAnnotatedWith($propertyMetaData['type'], Flow\ValueObject::class)) { | |||
| if ($valueObjectAnnotation = $this->reflectionService->getClassAnnotation($propertyMetaData['type'], Flow\ValueObject::class)) { | |||
There was a problem hiding this comment.
I am not really happy with this block, first please no value assignment in if statements, absolute no go for me. Second, why the continue 2? I see only one foreach here and also none of the other cases/ifs has a continue.
There was a problem hiding this comment.
Yeah, I'll have to recheck, but it didn't work out first time when I simply "continue;"'d, so I just quickly tried the "continue 2;" - might have been some caching weirdness.
There was a problem hiding this comment.
But can't we work around it in any way?
There was a problem hiding this comment.
Ok, just checked again, I get following error during warmup:
Failure while evaluating property annotations for class "TYPO3\Flow\Tests\Functi
onal\Persistence\Fixtures\TestEntity": Property "embeddedValueObject" in "TYPO3\
Flow\Tests\Functional\Persistence\Fixtures\TestEntity" was already declared, but
it must be declared only once
Type: Doctrine\ORM\Mapping\MappingException
Code: 1382003497
File:
...Data\Temporary\Testing\Cache\Code\Flow_Object_Classes\TYPO3_Flow_Persistence_Doctrine_Mapping_Driver_FlowAnnotationDriver.php
Line: 414
maybe you can verify.
There was a problem hiding this comment.
Yeah ok, now I get it too - the "2" is necessary to also skip out of the switch statement - it basically functions as a "break & continue;"
PS: Regarding the assignment inside if, I'm with you - that was a takeover of the rest of the coding style from the AnnotationDriver which again was taken over from Doctrine. I'm fine with changing our own additions at least.
|
Left a comment... |
|
Woohoo :) |
| The design/architectural answer is: because a value object might just | ||
| be more fitting your problem at hand. | ||
| The technical answer is: because value objects are immutable and | ||
| therefore avoid aliasing ([#aliasing]) problems, which are common cause |
There was a problem hiding this comment.
I'd suggest "are a common cause"
|
Ah indeed, I'll push a follow up to fix that typo! |
This change adds support for embedded ValueObjects by using the Embeddable features
of Doctrine ORM 2.5.
All ValueObjects can be made embedded by setting the annotation option
embedded=true.Depends on FLOW-260
FLOW-257 #close