[WIP] Value objects #634

Closed
wants to merge 6 commits into
from

Conversation

Projects
None yet
@beberlei
Owner

beberlei commented Mar 26, 2013

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

This comment has been minimized.

Show comment Hide comment
@doctrinebot

doctrinebot Mar 26, 2013

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

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

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

This comment has been minimized.

Show comment Hide comment
@stof

stof Mar 27, 2013

Member

You should typehint the ReflectionProperty arguments

@stof

stof Mar 27, 2013

Member

You should typehint the ReflectionProperty arguments

@stof

This comment has been minimized.

Show comment Hide comment
@stof

stof Mar 27, 2013

Member

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

Member

stof commented Mar 27, 2013

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

@beberlei

This comment has been minimized.

Show comment Hide comment
@beberlei

beberlei Mar 27, 2013

Owner

@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``

Owner

beberlei commented Mar 27, 2013

@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

This comment has been minimized.

Show comment Hide comment
@mvrhov

mvrhov Mar 27, 2013

Contributor

Why don't we raise the requirements and also drive up the php 5.5 adaption?

Why don't we raise the requirements and also drive up the php 5.5 adaption?

This comment has been minimized.

Show comment Hide comment
@stof

stof Mar 27, 2013

Member

@mvrhov bumping the requirement to 5.5 would be a BC break (what about people which cannot use 5.5 yet ?).
And doing this bump only to make the testsuite a bit more convenient would be weird IMO

Member

stof replied Mar 27, 2013

@mvrhov bumping the requirement to 5.5 would be a BC break (what about people which cannot use 5.5 yet ?).
And doing this bump only to make the testsuite a bit more convenient would be weird IMO

This comment has been minimized.

Show comment Hide comment
@mvrhov

mvrhov Mar 27, 2013

Contributor

If this is required just for the test, then I don't care. However if I need to provide this additionally in each of my classes, then it's a shame.
I've also listed my reasoning behind why it should be a good idea.

Contributor

mvrhov replied Mar 27, 2013

If this is required just for the test, then I don't care. However if I need to provide this additionally in each of my classes, then it's a shame.
I've also listed my reasoning behind why it should be a good idea.

This comment has been minimized.

Show comment Hide comment
@stof

stof Mar 27, 2013

Member

This is just a convenience for tests to avoid writing the class name as a string when doing a $em->find call, making it easier to move fixture classes. See line 34.

Member

stof replied Mar 27, 2013

This is just a convenience for tests to avoid writing the class name as a string when doing a $em->find call, making it easier to move fixture classes. See line 34.

@mvrhov

This comment has been minimized.

Show comment Hide comment
@mvrhov

mvrhov Mar 27, 2013

Contributor

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.

Contributor

mvrhov commented Mar 27, 2013

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

This comment has been minimized.

Show comment Hide comment
@beberlei

beberlei Mar 27, 2013

Owner

@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.

Owner

beberlei commented Mar 27, 2013

@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

This comment has been minimized.

Show comment Hide comment
@stof

stof Mar 27, 2013

Member

@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.

Member

stof commented Mar 27, 2013

@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

This comment has been minimized.

Show comment Hide comment
@mvrhov

mvrhov Mar 27, 2013

Contributor

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.

Contributor

mvrhov commented Mar 27, 2013

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

This comment has been minimized.

Show comment Hide comment
@stof

stof Mar 27, 2013

Member

@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

Member

stof commented Mar 27, 2013

@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

This comment has been minimized.

Show comment Hide comment
@Ocramius

Ocramius Mar 27, 2013

Owner

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 :)

Owner

Ocramius commented Mar 27, 2013

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

This comment has been minimized.

Show comment Hide comment
@beberlei

beberlei Mar 30, 2013

