DDC-3480: ORM\Embeddable does not create ManyToOne column in the database
Closed this issue · 56 comments
Jira issue originally created by user tvoslar:
Following Embeddable
/****
* @ORM\Embeddable
*/
class Address
{
/****
* @ORM\ManyToOne(targetEntity="Country")
*/
protected $country;
won't save country attribute into the database table, other simple attributes (like type="text") saved normally.
/****
* @ORM\Embedded(class="LuciniLucini\Adsender\CommonBundle\Entity\Address")
*/
private $address;
Comment created by @Ocramius:
The issue seems incomplete to me: what exactly is the failure? Can you make an example?
Comment created by tvoslar:
Okay, I am sorry. I have main entity which includes Embeddable entity (Address), this address entity contains country attribute with ManyToOne relation, but this country is not being saved into the database table, other attributes like street (string) are being saved normally.
Comment created by tvoslar:
Basically it creates main entity with all fields from embeddable except country which is ManyToOne.
Comment created by @Ocramius:
Ah, now I get it (I was probably misreading it). We don't support associations from embeddables right now: please check the test suite, but they really don't provide this functionality right now.
Comment created by tvoslar:
And is there any chance it will be supported in the near future? Or it's not possible?
Comment created by @Ocramius:
It will probably not implemented for now, as embeddables (in our vision) are fitting the use-case of ValueObjects. ValueObjects are (usually) supposed to be containing serializable data, and an entity reference is not serializable data.
Comment created by tvoslar:
Okay, understood. So it's not good idea to use embeddables for Address entity for example, with this country as relation to another table, right? It seemed to me like best case. Thank you for the support anyway.
Comment created by @Ocramius:
So it's not good idea to use embeddables for Address entity for example, with this country as relation to another table, right?
I'd rather just save the country identifier in the embeddable instead.
Comment created by tvoslar:
So you mean, instead of ManyToOne, save just id=integer (so no relation with entity)?
Comment created by @Ocramius:
Correct: simple scalar or serializable types
Comment created by tvoslar:
Line 121 method testDqlOnEmbeddedObjectsField, guess I am wrong, but it seems there should be support for embeddable with relations though.
Comment created by Yavin:
Hi,
if is is not supported i think an exception should be thrown when someone use association in embedded class. Now it is just silently ommited.
I found code where embedded metadata is added:
https://github.com/doctrine/doctrine2/blob/573153669c11a6f69201513831d3b8ce5e111d25/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php#L3200
Comment created by eugene-d:
It will probably not implemented for now, as embeddables (in our vision) are fitting the use-case of ValueObjects. ValueObjects are (usually) supposed to be containing serializable data, and an entity reference is not serializable data.
This should be explicitly stated in the documentation.
Comment created by @Ocramius:
[~eugene-d] please send a PR to the documentation with the wording you prefer.
Comment created by eugene-d:
[~ocramius] http://www.doctrine-project.org/jira/browse/[DDC-3556](http://www.doctrine-project.org/jira/browse/DDC-3556)
Any chance for embedded relations in 2016?
@scaytrase no, the proper patch would be #5809
Breaking the relation by just replacing it with and integer is not terrible IMHO.
The case of the address embeddable class is very useful and most of the case, the country is a relation.
I understand the goal of value object, but if you are telling to save an ID, why not just tell the serializer to save the id of the relation?
In any case, the usage should be the concern of the developer IMHO.
@Ocramius Would you accept a PR integrating relation on embedded objects?
@soullivaneuh no, I've been clear on that. The correct patch is #5809, but it is incomplete.
It will probably not implemented for now
For now does not mean forever. 😛
Well, I guess I tried.
What would you suggest if we want to store the relation id onto the embeddable object like you said and still acquire the related object on entity load?
@soullivaneuh storing the identifier is an acceptable solution. Loading the related entity would then require looping back to the ORM.
Loading the related entity would then require looping back to the ORM.
I'm sorry but I don't fully understand. Could you please elaborate a bit? What do you mean by looping back to the ORM? Maybe could you provide an example?
Thanks for your time.
I mean by using EntityManager#find() or EntityRepository#find() instead of relying on lazy loading.
I guess it's retarded to not allow associations on embeddables just because for some it represents VO's.
If i want to use the VO concept on my project i simply won't use associations on my embeddables. However if i want to use embeddables for other purposes such as split of concerns and composition of objects to avoid inheritance (as good oop practice). Doctrine is suposed to be a flexible framework and not stand in the way i want to design my project because of some extremist view.
Every pattern depends on context and might not be the answer for every problem. The address and country is a clear case that the embeddables are not exclusively VO's.
And don't forget @Ocramius, doctrine is not exactly a DDD implementation, its an ORM and its perfectly fine if people wants to use it without the DDD concept.
I guess it's retarded to not allow associations on embeddables just because for some it represents VO's.
I'm very well aware that I'm retarded :-)
And don't forget @Ocramius, doctrine is not exactly a DDD implementation, its an ORM and its perfectly fine if people wants to use it without the DDD concept.
It is not, but every foot-gun we can avoid shipping is helpful.
Every pattern depends on context and might not be the answer for every problem. The address and country is a clear case that the embeddables are not exclusively VO's.
Just design them as associations or entities: nothing wrong with that.
Closing here meanwhile: if someone wants to take on the job of adding association mapping validation (preventing it, specifically) on metadata loading, feel free to do so.
Do you realize that adding more associations on a large project mey represent a big performance hit?
if i want to decompose my objects without affecting database performance on my projects i simply can't?
The hability to decompose objects while keeping a single table to avoid joins (and sometimes the responsability for hold some of the associations lies within those separeted objects) should not be restricted by the library. The correct use of VO's should be the developer's responsability.
And i'm sorry for the "retarded" word, it wasn't meant to offend you, it was to towards the idea in question and not you as a person. You are not retarded you are a very well respected professional and a really smart person.
Since Embeddables is reserved to be VO. Why not create similar mechanism to allow required assotiations functionallity??
That's what you call "entities" :-)
@Ocramius nope! Entities requires distinct table. But I mean just an object inside entity object, this object has relations with other entities. I need that decomposition because this nested object has special domain meaning and will be used-and-type hinted in some methods. Well, at this point it might be made as entity, but imagine if I need to do such decomposition many times for many entities - now I have to make one extra table and one more query for every usage. This not critical in my case but neither practical and optimal.
@Ocramius I completely agree with @Arkemlar. Embeddables is a fantastic feature but it breaks the object flow having to save foreign keys directly instead of using an association. Also, VO Embeddables could just as easily be called "entities" - they provide no additional functionality over entities, just clean and logical grouping. Even if the performance hit for a join that you mentioned above is trivial, it breaks my code design and logical grouping having to move related associations outside of the Embeddable, and that for me somewhat defeats the point of having them in the first place.
@ryall Ocramius's point seems clear: he wont agree (at any case) with idea of making Embeddables to serve any other pattern/needs than VO. The question is: will @Ocramius agree if we (community) implement required functionality with other kind of objects (for example "EntityExtensions") - similar to Embeddables but with relations allowed?
@Arkemlar Why would he agree to that if he won't allow it in Embeddables? His decision to disallow associations seems to be somewhat unpopular. There's also no good argument that I've heard about why this should be so.
Actually, it would be nicer and potentially solve this problem indirectly if Doctrine were made more modular and extensible which would allow us to then just replace the bits we want. Currently it's quite difficult and often impossible to extend it without editing the source directly. I hope 3.0 will be more than just a few shiny new features - Doctrine needs a complete refactor IMO.
@Ocramius You still haven't really answered the question as to why they need to be value objects? Is it for compatibility reasons, a limitation of the system, or something else?
Maybe this is a little off topic but I'm happy to hear about the Metadata refactoring. I have no idea what you have planned but having one annotation per column type (ie StringColumn, IntegerColumn) and making these extensible (instead of only basic types) would be a very nice improvement.
@Ocramius OK so if that's the case, if @Arkemlar's suggestion were to be implemented with a new type, such as @Group or @EntityExtension, would this be acceptable for a pull request? Owning-side associations would be easy enough to implement and there'd need to be some logic, and possibly additional association configuration, for non-owning-side associations.
@ryall unlikely: we are moving our focus to 3.x, where embeddables have been completely removed for now, and will need to be re-introduced/redesigned.
For the reference, since @lcobucci and I spoke about this issue.
Embeddables will be re-implemented, properly implemented. That should allow us to follow JPA 2.1, which states:
An embeddable class (including an embeddable class within another embeddable
class) may contain a collection of a basic type or other embeddable class.[16]
An embeddable class may contain a relationship to an entity or collection of
entities. Since instances of embeddable classes themselves have no persistent
identity, the relationship from the referenced entity is to the entity that
contains the embeddable instance(s) and not to the embeddable itself.[17] An
embeddable class that is used as an embedded id or as a map key must not contain
such a relationship.
@guilhermeblanco It's a great news! :-)
Do we already have commit about the concerned chance or is this not worked yet?
@guilhermeblanco Is there a corresponding PR for this feature or do you publish any progress in this issue?
Doctrine 3 can't come soon enough!
Still actual in 2020
up ?
up?
Still actual
up
six years.. good news. nothing changed!
We should throw an exception
Reminder that this is intentional: embeddable types do not include associations by design.
…
On Fri, Jun 11, 2021, 15:54 Kirill @.***> wrote: six years.. good news. nothing changed! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4291 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABFVEE4ZO5ELDDXYVAJA3TTSIIQFANCNFSM4CLOQYYQ .
I cant believe this, the comunity is asking for this for 6 years, meaning that the comunity disagree with your design, even other maintainers agreed with the idea years ago. A framework should be flexible for all use cases, not only for those implementing DDD
@jhonatanTeixeira The community gets this software for free. The software is shipped as is. This change is not easy to implement. Feel free to invest a few weeks of work into this, as I expect this will take. Locking this conversation now.