Skip to content
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
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
b4b9709
adds a new output format
schmittjoh Mar 3, 2013
02d34bb
[DDC-93] Started ValueObjectsTest
beberlei Feb 19, 2013
32988b3
[DDC-93] Parse @Embedded and @Embeddable during SchemaTool processing…
beberlei Mar 26, 2013
0204a8b
[DDC-93] Implement first working version of value objects using a Ref…
beberlei Mar 26, 2013
011776f
[DDC-93] Add some TODOs in code.
beberlei Mar 26, 2013
879ab6e
[DDC-93] Show CRUD with value objects with current change tracking as…
beberlei Mar 27, 2013
9613f1d
[DDC-93] Rename ReflectionProxy to ReflectionEmbeddedProperty, Add DQ…
beberlei Mar 27, 2013
38b041d
Merge remote-tracking branch 'origin/ValueObjects'
schmittjoh Nov 1, 2013
c67ac8a
adds support for selecting based on embedded fields
schmittjoh Nov 1, 2013
30897c3
adds tests for update/delete DQL queries
schmittjoh Nov 1, 2013
41c937b
adds test for non-existent field
schmittjoh Nov 1, 2013
fd8b5bd
removes outdated todos
schmittjoh Nov 1, 2013
20fb827
make use of NamingStrategy for columns of embedded fields
schmittjoh Nov 1, 2013
4f6c150
fixes coding style
schmittjoh Nov 1, 2013
f86abd8
fixes annotation context
schmittjoh Nov 1, 2013
97836ef
some consistency fixes
schmittjoh Nov 1, 2013
d4e6618
Merge remote-tracking branch 'schmittjoh/ValueObjects'
schmittjoh Nov 2, 2013
ece62d6
adds support & tests for embeddables in inheritance schemes
schmittjoh Nov 2, 2013
5586ddd
removes restrictions on constructors of embedded objects
schmittjoh Nov 2, 2013
0cd6061
fixes a bad merge
schmittjoh Nov 2, 2013
2b2f489
fixes declaring class
schmittjoh Nov 2, 2013
17e0a7b
makes column prefix configurable
schmittjoh Nov 2, 2013
9ad376c
adds docs
schmittjoh Nov 12, 2013
fb3a06b
adds support for XML/Yaml drivers
schmittjoh Nov 12, 2013
2a73a6f
some cs fixes
schmittjoh Nov 12, 2013
0ee7b68
small fix
schmittjoh Nov 12, 2013
e5cab1d
adds embedded classes to cache
schmittjoh Nov 28, 2013
928c32d
Update XML schema to reflect addition of embeddables
Dec 7, 2013
fbb7b5a
Fix XmlDriver to accept embeddables
Dec 7, 2013
f7f7c46
Merge pull request #1 from jankramer/ValueObjects
schmittjoh Dec 7, 2013
4f585a3
Merge branch 'master' of github.com:doctrine/doctrine2 into ValueObjects
schmittjoh Jan 4, 2014
9464194
fixes bad merge
schmittjoh Jan 4, 2014
7020f41
skips DQL UPDATE/DELETE tests with SLC enabled
schmittjoh Jan 4, 2014
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions UPGRADE.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,11 @@
# Upgrade to 2.5

## BC BREAK: NamingStrategy has a new method ``embeddedFieldToColumnName($propertyName, $embeddedColumnName)``

This method generates the column name for fields of embedded objects. If you implement your custom NamingStrategy, you
now also need to implement this new method.


# Upgrade to 2.4

## BC BREAK: Compatibility Bugfix in PersistentCollection#matching()
Expand Down
5 changes: 5 additions & 0 deletions lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,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);
}

if ($parent && $parent->isInheritanceTypeSingleTable()) {
$class->setPrimaryTable($parent->table);
}
Expand Down
83 changes: 76 additions & 7 deletions lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,13 @@ class ClassMetadataInfo implements ClassMetadata
*/
public $isMappedSuperclass = false;

/**
* READ-ONLY: Wheather this class describes the mapping of an embeddable class.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo

*
* @var boolean
*/
public $isEmbeddedClass = false;

/**
* READ-ONLY: The names of the parent classes (ancestors).
*
Expand All @@ -260,6 +267,13 @@ class ClassMetadataInfo implements ClassMetadata
*/
public $subClasses = array();

/**
* READ-ONLY: The names of all embedded classes based on properties.
*
* @var array
*/
public $embeddedClasses = array();

