Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

fix reorder to allow multiple reorders on one node #278

Merged
merged 1 commit into from

2 participants

@dbu
Collaborator

@lsmith77 do you want to review this or should i simply merge?

composer.json
@@ -18,7 +18,8 @@
"phpcr/phpcr-utils": "~1.0.0-beta6"
},
"require-dev": {
- "symfony/yaml": ">=2.0.0,<2.4.0-dev"
+ "symfony/yaml": ">=2.0.0,<2.4.0-dev",
+ "jackalope/jackalope-jackrabbit":"dev-master"
@lsmith77 Owner

revert

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@lsmith77 lsmith77 commented on the diff
lib/Doctrine/ODM/PHPCR/UnitOfWork.php
((7 lines not shown))
case self::STATE_REMOVED:
- unset($this->scheduledRemovals[$oid]);
+ throw new \InvalidArgumentException('Removed document passed to reorder(): '.self::objToStr($document, $this->dm));
@lsmith77 Owner

i guess it makes sense that people first need to revert the remove .. but did you review how we deal with this in other places?

@dbu Collaborator
dbu added a note

i just stumbled over this and it looked a lot like being a copy-paste error from the above doScheduleInsert to me. i think only insert should ever lead to re-attach a document, everything else would more likely hide a programming error than be of any real value. if you think we need further discussion, i can take that out of this PR again so we can fix the actual issue and then discuss.

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

shall i take out the not directly related thing we discussed out of the PR? or can we merge as is?

@lsmith77
Owner

fine for me

@dbu dbu merged commit e4d723f into master
@dbu dbu deleted the fix-reorder branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Apr 22, 2013
  1. @dbu
This page is out of date. Refresh to see the latest.
View
68 lib/Doctrine/ODM/PHPCR/UnitOfWork.php
@@ -164,7 +164,7 @@ class UnitOfWork
/**
* List of parent documents that have children that will be reordered on next flush
- * parent oid => array(parent document, srcName, targetName, before) with
+ * parent oid => list of array with records array(parent document, srcName, targetName, before) with
* - parent document the document of the child to be reordered
* - srcName the Nodename of the document to be moved,
* - targetName the Nodename of the document to move srcName to
@@ -747,18 +747,17 @@ public function scheduleReorder($document, $srcName, $targetName, $before)
$state = $this->getDocumentState($document);
switch ($state) {
- case self::STATE_NEW:
- unset($this->scheduledInserts[$oid]);
- break;
case self::STATE_REMOVED:
- unset($this->scheduledRemovals[$oid]);
+ throw new \InvalidArgumentException('Removed document passed to reorder(): '.self::objToStr($document, $this->dm));
@lsmith77 Owner

i guess it makes sense that people first need to revert the remove .. but did you review how we deal with this in other places?

@dbu Collaborator
dbu added a note

i just stumbled over this and it looked a lot like being a copy-paste error from the above doScheduleInsert to me. i think only insert should ever lead to re-attach a document, everything else would more likely hide a programming error than be of any real value. if you think we need further discussion, i can take that out of this PR again so we can fix the actual issue and then discuss.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
break;
case self::STATE_DETACHED:
throw new \InvalidArgumentException('Detached document passed to reorder(): '.self::objToStr($document, $this->dm));
}
- $this->scheduledReorders[$oid] = array($document, $srcName, $targetName, $before);
- $this->setDocumentState($oid, self::STATE_MANAGED);
+ if (! isset($this->scheduledReorders[$oid])) {
+ $this->scheduledReorders[$oid] = array();
+ }
+ $this->scheduledReorders[$oid][] = array($document, $srcName, $targetName, $before);
}
public function scheduleRemove($document)
@@ -2244,37 +2243,38 @@ private function executeMoves($documents)
*/
private function executeReorders($documents)
{
- foreach ($documents as $oid => $value) {
+ foreach ($documents as $oid => $list) {
if (!$this->contains($oid)) {
continue;
}
- list($parent, $src, $target, $before) = $value;
-
- $parentNode = $this->session->getNode($this->getDocumentId($parent));
- $children = $parentNode->getNodes();
-
- // check for src and target ...
- $dest = $target;
- if (isset($children[$src]) && isset($children[$target])) {
- // there is no orderAfter, so we need to find the child after target to use it in orderBefore
- if (!$before) {
- $dest = null;
- $found = false;
- foreach ($children as $name => $child) {
- if ($name === $target) {
- $found = true;
- } elseif ($found) {
- $dest = $name;
- break;
+ foreach ($list as $value) {
+ list($parent, $src, $target, $before) = $value;
+ $parentNode = $this->session->getNode($this->getDocumentId($parent));
+
+ // check for src and target ...
+ $dest = $target;
+ if ($parentNode->hasNode($src) && $parentNode->hasNode($target)) {
+ // there is no orderAfter, so we need to find the child after target to use it in orderBefore
+ if (!$before) {
+ $dest = null;
+ $found = false;
+ foreach ($parentNode->getNodes() as $name => $child) {
+ if ($name === $target) {
+ $found = true;
+ } elseif ($found) {
+ $dest = $name;
+ break;
+ }
}
}
- }
- $parentNode->orderBefore($src, $dest);
- // set all children collection to initialized = false to force reload after reordering
- $class = $this->dm->getClassMetadata(get_class($parent));
- foreach ($class->childrenMappings as $fieldName) {
- $children = $class->reflFields[$fieldName]->getValue($parent);
- $children->setInitialized(false);
+
+ $parentNode->orderBefore($src, $dest);
+ // set all children collection to initialized = false to force reload after reordering
+ $class = $this->dm->getClassMetadata(get_class($parent));
+ foreach ($class->childrenMappings as $fieldName) {
+ $children = $class->reflFields[$fieldName]->getValue($parent);
+ $children->setInitialized(false);
+ }
}
}
}
@@ -3052,7 +3052,7 @@ public function getScheduledMoves()
*/
public function getScheduledReorders()
{
- throw new \BadMethodCallException('getScheduledReorders is not currently implementd.');
+ return $this->scheduledReorders;
}
/**
View
22 tests/Doctrine/Tests/ODM/PHPCR/Functional/ReorderTest.php
@@ -74,6 +74,21 @@ public function testReorder()
$this->assertSame(array('second', 'first', 'third', 'fourth'), $this->getChildrenNames($parent->getChildren()));
}
+ public function testReorderMultiple()
+ {
+ $parent = $this->dm->find(null, '/functional/source');
+
+ $children = $parent->getChildren();
+ $this->assertSame($this->childrenNames, $this->getChildrenNames($children));
+ $this->dm->reorder($parent, 'first', 'second', false);
+ $this->dm->reorder($parent, 'third', 'fourth', false);
+ $this->dm->flush();
+ $this->dm->clear();
+
+ $parent = $this->dm->find(null, '/functional/source');
+ $this->assertSame(array('second', 'first', 'fourth', 'third'), $this->getChildrenNames($parent->getChildren()));
+ }
+
public function testReorderNoop()
{
$parent = $this->dm->find(null, '/functional/source');
@@ -164,15 +179,14 @@ public function testRemoveAfterReorder()
$this->assertNull($parent);
}
+ /**
+ * @expectedException \InvalidArgumentException
+ */
public function testReorderAfterRemove()
{
$parent = $this->dm->find(null, '/functional/source');
$this->dm->remove($parent);
$this->dm->reorder($parent, 'first', 'second', false);
- $this->dm->flush();
-
- $parent = $this->dm->find(null, '/functional/source');
- $this->assertSame(array('second', 'first', 'third', 'fourth'), $this->getChildrenNames($parent->getChildren()));
}
private function getChildrenNames($children)
View
6 tests/Doctrine/Tests/ODM/PHPCR/Functional/UnitOfWorkTest.php
@@ -79,12 +79,10 @@ public function testSchedules()
$this->assertEquals(array($user1, '/foobar'), current($scheduledMoves));
}
- /**
- * @expectedException \BadMethodCallException
- */
public function testGetScheduledReorders()
{
- $this->uow->getScheduledReorders();
+ // TODO: do some real test
+ $this->assertCount(0, $this->uow->getScheduledReorders());
}
public function testComputeChangeSetForTranslatableDocument()
Something went wrong with that request. Please try again.