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

Merged
merged 4 commits into from Feb 28, 2013

Projects

None yet

2 participants

@lsmith77
Member

see http://www.doctrine-project.org/jira/browse/PHPCR-98
however instead of remove+add we are using merge

furthermore this PR ensures that for ChildrenCollection we actually use the repository strategy properly.

/cc @dbu @elhornair

@lsmith77 lsmith77 and 1 other commented on an outdated diff Feb 25, 2013
lib/Doctrine/ODM/PHPCR/UnitOfWork.php
if ($this->originalData[$oid][$fieldName] && $this->originalData[$oid][$fieldName] !== $actualData[$fieldName]) {
- throw PHPCRException::cannotMoveByAssignment(self::objToStr($actualData[$fieldName], $this->dm));
+ $this->unregisterDocument($this->originalData[$oid][$fieldName]);
+ $this->session->removeItem($id.'/'.$mapping['name']);
@lsmith77
lsmith77 Feb 25, 2013 Member

this is quite inelegant, since we are just computing changes as this point. the issue is that anything than a remove + add will be quite messy (how to figure out what to remove, how to handle children of the child, does it make sense to retain the uuid? etc). however we always first insert and then remove, but i guess the proper way to would to delay the remove until we actually execute the insert but this will require quite a bit of code.

@dbu
dbu Feb 26, 2013 Member

well adding before removing is really not possible (if we had same-name siblings it might work, but with several same-name children we would probably hit the wrong one still)

if any error occurs during calculating the changeset, do we refresh(false) the phpcr session? we probably should, then we get rid of the pending change.

@lsmith77
lsmith77 Feb 26, 2013 Member

not sure if we refresh .. but doing this remove() breaks a lot of assumptions which i would rather not. or in other words i fear to support this there is no way around adding code to simply track the need for this remove, but delay it until just before we do the actual addNode()

@dbu
Member
dbu commented Feb 26, 2013

thanks lukas. i think it makes sense to do this.

we still detect if i try to move a child by assignment, right? what happens if i try to move x from /node/x to /othernode/x by assigning it as a child of othernode? does it make any difference if i set the child of node to null as well?

@lsmith77
Member

while trying to get the remove working .. i realized that if we want to keep the current assumptions about the changes computation step, then there is no way around going in the direction of making this a merge operation, rather than a remove + insert.

@lsmith77
Member

ok use a merge operation if the new child is of the same class. otherwise throw an exception. the same can be supported for childrencollection.

@lsmith77
Member

@dbu: ready for final review/merge

@dbu dbu commented on the diff Feb 28, 2013
lib/Doctrine/ODM/PHPCR/UnitOfWork.php
$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
dbu Feb 28, 2013 Member

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.

@dbu dbu commented on the diff Feb 28, 2013
.../Doctrine/Tests/ODM/PHPCR/Functional/ChildrenTest.php
@@ -224,6 +225,49 @@ public function testModifyChildren()
$this->dm->clear();
}
+ public function testReplaceChildren()
@dbu
dbu Feb 28, 2013 Member

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
lsmith77 Feb 28, 2013 Member

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
lsmith77 Feb 28, 2013 Member

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
lsmith77 Feb 28, 2013 Member

and the also pass if you do:

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

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
lsmith77 Feb 28, 2013 Member

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
lsmith77 Feb 28, 2013 Member

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
lsmith77 Feb 28, 2013 Member

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
dbu Feb 28, 2013 Member

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
lsmith77 Feb 28, 2013 Member

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
lsmith77 Feb 28, 2013 Member

but that change breaks ChildrenTest::testReorderChildren :(

@lsmith77
lsmith77 Feb 28, 2013 Member

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

@dbu dbu merged commit 30db625 into master Feb 28, 2013

1 check passed

default The Travis build passed
Details
@dbu dbu deleted the PHPCR-98-handle-child-replacing branch Feb 28, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment