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.

Sign up for GitHub

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

Add support for nesting embeddables #1105

Merged
merged 2 commits into from
Aug 21, 2014
+292 −57
Merged

Add support for nesting embeddables #1105

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bca9d31
add support for nesting embeddables
deeky666 Aug 11, 2014
0768916
fix handling infinite nesting of embeddables
deeky666 Aug 12, 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
38 changes: 37 additions & 1 deletion lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,14 +142,22 @@ protected function doLoadMetadata($class, $parent, $rootEntityFound, array $nonS
}

if (!$class->isMappedSuperclass) {

foreach ($class->embeddedClasses as $property => $embeddableClass) {

if (isset($embeddableClass['inherited'])) {
continue;
}

if ($embeddableClass['class'] === $class->name) {
Copy link
Member

Choose a reason for hiding this comment

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

this is not the only case leading to an infinite nesting. You need to account for loops as well: A embed B embed A (and with any length for the cycle of course)

Copy link
Member

Choose a reason for hiding this comment

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

A good way to solve this is to keep an internal reference map (new property in the CMF) somehow. Finding loops would be easy then.

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right, I almost expected to be missing something here. Meh that is tricky... Have to think about a reasonable solution here :(

Copy link
Member

Choose a reason for hiding this comment

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

private $embeddablesInheritance = []; and then

private function checkEmbeddablesInheritance($embeddableName, $looping = [])
{
    // do the recursion stuff here
}

Something like that. I suggest starting from a test, as it makes it much easier to write the recursion

Copy link
Member Author

Choose a reason for hiding this comment

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

@Ocramius thanks I will try that.

Copy link
Member

Choose a reason for hiding this comment

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

Another data structure would maybe even be cleaner: could just be a small, final InheritanceMap object/utility

throw MappingException::infiniteEmbeddableNesting($class->name, $property);
}

$embeddableMetadata = $this->getMetadataFor($embeddableClass['class']);

if ($embeddableMetadata->isEmbeddedClass) {
$this->addNestedEmbeddedClasses($embeddableMetadata, $class, $property);
}

$class->inlineEmbeddable($property, $embeddableMetadata);
}
}
Expand Down Expand Up @@ -370,6 +378,34 @@ private function addInheritedEmbeddedClasses(ClassMetadata $subClass, ClassMetad
}
}

/**
* Adds nested embedded classes metadata to a parent class.
*
* @param ClassMetadata $subClass Sub embedded class metadata to add nested embedded classes metadata from.
* @param ClassMetadata $parentClass Parent class to add nested embedded classes metadata to.
* @param string $prefix Embedded classes' prefix to use for nested embedded classes field names.
*/
private function addNestedEmbeddedClasses(ClassMetadata $subClass, ClassMetadata $parentClass, $prefix)
{
foreach ($subClass->embeddedClasses as $property => $embeddableClass) {
if (isset($embeddableClass['inherited'])) {
continue;
}

$embeddableMetadata = $this->getMetadataFor($embeddableClass['class']);

$parentClass->mapEmbedded(array(
'fieldName' => $prefix . '.' . $property,
'class' => $embeddableMetadata->name,
'columnPrefix' => $embeddableClass['columnPrefix'],
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't both column prefixes be concatenated when nesting ?

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 don't think so. How would you expect to concatenate here? We don't know the naming strategy to use. Also if you have a look at the tests, it is working as expected IMO. The concatenation is done here.
Or am I getting you wrong?

'declaredField' => $embeddableClass['declaredField']
? $prefix . '.' . $embeddableClass['declaredField']
: $prefix,
'originalField' => $embeddableClass['originalField'] ?: $property,
));
}
}

/**
* Adds inherited named queries to the subclass mapping.
*
Expand Down
45 changes: 33 additions & 12 deletions lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -929,15 +929,31 @@ public function wakeupReflection($reflService)
// Restore ReflectionClass and properties
$this->reflClass = $reflService->getClass($this->name);

$parentReflFields = array();

foreach ($this->embeddedClasses as $property => $embeddedClass) {
if (isset($embeddedClass['declaredField'])) {
$parentReflFields[$property] = new ReflectionEmbeddedProperty(
$parentReflFields[$embeddedClass['declaredField']],
$reflService->getAccessibleProperty(
$this->embeddedClasses[$embeddedClass['declaredField']]['class'],
$embeddedClass['originalField']
),
$embeddedClass['class']
);

continue;
}

$parentReflFields[$property] = $reflService->getAccessibleProperty($this->name, $property);
}

foreach ($this->fieldMappings as $field => $mapping) {
if (isset($mapping['declaredField'])) {
$declaringClass = isset($this->embeddedClasses[$mapping['declaredField']]['declared'])
? $this->embeddedClasses[$mapping['declaredField']]['declared'] : $this->name;

$this->reflFields[$field] = new ReflectionEmbeddedProperty(
$reflService->getAccessibleProperty($declaringClass, $mapping['declaredField']),
$reflService->getAccessibleProperty($this->embeddedClasses[$mapping['declaredField']]['class'], $mapping['originalField']),
$this->embeddedClasses[$mapping['declaredField']]['class']
$parentReflFields[$mapping['declaredField']],
$reflService->getAccessibleProperty($mapping['originalClass'], $mapping['originalField']),
$mapping['originalClass']
);
continue;
}
Expand Down Expand Up @@ -3171,15 +3187,13 @@ public function getMetadataValue($name) {
*/
public function mapEmbedded(array $mapping)
{
if ($this->isEmbeddedClass) {
throw MappingException::noEmbeddablesInEmbeddable($this->name);
}

$this->assertFieldNotMapped($mapping['fieldName']);

$this->embeddedClasses[$mapping['fieldName']] = array(
'class' => $this->fullyQualifiedClassName($mapping['class']),
'columnPrefix' => $mapping['columnPrefix'],
'declaredField' => isset($mapping['declaredField']) ? $mapping['declaredField'] : null,
'originalField' => isset($mapping['originalField']) ? $mapping['originalField'] : null,
);
}

Expand All @@ -3192,8 +3206,15 @@ public function mapEmbedded(array $mapping)
public function inlineEmbeddable($property, ClassMetadataInfo $embeddable)
{
foreach ($embeddable->fieldMappings as $fieldMapping) {
$fieldMapping['declaredField'] = $property;
$fieldMapping['originalField'] = $fieldMapping['fieldName'];
$fieldMapping['originalClass'] = isset($fieldMapping['originalClass'])
? $fieldMapping['originalClass']
: $embeddable->name;
$fieldMapping['declaredField'] = isset($fieldMapping['declaredField'])
? $property . '.' . $fieldMapping['declaredField']
: $property;
$fieldMapping['originalField'] = isset($fieldMapping['originalField'])
? $fieldMapping['originalField']
: $fieldMapping['fieldName'];
$fieldMapping['fieldName'] = $property . "." . $fieldMapping['fieldName'];

if (! empty($this->embeddedClasses[$property]['columnPrefix'])) {
Expand Down
20 changes: 15 additions & 5 deletions lib/Doctrine/ORM/Mapping/MappingException.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -782,11 +782,21 @@ public static function missingSequenceName($className)
);
}

public static function noEmbeddablesInEmbeddable($className)
/**
* @param string $className
* @param string $propertyName
*
* @return MappingException
*/
public static function infiniteEmbeddableNesting($className, $propertyName)
{
return new self(sprintf(
"You embedded one or more embeddables in embeddable '%s', but this behavior is currently unsupported.",
$className
));
return new self(
sprintf(
'Infinite nesting detected for embedded property %s::%s. ' .
'You cannot embed an embeddable from the same type inside an embeddable.',
$className,
$propertyName
)
);
}
}
Loading