Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

initial support for DocumentManager::move() #103

Merged
merged 8 commits into from

3 participants

@lsmith77
Owner

see http://www.doctrine-project.org/jira/browse/PHPCR-48

This was surprisingly easy to implement. Open question for me is if its matters when exactly we execute the move (right now its done after everything else) and if its ok to skip the move if the node is no longer registered.

@stof

I think the behavior should match the behavior of the ORM: it you call persist() on an entity scheduled for removal in the UoW, it is removed from the scheduled deletions. the same should apply here: moving a node should cancel a previous deletion in the UoW, and deleting a node in the UoW should cancel its scheduled moves. This way, the second point in your comment is solved: there is no possible inconsistency as it cannot be moved an deleted at the same time

@lsmith77
Owner

ok .. good point.

@lsmith77
Owner

ok .. it seems we didn't handle this properly for deletes atm. so i fixed that and added support for this in move.

@lsmith77 lsmith77 merged commit d2fbfdb into master
@dbu
Collaborator

so any @Parent @Child(ren) annotated fields do not get updated by the move operation, right? we should document that, could be confusing.

@lsmith77
Owner

correct.

@dbu dbu referenced this pull request from a commit
@dbu dbu adding some doc for move operation #103 9556fc7
@dbu
Collaborator

actually the @Parent can not go wrong as the moved document is still refreshed, and any other document does not change its parent, but the @Id field of children will be wrong too

@lsmith77
Owner

yes .. i guess we could update the id's of children if they are connected to the parent by a mapping. for others it would be tricky .. however move operations are not common. so we could do the additional effort of updating id's when we do have move operations on all managed documents.

@dbu
Collaborator

agreed. i just made that last comment for completeness if anybody looks at this pull request again. the doc says to call clear after the move, and thats good enough for me for now. tracking that stuff inside jackalope was enough of a pain, no need to re-start in the odm again...

@lsmith77
Owner

