New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Value objects (Based on #634) #835

Merged
merged 33 commits into from Feb 8, 2014

Conversation

Projects
None yet
@schmittjoh
Member

schmittjoh commented Nov 1, 2013

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

schmittjoh and others added some commits Mar 3, 2013

[DDC-93] Implement first working version of value objects using a Ref…
…lectionProxy object, bypassing changes to UnitOfWork, Persisters and Hydrators.
@mvrhov

This comment has been minimized.

Contributor

mvrhov commented on tests/Doctrine/Tests/ORM/Functional/ValueObjectsTest.php in 32988b3 Mar 27, 2013

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

This comment has been minimized.

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.

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.

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.

beberlei and others added some commits Mar 27, 2013

@doctrinebot

This comment has been minimized.

doctrinebot commented Nov 1, 2013

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.

schmittjoh added some commits Nov 1, 2013

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

This comment has been minimized.

@beberlei

beberlei Nov 1, 2013

Member

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

This comment has been minimized.

@schmittjoh

schmittjoh Nov 1, 2013

Member

You mean as an additional method?

This comment has been minimized.

@beberlei

beberlei Nov 1, 2013

Member

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.

This comment has been minimized.

@schmittjoh

schmittjoh Nov 1, 2013

Member

done

On Fri, Nov 1, 2013 at 9:33 PM, Benjamin Eberlei
notifications@github.comwrote:

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
.

@@ -1065,6 +1065,12 @@ public function PathExpression($expectedTypes)
$this->match(Lexer::T_IDENTIFIER);
$field = $this->lexer->token['value'];
while ($this->lexer->isNextToken(Lexer::T_DOT)) {

This comment has been minimized.

@beberlei

beberlei Nov 1, 2013

Member

Does this loop need validation of sorts? Or are there sane failures for it?

This comment has been minimized.

@schmittjoh

schmittjoh Nov 1, 2013

Member

If the field does not exist, there is a QueryException at some later point. I don't think that we need to add more validation here, or did you have anything specific in mind?

This comment has been minimized.

@beberlei

beberlei Nov 1, 2013

Member

query exception is perfect, i was afraid it might notice out or something ugly.

This comment has been minimized.

@guilhermeblanco

guilhermeblanco Nov 14, 2013

Member

This seems incomplete. We're relying on a breakage somewhere as a Query Exception else instead of properly throwing a Semantical or Parser exception properly.
I'd say that here we should properly add fields to PathExpression and then evaluating them on processDeferredPathExpressions.

This comment has been minimized.

@beberlei

beberlei Nov 14, 2013

Member

Since this fields are saved as "foo.bar" in fieldMappings this is passed to deferred path expressions etc.

This comment has been minimized.

@schmittjoh

schmittjoh Nov 14, 2013

Member

The beauty of this implementation really is that almost all parts except the metadata drivers can be left completely unaware of embeddables. We now just allow field names to contain dots.

This comment has been minimized.

@guilhermeblanco

guilhermeblanco Nov 14, 2013

Member

@beberlei but it's not properly validated. Is it "foo" or "bar" that is wrong? I describe some of my concerns below.

@schmittjoh I see a problem with this. There's no way to differ a field from an embedded (and this is a huge problem IMHO).
Also, "user.location.geo.latitude" doesn't seem to be supported.
Finally, Embeddeable ClassMetadata is created purely for 3rd-party consumers, but never consumed internally on DQL, Hydrator, Persister, etc.
It seems to me we're relying on an unintentional support to build a big feature which may bring a lot of headaches in the future.

This comment has been minimized.

@schmittjoh

schmittjoh Nov 14, 2013

Member

I can address some of these concerns at least:

  1. You can distinguish an embedded field vs a normal field by the fact that it contains dots. We can make the exception message more precise for these cases if deemed necessary.
  2. Embedded fields can work over multiple levels

I cannot address the other things. I really don't know whether this feature will cause headaches in the future. At this point, I don't see why, but I'm not a fortune teller :) however, the internal implementation could be changed for a Doctrine 3 release without breaking BC with the end-user facing part.

/**
* @expectedException Doctrine\ORM\Query\QueryException
* @expectedExceptionMessage no field or association named address.asdfasdf

This comment has been minimized.

@beberlei

beberlei Nov 1, 2013

Member

We use $this->setExpectedException in the code by convention, can you change it to using the method?

This comment has been minimized.

@schmittjoh

schmittjoh Nov 1, 2013

Member

done.

On Fri, Nov 1, 2013 at 9:34 PM, Benjamin Eberlei
notifications@github.comwrote:

In tests/Doctrine/Tests/ORM/Functional/ValueObjectsTest.php:

  •    $this->_em->refresh($person);
    
  •    $this->assertEquals('Boo', $person->address->street);
    
  •    // DELETE
    
  •    $this->_em->createQuery("DELETE " . __NAMESPACE__ . "\DDC93Person p WHERE p.address.city = :city")
    
  •        ->setParameter('city', 'Karlsruhe')
    
  •        ->execute();
    
  •    $this->_em->clear();
    
  •    $this->assertNull($this->_em->find(__NAMESPACE__.'\DDC93Person', $person->id));
    
  • }
  • /**
  • \* @expectedException Doctrine\ORM\Query\QueryException
    
  • \* @expectedExceptionMessage no field or association named address.asdfasdf
    

We use $this->setExpectedException in the code by convention, can you
change it to using the method?


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

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

This comment has been minimized.

@FabioBatSilva

FabioBatSilva Nov 1, 2013

Member

Would be nice to have $entityClass and $embeddedClass

@@ -53,6 +53,14 @@ public function propertyToColumnName($propertyName, $className = null)
/**
* {@inheritdoc}
*/
public function embeddedFieldToColumnName($propertyName, $embeddedColumnName)
{
return $propertyName.ucfirst($embeddedColumnName);

This comment has been minimized.

@FabioBatSilva

FabioBatSilva Nov 1, 2013

Member

IMO should be $propertyName . '_' . $embeddedColumnName;

schmittjoh added some commits Nov 1, 2013

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

This comment has been minimized.

@schmittjoh

schmittjoh Nov 1, 2013

Member

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

@schmittjoh

This comment has been minimized.

Member

schmittjoh commented Feb 8, 2014

You're welcome :)

@schmittjoh schmittjoh deleted the schmittjoh:ValueObjects branch Feb 8, 2014

@alsar

This comment has been minimized.

alsar commented Feb 8, 2014

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

@raul782

This comment has been minimized.

raul782 commented Feb 8, 2014

Thanks a lot guys.

@ghost

This comment has been minimized.

ghost commented Feb 8, 2014

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

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

This comment has been minimized.

@Taluu

Taluu Feb 9, 2014

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

This comment has been minimized.

@Ocramius

Ocramius Feb 9, 2014

Member

@Taluu see standing TODO in the class header`.

@beberlei

This comment has been minimized.

Member

beberlei commented Feb 9, 2014

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

@beberlei

This comment has been minimized.

Member

beberlei commented Feb 9, 2014

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

@marcospassos

This comment has been minimized.

marcospassos commented Feb 10, 2014

Thanks a lot guys.

@mbadolato

This comment has been minimized.

mbadolato commented Feb 10, 2014

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

This comment has been minimized.

Member

Ocramius commented Feb 10, 2014

@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

This comment has been minimized.

mbadolato commented Feb 10, 2014

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

@mvrhov

This comment has been minimized.

Contributor

mvrhov commented Feb 11, 2014

@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

This comment has been minimized.

Member

schmittjoh commented Feb 11, 2014

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

@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

This comment has been minimized.

mbadolato commented Feb 11, 2014

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

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

@fpaterno

This comment has been minimized.

fpaterno commented Mar 13, 2014

@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

This comment has been minimized.

Member

Ocramius commented Mar 13, 2014

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

@mkoubik

This comment has been minimized.

mkoubik commented Mar 23, 2014

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

@erik-am

This comment has been minimized.

erik-am commented Jun 5, 2014

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

This comment has been minimized.

Member

stof commented Jun 5, 2014

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

This comment has been minimized.

mkoubik commented Jun 5, 2014

@erik-am 👍

@erik-am

This comment has been minimized.

erik-am commented Jun 5, 2014

@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

This comment has been minimized.

mbadolato commented Jun 10, 2014

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

@Ocramius

This comment has been minimized.

Member

Ocramius commented Jun 11, 2014

@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

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