/**
* READ-ONLY: The named queries allowed to be called directly from Repository.
*
Expand Down Expand Up @@ -884,6 +898,15 @@ public function wakeupReflection($reflService)
$this->reflClass = $reflService->getClass($this->name);

foreach ($this->fieldMappings as $field => $mapping) {
if (isset($mapping['declaredField'])) {
$this->reflFields[$field] = new ReflectionEmbeddedProperty(
$reflService->getAccessibleProperty($this->name, $mapping['declaredField']),
$reflService->getAccessibleProperty($this->embeddedClasses[$mapping['declaredField']], $mapping['originalField']),
$this->embeddedClasses[$mapping['declaredField']]
);
continue;
}

$this->reflFields[$field] = isset($mapping['declared'])
? $reflService->getAccessibleProperty($mapping['declared'], $field)
: $reflService->getAccessibleProperty($this->name, $field);
Expand Down Expand Up @@ -925,8 +948,12 @@ public function initializeReflection($reflService)
*/
public function validateIdentifier()
{
if ($this->isMappedSuperclass || $this->isEmbeddedClass) {
return;
}

// Verify & complete identifier mapping
if ( ! $this->identifier && ! $this->isMappedSuperclass) {
if ( ! $this->identifier) {
throw MappingException::identifierRequired($this->name);
}

Expand Down Expand Up @@ -2162,9 +2189,8 @@ private function _isInheritanceType($type)
public function mapField(array $mapping)
{
$this->_validateAndCompleteFieldMapping($mapping);
if (isset($this->fieldMappings[$mapping['fieldName']]) || isset($this->associationMappings[$mapping['fieldName']])) {
throw MappingException::duplicateFieldMapping($this->name, $mapping['fieldName']);
}
$this->assertFieldNotMapped($mapping['fieldName']);

$this->fieldMappings[$mapping['fieldName']] = $mapping;
}

Expand Down Expand Up @@ -2412,9 +2438,7 @@ protected function _storeAssociationMapping(array $assocMapping)
{
$sourceFieldName = $assocMapping['fieldName'];

if (isset($this->fieldMappings[$sourceFieldName]) || isset($this->associationMappings[$sourceFieldName])) {
throw MappingException::duplicateFieldMapping($this->name, $sourceFieldName);
}
$this->assertFieldNotMapped($sourceFieldName);

$this->associationMappings[$sourceFieldName] = $assocMapping;
}
Expand Down Expand Up @@ -3044,4 +3068,49 @@ public function getMetadataValue($name) {

return null;
}

/**
* Map Embedded Class
*
* @array $mapping
* @return void
*/
public function mapEmbedded(array $mapping)
{
$this->assertFieldNotMapped($mapping['fieldName']);

$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'] = $this->namingStrategy->embeddedFieldToColumnName($property, $fieldMapping['columnName'], $this->reflClass->name, $embeddable->reflClass->name);

$this->mapField($fieldMapping);
}
}

/**
* @param string $fieldName
* @throws MappingException
*/
private function assertFieldNotMapped($fieldName)
{
if (isset($this->fieldMappings[$fieldName]) ||
isset($this->associationMappings[$fieldName]) ||
isset($this->embeddedClasses[$fieldName])) {

throw MappingException::duplicateFieldMapping($this->name, $fieldName);
}
}
}
8 changes: 8 additions & 0 deletions lib/Doctrine/ORM/Mapping/DefaultNamingStrategy.php
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,14 @@ public function propertyToColumnName($propertyName, $className = null)
return $propertyName;
}

/**
* {@inheritdoc}
*/
public function embeddedFieldToColumnName($propertyName, $embeddedColumnName, $className = null, $embeddedClassName = null)
{
return $propertyName.'_'.$embeddedColumnName;
}

/**
* {@inheritdoc}
*/
Expand Down
5 changes: 5 additions & 0 deletions lib/Doctrine/ORM/Mapping/Driver/AnnotationDriver.php
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,8 @@ public function loadMetadataForClass($className, ClassMetadata $metadata)
$mappedSuperclassAnnot = $classAnnotations['Doctrine\ORM\Mapping\MappedSuperclass'];
$metadata->setCustomRepositoryClass($mappedSuperclassAnnot->repositoryClass);
$metadata->isMappedSuperclass = true;
} else if (isset($classAnnotations['Doctrine\ORM\Mapping\Embeddable'])) {
$metadata->isEmbeddedClass = true;
} else {
throw MappingException::classIsNotAValidEntityOrMappedSuperClass($className);
}
Expand Down Expand Up @@ -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);
}
}

Expand Down
4 changes: 3 additions & 1 deletion lib/Doctrine/ORM/Mapping/Driver/DoctrineAnnotations.php
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@

require_once __DIR__.'/../Annotation.php';
require_once __DIR__.'/../Entity.php';
require_once __DIR__.'/../Embeddable.php';
require_once __DIR__.'/../Embedded.php';
require_once __DIR__.'/../MappedSuperclass.php';
require_once __DIR__.'/../InheritanceType.php';
require_once __DIR__.'/../DiscriminatorColumn.php';
Expand Down Expand Up @@ -64,4 +66,4 @@
require_once __DIR__.'/../AssociationOverrides.php';
require_once __DIR__.'/../AttributeOverride.php';
require_once __DIR__.'/../AttributeOverrides.php';
require_once __DIR__.'/../EntityListeners.php';
require_once __DIR__.'/../EntityListeners.php';
28 changes: 28 additions & 0 deletions lib/Doctrine/ORM/Mapping/Embeddable.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
<?php
/*
* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
* "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
* LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
* A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
* OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
* SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
* LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
* DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
* THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
* (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
* OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
*
* This software consists of voluntary contributions made by many individuals
* and is licensed under the MIT license. For more information, see
* <http://www.doctrine-project.org>.
*/

namespace Doctrine\ORM\Mapping;

/**
* @Annotation
* @Target("CLASS")
*/
final class Embeddable implements Annotation
{
}
32 changes: 32 additions & 0 deletions lib/Doctrine/ORM/Mapping/Embedded.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
<?php
/*
* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
* "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
* LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
* A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
* OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
* SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
* LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
* DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
* THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
* (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
* OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
*
* This software consists of voluntary contributions made by many individuals
* and is licensed under the MIT license. 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;
}
10 changes: 10 additions & 0 deletions lib/Doctrine/ORM/Mapping/NamingStrategy.php
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,16 @@ function classToTableName($className);
*/
function propertyToColumnName($propertyName, $className = null);

/**
* Returns a column name for an embedded property.
*
* @param string $propertyName
* @param string $embeddedColumnName
*
* @return string
*/
function embeddedFieldToColumnName($propertyName, $embeddedColumnName, $className = null, $embeddedClassName = null);

/**
* Returns the default reference column name.
*
Expand Down
66 changes: 66 additions & 0 deletions lib/Doctrine/ORM/Mapping/ReflectionEmbeddedProperty.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
<?php
/*
* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
* "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
* LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
* A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
* OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
* SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
* LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
* DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
* THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
* (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
* OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
*
* This software consists of voluntary contributions made by many individuals
* and is licensed under the MIT license. For more information, see
* <http://www.doctrine-project.org>.
*/

namespace Doctrine\ORM\Mapping;

/**
* Acts as a proxy to a nested Property structure, making it look like
* just a single scalar property.
*
* This way value objects "just work" without UnitOfWork, Persisters or Hydrators
* needing any changes.
*
* TODO: Move this class into Common\Reflection
*/
class ReflectionEmbeddedProperty
{
private $parentProperty;
private $childProperty;
private $class;

public function __construct($parentProperty, $childProperty, $class)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Taluu see standing TODO in the class header`.

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

public function getValue($object)
{
$embeddedObject = $this->parentProperty->getValue($object);

if ($embeddedObject === null) {
return null;
}

return $this->childProperty->getValue($embeddedObject);
}

public function setValue($object, $value)
{
$embeddedObject = $this->parentProperty->getValue($object);

if ($embeddedObject === null) {
$embeddedObject = new $this->class; // TODO
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

$this->parentProperty->setValue($object, $embeddedObject);
}

$this->childProperty->setValue($embeddedObject, $value);
}
}
8 changes: 8 additions & 0 deletions lib/Doctrine/ORM/Mapping/UnderscoreNamingStrategy.php
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,14 @@ public function propertyToColumnName($propertyName, $className = null)
return $this->underscore($propertyName);
}

/**
* {@inheritdoc}
*/
public function embeddedFieldToColumnName($propertyName, $embeddedColumnName, $className = null, $embeddedClassName = null)
{
return $this->underscore($propertyName).'_'.$embeddedColumnName;
}

/**
* {@inheritdoc}
*/
Expand Down
8 changes: 7 additions & 1 deletion lib/Doctrine/ORM/Query/Parser.php
Original file line number Diff line number Diff line change
Expand Up @@ -1049,7 +1049,7 @@ public function JoinAssociationPathExpression()
* Parses an arbitrary path expression and defers semantical validation
* based on expected types.
*
* PathExpression ::= IdentificationVariable "." identifier
* PathExpression ::= IdentificationVariable "." identifier [ ("." identifier)* ]
*
* @param integer $expectedTypes
*
Expand All @@ -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)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

$this->match(Lexer::T_DOT);
$this->match(Lexer::T_IDENTIFIER);
$field .= '.'.$this->lexer->token['value'];
}
}

// Creating AST node
Expand Down
1 change: 1 addition & 0 deletions lib/Doctrine/ORM/Tools/SchemaTool.php
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,7 @@ private function processingNotRequired($class, array $processedClasses)
return (
isset($processedClasses[$class->name]) ||
$class->isMappedSuperclass ||
$class->isEmbeddedClass ||
($class->isInheritanceTypeSingleTable() && $class->name != $class->rootEntityName)
);
}
Expand Down
Loading