i guess if we had observation .. we could hook in there ..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
12 lib/Doctrine/ODM/PHPCR/DocumentManager.php
@@ -482,6 +482,18 @@ public function getLocalesFor($document)
}
/**
+ * Move the previously persisted document and all its children in the tree
+ *
+ * @param object $object
+ * @param string $targetPath
+ */
+ public function move($object, $targetPath)
+ {
+ $this->errorIfClosed();
+ $this->unitOfWork->scheduleMove($object, $targetPath);
+ }
+
+ /**
* Remove the previously persisted document and all its children from the tree
*
* Be aware of the PHPCR tree structure: this removes all nodes with a path under
View
88 lib/Doctrine/ODM/PHPCR/UnitOfWork.php
@@ -54,6 +54,7 @@ class UnitOfWork
const STATE_MANAGED = 2;
const STATE_REMOVED = 3;
const STATE_DETACHED = 4;
+ const STATE_MOVED = 5;
/**
* @var DocumentManager
@@ -145,6 +146,11 @@ class UnitOfWork
/**
* @var array
*/
+ private $scheduledMoves = array();
+
+ /**
+ * @var array
+ */
private $scheduledRemovals = array();
/**
@@ -494,10 +500,10 @@ private function doScheduleInsert($document, &$visited, $overrideIdGenerator = n
$visited[$oid] = true;
$class = $this->dm->getClassMetadata(get_class($document));
- $state = $this->getDocumentState($document);
$this->cascadeScheduleParentInsert($class, $document, $visited);
+ $state = $this->getDocumentState($document);
switch ($state) {
case self::STATE_NEW:
$this->persistNew($class, $document, $overrideIdGenerator);
@@ -505,8 +511,11 @@ private function doScheduleInsert($document, &$visited, $overrideIdGenerator = n
case self::STATE_MANAGED:
// TODO: Change Tracking Deferred Explicit
break;
+ case self::STATE_MOVED:
+ unset($this->scheduledMoves[$oid]);
+ $this->documentState[$oid] = self::STATE_MANAGED;
+ break;
case self::STATE_REMOVED:
- // document becomes managed again
unset($this->scheduledRemovals[$oid]);
$this->documentState[$oid] = self::STATE_MANAGED;
break;
@@ -600,10 +609,45 @@ private function getIdGenerator($type)
return $this->idGenerators[$type];
}
+ public function scheduleMove($document, $targetPath)
+ {
+ $oid = spl_object_hash($document);
+ $this->scheduledMoves[$oid] = array($document, $targetPath);
+
+ $state = $this->getDocumentState($document);
+ switch ($state) {
+ case self::STATE_NEW:
+ unset($this->scheduledInserts[$oid]);
+ break;
+ case self::STATE_REMOVED:
+ unset($this->scheduledRemovals[$oid]);
+ break;
+ case self::STATE_DETACHED:
+ throw new \InvalidArgumentException('Detached document passed to move(): '.self::objToStr($document));
+ break;
+ }
+
+ $this->documentState[$oid] = self::STATE_MOVED;
+ }
+
public function scheduleRemove($document)
{
$oid = spl_object_hash($document);
$this->scheduledRemovals[$oid] = $document;
+
+ $state = $this->getDocumentState($document);
+ switch ($state) {
+ case self::STATE_NEW:
+ unset($this->scheduledInserts[$oid]);
+ break;
+ case self::STATE_MOVED:
+ unset($this->scheduledMoves[$oid]);
+ break;
+ case self::STATE_DETACHED:
+ throw new \InvalidArgumentException('Detached document passed to remove(): '.self::objToStr($document));
+ break;
+ }
+
$this->documentState[$oid] = self::STATE_REMOVED;
$class = $this->dm->getClassMetadata(get_class($document));
@@ -631,6 +675,7 @@ private function purgeChildren($document)
$oid = spl_object_hash($child);
unset(
$this->scheduledRemovals[$oid],
+ $this->scheduledMoves[$oid],
$this->scheduledInserts[$oid],
$this->scheduledUpdates[$oid],
$this->scheduledAssociationUpdates[$oid]
@@ -759,7 +804,7 @@ public function computeChangeSet(ClassMetadata $class, $document)
&& isset($actualData[$class->nodename])
&& $this->originalData[$oid][$class->nodename] !== $actualData[$class->nodename]
) {
- throw new PHPCRException('The Nodename property is immutable ('.$this->originalData[$oid][$class->nodename].' !== '.$actualData[$class->nodename].'). Please use PHPCR\Session::move to rename the document: '.self::objToStr($document));
+ throw new PHPCRException('The Nodename property is immutable ('.$this->originalData[$oid][$class->nodename].' !== '.$actualData[$class->nodename].'). Please use DocumentManager::move to rename the document: '.self::objToStr($document));
}
if (isset($this->originalData[$oid][$class->parentMapping])
&& isset($actualData[$class->parentMapping])
@@ -771,7 +816,7 @@ public function computeChangeSet(ClassMetadata $class, $document)
&& isset($actualData[$class->identifier])
&& $this->originalData[$oid][$class->identifier] !== $actualData[$class->identifier]
) {
- throw new PHPCRException('The Id is immutable ('.$this->originalData[$oid][$class->identifier].' !== '.$actualData[$class->identifier].'). Please use PHPCR\Session::move to move the document: '.self::objToStr($document));
+ throw new PHPCRException('The Id is immutable ('.$this->originalData[$oid][$class->identifier].' !== '.$actualData[$class->identifier].'). Please use DocumentManager::move to move the document: '.self::objToStr($document));
}
// Document is "fully" MANAGED: it was already fully persisted before
@@ -834,7 +879,7 @@ public function computeChangeSet(ClassMetadata $class, $document)
foreach ($class->childMappings as $name => $childMapping) {
if ($actualData[$name]) {
if ($this->originalData[$oid][$name] && $this->originalData[$oid][$name] !== $actualData[$name]) {
- throw new PHPCRException('Cannot move/copy children by assignment as it would be ambiguous. Please use the PHPCR\Session::move() or PHPCR\Session::copy() operations for this: '.self::objToStr($document));
+ throw new PHPCRException('Cannot move/copy children by assignment as it would be ambiguous. Please use the DocumentManager::move() or PHPCR\Session::copy() operations for this: '.self::objToStr($document));
}
$this->computeChildChanges($childMapping, $actualData[$name], $id);
}
@@ -1085,6 +1130,8 @@ public function commit($document = null)
$this->executeRemovals($this->scheduledRemovals);
+ $this->executeMoves($this->scheduledMoves);
+
$this->session->save();
if ($utx) {
@@ -1115,6 +1162,7 @@ public function commit($document = null)
$this->scheduledUpdates =
$this->scheduledAssociationUpdates =
$this->scheduledRemovals =
+ $this->scheduledMoves =
$this->scheduledInserts =
$this->visitedCollections = array();
}
@@ -1305,7 +1353,7 @@ private function executeUpdates($documents, $dispatchEvents = true)
$child->remove();
}
} elseif ($this->originalData[$oid][$fieldName] && $this->originalData[$oid][$fieldName] !== $fieldValue) {
- throw new PHPCRException('Cannot move/copy children by assignment as it would be ambiguous. Please use the PHPCR\Session::move() or PHPCR\Session::copy() operations for this.');
+ throw new PHPCRException('Cannot move/copy children by assignment as it would be ambiguous. Please use the DocumentManager::move() or PHPCR\Session::copy() operations for this.');
}
}
}
@@ -1324,6 +1372,30 @@ private function executeUpdates($documents, $dispatchEvents = true)
}
/**
+ * Executes all document moves
+ *
+ * @param array $documents array of all to be moved documents
+ */
+ private function executeMoves($documents)
+ {
+ foreach ($documents as $oid => $value) {
+ list($document, $targetPath) = $value;
+ if (!isset($this->nodesMap[$oid])) {
+ continue;
+ }
+
+ $class = $this->dm->getClassMetadata(get_class($document));
+ $path = $class->getIdentifierValue($document);
+ $this->session->move($path, $targetPath);
+ if ($targetPath !== $this->nodesMap[$oid]->getPath()) {
+ throw new \RuntimeException("Move failed to move from '$path' to '$targetPath' for document: ".self::objToStr($document));
+ }
+ $class->setIdentifierValue($document, $targetPath);
+ $this->originalData[$oid][$class->identifier] = $targetPath;
+ }
+ }
+
+ /**
* Executes all document removals
*
* @param array $documents array of all to be removed documents
@@ -1331,11 +1403,11 @@ private function executeUpdates($documents, $dispatchEvents = true)
private function executeRemovals($documents)
{
foreach ($documents as $oid => $document) {
- $class = $this->dm->getClassMetadata(get_class($document));
if (!isset($this->nodesMap[$oid])) {
continue;
}
+ $class = $this->dm->getClassMetadata(get_class($document));
$this->doRemoveAllTranslations($document, $class);
$this->nodesMap[$oid]->remove();
@@ -1351,7 +1423,6 @@ private function executeRemovals($documents)
}
}
-
/**
* @see DocumentManager::findVersionByName
*/
@@ -1701,6 +1772,7 @@ public function clear()
$this->scheduledUpdates =
$this->scheduledAssociationUpdates =
$this->scheduledInserts =
+ $this->scheduledMoves =
$this->scheduledRemovals =
$this->visitedCollections =
$this->documentHistory =
View
164 tests/Doctrine/Tests/ODM/PHPCR/Functional/BasicCrudTest.php
@@ -111,14 +111,86 @@ public function testMultivaluePropertyWithOnlyOneValueUpdatedToMultiValue()
$this->assertEquals($userNew->numbers->toArray(), $userNew2->numbers->toArray());
}
- public function testDelete()
+ public function testMoveWithClear()
{
$this->dm->clear();
$user = $this->dm->find($this->type, '/functional/user');
$this->assertNotNull($user, 'User must exist');
+ $this->dm->move($user, '/functional/user2');
+ $user = $this->dm->find($this->type, '/functional/user');
+ $this->assertNotNull($user, 'User must exist');
+ $this->dm->flush();
+ $this->dm->clear();
+
+ $user = $this->dm->find($this->type, '/functional/user2');
+ $this->assertNotNull($user, 'User must exist');
+ }
+
+ public function testMove()
+ {
+ $this->dm->clear();
+ $user = $this->dm->find($this->type, '/functional/user');
+ $this->assertNotNull($user, 'User must exist');
+
+ $this->dm->move($user, '/functional/user2');
+ $this->dm->flush();
+
+ $user = $this->dm->find($this->type, '/functional/user2');
+ $this->assertNotNull($user, 'User must exist');
+ }
+
+ public function testMoveWithPersist()
+ {
+ $this->dm->clear();
+ $user = $this->dm->find($this->type, '/functional/user');
+ $this->assertNotNull($user, 'User must exist');
+
+ $this->dm->move($user, '/functional/user2');
+ $this->dm->persist($user);
+ $this->dm->flush();
+
+ $user = $this->dm->find($this->type, '/functional/user');
+ $this->assertNotNull($user, 'User must exist');
+ }
+
+ public function testMoveWithRemove()
+ {
+ $this->dm->clear();
+ $user = $this->dm->find($this->type, '/functional/user');
+ $this->assertNotNull($user, 'User must exist');
+
+ $this->dm->move($user, '/functional/user2');
$this->dm->remove($user);
$this->dm->flush();
+
+ $user = $this->dm->find($this->type, '/functional/user');
+ $this->assertNull($user, 'User must be null after deletion');
+ $user = $this->dm->find($this->type, '/functional/user2');
+ $this->assertNull($user, 'User must be null after deletion');
+ }
+
+ public function testMoveNoFlush()
+ {
+ $this->dm->clear();
+ $user = $this->dm->find($this->type, '/functional/user');
+ $this->assertNotNull($user, 'User must exist');
+
+ $this->dm->move($user, '/functional/user2');
+ $user = $this->dm->find($this->type, '/functional/user');
+ $this->assertNotNull($user, 'User must exist');
+ }
+
+ public function testRemoveWithClear()
+ {
+ $this->dm->clear();
+ $user = $this->dm->find($this->type, '/functional/user');
+ $this->assertNotNull($user, 'User must exist');
+
+ $this->dm->remove($user);
+ $user = $this->dm->find($this->type, '/functional/user');
+ $this->assertNotNull($user, 'User must exist');
+ $this->dm->flush();
$this->dm->clear();
$user = $this->dm->find($this->type, '/functional/user');
@@ -138,6 +210,47 @@ public function testRemove()
$this->assertNull($user, 'User must be null after deletion');
}
+ public function testRemoveWithMove()
+ {
+ $this->dm->clear();
+ $user = $this->dm->find($this->type, '/functional/user');
+ $this->assertNotNull($user, 'User must exist');
+
+ $this->dm->remove($user);
+ $this->dm->move($user, '/functional/user2');
+ $this->dm->flush();
+
+ $user = $this->dm->find($this->type, '/functional/user');
+ $this->assertNull($user, 'User must be null after deletion');
+ $user = $this->dm->find($this->type, '/functional/user2');
+ $this->assertNotNull($user, 'User must exist');
+ }
+
+ public function testRemoveWithPersist()
+ {
+ $this->dm->clear();
+ $user = $this->dm->find($this->type, '/functional/user');
+ $this->assertNotNull($user, 'User must exist');
+
+ $this->dm->remove($user);
+ $this->dm->persist($user);
+ $this->dm->flush();
+
+ $user = $this->dm->find($this->type, '/functional/user');
+ $this->assertNotNull($user, 'User must exist');
+ }
+
+ public function testRemoveNoFlush()
+ {
+ $this->dm->clear();
+ $user = $this->dm->find($this->type, '/functional/user');
+ $this->assertNotNull($user, 'User must exist');
+
+ $this->dm->remove($user);
+ $user = $this->dm->find($this->type, '/functional/user');
+ $this->assertNotNull($user, 'User must exist');
+ }
+
public function testRemoveAndInsertAfterFlush()
{
$this->dm->clear();
@@ -438,6 +551,55 @@ public function testFlushSingleDocumentWithParent()
$user3 = $this->dm->find('Doctrine\Tests\ODM\PHPCR\Functional\User', '/functional/test/team/team');
$this->assertEquals('changed', $user3->username);
}
+
+ public function testDetach()
+ {
+ $user = $this->dm->find($this->type, '/functional/user');
+ $user->username = "new-name";
+
+ $this->dm->detach($user);
+ $this->dm->flush();
+ $this->dm->clear();
+
+ $newUser = $this->dm->find($this->type, '/functional/user');
+ $this->assertEquals('lsmith', $newUser->username);
+ }
+
+ /**
+ * @expectedException \InvalidArgumentException
+ */
+ public function testDetachWithPerist()
+ {
+ $user = $this->dm->find($this->type, '/functional/user');
+ $user->username = "new-name";
+
+ $this->dm->detach($user);
+ $this->dm->persist($user);
+ }
+
+ /**
+ * @expectedException \InvalidArgumentException
+ */
+ public function testDetachWithMove()
+ {
+ $user = $this->dm->find($this->type, '/functional/user');
+ $user->username = "new-name";
+
+ $this->dm->detach($user);
+ $this->dm->move($user, '/functional/user2');
+ }
+
+ /**
+ * @expectedException \InvalidArgumentException
+ */
+ public function testDetachWithRemove()
+ {
+ $user = $this->dm->find($this->type, '/functional/user');
+ $user->username = "new-name";
+
+ $this->dm->detach($user);
+ $this->dm->remove($user);
+ }
}
/**
Something went wrong with that request. Please try again.