[WIP] Value objects

[beberlei]

This pull request takes a different approach than GH-265 to implement ValueObjects. Instead of changing most of the code in every layer, we just inline embedded object class metadata into an entities metadata and then use a reflection proxy that looks like "ReflectionProperty" to do the rewiring.

The idea is inspired from Symfony Forms 'property_path' option, where you can write and read values to different parts of an object graph.

This is a WIP, there have been no further tests made about the consequences of this approach. The implementation is up for discussion.

[doctrinebot]

Hello,

thank you for positing this Pull Request. I have automatically opened an issue on our Jira Bug Tracker for you with the details of this Pull-Request. See the Link:

http://doctrine-project.org/jira/browse/DDC-2374

[stof]

You should typehint the ReflectionProperty arguments

[stof]

Does this implement still compare the embedded objects by reference or does it track changes in them ?

[beberlei]

@stof i didn't take care of change tracking yet. Currently it compares the values of the objects as if they were scalars on the @entity. This needs to change somehow though, at least if the `@embeddableis@readOnly``

[mvrhov]

I have to admit, that I skimmed through the code, but I didn't find how the object gets serialized.
Now I know that for now the only database that natively supports JSON type is postgesql from 9.2, but having serialization format being json some magic could be worked up in the db itself if needed.
The second thing with json is that if someone else needs to read the data or data dump there aren't any problems because the json decoders exist for multiple languages.

[beberlei]

@mvrhov The object doesnt get serialized, there is a table "DDC93Person (id, name, street, zip, city)", that means the columns are in the person table.

Doctrine 2.* will always be >= PHP 5.3.3, so no raising requirements here.

[stof]

@mvrhov If you want some JSON serialization, use a custom DBAL type for it, and no change in the ORM. The goal of embeddable is to store it in fields so that the queries can do what they want with the fields.

[mvrhov]

I've already done that in the project I needed a historical data appended.
@beberlei nicely explained that the separate table is created for embeddable object, which in fact makes this an excellent feature.

[stof]

@mvrhov It is not a separate table. It is extra fields in the table where you embed it. To store them in a separate table, use @OneToOne

[Ocramius]

As discussed on IRC, I've patched up something to allow this version of embeddables to be used for identifiers:

Ocramius/doctrine2@doctrine:ValueObjects...Ocramius:feature/vo-as-id

It's just a PoC and I don't think it's clean, so for now I'm just pasting the link to the branch.

EDIT: probably won't be included - I guess VOs as identifiers is not needed right now :)

[beberlei]

@guilhermeblanco @FabioBatSilva anything?

[FabioBatSilva]

You think it's possible to hydrate as $person['address']['street'] ?

[guilhermeblanco]

Exactly. This is what I highlighted on previous comment. It'd be pretty hard to handle this on DQL parser and also hydrator.

[beberlei]

I doubt its much more to handle in DQL parser than the explicit approach, and as for hydrator, this is array hydration, the object hydration already works without a single change. I think we can argue that slightly complicating array hydration and in turn not even touching object hydration is a much better solution than changing everything.

[marcospassos]

$person['address']['street'] sounds much better.

[FabioBatSilva]