Owner
+ $persons = $this->_em->createQuery($dql)->getArrayResult();
+
+ foreach ($persons as $person) {
+ $this->assertEquals('Tree', $person['address.street']);

This comment has been minimized.

Show comment Hide comment
@FabioBatSilva

FabioBatSilva Mar 30, 2013

Owner

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

@FabioBatSilva

FabioBatSilva Mar 30, 2013

Owner

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

This comment has been minimized.

Show comment Hide comment
@guilhermeblanco

guilhermeblanco Apr 5, 2013

Owner

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

@guilhermeblanco

guilhermeblanco Apr 5, 2013

Owner

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

This comment has been minimized.

Show comment Hide comment
@beberlei

beberlei Apr 5, 2013

Owner

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.

@beberlei

beberlei Apr 5, 2013

Owner

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.

This comment has been minimized.

Show comment Hide comment
@marcospassos

marcospassos Jul 18, 2013

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

@marcospassos

marcospassos Jul 18, 2013

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

@FabioBatSilva

This comment has been minimized.

Show comment Hide comment
@FabioBatSilva

FabioBatSilva Mar 30, 2013

Owner

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.

Owner

FabioBatSilva commented Mar 30, 2013

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

This comment has been minimized.

Show comment Hide comment
@Ocramius

Ocramius Mar 30, 2013

Owner

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

Owner

Ocramius commented Mar 30, 2013

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

@beberlei

This comment has been minimized.

Show comment Hide comment
@beberlei

beberlei Mar 30, 2013

Owner

@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

Owner

beberlei commented Mar 30, 2013

@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

This comment has been minimized.

Show comment Hide comment
@ghost

ghost Mar 31, 2013

Contributor

How about just @Embed? Short, clear :)

Contributor

ghost commented Mar 31, 2013

How about just @Embed? Short, clear :)

@gseric

This comment has been minimized.

Show comment Hide comment
@gseric

gseric Apr 1, 2013

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:-)

gseric commented Apr 1, 2013

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

This comment has been minimized.

Show comment Hide comment
@guilhermeblanco

guilhermeblanco Apr 1, 2013

Owner

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

Owner

guilhermeblanco commented Apr 1, 2013

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

This comment has been minimized.

Show comment Hide comment
@ghost

ghost Apr 1, 2013

Contributor

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
notifications@github.comwrote:

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/doctrine/doctrine2/pull/634#issuecomment-15717747
.

Contributor

ghost commented Apr 1, 2013

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
notifications@github.comwrote:

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/doctrine/doctrine2/pull/634#issuecomment-15717747
.

@@ -136,6 +136,11 @@ protected function doLoadMetadata($class, $parent, $rootEntityFound, array $nonS
$this->completeIdGeneratorMapping($class);
}
+ foreach ($class->embeddedClasses as $property => $embeddableClass) {
+ $embeddableMetadata = $this->getMetadataFor($embeddableClass);
+ $class->inlineEmbeddable($property, $embeddableMetadata);

This comment has been minimized.

Show comment Hide comment
@guilhermeblanco

guilhermeblanco Apr 5, 2013

Owner

Missing line break

@guilhermeblanco

guilhermeblanco Apr 5, 2013

Owner

Missing line break

+ $reflService->getAccessibleProperty($this->embeddedClasses[$mapping['declaredField']], $mapping['originalField']),
+ $this->embeddedClasses[$mapping['declaredField']]
+ );
+ continue;

This comment has been minimized.

Show comment Hide comment
@guilhermeblanco

guilhermeblanco Apr 5, 2013

Owner

Missing line break

@guilhermeblanco

guilhermeblanco Apr 5, 2013

Owner

Missing line break

+ /**
+ * Map Embedded Class
+ *
+ * @array $mapping

This comment has been minimized.

Show comment Hide comment
@guilhermeblanco

guilhermeblanco Apr 5, 2013

Owner

@var array $mapping

@guilhermeblanco

guilhermeblanco Apr 5, 2013

Owner

@var array $mapping

This comment has been minimized.

Show comment Hide comment
@stof

stof Apr 5, 2013

Member

@param, not @var actually

@stof

stof Apr 5, 2013

Member

@param, not @var actually

+ * Map Embedded Class
+ *
+ * @array $mapping
+ * @return void

This comment has been minimized.

Show comment Hide comment
@guilhermeblanco

guilhermeblanco Apr 5, 2013

Owner

Missing line break

@guilhermeblanco

guilhermeblanco Apr 5, 2013

Owner

Missing line break

+
+ /**
+ * @param string $fieldName
+ * @throws MappingException

This comment has been minimized.

Show comment Hide comment
@guilhermeblanco

guilhermeblanco Apr 5, 2013

Owner

Use FQCN for Exception

@guilhermeblanco

guilhermeblanco Apr 5, 2013

Owner

Use FQCN for Exception

@@ -364,6 +366,9 @@ public function loadMetadataForClass($className, ClassMetadata $metadata)
}
$metadata->mapManyToMany($mapping);
+ } else if ($embeddedAnnot = $this->reader->getPropertyAnnotation($property, 'Doctrine\ORM\Mapping\Embedded')) {
+ $mapping['class'] = $embeddedAnnot->class;
+ $metadata->mapEmbedded($mapping);

This comment has been minimized.

Show comment Hide comment
@guilhermeblanco

guilhermeblanco Apr 5, 2013

Owner

Missing line break

@guilhermeblanco

guilhermeblanco Apr 5, 2013

Owner

Missing line break

+ {
+ $this->parentProperty = $parentProperty;
+ $this->childProperty = $childProperty;
+ $this->class = $class;

This comment has been minimized.

Show comment Hide comment
@guilhermeblanco

guilhermeblanco Apr 5, 2013

Owner

Align = signs

@guilhermeblanco

guilhermeblanco Apr 5, 2013

Owner

Align = signs

+ public function inlineEmbeddable($property, ClassMetadataInfo $embeddable)
+ {
+ foreach ($embeddable->fieldMappings as $fieldMapping) {
+ $fieldMapping['declaredField'] = $property;

This comment has been minimized.

Show comment Hide comment
@guilhermeblanco

guilhermeblanco Apr 5, 2013

Owner

Align = signs

@guilhermeblanco

guilhermeblanco Apr 5, 2013

Owner

Align = signs

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

This comment has been minimized.

Show comment Hide comment
@guilhermeblanco

guilhermeblanco Apr 5, 2013

Owner

Why not:

return ($embeddedObject !== null)
    ? $this->childProperty->getValue($embeddedObject)
    : null; 
@guilhermeblanco

guilhermeblanco Apr 5, 2013

Owner

Why not:

return ($embeddedObject !== null)
    ? $this->childProperty->getValue($embeddedObject)
    : null; 
+
+ if ($embeddedObject === null) {
+ $embeddedObject = new $this->class; // TODO
+ $this->parentProperty->setValue($object, $embeddedObject);

This comment has been minimized.

Show comment Hide comment
@guilhermeblanco

guilhermeblanco Apr 5, 2013

Owner

Missing line break

@guilhermeblanco

guilhermeblanco Apr 5, 2013

Owner

Missing line break

+ foreach ($embeddable->fieldMappings as $fieldMapping) {
+ $fieldMapping['declaredField'] = $property;
+ $fieldMapping['originalField'] = $fieldMapping['fieldName'];
+ $fieldMapping['fieldName'] = $property . "." . $fieldMapping['fieldName']; // TODO: Change DQL parser to accept this dot notation

This comment has been minimized.

Show comment Hide comment
@guilhermeblanco

guilhermeblanco Apr 5, 2013

Owner

That seems wrong at first glance to me... it would make near to impossible to handle this in DQL. =
Also, you have to consider that according to DQL original EBNF, it should be possible to have nested embeddables. Something like: user.address.geolocation.latitude

@guilhermeblanco

guilhermeblanco Apr 5, 2013

Owner

That seems wrong at first glance to me... it would make near to impossible to handle this in DQL. =
Also, you have to consider that according to DQL original EBNF, it should be possible to have nested embeddables. Something like: user.address.geolocation.latitude

This comment has been minimized.

Show comment Hide comment
@beberlei

beberlei Apr 5, 2013

Owner

I haven't touched DQL yet, my feeling was that this would not lead to more code in the DQL parts than your approach with explicit sub classes.

The dot is actually a pretty nice trick to prevent collisions, because its not a valid field name sign, and reads naturally in our DQL syntax.

@beberlei

beberlei Apr 5, 2013

Owner

I haven't touched DQL yet, my feeling was that this would not lead to more code in the DQL parts than your approach with explicit sub classes.

The dot is actually a pretty nice trick to prevent collisions, because its not a valid field name sign, and reads naturally in our DQL syntax.

@beberlei

This comment has been minimized.

Show comment Hide comment
@beberlei

beberlei Apr 5, 2013

Owner

@guilhermeblanco a comment on the general approach? Do you think this is good?

My idea was to try to change as little code as possible. We always feared that ValueObjects will requiring changing everything. With this patch i can localize changes to:

  • ClassMetadata
  • DQL Parser/Walker

This is in contrast to our estimates where we needed to also change:

  • UnitOfWork
  • Hydration
  • Persisters

Its much less risk, and touches no existing runtime paths at all, causing absolutly no performance overhead for existing code.

Owner

beberlei commented Apr 5, 2013

@guilhermeblanco a comment on the general approach? Do you think this is good?

My idea was to try to change as little code as possible. We always feared that ValueObjects will requiring changing everything. With this patch i can localize changes to:

  • ClassMetadata
  • DQL Parser/Walker

This is in contrast to our estimates where we needed to also change:

  • UnitOfWork
  • Hydration
  • Persisters

Its much less risk, and touches no existing runtime paths at all, causing absolutly no performance overhead for existing code.

@stof

This comment has been minimized.

Show comment Hide comment
@stof

stof Apr 5, 2013

Member

@beberlei But you are not treating value objects by reference, right ? It makes it a bit inconsistent with other places where the fields are compared by reference.

Member

stof commented Apr 5, 2013

@beberlei But you are not treating value objects by reference, right ? It makes it a bit inconsistent with other places where the fields are compared by reference.

@beberlei

This comment has been minimized.

Show comment Hide comment
@beberlei

beberlei Apr 6, 2013

Owner

@stof exchanging the reference only leads to a dirty object, if one of the properties changed. This is the same behavior that Hibernate or the ODMs have for their embedded objects.

Owner

beberlei commented Apr 6, 2013

@stof exchanging the reference only leads to a dirty object, if one of the properties changed. This is the same behavior that Hibernate or the ODMs have for their embedded objects.

@stof

This comment has been minimized.

Show comment Hide comment
@stof

stof Apr 6, 2013

Member

yeah, but currently, changing a property of the value object without replacing the value object also lead to a dirty entity, which is not the case for a Datetime for instance.

Member

stof commented Apr 6, 2013

yeah, but currently, changing a property of the value object without replacing the value object also lead to a dirty entity, which is not the case for a Datetime for instance.

@beberlei

This comment has been minimized.

Show comment Hide comment
@beberlei

beberlei Apr 6, 2013

Owner

That is true indeed, but the same applies to ODMs which handle datetime as immutable, but not embeddeds.

Owner

beberlei commented Apr 6, 2013

That is true indeed, but the same applies to ODMs which handle datetime as immutable, but not embeddeds.

@mnapoli

This comment has been minimized.

Show comment Hide comment
@mnapoli

mnapoli Apr 11, 2013

Contributor

Shouldn't VO be treated as immutable (at least that's the case in domain driven design), and therefore changes should be compared by reference? -> If a VO is immutable, then if the reference didn't change, the value didn't change either.

DateTime is a typical example of a VO, even though there are database types for DateTimes so that's not really fair. And by the way that's one of the reason there will (probably) be a DateTimeImmutable in next PHP version (but you already know that).

Contributor

mnapoli commented Apr 11, 2013

Shouldn't VO be treated as immutable (at least that's the case in domain driven design), and therefore changes should be compared by reference? -> If a VO is immutable, then if the reference didn't change, the value didn't change either.

DateTime is a typical example of a VO, even though there are database types for DateTimes so that's not really fair. And by the way that's one of the reason there will (probably) be a DateTimeImmutable in next PHP version (but you already know that).

@gseric

This comment has been minimized.

Show comment Hide comment
@gseric

gseric Apr 13, 2013

@mnapoli how would you enforce immutability of embedded objects in user's code? Embedded objects should be immutable but it's up to users to keep them that way. Worst scenario would be to expect immutability when it's not the case. Therefore, I don't think it is good to rely on reference. VOs should be compared by value so "===" is probably the best option.

gseric commented Apr 13, 2013

@mnapoli how would you enforce immutability of embedded objects in user's code? Embedded objects should be immutable but it's up to users to keep them that way. Worst scenario would be to expect immutability when it's not the case. Therefore, I don't think it is good to rely on reference. VOs should be compared by value so "===" is probably the best option.

@mnapoli

This comment has been minimized.

Show comment Hide comment
@mnapoli

mnapoli Apr 13, 2013

Contributor

@gseric You are right, you can't enforce that. But as of today, DateTime objects are considered by Doctrine as immutable (i.e. modifying the date object doesn't update it in DB). And this is the same when using custom Type mappings to simulate VO (in stable release) -> Doctrine consider them as immutable and compare references.

So at least it would be better to be coherent:

  • either all VO (DateTime incl) are considered as immutable ("considered" means that can't be enforced of course), so === should be used
  • either all VO (DateTime incl) are not considered immutable, so == should be used, but that may be a BC break
Contributor

mnapoli commented Apr 13, 2013

@gseric You are right, you can't enforce that. But as of today, DateTime objects are considered by Doctrine as immutable (i.e. modifying the date object doesn't update it in DB). And this is the same when using custom Type mappings to simulate VO (in stable release) -> Doctrine consider them as immutable and compare references.

So at least it would be better to be coherent:

  • either all VO (DateTime incl) are considered as immutable ("considered" means that can't be enforced of course), so === should be used
  • either all VO (DateTime incl) are not considered immutable, so == should be used, but that may be a BC break
@gseric

This comment has been minimized.

Show comment Hide comment
@gseric

gseric Apr 13, 2013

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 :)

gseric commented Apr 13, 2013

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

This comment has been minimized.

Show comment Hide comment
@beberlei

beberlei Apr 13, 2013

Owner

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

Owner

beberlei commented Apr 13, 2013

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

This comment has been minimized.

Show comment Hide comment
@ondrejmirtes

ondrejmirtes Apr 23, 2013

Contributor

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

Contributor

ondrejmirtes commented Apr 23, 2013

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

@Ocramius

This comment has been minimized.

Show comment Hide comment
@Ocramius

Ocramius Apr 23, 2013

Owner

@ondrejmirtes probably 2.5

Owner

Ocramius commented Apr 23, 2013

@ondrejmirtes probably 2.5

@gseric

This comment has been minimized.

Show comment Hide comment
@gseric

gseric Apr 23, 2013

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

gseric commented Apr 23, 2013

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

This comment has been minimized.

Show comment Hide comment
@Majkl578

Majkl578 Apr 23, 2013

Member

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.

Member

Majkl578 commented Apr 23, 2013

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

This comment has been minimized.

Show comment Hide comment
@mnapoli

mnapoli Apr 23, 2013

Contributor

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)

Contributor

mnapoli commented Apr 23, 2013

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

This comment has been minimized.

Show comment Hide comment
@Majkl578

Majkl578 Apr 23, 2013

Member

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).

Member

Majkl578 commented Apr 23, 2013

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

This comment has been minimized.

Show comment Hide comment
@beberlei

beberlei Apr 23, 2013

Owner

@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.

Owner

beberlei commented Apr 23, 2013

@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

This comment has been minimized.

Show comment Hide comment
@mnapoli

mnapoli Apr 23, 2013

Contributor

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 ;)

Contributor

mnapoli commented Apr 23, 2013

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

This comment has been minimized.

Show comment Hide comment
@mvrhov

mvrhov May 14, 2013

Contributor

@beberlei: What is the status of this feature?

Contributor

mvrhov commented May 14, 2013

@beberlei: What is the status of this feature?

@stof

This comment has been minimized.

Show comment Hide comment
@stof

stof May 17, 2013

Member

@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

Member

stof commented May 17, 2013

@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

This comment has been minimized.

Show comment Hide comment
@mvrhov

mvrhov May 17, 2013

Contributor

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...

Contributor

mvrhov commented May 17, 2013

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 comment has been minimized.

Show comment Hide comment
@marcospassos

marcospassos Jul 18, 2013

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.

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.

This comment has been minimized.

Show comment Hide comment
@ghost

ghost Jul 21, 2013

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?

ghost commented Jul 21, 2013

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

This comment has been minimized.

Show comment Hide comment
@Majkl578

Majkl578 Jul 21, 2013

Member

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

Member

Majkl578 commented Jul 21, 2013

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

@Burgov

This comment has been minimized.

Show comment Hide comment
@Burgov

Burgov Aug 24, 2013

Contributor

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

Contributor

Burgov commented Aug 24, 2013

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

@stof

This comment has been minimized.

Show comment Hide comment
@stof

stof Oct 14, 2013

Member

@beberlei what is the state of this PR ?

Member

stof commented Oct 14, 2013

@beberlei what is the state of this PR ?

This comment has been minimized.

Show comment Hide comment
@ghost

ghost Oct 14, 2013

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;
}

ghost commented Oct 14, 2013

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

This comment has been minimized.

Show comment Hide comment
@Majkl578

Majkl578 Oct 14, 2013

Member

@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.)

Member

Majkl578 commented Oct 14, 2013

@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.)

This comment has been minimized.

Show comment Hide comment
@ghost

ghost Oct 14, 2013

@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

ghost commented Oct 14, 2013

@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

This comment has been minimized.

Show comment Hide comment
@beberlei

beberlei Jan 2, 2014

Owner

Superseded by #835

Owner

beberlei commented Jan 2, 2014

Superseded by #835

@beberlei beberlei closed this Jan 2, 2014

beberlei added a commit that referenced this pull request Feb 8, 2014

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

@Ocramius Ocramius deleted the ValueObjects branch Jul 7, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment