Skip to content

[PHPCR-98] and allow replacing child documents #253

Merged
merged 4 commits into from Feb 28, 2013
View
55 lib/Doctrine/ODM/PHPCR/UnitOfWork.php
@@ -20,6 +20,7 @@
namespace Doctrine\ODM\PHPCR;
use Doctrine\ODM\PHPCR\Mapping\ClassMetadata;
+use PHPCR\Util\PathHelper;
use PHPCR\Util\NodeHelper;
use PHPCR\PathNotFoundException;
use Doctrine\ODM\PHPCR\Exception\CascadeException;
@@ -902,13 +903,13 @@ private function getDocumentActualData(ClassMetadata $class, $document)
return $actualData;
}
- private function getChildNodename($id, $nodename, $child)
+ private function getChildNodename($id, $nodename, $child, $parent)
{
$childClass = $this->dm->getClassMetadata(get_class($child));
if ($childClass->nodename && $childClass->reflFields[$childClass->nodename]->getValue($child)) {
$nodename = $childClass->reflFields[$childClass->nodename]->getValue($child);
} else {
- $childId = $childClass->getIdentifierValue($child);
+ $childId = $childClass->getIdentifierValue($child) ?: $this->getIdGenerator($childClass->idGenerator)->generate($child, $childClass, $this->dm, $parent);
if ('' !== $childId) {
if ($childId !== $id.'/'.basename($childId)) {
throw PHPCRException::cannotMoveByAssignment(self::objToStr($child, $this->dm));
@@ -954,8 +955,8 @@ public function computeChangeSet(ClassMetadata $class, $document)
$mapping = $class->mappings[$fieldName];
if ($actualData[$fieldName]) {
foreach ($actualData[$fieldName] as $nodename => $child) {
- $nodename = $this->getChildNodename($id, $nodename, $child);
- $this->computeChildChanges($mapping, $child, $id, $nodename, $document);
+ $nodename = $this->getChildNodename($id, $nodename, $child, $document);
+ $actualData[$fieldName][$nodename] = $this->computeChildChanges($mapping, $child, $id, $nodename, $document);
$childNames[] = $nodename;
}
}
@@ -974,11 +975,8 @@ public function computeChangeSet(ClassMetadata $class, $document)
foreach ($class->childMappings as $fieldName) {
if ($actualData[$fieldName]) {
- if ($this->originalData[$oid][$fieldName] && $this->originalData[$oid][$fieldName] !== $actualData[$fieldName]) {
- throw PHPCRException::cannotMoveByAssignment(self::objToStr($actualData[$fieldName], $this->dm));
- }
$mapping = $class->mappings[$fieldName];
- $this->computeChildChanges($mapping, $actualData[$fieldName], $id);
+ $actualData[$fieldName] = $this->computeChildChanges($mapping, $actualData[$fieldName], $id, $mapping['name']);
}
}
@@ -1072,8 +1070,8 @@ public function computeChangeSet(ClassMetadata $class, $document)
$childNames = array();
if ($actualData[$fieldName]) {
foreach ($actualData[$fieldName] as $nodename => $child) {
- $nodename = $this->getChildNodename($id, $nodename, $child);
- $this->computeChildChanges($mapping, $child, $id, $nodename, $document);
+ $nodename = $this->getChildNodename($id, $nodename, $child, $document);
+ $actualData[$fieldName][$nodename] = $this->computeChildChanges($mapping, $child, $id, $nodename, $document);
$childNames[] = $nodename;
}
}
@@ -1145,8 +1143,10 @@ public function computeChangeSet(ClassMetadata $class, $document)
* @param string $parentId
* @param string $nodename
* @param mixed $parent
+ *
+ * @return object the child instance (if we are replacing a child this can be a different instance than was originally provided)
*/
- private function computeChildChanges($mapping, $child, $parentId, $nodename = null, $parent = null)
+ private function computeChildChanges($mapping, $child, $parentId, $nodename, $parent = null)
{
$targetClass = $this->dm->getClassMetadata(get_class($child));
$state = $this->getDocumentState($child);
@@ -1156,16 +1156,27 @@ private function computeChildChanges($mapping, $child, $parentId, $nodename = nu
if (!($mapping['cascade'] & ClassMetadata::CASCADE_PERSIST) ) {
throw CascadeException::newDocumentFound(self::objToStr($child));
}
- $nodename = $nodename ?: $mapping['name'];
- if ($nodename) {
- $targetClass->setIdentifierValue($child, $parentId.'/'.$nodename);
+
+ $childId = $parentId.'/'.$nodename;
+ $targetClass->setIdentifierValue($child, $childId);
+
+ if ($this->getDocumentById($childId)) {
+ $child = $this->merge($child);
+ } else {
+ $this->persistNew($targetClass, $child, ClassMetadata::GENERATOR_TYPE_ASSIGNED, $parent);
}
- $this->persistNew($targetClass, $child, ClassMetadata::GENERATOR_TYPE_ASSIGNED, $parent);
+
$this->computeChangeSet($targetClass, $child);
break;
case self::STATE_DETACHED:
throw new \InvalidArgumentException('A detached document was found through a child relationship during cascading a persist operation: '.self::objToStr($child, $this->dm));
+ default:
+ if (PathHelper::getParentPath($this->getDocumentId($child)) !== $parentId) {
+ throw PHPCRException::cannotMoveByAssignment(self::objToStr($child, $this->dm));
+ }
@dbu
Doctrine member
dbu added a note Feb 28, 2013

this is the fix needed to detect moving single node by assignment. once the number of PR is reduced, we should go through all code and replace basename/dirname calls to use the phpcr utils too. already do it right in this place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
}
+
+ return $child;
}
/**
@@ -1934,15 +1945,11 @@ private function executeUpdates($documents, $dispatchEvents = true)
}
}
} elseif ('child' === $mapping['type']) {
- if ($fieldValue === null) {
- if ($node->hasNode($mapping['name'])) {
- $child = $node->getNode($mapping['name']);
- $childDocument = $this->getOrCreateDocument(null, $child);
- $this->purgeChildren($childDocument);
- $child->remove();
- }
- } elseif ($this->originalData[$oid][$fieldName] && $this->originalData[$oid][$fieldName] !== $fieldValue) {
- throw PHPCRException::cannotMoveByAssignment(self::objToStr($fieldValue, $this->dm));
+ if ($fieldValue === null && $node->hasNode($mapping['name'])) {
+ $child = $node->getNode($mapping['name']);
+ $childDocument = $this->getOrCreateDocument(null, $child);
+ $this->purgeChildren($childDocument);
+ $child->remove();
}
}
}
View
37 tests/Doctrine/Tests/ODM/PHPCR/Functional/ChildTest.php
@@ -225,7 +225,36 @@ public function testChildSetNull()
$this->assertNull($parent->child);
}
- /* this fails as the newChild is not persisted */
+ /**
+ * @expectedException \Doctrine\ODM\PHPCR\PHPCRException
+ */
+ public function testMoveByAssignment()
+ {
+ $original = new ChildTestObj();
+ $child = new ChildChildTestObj();
+ $original->name = 'Parent';
+ $original->id = '/functional/original';
+ $original->child = $child;
+ $child->name = 'Child';
+
+ $this->dm->persist($original);
+
+ $other = new ChildTestObj();
+ $other->name = 'newparent';
+ $other->id = '/functional/newlocation';
+ $this->dm->persist($other);
+
+ $this->dm->flush();
+ $this->dm->clear();
+
+ $original = $this->dm->find($this->type, '/functional/original');
+ $other = $this->dm->find($this->type, '/functional/newlocation');
+
+ $other->child = $original->child;
+
+ $this->dm->flush();
+ }
+
public function testChildReplace()
{
$parent = new ChildTestObj();
@@ -244,9 +273,11 @@ public function testChildReplace()
$newChild->name = 'new name';
$parent->child = $newChild;
- $this->setExpectedException('Doctrine\ODM\PHPCR\PHPCRException');
-
$this->dm->flush();
+ $this->dm->clear();
+
+ $parent = $this->dm->find($this->type, '/functional/childtest');
+ $this->assertEquals('new name', $parent->child->name);
}
public function testModificationAfterPersist()
View
124 tests/Doctrine/Tests/ODM/PHPCR/Functional/ChildrenTest.php
@@ -5,6 +5,7 @@
use Doctrine\ODM\PHPCR\Id\RepositoryIdInterface,
Doctrine\ODM\PHPCR\DocumentRepository,
Doctrine\ODM\PHPCR\Mapping\Annotations as PHPCRODM;
+use Doctrine\ODM\PHPCR\ChildrenCollection;
use PHPCR\RepositoryInterface;
use Doctrine\Common\Collections\ArrayCollection;
@@ -37,22 +38,22 @@ public function setUp()
$this->dm->persist($parent);
$child = new ChildrenTestObj();
- $child->id = '/functional/parent/child-a';
+ $child->id = '/functional/parent/Child A';
$child->name = 'Child A';
$this->dm->persist($child);
$child = new ChildrenTestObj();
- $child->id = '/functional/parent/child-b';
+ $child->id = '/functional/parent/Child B';
$child->name = 'Child B';
$this->dm->persist($child);
$child = new ChildrenTestObj();
- $child->id = '/functional/parent/child-c';
+ $child->id = '/functional/parent/Child C';
$child->name = 'Child C';
$this->dm->persist($child);
$child = new ChildrenTestObj();
- $child->id = '/functional/parent/child-d';
+ $child->id = '/functional/parent/Child D';
$child->name = 'Child D';
$this->dm->persist($child);
@@ -66,29 +67,29 @@ public function testChildrenCollection()
$col = $this->dm->getChildren($parent);
$this->assertCount(4, $col);
- $childA = $col['child-a'];
+ $childA = $col['Child A'];
$this->assertEquals('Child A', $childA->name);
- $this->assertEquals('child-a', $col->key());
+ $this->assertEquals('Child A', $col->key());
- $col = $this->dm->getChildren($parent, 'child*');
+ $col = $this->dm->getChildren($parent, 'Child*');
$this->assertCount(4, $col);
- $col = $this->dm->getChildren($parent, '*a');
+ $col = $this->dm->getChildren($parent, '*A');
$this->assertCount(1, $col);
$this->assertTrue($childA === $col->first());
$this->dm->clear();
- $this->dm->find('Doctrine\Tests\ODM\PHPCR\Functional\ChildrenTestObj', '/functional/parent/child-d');
+ $this->dm->find('Doctrine\Tests\ODM\PHPCR\Functional\ChildrenTestObj', '/functional/parent/Child D');
$parent = $this->dm->find('Doctrine\Tests\ODM\PHPCR\Functional\ChildrenTestObj', '/functional/parent');
$col = $this->dm->getChildren($parent);
- $this->assertEquals('child-a', $col->key());
+ $this->assertEquals('Child A', $col->key());
$this->dm->clear();
- $this->dm->find('Doctrine\Tests\ODM\PHPCR\Functional\ChildrenTestObj', '/functional/parent/child-d');
+ $this->dm->find('Doctrine\Tests\ODM\PHPCR\Functional\ChildrenTestObj', '/functional/parent/Child D');
$parent = $this->dm->find('Doctrine\Tests\ODM\PHPCR\Functional\ChildrenTestObj', '/functional/parent');
- $this->assertEquals('child-a', $parent->allChildren->key());
+ $this->assertEquals('Child A', $parent->allChildren->key());
}
public function testNoChildrenInitOnFlush()
@@ -141,24 +142,24 @@ public function testCreateChildren()
{
$children = array();
$child = new ChildrenTestObj();
- $child->id = '/functional/parent/child-a/child-create-1';
+ $child->id = '/functional/parent/Child A/Child Create-1';
$child->name = 'Child A';
$children[] = $child;
$child = new ChildrenTestObj();
- $child->id = '/functional/parent/child-a/child-create-2';
+ $child->id = '/functional/parent/Child A/Child Create-2';
$child->name = 'Child B';
$children[] = $child;
- $parent = $this->dm->find('Doctrine\Tests\ODM\PHPCR\Functional\ChildrenTestObj', '/functional/parent/child-a');
+ $parent = $this->dm->find('Doctrine\Tests\ODM\PHPCR\Functional\ChildrenTestObj', '/functional/parent/Child A');
$this->assertCount(0, $parent->allChildren);
$parent->allChildren = $children;
$this->dm->persist($parent);
$this->dm->flush();
$this->dm->clear();
- $parent = $this->dm->find('Doctrine\Tests\ODM\PHPCR\Functional\ChildrenTestObj', '/functional/parent/child-a');
+ $parent = $this->dm->find('Doctrine\Tests\ODM\PHPCR\Functional\ChildrenTestObj', '/functional/parent/Child A');
$this->assertCount(2, $parent->allChildren);
}
@@ -183,11 +184,11 @@ public function testModifyChildren()
$child = $parent->allChildren->first();
$child->name = 'New name';
- $parent->allChildren->remove('child-b');
- $parent->allChildren->remove('child-c');
+ $parent->allChildren->remove('Child B');
+ $parent->allChildren->remove('Child C');
$child = new ChildrenTestObj();
- $child->id = '/functional/parent/child-e';
+ $child->id = '/functional/parent/Child E';
$child->name = 'Child E';
$parent->allChildren->add($child);
@@ -209,13 +210,13 @@ public function testModifyChildren()
$this->assertCount(0, $parent->allChildren);
$child = new ChildrenTestObj();
- $child->id = '/functional/parent/child-f';
+ $child->id = '/functional/parent/Child F';
$child->name = 'Child F';
$parent->allChildren->add($child);
$child = new ChildrenTestObj();
- $child->id = '/functional/parent/child-g';
+ $child->id = '/functional/parent/Child G';
$child->name = 'Child G';
$parent->allChildren->add($child);
@@ -224,6 +225,50 @@ public function testModifyChildren()
$this->dm->clear();
}
+ public function testReplaceChildren()
@dbu
Doctrine member
dbu added a note Feb 28, 2013

this test now fails. previously the test in ChildTest just added a child to a collection that did not have that child.

if i comment out adding the new child, child-a gets deleted as expected. but with this code, my repo does not execute the removal of child-a, but instead keep it with old data, and add the new node as "'0" (because of the index i guess). afaik having the correct index is not mandatory. and anyways it should not prevent deletion.

@lsmith77
Doctrine member
lsmith77 added a note Feb 28, 2013

well if you are adding a new child without setting a nodename on the model or setting the proper key on the collection, you are not replacing. you are adding a new node.

@lsmith77
Doctrine member
lsmith77 added a note Feb 28, 2013

in other words the tests pass if you do:

        $parent->allChildren->set('child-a', $newChild);

they also pass if you do:

        $parent->allChildren->add($newChild);
        $newChild->nodename = 'child-a';
@lsmith77
Doctrine member
lsmith77 added a note Feb 28, 2013

and the also pass if you do:

        $this->assertEquals('child-a', $parent->allChildren['0']->name);
@dbu
Doctrine member
dbu added a note Feb 28, 2013

i have two "buts":

  • even if this is true, it is wrong that child-a is still present as a child after the flush. we removed it
  • i think the repository strategy would create the nodename from the name property. so i thought it should be enough to set the name. looks like the repository strategy case is somehow confusing us at this point?
@lsmith77
Doctrine member
lsmith77 added a note Feb 28, 2013

yes .. the issue is that we are not handling the repository strategy. the net result is that by adding the node even without a nodename it ends up being added as child-a but the entire code that decides if this is a replace or not doesnt know this yet.

@lsmith77
Doctrine member
lsmith77 added a note Feb 28, 2013

hmm i dont think we can make it work with the repository strategy however .. since the document is not yet persisted. so i fear that if you use the repository strategy, then you cannot use replace unless you set the nodename.

@lsmith77
Doctrine member
lsmith77 added a note Feb 28, 2013

theoretically we could trigger the generator earlier to check .. but this seems a bit risky as this can break assumptions people might have in the generator and i am not sure if its worth it.

@dbu
Doctrine member
dbu added a note Feb 28, 2013

it is surprising if the repository strategy does not work in this case. but i can live with that if the alternative is a big messup that affects everything else. we then just have to make people understand what the proper way to add a named child to the collection is. is adding to the collection always ignoring the repository strategy, or only in the case of merge?

but unless i am really confused, the test as i wrote it has the side effect of preventing the removal of the old child-a so there is some mixup/sideffect.

@lsmith77
Doctrine member
lsmith77 added a note Feb 28, 2013

if we do want to do this:


diff --git a/lib/Doctrine/ODM/PHPCR/UnitOfWork.php b/lib/Doctrine/ODM/PHPCR/UnitOfWork.php
index 3d9df08..c9cca69 100644
--- a/lib/Doctrine/ODM/PHPCR/UnitOfWork.php
+++ b/lib/Doctrine/ODM/PHPCR/UnitOfWork.php
@@ -903,13 +903,16 @@ class UnitOfWork
         return $actualData;
     }

-    private function getChildNodename($id, $nodename, $child)
+    private function getChildNodename($id, $nodename, $child, $parent)
     {
         $childClass = $this->dm->getClassMetadata(get_class($child));
         if ($childClass->nodename && $childClass->reflFields[$childClass->nodename]->getValue($child)) {
             $nodename = $childClass->reflFields[$childClass->nodename]->getValue($child);
         } else {
             $childId = $childClass->getIdentifierValue($child);
+
+            $childId = $childId ?: $this->getIdGenerator($childClass->idGenerator)->generate($child, $childClass, $this->dm, $parent);
+
             if ('' !== $childId) {
                 if ($childId !== $id.'/'.basename($childId)) {
                     throw PHPCRException::cannotMoveByAssignment(self::objToStr($child, $this->dm));
@@ -955,7 +958,7 @@ class UnitOfWork
                 $mapping = $class->mappings[$fieldName];
                 if ($actualData[$fieldName]) {
                     foreach ($actualData[$fieldName] as $nodename => $child) {
-                        $nodename = $this->getChildNodename($id, $nodename, $child);
+                        $nodename = $this->getChildNodename($id, $nodename, $child, $document);
                         $actualData[$fieldName][$nodename] = $this->computeChildChanges($mapping, $child, $id, $nodename, $document);
                         $childNames[] = $nodename;
                     }
@@ -1070,7 +1073,7 @@ class UnitOfWork
                 $childNames = array();
                 if ($actualData[$fieldName]) {
                     foreach ($actualData[$fieldName] as $nodename => $child) {
-                        $nodename = $this->getChildNodename($id, $nodename, $child);
+                        $nodename = $this->getChildNodename($id, $nodename, $child, $document);
                         $actualData[$fieldName][$nodename] = $this->computeChildChanges($mapping, $child, $id, $nodename, $document);
                         $childNames[] = $nodename;
                     }
@lsmith77
Doctrine member
lsmith77 added a note Feb 28, 2013

but that change breaks ChildrenTest::testReorderChildren :(

@lsmith77
Doctrine member
lsmith77 added a note Feb 28, 2013

aha .. but this might actually expose a bug .. because we were generally ignoring the repository strategy if we have a node name.

@lsmith77
Doctrine member
lsmith77 added a note Feb 28, 2013

ok see e19624b

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ {
+ /** @var $parent ChildrenTestObj */
+ $parent = $this->dm->find('Doctrine\Tests\ODM\PHPCR\Functional\ChildrenTestObj', '/functional/parent');
+ $this->assertCount(4, $parent->allChildren);
+
+ $parent->allChildren->remove('Child A');
+
+ $newChild = new ChildrenTestObj();
+ $newChild->name = 'Child A';
+
+ $parent->allChildren->add($newChild);
+
+ $this->dm->flush();
+ $this->dm->clear();
+
+ $parent = $this->dm->find('Doctrine\Tests\ODM\PHPCR\Functional\ChildrenTestObj', '/functional/parent');
+ $this->assertTrue($parent->allChildren->containsKey('Child A'));
+ $this->assertFalse($parent->allChildren->containsKey('0'));
+ $this->assertEquals('Child A', $parent->allChildren['Child A']->name);
+ }
+
+ /**
+ * @expectedException \Doctrine\ODM\PHPCR\PHPCRException
+ */
+ public function testMoveByAssignment()
+ {
+ $other = new ChildrenTestObj();
+ $other->id = '/functional/other';
+ $this->dm->persist($other);
+ $this->dm->flush();
+ $this->dm->clear();
+
+
+ /** @var $parent ChildrenTestObj */
+ $parent = $this->dm->find('Doctrine\Tests\ODM\PHPCR\Functional\ChildrenTestObj', '/functional/parent');
+ $this->assertCount(4, $parent->allChildren);
+
+ $other = $this->dm->find('Doctrine\Tests\ODM\PHPCR\Functional\ChildrenTestObj', '/functional/other');
+ $other->allChildren->add($parent->allChildren['Child A']);
+
+ $this->dm->flush();
+ }
+
/**
* @depends testModifyChildren
*/
@@ -243,8 +288,8 @@ public function testReorderChildren()
$this->assertCount(2, $parent->allChildren);
$data = array(
- 'child-g' => $parent->allChildren->last(),
- 'child-f' => $parent->allChildren->first(),
+ 'Child G' => $parent->allChildren->last(),
+ 'Child F' => $parent->allChildren->first(),
);
$parent->allChildren = new ArrayCollection($data);
@@ -266,11 +311,11 @@ public function testReorderChildren()
$child3->name = 'Child J';
$data = array(
- 'child-i' => $child2,
- 'child-h' => $child1,
- 'child-f' => $parent->allChildren->last(),
- 'child-g' => $parent->allChildren->first(),
- 'child-j' => $child3
+ 'Child I' => $child2,
+ 'Child H' => $child1,
+ 'Child F' => $parent->allChildren->last(),
+ 'Child G' => $parent->allChildren->first(),
+ 'Child J' => $child3
);
$parent->allChildren = new ArrayCollection($data);
@@ -293,11 +338,11 @@ public function testReorderChildren()
$this->dm->clear();
$keys = array(
- 'child-i',
- 'child-h',
- 'child-f',
- 'child-j',
- 'child-g',
+ 'Child I',
+ 'Child H',
+ 'Child F',
+ 'Child J',
+ 'Child G',
);
$parent = $this->dm->find('Doctrine\Tests\ODM\PHPCR\Functional\ChildrenTestObj', '/functional/parent');
@@ -325,7 +370,7 @@ public function testReorderChildrenLast()
$childrenCollection->clear();
- $expectedOrder = array('child-a', 'child-d', 'child-c', 'child-b');
+ $expectedOrder = array('Child A', 'Child D', 'Child C', 'Child B');
foreach ($expectedOrder as $name) {
$childrenCollection->set($name, $children[$name]);
@@ -352,14 +397,14 @@ public function testInsertWithCustomIdStrategy()
$child->name = 'ChildB';
$children[] = $child;
- $parent = $this->dm->find('Doctrine\Tests\ODM\PHPCR\Functional\ChildrenTestObj', '/functional/parent/child-a');
+ $parent = $this->dm->find('Doctrine\Tests\ODM\PHPCR\Functional\ChildrenTestObj', '/functional/parent/Child A');
$this->assertCount(0, $parent->allChildren);
$parent->allChildren = $children;
$this->dm->flush();
$this->dm->clear();
- $parent = $this->dm->find('Doctrine\Tests\ODM\PHPCR\Functional\ChildrenTestObj', '/functional/parent/child-a');
+ $parent = $this->dm->find('Doctrine\Tests\ODM\PHPCR\Functional\ChildrenTestObj', '/functional/parent/Child A');
$this->assertCount(2, $parent->allChildren);
}
}
@@ -375,10 +420,13 @@ class ChildrenTestObj
/** @PHPCRODM\String */
public $name;
- /** @PHPCRODM\Children(filter="*a", fetchDepth=1, cascade="persist") */
+ /** @PHPCRODM\Children(filter="*A", fetchDepth=1, cascade="persist") */
public $aChildren;
- /** @PHPCRODM\Children(fetchDepth=2, cascade="persist") */
+ /**
+ * @var \Doctrine\ODM\PHPCR\ChildrenCollection
+ * @PHPCRODM\Children(fetchDepth=2, cascade="persist")
+ */
public $allChildren;
}
Something went wrong with that request. Please try again.