It looks pretty good,
Especially `cause it has a very low effect over the current code base..

I'd like to sugest @EmbedOne & @EmbedMany instead of @Embedded.
As sugested by Jonathan in guilherme's PR.

[Ocramius]

@beberlei do you actually think this can be sneaked into 2.4 or is it way too risky?

[beberlei]

@Ocramius not in 2.4 - i want to find out the implications

@FabioBatSilva @EmbedOne makes sense, but @EmbedMany is not possible with ORM, so its not really available as a choice :-) Making the EmbedOne seem overqualified

[v3labs]

How about just @Embed? Short, clear :)

[gseric]

If @Embedded/@embeddable is good for Hibernate I don't see why it should be different in Doctrine. IMHO same naming convention is beneficial for end users who migrated from Java... if any:-)

[guilhermeblanco]

As we discussed before, @EmbedOne kind of makes sense, but @EmbedMany is impossible. That said, we should stick to JPA convention since differing between One to Many is not valid. I vote for @embeddable.

@beberlei gonna review this tonight I think

[v3labs]

I didn't know that about Hibernate. JPA convention is preferable in my
opinion.

On Mon, Apr 1, 2013 at 5:35 PM, Guilherme Blanco
[email protected]:

As we discussed before, @EmbedOne kind of makes sense, but @EmbedMany is
impossible. That said, we should stick to JPA convention since differing
between One to Many is not valid. I vote for @embeddable.

@beberlei https://github.com/beberlei gonna review this tonight I think

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/634#issuecomment-15717747
.

[guilhermeblanco]

Missing line break

[gseric]

I understand the problem now, ty for explaining. I suggest not making mistake with value objects, they really should be compared by value, otherwise they are not value objects (in my last comment i stated they should be compared with "===", my intention was "==").
http://en.wikipedia.org/wiki/Value_object - "two value objects are equal when they have the same value, not necessarily being the same object"... plain and simple.
Regarding inconsistency with DateTime, etc... IMHO the way how Doctrine works currently seems wrong to me, DateTime should be treated like real VO.
Still, better inconsistent then wrong about embedded objects :)

[beberlei]

The PR is not about ValueObjects, DDC-93 just calls them this way. We introduce embeddables, which are not necessarily value objects. Its up to the user to decide how they implement them.

This is indeed inconsistent to how types work, but that is a performance constraint

[ondrejmirtes]

When this feature will be able to make it into stable version of Doctrine?

[Ocramius]

@ondrejmirtes probably 2.5

[gseric]

Regarding embeddable/datetime comparison, would it be possible to introduce additional parameter to choose which way we want to compare instances? Eg:
@embeddable(compareBy="value") --- slower but safer (default)
@embeddable(compareBy="reference") --- faster but safe to use only with immutables

This way embeddables would be safe to use for both beginners (naive embeddable classes) and advanced users (value objects, immutables...)

Same could be applied for DateTime objects

[Majkl578]

Same could be applied for DateTime objects

Not really, since there is native DateTimeImmutable now. :)

@ondrejmirtes probably 2.5

That's sad, it would be very nice addition to otherwise featureless 2.4.

[mnapoli]

Same could be applied for DateTime objects

Not really, since there is native DateTimeImmutable now. :)

Not 100% sure AFAIK (many want it delayed to another version since it's not correctly implemented)

[Majkl578]

Not 100% sure AFAIK (many want it delayed to another version since it's not correctly implemented)

It has been reimplemented to not use inheritance but to be siblings and use a shared interface DateTimeInterface, see this commit: php/php-src@68a7fec. It's going to be part of 5.5 (actually, it already is in 5.5.0beta3).

[beberlei]

@gseric The embeddables will be writable by default and cannot be compared by reference the way this feature is implemented right now. A different implementation would be much larger and definately not make it into 2.5.

It might be possible to do something like @ReadOnly, but then the change would not be possible at all.

However the way you described it is too expensive to implement at the moment and will be delayed to a future 3.0, where we can rewrite larger chunks of the ClassMetadata API.

[mnapoli]

It has been reimplemented to not use inheritance but to be siblings and use a shared interface DateTimeInterface, see this commit: php/php-src@68a7fec. It's going to be part of 5.5 (actually, it already is in 5.5.0beta3).

Nice! Thanks for relaying the good news ;)

[mvrhov]

@beberlei: What is the status of this feature?

[stof]

@mvrhov it is scheduled for Doctrine 2.5. the focus right now is to stabilize 2.4 to release it as we are in beta

[mvrhov]

I was asking because I have the use case (storing a snapshot the data on invoice/order line at the time it was created). Right now I'm serializing that data into a json and unserializing it on demand behind the scene...

[marcospassos]

This is one of the most wanted feature in Doctrine currently IMO. A quick search on Google for "doctrine value object" results in a lot of entries. I hope to see it here soon.

[ghost]

Can someone please explain to me how this gonna work with Symfony2 forms ?
Currently the forms are bind to an entity that uses primitive data types. If a form is mapped to an entity then the entity will be loaded with the form data after validation.

What will be the expected behavior if the entity has value objects where each of them encapsulates its own validation rules?

[Majkl578]

@sherifsabry20000: That is off-topic, you should probably ask on a different, more appropriate place.

[Burgov]

Is this still alive? I'd love to see this feature land in the next release!

[stof]

@beberlei what is the state of this PR ?

[devhelp]

Great to see that you've also thought of that feature. I wanted to propose some improvements to current approach.

In order to have multiple embedables of the same type embeded in entity I believe you need to define column prefix,

like this

/**
 * @Entity
 * @Table(name='users')
 */
class User
{
   /**
     * @Embeded(class='Address', columnPrefix='address_')
     */
    protected $address;

    /**
     * @Embeded(class='Address', columnPrefix='mailing_address_')
     */
    protected $mailingAddress;
    //...
}

/**
 * @Embedable
 */
class Address
{
    /**
     * @Column
     */
    protected $street;

    /**
     * @Column
     */
    protected $city;

    /**
     * @Column
     */
    protected $country;
    //...
}

and this should map to

users table
columns:
- address_street
- address_city
- address_country
- mailing_address_street
- mailing_address_city
- mailing_address_country
- ...

also please consider updating DQL syntax with something like this:

Then we can have DQL used like this:

SELECT u.address.city FROM User u WHERE u.address.country = 'Poland'

or this:
SELECT u.address.* FROM User u

and make it possible for embedable to embed another embedable:

/**
 * @Embedale
 */
class Price
{
    /**
     * @Column(name='value')
     */
    protected $value;

    /**
     * @Embeded(class='Currency')
     */
    protected $currency;
}

/**
 * @Embedale
 */
class Currency
{
    /**
     * @Column(name='name')
     */
    protected $name;
}

[Majkl578]

@devhelp: That's good point. There should be some possibility to override column (name etc.) definitions, like with using columns defined on traits or like association overrides.
(Btw. I think address is better suited as association as it seems to be too complex complex for being embeddable.)

[devhelp]

@Majkl578 as for Address you may be right - let's put just Price class there and use it in context of 'sellingPrice' & 'purchasePrice' of Merchandise class and we're good

[beberlei]

Superseded by #835