Value objects (Based on #634) #835

schmittjoh · 2013-11-01T19:49:59Z

This is PR #634 with the following additional changes:

Merged into master (fixed some conflict in Metadata classes)
Support for DQL queries on fields of embedded objects

… to make parsing work.

…lectionProxy object, bypassing changes to UnitOfWork, Persisters and Hydrators.

…sumptions.

…L test with Object and Array Hydration.

Conflicts: lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php

doctrinebot · 2013-11-01T19:50:05Z

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DDC-2773

We use Jira to track the state of pull requests and the versions they got
included in.

beberlei · 2013-11-01T20:18:26Z

lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php

+            $fieldMapping['declaredField'] = $property;
+            $fieldMapping['originalField'] = $fieldMapping['fieldName'];
+            $fieldMapping['fieldName'] = $property . "." . $fieldMapping['fieldName'];
+            $fieldMapping['columnName'] = $property . "_" . $fieldMapping['columnName'];


This probably needs to be delegated to the strategy handling automatic column generation instead of doing it inline here.

You mean as an additional method?

No Doctrine\ORM\Mapping\NamingStrategy handles automatic naming of columns based on class and field names. However changing the interface is obviously a BC break. I think its small enough and easy enough to handle to make it, we need a mention in UPGRADE file though.

done

On Fri, Nov 1, 2013 at 9:33 PM, Benjamin Eberlei
[email protected]:

In lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php:

$this->embeddedClasses[$mapping['fieldName']] = $this->fullyQualifiedClassName($mapping['class']);

}

/**

\* Inline the embeddable class

*

\* @param string $property

\* @param ClassMetadataInfo $embeddable

*/

public function inlineEmbeddable($property, ClassMetadataInfo $embeddable)

{

foreach ($embeddable->fieldMappings as $fieldMapping) {

$fieldMapping['declaredField'] = $property;

$fieldMapping['originalField'] = $fieldMapping['fieldName'];

$fieldMapping['fieldName'] = $property . "." . $fieldMapping['fieldName'];

$fieldMapping['columnName'] = $property . "_" . $fieldMapping['columnName'];

No Doctrine\ORM\Mapping\NamingStrategy handles automatic naming of
columns based on class and field names. However changing the interface is
obviously a BC break. I think its small enough and easy enough to handle to
make it, we need a mention in UPGRADE file though.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/835/files#r7380794
.

FabioBatSilva · 2013-11-01T21:02:10Z

lib/Doctrine/ORM/Mapping/NamingStrategy.php

+     *
+     * @return string
+     */
+    function embeddedFieldToColumnName($propertyName, $embeddedColumnName);


Would be nice to have $entityClass and $embeddedClass

schmittjoh · 2013-11-01T23:17:06Z

lib/Doctrine/ORM/Mapping/ReflectionEmbeddedProperty.php

+        $embeddedObject = $this->parentProperty->getValue($object);
+
+        if ($embeddedObject === null) {
+            $embeddedObject = new $this->class; // TODO


How about changing this to use the unserialize trick, or did you have something else in mind?

beberlei · 2013-11-12T10:14:38Z

@guilhermeblanco Feedback please, this is mergable from my POV.

alsar · 2014-02-08T18:38:58Z

I played around with the new functionality and encountered a problem with embeddables inside embeddables.
Is this behaviour not supported yet?

raul782 · 2014-02-08T18:55:08Z

Thanks a lot guys.

ghost · 2014-02-08T23:59:09Z

Finally the feature was added :) Thank you all who contributed.

Maybe we can now look forward to adding support for mapping collections of embeddable objects
http://www.doctrine-project.org/jira/browse/DDC-2826

Taluu · 2014-02-09T00:11:03Z

lib/Doctrine/ORM/Mapping/ReflectionEmbeddedProperty.php

+    private $childProperty;
+    private $class;
+
+    public function __construct($parentProperty, $childProperty, $class)


Aren't these parameters missing some typehint ? As it is used as (possibly ?) ReflectionProperty afterwards...

@Taluu see standing TODO in the class header`.

beberlei · 2014-02-09T08:42:17Z

@alsar Ah yes, i have to disable doing that for now. Its not yet supported.

beberlei · 2014-02-09T08:43:09Z

@sherifsabry20000 a collection of embeddable objects is just a @OneToMany/@ManyToOne with {"cascade":"all"}

marcospassos · 2014-02-10T13:59:19Z

Thanks a lot guys.

mbadolato · 2014-02-10T22:52:30Z

Is it possible to use, for example, Address as both an entity and an embeddable? A contrived example, that has a basis in our application.

Let's say I have an entity Client that can have multiple Addresses (physical, mailing, etc). In that case Address needs to be a collection, and a OneToMany needs to be established.

In addition, each Client has a OneToMany for Facility. A Facility has one embedded Address.

Would we have to define two things with the basically the same definition (i.e., an Address entity and Address as embeddable), or can one definition serve both purposes?

Does that make sense? Or, am I thinking about the problem in entirely the wrong way?

Ocramius · 2014-02-10T22:54:37Z

@mbadolato an entity has an ID, an embeddable doesn't, so I don't think it makes sense here. I'd also suggest moving these discussions to the mailing list

mbadolato · 2014-02-10T23:04:09Z

@Ocramius Re: having an id vs not having an id: Yep, hence my question. Address cannot be used in both contexts?

mvrhov · 2014-02-11T10:14:07Z

@mbadolato I thought that was possible. The use case would be an invoice and a customer associated with it. When you create an invoice almost full customer information should be persisted together with it.

schmittjoh · 2014-02-11T10:16:17Z

I don't think that is possible just yet as we just have a single metadata
instance per class. We would need something like the @ElementCollection
mentioned earlier to make that work.

On Tue, Feb 11, 2014 at 11:14 AM, Miha Vrhovnik [email protected]:

@mbadolato https://github.com/mbadolato I thought that was possible.
The use case would be an invoice and a customer associated with it. When
you create an invoice almost full customer information should be persisted
together with it.

Reply to this email directly or view it on GitHubhttps://github.com//pull/835#issuecomment-34741113
.

mbadolato · 2014-02-11T16:22:10Z

That's what I was confirming. Thanks @schmittjoh.

@mvrhov Yes that is a good example of a use case for it.

fpaterno · 2014-03-13T08:55:27Z

@beberlei How can you use manyToOne relation between entities and embeddable element ?
I have used manyToOne but nothing happened when i've created the database.

Ocramius · 2014-03-13T10:44:36Z

@fpaterno please open a new issue at http://www.doctrine-project.org/jira/browse/DDC or ask on the mailing list.

mkoubik · 2014-03-23T19:50:52Z

@mvrhov then you should have @Embeddable CustomerData embedded both in Customer and in Invoice.

erik-am · 2014-06-05T11:26:13Z

I'm quite enthousiastic about Embeddables being added to Doctrine, but it's a pity that true Value Objects, which are compared by their properties, are not supported yet.

Given a Value Object Address with properties street and house number that you can instantiate with new Address("High Street", 1).

The following is now supported:
DQL1: SELECT u FROM User u WHERE (u.address.street = :street AND u.address.nr = :number) with { 'street' => 'High Street', 'number' => 1 }.

But then you should know the internal properties when writing your query. That's not how Value Objects usually are compared.

Instead, I expect to be able to do this:
DQL2: SELECT u FROM User u WHERE (u.address = :address) with { 'address' => new Address("High Street", 1) }.
Internally, DQL2 could simply be transformed to the equivalent DQL1 by replacing the condition with conditions for each internal property. The advantage is that the one writing the query does not have to refer to the internal fields; the transformation is hidden.

A complicating factor is that Value Objects are Embeddables, but not every Embeddable is a Value Object. So there is always the question if objects need to be compared by reference or by their properties.

So, perhaps it's an idea to introduce a special operator ~ for comparing objects by their value to make the distinction explicit? Like so:
DQL3: SELECT u FROM User u WHERE (u.address ~ :address) with { 'address' => new Address("High Street", 1) }.

Finally, for those who would also like more support for Value Objects, please check out my pull request for Collections. It expresses the same ideas for in-memory collections and adds the ~ operator for criterias.

Just some thoughts and ideas. I'd love to hear some discussion on this as I think it would make Doctrine really powerful in supporting rich, expressive domain models.

stof · 2014-06-05T12:26:17Z

please open a ticket in the issue tracker for this feature request rather than commenting on a merged PR. Otherwise the discussion will get lost

mkoubik · 2014-06-05T12:31:54Z

@erik-am 👍

erik-am · 2014-06-05T12:58:12Z

@stof Thanks for pointing me to the right direction. I created an issue for it: http://www.doctrine-project.org/jira/browse/DDC-3154

mbadolato · 2014-06-10T19:39:23Z

This thread would probably be a good candidate for the new Github Conversation Lock

Ocramius · 2014-06-11T05:52:19Z

@mbadolato I don't see any fights going on, though yes, if anyone else has anything to add, please do so with a new issue on our issue tracker at http://www.doctrine-project.org/jira/ or on the mailing list at https://groups.google.com/forum/#!search/doctrine-user

schmittjoh and others added 10 commits March 3, 2013 11:01

adds a new output format

b4b9709

[DDC-93] Started ValueObjectsTest

02d34bb

[DDC-93] Parse @Embedded and @embeddable during SchemaTool processing…

32988b3

… to make parsing work.

[DDC-93] Implement first working version of value objects using a Ref…

0204a8b

…lectionProxy object, bypassing changes to UnitOfWork, Persisters and Hydrators.

[DDC-93] Add some TODOs in code.

011776f

[DDC-93] Show CRUD with value objects with current change tracking as…

879ab6e

…sumptions.

[DDC-93] Rename ReflectionProxy to ReflectionEmbeddedProperty, Add DQ…

9613f1d

…L test with Object and Array Hydration.

Merge remote-tracking branch 'origin/ValueObjects'

38b041d

Conflicts: lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php

adds support for selecting based on embedded fields

c67ac8a

adds tests for update/delete DQL queries

30897c3

schmittjoh added 2 commits November 1, 2013 20:54

adds test for non-existent field

41c937b

removes outdated todos

fd8b5bd

beberlei reviewed Nov 1, 2013
View reviewed changes

schmittjoh added 2 commits November 1, 2013 21:44

make use of NamingStrategy for columns of embedded fields

20fb827

fixes coding style

4f6c150

FabioBatSilva reviewed Nov 1, 2013
View reviewed changes

schmittjoh added 2 commits November 1, 2013 22:33

fixes annotation context

f86abd8

some consistency fixes

97836ef

schmittjoh reviewed Nov 1, 2013
View reviewed changes

schmittjoh added 6 commits November 2, 2013 11:20

Merge remote-tracking branch 'schmittjoh/ValueObjects'

d4e6618

adds support & tests for embeddables in inheritance schemes

ece62d6

removes restrictions on constructors of embedded objects

5586ddd

fixes a bad merge

0cd6061

fixes declaring class

2b2f489

makes column prefix configurable

17e0a7b

schmittjoh deleted the ValueObjects branch February 8, 2014 17:09

Taluu reviewed Feb 9, 2014
View reviewed changes

renan added a commit to renan/doctrine2 that referenced this pull request Jul 7, 2014

Removing Value Objects from limitations as per doctrine#835

c9901c9

renan mentioned this pull request Jul 7, 2014

Removing Value Objects from limitations #1078

Merged

This was referenced Dec 6, 2015

DDC-93: It would be nice if we could have support for ValueObjects #1581

Closed

DDC-2773: [GH-835] Value objects (Based on #634) #3520

Closed

DDC-2826: Add support for mapping collections of embeddable objects #3579

Open

localheinz mentioned this pull request Mar 5, 2018

Fix: Do not persist embeddables HelloGrayson/factory-girl-php#44

Merged

2 tasks

greg0ire mentioned this pull request May 11, 2022

Document types as they are passed #9756

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Value objects (Based on #634) #835

Value objects (Based on #634) #835

schmittjoh commented Nov 1, 2013

doctrinebot commented Nov 1, 2013

beberlei Nov 1, 2013

schmittjoh Nov 1, 2013

beberlei Nov 1, 2013

schmittjoh Nov 1, 2013

FabioBatSilva Nov 1, 2013

schmittjoh Nov 1, 2013

beberlei commented Nov 12, 2013

alsar commented Feb 8, 2014

raul782 commented Feb 8, 2014

ghost commented Feb 8, 2014

Taluu Feb 9, 2014

Ocramius Feb 9, 2014

beberlei commented Feb 9, 2014

beberlei commented Feb 9, 2014

marcospassos commented Feb 10, 2014

mbadolato commented Feb 10, 2014

Ocramius commented Feb 10, 2014

mbadolato commented Feb 10, 2014

mvrhov commented Feb 11, 2014

schmittjoh commented Feb 11, 2014

mbadolato commented Feb 11, 2014

fpaterno commented Mar 13, 2014

Ocramius commented Mar 13, 2014

mkoubik commented Mar 23, 2014

erik-am commented Jun 5, 2014

stof commented Jun 5, 2014

mkoubik commented Jun 5, 2014

erik-am commented Jun 5, 2014

mbadolato commented Jun 10, 2014

Ocramius commented Jun 11, 2014

Value objects (Based on #634) #835

Value objects (Based on #634) #835

Conversation

schmittjoh commented Nov 1, 2013

doctrinebot commented Nov 1, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beberlei commented Nov 12, 2013

alsar commented Feb 8, 2014

raul782 commented Feb 8, 2014

ghost commented Feb 8, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beberlei commented Feb 9, 2014

beberlei commented Feb 9, 2014

marcospassos commented Feb 10, 2014

mbadolato commented Feb 10, 2014

Ocramius commented Feb 10, 2014

mbadolato commented Feb 10, 2014

mvrhov commented Feb 11, 2014

schmittjoh commented Feb 11, 2014

mbadolato commented Feb 11, 2014

fpaterno commented Mar 13, 2014

Ocramius commented Mar 13, 2014

mkoubik commented Mar 23, 2014

erik-am commented Jun 5, 2014

stof commented Jun 5, 2014

mkoubik commented Jun 5, 2014

erik-am commented Jun 5, 2014

mbadolato commented Jun 10, 2014

Ocramius commented Jun 11, 2014