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

[WIP] Nullable embedded objects. #1275

Closed
wants to merge 11 commits into from
3 changes: 2 additions & 1 deletion lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,7 @@ protected function doLoadMetadata($class, $parent, $rootEntityFound, array $nonS
$this->addNestedEmbeddedClasses($embeddableMetadata, $class, $property);
}

$class->inlineEmbeddable($property, $embeddableMetadata);
$class->inlineEmbeddable($property, $embeddableMetadata, $embeddableClass['nullable']);

unset($this->embeddablesActiveNesting[$class->name]);
}
Expand Down Expand Up @@ -433,6 +433,7 @@ private function addNestedEmbeddedClasses(ClassMetadata $subClass, ClassMetadata
$parentClass->mapEmbedded(array(
'fieldName' => $prefix . '.' . $property,
'class' => $embeddableMetadata->name,
'nullable' => $embeddableClass['nullable'],
'columnPrefix' => $embeddableClass['columnPrefix'],
'declaredField' => $embeddableClass['declaredField']
? $prefix . '.' . $embeddableClass['declaredField']
Expand Down
8 changes: 7 additions & 1 deletion lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php
Original file line number Diff line number Diff line change
Expand Up @@ -3213,6 +3213,7 @@ public function mapEmbedded(array $mapping)
$this->embeddedClasses[$mapping['fieldName']] = array(
'class' => $this->fullyQualifiedClassName($mapping['class']),
'columnPrefix' => $mapping['columnPrefix'],
'nullable' => isset($mapping['nullable']) ? $mapping['nullable'] : null,
'declaredField' => isset($mapping['declaredField']) ? $mapping['declaredField'] : null,
'originalField' => isset($mapping['originalField']) ? $mapping['originalField'] : null,
);
Expand All @@ -3223,8 +3224,9 @@ public function mapEmbedded(array $mapping)
*
* @param string $property
* @param ClassMetadataInfo $embeddable
* @param boolean $nullable
*/
public function inlineEmbeddable($property, ClassMetadataInfo $embeddable)
public function inlineEmbeddable($property, ClassMetadataInfo $embeddable, $nullable)
Copy link
Member

Choose a reason for hiding this comment

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

Is that extra parameters really required? Can't you use $this->embeddedClasses[$property]['nullable'] in this method instead?

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, my mistake.

{
foreach ($embeddable->fieldMappings as $fieldMapping) {
$fieldMapping['originalClass'] = isset($fieldMapping['originalClass'])
Expand All @@ -3238,6 +3240,10 @@ public function inlineEmbeddable($property, ClassMetadataInfo $embeddable)
: $fieldMapping['fieldName'];
$fieldMapping['fieldName'] = $property . "." . $fieldMapping['fieldName'];

if ($nullable !== null) {
$fieldMapping['nullable'] = $nullable;
}

if (! empty($this->embeddedClasses[$property]['columnPrefix'])) {
$fieldMapping['columnName'] = $this->embeddedClasses[$property]['columnPrefix'] . $fieldMapping['columnName'];
} elseif ($this->embeddedClasses[$property]['columnPrefix'] !== false) {
Expand Down
1 change: 1 addition & 0 deletions lib/Doctrine/ORM/Mapping/Driver/AnnotationDriver.php
Original file line number Diff line number Diff line change
Expand Up @@ -394,6 +394,7 @@ public function loadMetadataForClass($className, ClassMetadata $metadata)
} else if ($embeddedAnnot = $this->reader->getPropertyAnnotation($property, 'Doctrine\ORM\Mapping\Embedded')) {
$mapping['class'] = $embeddedAnnot->class;
$mapping['columnPrefix'] = $embeddedAnnot->columnPrefix;
$mapping['nullable'] = $embeddedAnnot->nullable;
$metadata->mapEmbedded($mapping);
}

Expand Down
5 changes: 5 additions & 0 deletions lib/Doctrine/ORM/Mapping/Driver/XmlDriver.php
Original file line number Diff line number Diff line change
Expand Up @@ -263,9 +263,14 @@ public function loadMetadataForClass($className, ClassMetadata $metadata)
? $this->evaluateBoolean($embeddedMapping['use-column-prefix'])
: true;

$nullable = isset($embeddedMapping['nullable'])
? $this->evaluateBoolean($embeddedMapping['nullable'])
: null;

$mapping = array(
'fieldName' => (string) $embeddedMapping['name'],
'class' => (string) $embeddedMapping['class'],
'nullable' => $nullable,
'columnPrefix' => $useColumnPrefix ? $columnPrefix : false
);

Expand Down
1 change: 1 addition & 0 deletions lib/Doctrine/ORM/Mapping/Driver/YamlDriver.php
Original file line number Diff line number Diff line change
Expand Up @@ -334,6 +334,7 @@ public function loadMetadataForClass($className, ClassMetadata $metadata)
$mapping = array(
'fieldName' => $name,
'class' => $embeddedMapping['class'],
'nullable' => isset($embeddedMapping['nullable']) ? $embeddedMapping['nullable'] : null,
'columnPrefix' => isset($embeddedMapping['columnPrefix']) ? $embeddedMapping['columnPrefix'] : null,
);
$metadata->mapEmbedded($mapping);
Expand Down
5 changes: 5 additions & 0 deletions lib/Doctrine/ORM/Mapping/Embedded.php
Original file line number Diff line number Diff line change
Expand Up @@ -35,4 +35,9 @@ final class Embedded implements Annotation
* @var mixed
*/
public $columnPrefix;

/**
* @var boolean
*/
public $nullable;
}
30 changes: 30 additions & 0 deletions lib/Doctrine/ORM/UnitOfWork.php
Original file line number Diff line number Diff line change
Expand Up @@ -2564,6 +2564,8 @@ public function createEntity($className, array $data, &$hints = array())
return $entity;
}

$this->removeNullableEmbeddedReferences($data, $class);

foreach ($data as $field => $value) {
if (isset($class->fieldMappings[$field])) {
$class->reflFields[$field]->setValue($entity, $value);
Expand Down Expand Up @@ -2767,6 +2769,34 @@ public function createEntity($className, array $data, &$hints = array())
return $entity;
}

/**
* @param array $data
* @param ClassMetadata $class
*/
private function removeNullableEmbeddedReferences(array &$data, ClassMetadata $class)
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 I've tried to keep the things simple here, but if we have a huge amount of data or a large result set I think we might have a little delay to process. I just don't have any idea to improve this =/

Copy link
Contributor

Choose a reason for hiding this comment

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

What about something like this:

public function createEntity($className, array $data, &$hints = array())
{
    // ...
    //$this->removeNullableEmbeddedReferences($data, $class);

    foreach ($data as $field => $value) {
        if (null === $value && $this->isNullableEmbeddedField($class, $field)) {
            continue;
        }

        if (isset($class->fieldMappings[$field])) {
            $class->reflFields[$field]->setValue($entity, $value);
        }
    }
    // ...
}

private function isNullableEmbeddedField(ClassMetadata $class, $field)
{
    return isset(
        $class->fieldMappings[$field]['declaredField'],
        $class->embeddedClasses[$name = $class->fieldMappings[$field]['declaredField']]
    ) ? true === $class->embeddedClasses[$name]['nullable'] : false;
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @1ed, its a good alternative. I'll play a lit bit now 😄

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, @1ed and @beberlei what do you think to extract that logic from UnitOfWork into ClassMetadata? It seems to be proper location, considering that UnitOfWork is already huge.

I thought something like ClassMetadata::updateEntity($entity, array $data) or ClassMetadata::applyData($entity, array $data).

{
$nullables = array_keys(
array_filter(
$class->embeddedClasses,
function (array $embedded) {
return $embedded['nullable'] === true;
}
)
);

if (empty($nullables)) {
return;
}

foreach ($data as $field => $value) {
$property = substr($field, 0, strpos($field, '.'));

if (in_array($property, $nullables) && $value === null) {
unset($data[$field]);
}
}
}

/**
* @return void
*/
Expand Down
84 changes: 84 additions & 0 deletions tests/Doctrine/Tests/ORM/Functional/ValueObjectsTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ public function setUp()
$this->_em->getClassMetadata(__NAMESPACE__ . '\DDC93Car'),
$this->_em->getClassMetadata(__NAMESPACE__ . '\DDC3027Animal'),
$this->_em->getClassMetadata(__NAMESPACE__ . '\DDC3027Dog'),
$this->_em->getClassMetadata(__NAMESPACE__ . '\DDC93Event'),
));
} catch(\Exception $e) {
}
Expand Down Expand Up @@ -242,6 +243,48 @@ public function testThrowsExceptionOnInfiniteEmbeddableNesting($embeddableClassN
));
}

public function testCRUDOfNullableEmbedded()
Copy link
Member

Choose a reason for hiding this comment

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

@lcobucci consider splitting this test into smaller tests depending on each other via @depends

Copy link
Member Author

Choose a reason for hiding this comment

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

Perfect, I followed the other test case (testCRUD). But using @depends is way better.

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'll also change the name of the classes that I've created on that file to match the issue on JIRA (DDC-3529)

{
$event = new DDC93Event();
$event->name = 'PHP Conference';
$event->country = new DDC93Country('Brazil');
$event->period = new DDC93DateInterval(new \DateTime('2015-01-20 08:00:00'), new \DateTime('2015-01-23 19:00:00'));

// 1. check saving value objects works
$this->_em->persist($event);
$this->_em->flush();

$this->_em->clear();

// 2. check loading value objects works
$event = $this->_em->find(DDC93Event::CLASSNAME, $event->id);

$this->assertEquals('PHP Conference', $event->name);
$this->assertEquals('Brazil', $event->country->name);
$this->assertEquals('2015-01-20 08:00:00', $event->period->begin->format('Y-m-d H:i:s'));
$this->assertEquals('2015-01-23 19:00:00', $event->period->end->format('Y-m-d H:i:s'));
$this->assertNull($event->submissions);

// 3. check changing value objects works
$event->submissions = new DDC93DateInterval(new \DateTime('2014-11-20 08:00:00'), new \DateTime('2014-12-23 19:00:00'));

$this->_em->persist($event);
$this->_em->flush();
$this->_em->clear();

$event = $this->_em->find(DDC93Event::CLASSNAME, $event->id);

$this->assertEquals('2014-11-20 08:00:00', $event->submissions->begin->format('Y-m-d H:i:s'));
$this->assertEquals('2014-12-23 19:00:00', $event->submissions->end->format('Y-m-d H:i:s'));

// 4. check deleting works
$eventId = $event->id;;
$this->_em->remove($event);
$this->_em->flush();

$this->assertNull($this->_em->find(DDC93Event::CLASSNAME, $eventId));
}

public function getInfiniteEmbeddableNestingData()
{
return array(
Expand Down Expand Up @@ -279,6 +322,47 @@ public function __construct($name = null, DDC93Address $address = null)
}
}

/**
* @Embeddable
*/
class DDC93DateInterval
{
/** @Column(type = "datetime") */
public $begin;

/** @Column(type = "datetime") */
public $end;

public function __construct(\DateTime $begin, \DateTime $end)
{
$this->begin = $begin;
$this->end = $end;
}
}

/**
* @Entity
*/
class DDC93Event
{
const CLASSNAME = __CLASS__;

/** @Id @GeneratedValue @Column(type="integer") */
public $id;

/** @Column(type = "string") */
public $name;

/** @Embedded(class = "DDC93DateInterval") */
public $period;

/** @Embedded(class = "DDC93DateInterval", nullable = true) */
public $submissions;

/** @Embedded(class = "DDC93Country", nullable = false) */
public $country;
}

/**
* @Embeddable
*/
Expand Down
1 change: 1 addition & 0 deletions tests/Doctrine/Tests/ORM/Mapping/XmlMappingDriverTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ public function testEmbeddedMapping()
'columnPrefix' => 'nm_',
'declaredField' => null,
'originalField' => null,
'nullable' => null,
)
),
$class->embeddedClasses
Expand Down