[WIP] Mapping support for Embeddables (VOs). #265

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
Owner

guilhermeblanco commented Jan 20, 2012

This patch is going to address: http://www.doctrine-project.org/jira/browse/DDC-93

Embeddable classes are a good way for organizing your domain model. Specially larger systems, which contains similar domain objects with similar properties.
This patch proposes a cleaner way to structure these duplicating attributes (and logic) into an @embeddable object. Abstract the common information into an @embeddedable object and use it as @Embedded.

/**
 * @Embeddable
 */
class Location
{
    /** @Column(type="decimal", scale=8, precision=12, nullable=true) */
    protected $latitude;

    /** @Column(type="decimal", scale=8, precision=12, nullable=true) */
    protected $longitude;
}

/**
 * @Entity 
 */
class User
{
    /** @Id @GeneratedValue @Column(type="integer") */
    protected $id;

    // ...

    /** @Embedded(class="Location") */
    protected $location;
}

Querying:

$query = $entityManager->createQuery('SELECT u FROM User u WHERE u.location.latitude = ?1');

i vote for "isEmbedded". This is the same we use in the ODMs and compared to isEmbeddable does not suggest that you can also use it standalone.

Owner

jwage replied Jan 20, 2012

In MongoDB ODM we use isEmbeddedDocument :( I suppose we could change it to be consistent.

Owner

guilhermeblanco commented Jan 20, 2012

I'll change these flags to an entityType property and assign a value for it in an easier way. That way, we could always inspect for entity type by doing:

$metadata->entityType === ClassMetadata::TYPE_ENTITY
$metadata->entityType === ClassMetadata::TYPE_MAPPEDSUPERCLASS
$metadata->entityType === ClassMetadata::TYPE_EMBEDDABLE
Owner

guilhermeblanco commented Jan 20, 2012

According to @jwage suggest, we should use @EmbedOne instead of @Embedded.
This is valid in the case we want to support @EmbedMany in the future.

Member

stof commented Jan 20, 2012

just a note before looking at the diff: you should not use public properties when giving an example in a PR (or elsewhere). It would make it even more difficult when explaining to users that using public properties are forbidden because of proxies

Owner

guilhermeblanco commented Jan 20, 2012

@stof changed. =P

@Ocramius Ocramius commented on the diff Jan 20, 2012

lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php
@@ -340,8 +346,9 @@ protected function loadMetadata($name)
$parent = $class;
- if ( ! $class->isMappedSuperclass) {
+ if ( ! $class->isMappedSuperclass && ! $class->isEmbeddable) {
@Ocramius

Ocramius Jan 20, 2012

Owner
$class->isMappedSuperclass && ! $class->isEmbeddable

This check is used also later on. Could it be implemented as a method of ClassMetadata?

@guilhermeblanco

guilhermeblanco Jan 20, 2012

Owner

This will be changed soon with ->entityType refactoring.

@stof stof commented on the diff Jan 20, 2012

lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php
@@ -803,6 +834,20 @@ public function validateIdentifier()
}
/**
+ * Validate that emdeddables actually exist.
+ *
+ * @return void
+ */
+ public function validateEmdeddeds()
+ {
+ foreach ($this->embeddedMappings as $field => $mapping) {
+ if ( ! \Doctrine\Common\ClassLoader::classExists($mapping['class']) ) {
@stof

stof Jan 20, 2012

Member

you should add a use statement for it IMO

@jwage jwage commented on the diff Jan 20, 2012

lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php
+ * @param array $mapping The embedded mapping to validated & complete.
+ * @return array The validated and completed embedded mapping.
+ */
+ protected function _validateAndCompleteEmbeddedMapping(array $mapping)
+ {
+ // Check mandatory fields
+ if ( ! isset($mapping['fieldName']) || strlen($mapping['fieldName']) == 0) {
+ throw MappingException::missingFieldName($this->name);
+ }
+
+ if ( ! isset($mapping['class'])) {
+ throw MappingException::missingEmbeddedClass($mapping['fieldName']);
+ }
+
+ if (strlen($this->namespace) > 0 && strpos($mapping['class'], '\\') === false) {
+ $mapping['class'] = $this->namespace . '\\' . $mapping['class'];
@jwage

jwage Jan 20, 2012

Owner

Should we rename this target to match the targetEntity name?

@guilhermeblanco

guilhermeblanco Jan 20, 2012

Owner

I'm quite strict to Hibernate, to minimize the need of thousands of documentation just for a simple change. They use "class".
Also, the concept of @Embeddable is not an Entity, so it's semantically wrong to use "targetEntity", when actually you need to point a non-Entity.

@jwage

jwage Jan 20, 2012

Owner

I didn't mean targetEntity, it'd just be targetClass or target but if hibernate is that then go with that.

@stof stof commented on the diff Jan 20, 2012

lib/Doctrine/ORM/Mapping/Driver/AnnotationDriver.php
@@ -162,6 +162,9 @@ public function loadMetadataForClass($className, ClassMetadataInfo $metadata)
$mappedSuperclassAnnot = $classAnnotations['Doctrine\ORM\Mapping\MappedSuperclass'];
$metadata->setCustomRepositoryClass($mappedSuperclassAnnot->repositoryClass);
$metadata->isMappedSuperclass = true;
+ } else if (isset($classAnnotations['Doctrine\ORM\Mapping\Embeddable'])) {
+ $metadata->markReadOnly();
@stof

stof Jan 20, 2012

Member

embedded are always read-only ?

@guilhermeblanco

guilhermeblanco Jan 20, 2012

Owner

Yes, they are Immutable conceptually.

Member

stof commented Jan 20, 2012

@guilhermeblanco how would these be stored ? a separate table in the DB ?

Owner

Ocramius commented Jan 20, 2012

Additional columns on the same table...

@Ocramius Ocramius commented on the diff Jan 20, 2012

lib/Doctrine/ORM/Mapping/Embedded.php
+ *
+ * This software consists of voluntary contributions made by many individuals
+ * and is licensed under the LGPL. For more information, see
+ * <http://www.doctrine-project.org>.
+ */
+
+namespace Doctrine\ORM\Mapping;
+
+/**
+ * @Annotation
+ * @Target("PROPERTY")
+ */
+final class Embedded implements Annotation
+{
+ /** @var string */
+ public $class;
@Ocramius

Ocramius Jan 20, 2012

Owner

We surely could need an additional property to allow the user to define what the prefix used when building embedded fields would look like.

By prefix, I would also suggest it is not just "fieldName" in "fieldName_embeddableField", but I'd also include the "_", allowing the user not to depend on a pre-defined separator

Considering the fix to use @EmbedOne, this is a very nice feature.
It will clearly easy some stuffs and improve performance (in some of my use cases).

Owner

beberlei commented Jan 21, 2012

Regarding the implementation in Hydration i think it would make sense to "simulate" as if the value object were an entity, that means:

  1. SQLWalker creates DQL Alias for VOs, i.e. "ParentEntityAlias__VO_fieldName".
  2. ResultSetMapping has this DQL Alias
  3. Value Objects have all their fields defined as identifier. So they are used to generate an ID hash.
  4. ObjectHydrator::createEntity has a shortcut for value objects, so the UnitOfWork does not need to know about them.

This assumes that value objects are ALWAYS read-only. This would simplify this patch considerably. Updating "values" has to be done by switching the whole object. Then only the persisters have to be updated, not the Hydrators.

you should add an empty line after this set of constant (and probably a comment to describe the next set of constants)

should be @var integer

why using a locale variable instead of setting it directly in the property ?

you changed the logic here during the reformatting. Is it intended ?

Yes... it was a code that was already done. Basically I addressed the BasicEntityPersister by changing its logic.
It was just included in the commit when I pushed everything.

Contributor

jonathaningram commented Mar 6, 2012

Does anyone know the status of this PR? Is there any way I could contribute to get the ball rolling on this feature?

Contributor

lstrojny commented Mar 11, 2012

Very cool feature. Would love to see it making its way into Doctrine. Two things would be cool: optionally define the mapping in the embedding class and to pass value objects as query parameters.

+1 to this feature

Contributor

rouffj commented Apr 10, 2012

+1

@marijn marijn commented on the diff May 11, 2012

tests/Doctrine/Tests/Models/CMS/CmsParents.php
@@ -0,0 +1,34 @@
+<?php
+
+namespace Doctrine\Tests\Models\CMS;
+
+/**
+ * @Embeddable
+ */
+class CmsParents
+{
+ /**
+ * @Column(length=250)
+ */
+ public $father;
@marijn

marijn May 11, 2012

Why are these and other @Embeddable fields marked as public. A value object should be immutable, right?

@lstrojny

lstrojny May 11, 2012

Contributor

Immutability is based on the value object itself. Being public doesn’t make the value object itself mutable.

@stof

stof May 11, 2012

Member

however, public properties break Doctrine proxies so they should be avoided in tests too IMO (this object will work in the cases used by the tests but they will confuse users looking at it)

@lstrojny

lstrojny May 11, 2012

Contributor

Is that because public properties don’t trigger the internal lazy loading?

@guilhermeblanco

guilhermeblanco May 11, 2012

Owner

In tests they are public for simplicity... I can mark as protected if you have stronger arguments.

@stof

stof May 11, 2012

Member

@lstrojny exactly, because there is no way to hook into the access to a public property. If you want to use $entity->foo in your code because you like property access, you can still do it by implementing __get and __set, which don't break it as these method can be overwritten to load the proxy.

@guilhermeblanco it is mainly to avoid people reading the tests and thinking they can use public in their own code too (and then complaining about the broken stuff). I know the doc explicitly warns against public properties, but I also know that many devs don't read the doc.

@lstrojny

lstrojny May 11, 2012

Contributor

@stof this issue seems fixable: when setting up the proxy one could unset the public properties of the instance and provide __get/__set so that lazy loading can be triggered. Little bit hackish but it should work.

@stof

stof May 11, 2012

Member

@lstrojny the proxy factory cannot edit the code of your own classes. We don't want this at all

@guilhermeblanco

guilhermeblanco May 11, 2012

Owner

That's a problem of PHP. @lstrojny maybe you could help PHP core to implement a concept that Java has called Interceptors.
That's how Hibernate solves clearly the issue in Java. The get/set property patch (similar to .NET which was proposed) may also fix, but it needs approval afaik.

@lstrojny

lstrojny May 11, 2012

Contributor

I know we are getting more and more OT :)

@stof Editing is not necessary. It could be done dynamically in the proxy itself.

@guilhermeblanco I'm not sure we need this, support for better accessors is indeed quite an interesting patch, nevertheless we don’t need it for this issue. What’s not very well known is that unset() works on object properties and after unset() is called magic methods are called again. This is something Doctrine could use. See this gist for illustration: https://gist.github.com/2662665

@stof

stof May 11, 2012

Member

but it could cause some issues if someone uses both public properties and __call in a class

@marijn

marijn May 12, 2012

What simplicity is associated with marking these properties as public?

marijn commented May 11, 2012

Nice work @guilhermeblanco! 👍 👍 👍

What is the status of this PR?

@stof stof commented on the diff May 11, 2012

tests/Doctrine/Tests/Models/CMS/CmsParents.php
+
+/**
+ * @Embeddable
+ */
+class CmsParents
+{
+ /**
+ * @Column(length=250)
+ */
+ public $father;
+ /**
+ * @Column(length=250)
+ */
+ public $mother;
+
+ public function setFather($father) {
@stof

stof May 11, 2012

Member

CS issue (and for next methods too)

mhlavac commented Sep 5, 2012

What is the status? Is there a need for this PR any longer? Same functionality can be achieved with traits in PHP 5.4?

trait Location
{
    /** @Column(type="decimal", scale=8, precision=12, nullable=true) */
    protected $latitude;

    /** @Column(type="decimal", scale=8, precision=12, nullable=true) */
    protected $longitude;
}

/**
 * @Entity 
 */
class User
{
    /** @Id @GeneratedValue @Column(type="integer") */
    protected $id;

    // ...

    use Location;
}

It's always good to implement an interface when using trait. Also you can't access embedded variable with DQL like this:

SELECT u FROM User u WHERE u.location.latitude = 1234.00

You must use original names instead:

SELECT u FROM User u WHERE u.latitude = 1234.00
Member

stof commented Sep 5, 2012

@mhlavac traits are not the same. They would put the property in the entity itself, not in an embedded value object

FZ14 commented Sep 12, 2012

Hi!
Will Doctrine\ORM 2.3 adds the ability to use Value Objects?
If not, when do you plan to add this feature?

Member

stof commented Sep 13, 2012

@FZ14 no as Doctrine 2.3 is RC3 (so no new features in it anymore) and is due in a few days, whereas this feature is far from being ready.

Owner

beberlei commented Nov 25, 2012

Closing this for now as a rebase will be virtually impossible given recent changes and what Fabio plans.

beberlei closed this Nov 25, 2012

FZ14 commented Nov 26, 2012

@beberlei Why is it impossible? What recent changes did you mean, and what Fabio plans?
Thanks.

Member

stof commented Nov 26, 2012

@FZ14 A big part of the ClassMetadataFactory has been moved to Doctrine Common in 2.3

gseric commented Nov 27, 2012

Any ETA or target version for this feature? IMHO it's essential for any medium to large scale application.

Owner

beberlei commented Nov 27, 2012

I plan to POC this feature for the next release (2.5), but i cannot make any promises. I agree that its very important.

masi commented Dec 1, 2012

2.5? What happened to 2.4? Sorry for crashing this issue, but I was looking for this feature in TYPO3 Flow which is based on Doctrine2. Too bad that the latter seems to lack a custom support and Doctrine istelf isn't ready yet for that.

Member

stof commented Dec 1, 2012

@masi it is too late to start a new big feature for 2.4 now, as its beta release is scheduled for December

Contributor

Majkl578 commented Dec 1, 2012

@stof: I'm wondering if there is something new for 2.4? According to diff between 2.3.0 and master, I don't see anything big, only some small changes and some BC breaks...

Contributor

kdambekalns commented Dec 15, 2012

@masi Well, down in the internals we handle VOs like entities, but some optimization is done - if you create the "same" object it will be "normalized" when being saved, so no duplication in that regard at least.

Ocramius deleted the DDC-93 branch Jul 7, 2016

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