Fix parent id order #120

Merged
merged 5 commits into from Mar 9, 2012
@@ -84,7 +84,7 @@ class TranslatableProperty extends Property
public $id = true;
public $type = 'string';
/** @var string */
- public $strategy = 'assigned';
+ public $strategy;
@uwej711

uwej711 Mar 8, 2012

Contributor

This is necessary since otherwise we never know whether a strategy was set by the developer or not. This was actually different from YAML and XML mapping. Internally we still default to assigned.

}
/**
* @Annotation
@@ -45,6 +45,11 @@ class ClassMetadata implements ClassMetadataInterface
const MANY_TO_ONE = 4;
const MANY_TO_MANY = 8;
+ /**
+ * means no strategy has been set so far.
+ */
+ const GENERATOR_TYPE_NONE = 0;
+
/**
* means the repository will need to be able to generate the id.
*/
@@ -84,6 +89,13 @@ class ClassMetadata implements ClassMetadataInterface
*/
public $idGenerator = self::GENERATOR_TYPE_ASSIGNED;
+ /**
+ * keep track whether an id strategy was explicitly set
+ *
+ * @var boolean
+ */
+ private $idStrategySet = false;
+
/**
* READ-ONLY: The field name of the document identifier.
*/
@@ -461,6 +473,9 @@ public function mapId(array $mapping)
$this->setIdentifier($mapping['fieldName']);
if (isset($mapping['strategy'])) {
$this->setIdGenerator($mapping['strategy']);
+ $this->idStrategySet = true;
+ } elseif (null !== $this->parentMapping && null !== $this->nodename) {
+ $this->setIdGenerator(self::GENERATOR_TYPE_PARENT);
}
}
@@ -487,13 +502,18 @@ public function mapNodename(array $mapping)
{
$this->validateAndCompleteFieldMapping($mapping, false);
$this->nodename = $mapping['fieldName'];
+ if (null !== $this->parentMapping && !$this->idStrategySet) {
+ $this->setIdGenerator(self::GENERATOR_TYPE_PARENT);
+ }
}
public function mapParentDocument(array $mapping)
{
$this->validateAndCompleteFieldMapping($mapping, false);
$this->parentMapping = $mapping['fieldName'];
- $this->setIdGenerator(self::GENERATOR_TYPE_PARENT);
+ if (null !== $this->nodename && !$this->idStrategySet) {
+ $this->setIdGenerator(self::GENERATOR_TYPE_PARENT);
+ }
}
public function mapChild(array $mapping)
@@ -650,7 +670,7 @@ public function getFieldMapping($fieldName)
/**
* Sets the ID generator used to generate IDs for instances of this class.
*
- * @param AbstractIdGenerator $generator
+ * @param string $generator
*/
public function setIdGenerator($generator)
{
@@ -86,7 +86,7 @@ public function loadMetadataForClass($className, ClassMetadata $class)
if (isset($element['id'])) {
$mapping = array('fieldName' => $element['id'], 'id' => true);
if (isset($element['id']['generator']['strategy'])) {
- $mapping = $element['id']['generator']['strategy'];
+ $mapping['strategy'] = $element['id']['generator']['strategy'];
}
$class->mapId($mapping);
}
@@ -173,4 +173,4 @@ protected function loadMappingFile($file)
{
return Yaml::parse($file);
}
-}
+}
@@ -225,14 +225,14 @@ function testProxyForChildIsUsed()
*/
class NameDoc
{
+ /** @PHPCRODM\ParentDocument */
+ public $parent;
@dbu

dbu Mar 8, 2012

Member

is the order now still relevant or not anymore?

@uwej711

uwej711 Mar 8, 2012

Contributor

Goal of the change is to make the order irrelevant

@stof

stof Mar 8, 2012

Member

but if the order is irrelevant, why do you have to change it in the test fixture ?

@lsmith77

lsmith77 Mar 8, 2012

Member

to highlight that the issue is fixed ..

@uwej711

uwej711 Mar 8, 2012

Contributor

Because changing the order showed the presence of the issue before the mapping code was changed ...

@stof

stof Mar 9, 2012

Member

@lsmith77 what really highlights that the issue is gone is the next testcase where different order are used. Having only one example could mean that the issue is still there but with the reverse order :p

/** @PHPCRODM\Id */
public $id;
/** @PHPCRODM\Node */
public $node;
/** @PHPCRODM\Nodename */
public $nodename;
- /** @PHPCRODM\ParentDocument */
- public $parent;
/** @PHPCRODM\Children */
public $children;
/** @PHPCRODM\Child */
@@ -4,6 +4,7 @@
use Doctrine\ODM\PHPCR\Id\RepositoryIdInterface,
Doctrine\ODM\PHPCR\DocumentRepository,
+ Doctrine\ODM\PHPCR\Mapping\ClassMetadata,
Doctrine\ODM\PHPCR\Mapping\Annotations as PHPCRODM,
Doctrine\ODM\PHPCR\Translation\LocaleChooser\LocaleChooser;
@@ -75,6 +76,22 @@ public function testSecoundLevelOverwrite()
$this->assertEquals($tmpDocEn->text, 'english');
}
+ public function testIdStrategy()
+ {
+ $metadata = $this->dm->getClassMetadata('\Doctrine\Tests\ODM\PHPCR\Functional\ParentIdStrategy');
+ $this->assertEquals(ClassMetadata::GENERATOR_TYPE_PARENT, $metadata->idGenerator, 'parentId');
+ $metadata = $this->dm->getClassMetadata('\Doctrine\Tests\ODM\PHPCR\Functional\ParentIdStrategyDifferentOrder');
+ $this->assertEquals(ClassMetadata::GENERATOR_TYPE_PARENT, $metadata->idGenerator, 'parentId2');
+ $metadata = $this->dm->getClassMetadata('\Doctrine\Tests\ODM\PHPCR\Functional\AssignedIdStrategy');
+ $this->assertEquals(ClassMetadata::GENERATOR_TYPE_ASSIGNED, $metadata->idGenerator, 'assigned');
+ $metadata = $this->dm->getClassMetadata('\Doctrine\Tests\ODM\PHPCR\Functional\RepositoryIdStrategy');
+ $this->assertEquals(ClassMetadata::GENERATOR_TYPE_REPOSITORY, $metadata->idGenerator, 'repository');
+ $metadata = $this->dm->getClassMetadata('\Doctrine\Tests\ODM\PHPCR\Functional\AutoAssignedIdStrategy');
+ $this->assertEquals(ClassMetadata::GENERATOR_TYPE_ASSIGNED, $metadata->idGenerator, 'autoassigned');
+ $metadata = $this->dm->getClassMetadata('\Doctrine\Tests\ODM\PHPCR\Functional\StandardCase');
+ $this->assertEquals(ClassMetadata::GENERATOR_TYPE_ASSIGNED, $metadata->idGenerator, 'standardcase');
+ }
+
// TODO comprehensive test for all possible mapped fields in an abstract test, trying to persist and check if properly set
// then dm->clear and check if still properly set.
@@ -136,3 +153,84 @@ class SecondLevelWithDuplicateOverwrite extends ExtendingClass
/** @PHPCRODM\String(translated=true) */
public $text;
}
+
+/**
+ * @PHPCRODM\Document
+ */
+class ParentIdStrategy
+{
+ /** @PHPCRODM\Id */
+ public $id;
+
+ /** @PHPCRODM\Nodename */
+ public $name;
+
+ /** @PHPCRODM\ParentDocument */
+ public $parent;
+}
+
+/**
+ * @PHPCRODM\Document
+ */
+class ParentIdStrategyDifferentOrder
+{
+ /** @PHPCRODM\Nodename */
+ public $name;
+
+ /** @PHPCRODM\ParentDocument */
+ public $parent;
+
+ /** @PHPCRODM\Id */
+ public $id;
+}
+
+/**
+ * @PHPCRODM\Document
+ */
+class AssignedIdStrategy
+{
+ /** @PHPCRODM\Id(strategy="assigned") */
+ public $id;
+
+ /** @PHPCRODM\Nodename */
+ public $name;
+
+ /** @PHPCRODM\ParentDocument */
+ public $parent;
+}
+
+/**
+ * @PHPCRODM\Document
+ */
+class RepositoryIdStrategy
+{
+ /** @PHPCRODM\Nodename */
+ public $name;
+
+ /** @PHPCRODM\ParentDocument */
+ public $parent;
+
+ /** @PHPCRODM\Id(strategy="repository") */
+ public $id;
+}
+
+/**
+* @PHPCRODM\Document
+*/
+class AutoAssignedIdStrategy
+{
+ /** @PHPCRODM\ParentDocument */
+ public $parent;
+
+ /** @PHPCRODM\Id() */
+ public $id;
+}
+
+/**
+ * @PHPCRODM\Document
+ */
+class StandardCase
+{
+ /** @PHPCRODM\Id */
+ public $id;
+}