Skip to content

Loading…

when replacing children having children of their own, things fail #262

Open
wants to merge 1 commit into from

2 participants

@dbu
Doctrine member
dbu commented

this is a WIP with failing tests and some tentative fixes that seem to be wrong.

see https://github.com/symfony-cmf/symfony-cmf/wiki/PHPCR-ODM-replace-document for a proposal how to actually solve the problems.

This needs hacks for example in the BlockBundle\Document\ImagineBlock::setImage

@dbu dbu commented on the diff
lib/Doctrine/ODM/PHPCR/DocumentManager.php
@@ -588,6 +588,10 @@ public function persist($document)
throw new \InvalidArgumentException('Parameter $document needs to be an object, '.gettype($document).' given');
}
+ if (UnitOfWork::STATE_MANAGED === $this->unitOfWork->getDocumentState($document)) {
+ return;
@dbu Doctrine member
dbu added a note

this causes CascadePersistTest::testCascadePersistForManagedEntity to fail. i would have expected that the new children automatically get persisted, without needing to call persist again.

@dbu Doctrine member
dbu added a note

ok, whatever else we do, this is wrong as we diverge from the logic of what orm is doing in this case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dbu dbu commented on the diff
lib/Doctrine/ODM/PHPCR/UnitOfWork.php
@@ -1349,7 +1355,7 @@ private function doMergeSingleDocumentProperty($managedCopy, $document, \Reflect
$prop->setValue($managedCopy, $document);
} else {
$targetClass = $this->dm->getClassMetadata(get_class($document));
- $id = $targetClass->getIdentifierValues($document);
+ $id = $targetClass->getIdentifierValue($document);
@dbu Doctrine member
dbu added a note

not sure why this was like this, but with the new situations we tried to get the proxy with an $id that was actually an array, leading to an illegal offset error.

@dbu Doctrine member
dbu added a note

note that while doctrine orm can have multi column primary key and thus has the identifier values with an array, phpcr-odm only ever has one key which is the path to the node.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dbu dbu commented on the diff
lib/Doctrine/ODM/PHPCR/UnitOfWork.php
((11 lines not shown))
if (null !== $other) {
+ $childMeta = $this->dm->getClassMetadata(get_class($other));
+ $childMeta->setIdentifierValue($other, $childId);
@dbu Doctrine member
dbu added a note

this was my attempt to make sure the child of the child has an id as well and can be merged. but at this point we no longer get an error, but keep the old grandchild.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dbu dbu commented on the diff
tests/Doctrine/Tests/ODM/PHPCR/Functional/ChildTest.php
@@ -280,6 +280,98 @@ public function testChildReplace()
$this->assertEquals('new name', $parent->child->name);
}
+ public function testChildReplaceRemoveChild()
@dbu Doctrine member
dbu added a note

here the merge leeds to have the grand child as child of the replacing document. imo it should be dropped as the new node has child == null.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dbu dbu commented on the diff
tests/Doctrine/Tests/ODM/PHPCR/Functional/ChildTest.php
((49 lines not shown))
+ $newChild->name = 'new name';
+ $parent->child = $newChild;
+ $newGrandChild = new ChildChildTestObj();
+ $newChild->child = $newGrandChild;
+ $newGrandChild->name = 'new grandchild';
+
+ $this->dm->flush();
+ $this->dm->clear();
+
+ $parent = $this->dm->find($this->type, '/functional/childtest');
+ $this->assertEquals('new name', $parent->child->name);
+ $this->assertEquals('new grandchild', $parent->child->child->name);
+ }
+
+
+ public function testChildReplaceDeepUnneededPersist()
@dbu Doctrine member
dbu added a note

this is to emulate the sonata behaviour of calling persist even for already managed documents. i would have expected this to be unnecessary, but in this case it leads to an exception, while deactivating persist for managed documents in general leads to the cascading test to fail, as noted above

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dbu dbu commented on the diff
tests/Doctrine/Tests/ODM/PHPCR/Functional/ChildTest.php
((38 lines not shown))
+ $parent->id = '/functional/childtest';
+ $parent->child = $child;
+ $child->name = 'Child';
+ $child->child = $grandchild;
+ $grandchild->name = 'Grandchild';
+ $this->dm->persist($parent);
+ $this->dm->flush();
+ $this->dm->clear();
+
+ $parent = $this->dm->find($this->type, '/functional/childtest');
+ $newChild = new ChildTestObj();
+ $newChild->name = 'new name';
+ $parent->child = $newChild;
+ $newGrandChild = new ChildChildTestObj();
+ $newChild->child = $newGrandChild;
+ $newGrandChild->name = 'new grandchild';
@dbu Doctrine member
dbu added a note

the new grandchild is lost and we still get the old one

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dbu
Doctrine member

do we still consider this a blocker for a stable 1.0 release? or can we consider this a known bug and go stable without solving it?

@lsmith77 lsmith77 added review wip/poc and removed review labels
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 8, 2013
  1. @dbu
View
4 lib/Doctrine/ODM/PHPCR/DocumentManager.php
@@ -588,6 +588,10 @@ public function persist($document)
throw new \InvalidArgumentException('Parameter $document needs to be an object, '.gettype($document).' given');
}
+ if (UnitOfWork::STATE_MANAGED === $this->unitOfWork->getDocumentState($document)) {
+ return;
@dbu Doctrine member
dbu added a note

this causes CascadePersistTest::testCascadePersistForManagedEntity to fail. i would have expected that the new children automatically get persisted, without needing to call persist again.

@dbu Doctrine member
dbu added a note

ok, whatever else we do, this is wrong as we diverge from the logic of what orm is doing in this case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ }
+
$this->errorIfClosed();
$this->unitOfWork->scheduleInsert($document);
}
View
2 lib/Doctrine/ODM/PHPCR/Id/AssignedIdGenerator.php
@@ -36,7 +36,7 @@ public function generate($document, ClassMetadata $cm, DocumentManager $dm)
{
$id = $cm->getFieldValue($document, $cm->identifier);
if (!$id) {
- throw new \RuntimeException('ID could not be determined. Make sure the document has a property with Doctrine\ODM\PHPCR\Mapping\Annotations\Id annotation and that the property is set to the path where the document is to be stored.');
+ throw new \RuntimeException('ID could not be determined. Make sure the document has a property mapped as id and that the property is set to the path where the document is to be stored.');
}
return $id;
View
17 lib/Doctrine/ODM/PHPCR/UnitOfWork.php
@@ -1340,6 +1340,12 @@ public function merge($document)
return $this->doMerge($document, $visited);
}
+ /**
+ * @param object $managedCopy The document containing this property
+ * @param object $document The new child document that is found in this property
+ * @param \ReflectionProperty $prop The property
+ * @param array $mapping Mapping options for the property
+ */
private function doMergeSingleDocumentProperty($managedCopy, $document, \ReflectionProperty $prop, array $mapping)
{
if (null === $document) {
@@ -1349,7 +1355,7 @@ private function doMergeSingleDocumentProperty($managedCopy, $document, \Reflect
$prop->setValue($managedCopy, $document);
} else {
$targetClass = $this->dm->getClassMetadata(get_class($document));
- $id = $targetClass->getIdentifierValues($document);
+ $id = $targetClass->getIdentifierValue($document);
@dbu Doctrine member
dbu added a note

not sure why this was like this, but with the new situations we tried to get the proxy with an $id that was actually an array, leading to an illegal offset error.

@dbu Doctrine member
dbu added a note

note that while doctrine orm can have multi column primary key and thus has the identifier values with an array, phpcr-odm only ever has one key which is the path to the node.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
$proxy = $this->getOrCreateProxy($id, $targetClass->name);
$prop->setValue($managedCopy, $proxy);
$this->registerDocument($proxy, $id);
@@ -1459,7 +1465,16 @@ private function doMerge($document, array &$visited, $prevManagedCopy = null, $a
}
$this->cascadeMergeCollection($managedCol, $mapping);
} elseif ('child' === $mapping['type']) {
+ $parentId = $class->getIdentifierValue($document);
+ $nodename = $mapping['fieldName'];
+ // TODO from looking in other places, i got the impression we should to this
+ // but this gives an exception as it tries to read the nodename property on the $other document
+ // $nodename = $this->getChildNodename($parentId, $mapping['fieldName'], $other, $document);
+ $childId = $parentId . '/' . $nodename;
+
if (null !== $other) {
+ $childMeta = $this->dm->getClassMetadata(get_class($other));
+ $childMeta->setIdentifierValue($other, $childId);
@dbu Doctrine member
dbu added a note

this was my attempt to make sure the child of the child has an id as well and can be merged. but at this point we no longer get an error, but keep the old grandchild.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
$this->doMergeSingleDocumentProperty($managedCopy, $other, $prop, $mapping);
}
} elseif ('children' === $mapping['type']) {
View
92 tests/Doctrine/Tests/ODM/PHPCR/Functional/ChildTest.php
@@ -280,6 +280,98 @@ public function testChildReplace()
$this->assertEquals('new name', $parent->child->name);
}
+ public function testChildReplaceRemoveChild()
@dbu Doctrine member
dbu added a note

here the merge leeds to have the grand child as child of the replacing document. imo it should be dropped as the new node has child == null.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ {
+ $parent = new ChildTestObj();
+ $child = new ChildTestObj();
+ $grandchild = new ChildChildTestObj();
+ $parent->name = 'Parent';
+ $parent->id = '/functional/childtest';
+ $parent->child = $child;
+ $child->name = 'Child';
+ $child->child = $grandchild;
+ $grandchild->name = 'Grandchild';
+ $this->dm->persist($parent);
+ $this->dm->flush();
+ $this->dm->clear();
+ $parent = $this->dm->find($this->type, '/functional/childtest');
+ $newChild = new ChildTestObj();
+ $newChild->name = 'new name';
+ $parent->child = $newChild;
+
+ $this->dm->flush();
+ $this->dm->clear();
+
+ $parent = $this->dm->find($this->type, '/functional/childtest');
+ $this->assertEquals('new name', $parent->child->name);
+ // should be assertNull but then tries to dump the object which goes into endless loop
+ $this->assertTrue(null == $parent->child->child, 'the child was not deleted');
+ }
+
+ public function testChildReplaceDeep()
+ {
+ $parent = new ChildTestObj();
+ $child = new ChildTestObj();
+ $grandchild = new ChildChildTestObj();
+ $parent->name = 'Parent';
+ $parent->id = '/functional/childtest';
+ $parent->child = $child;
+ $child->name = 'Child';
+ $child->child = $grandchild;
+ $grandchild->name = 'Grandchild';
+ $this->dm->persist($parent);
+ $this->dm->flush();
+ $this->dm->clear();
+
+ $parent = $this->dm->find($this->type, '/functional/childtest');
+ $newChild = new ChildTestObj();
+ $newChild->name = 'new name';
+ $parent->child = $newChild;
+ $newGrandChild = new ChildChildTestObj();
+ $newChild->child = $newGrandChild;
+ $newGrandChild->name = 'new grandchild';
@dbu Doctrine member
dbu added a note

the new grandchild is lost and we still get the old one

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+
+ $this->dm->flush();
+ $this->dm->clear();
+
+ $parent = $this->dm->find($this->type, '/functional/childtest');
+ $this->assertEquals('new name', $parent->child->name);
+ $this->assertEquals('new grandchild', $parent->child->child->name);
+ }
+
+
+ public function testChildReplaceDeepUnneededPersist()
@dbu Doctrine member
dbu added a note

this is to emulate the sonata behaviour of calling persist even for already managed documents. i would have expected this to be unnecessary, but in this case it leads to an exception, while deactivating persist for managed documents in general leads to the cascading test to fail, as noted above

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ {
+ $parent = new ChildTestObj();
+ $child = new ChildTestObj();
+ $grandchild = new ChildChildTestObj();
+ $parent->name = 'Parent';
+ $parent->id = '/functional/childtest';
+ $parent->child = $child;
+ $child->name = 'Child';
+ $child->child = $grandchild;
+ $grandchild->name = 'Grandchild';
+ $this->dm->persist($parent);
+ $this->dm->flush();
+ $this->dm->clear();
+
+ $parent = $this->dm->find($this->type, '/functional/childtest');
+ $newChild = new ChildTestObj();
+ $newChild->name = 'new name';
+ $parent->child = $newChild;
+ $newGrandChild = new ChildChildTestObj();
+ $newChild->child = $newGrandChild;
+ $newGrandChild->name = 'new grandchild';
+ $this->dm->persist($parent);
+
+ $this->dm->flush();
+ $this->dm->clear();
+
+ $parent = $this->dm->find($this->type, '/functional/childtest');
+ $this->assertEquals('new name', $parent->child->name);
+ $this->assertEquals('new grandchild', $parent->child->child->name);
+ }
+
public function testModificationAfterPersist()
{
$parent = new ChildTestObj();
Something went wrong with that request. Please try again.