Implemented a non-recursive version of moveUp

src/Model/Behavior/TreeBehavior.php

@@ -380,7 +380,7 @@ class TreeBehavior extends Behavior {
  * @param \Cake\ORM\Entity $node The node to move
  * @param integer|boolean $number How many places to move the node, or true to move to first position
  * @throws \Cake\ORM\Error\RecordNotFoundException When node was not found
- * @return boolean true on success, false on failure
+ * @return \Cake\ORM\Entity|boolean $node The node after being moved or false on failure
  */
 	public function moveUp($node, $number = 1) {
 		$config = $this->config();
@@ -390,37 +390,45 @@ class TreeBehavior extends Behavior {
 			return false;
 		}
 
-		if ($node->{$parent}) {
-			$parentNode = $this->_table->get($node->{$parent}, ['fields' => [$left, $right]]);
+		$parentLeft = 0;
+		if ($node->get($parent)) {
+			$parentLeft = $this->_getNode($node->get($parent))->get($left);
+		}
 
-			if (($node->{$left} - 1) == $parentNode->{$left}) {
-				return false;
+		$edge = $this->_getMax();
+		while ($number-- > 0) {
+			list($nodeLeft, $nodeRight) = array_values($node->extract([$left, $right]));
+
+			if ($parentLeft && ($nodeLeft - 1 == $parentLeft)) {
+				break;
 			}
-		}
 
-		$previousNode = $this->_scope($this->_table->find())
-			->select([$left, $right])
-			->where([$right => ($node->{$left} - 1)])
-			->first();
+			$nextNode = $this->_scope($this->_table->find())
+				->select([$left, $right])
+				->where([$right => ($nodeLeft - 1)])
+				->first();
 
-		if (!$previousNode) {
-			return false;
-		}
+			if (!$nextNode) {
+				break;
+			}
 
-		$edge = $this->_getMax();
-		$this->_sync($edge - $previousNode->{$left} + 1, '+', "BETWEEN {$previousNode->{$left}} AND {$previousNode->{$right}}");
-		$this->_sync($node->{$left} - $previousNode->{$left}, '-', "BETWEEN {$node->{$left}} AND {$node->{$right}}");
-		$this->_sync($edge - $previousNode->{$left} - ($node->{$right} - $node->{$left}), '-', "> {$edge}");
+			$this->_sync($edge - $nextNode->{$left} + 1, '+', "BETWEEN {$nextNode->{$left}} AND {$nextNode->{$right}}");
+			$this->_sync($nodeLeft - $nextNode->{$left}, '-', "BETWEEN {$nodeLeft} AND {$nodeRight}");
+			$this->_sync($edge - $nextNode->{$left} - ($nodeRight - $nodeLeft), '-', "> {$edge}");
 
-		if (is_int($number)) {
-			$number--;
-		}
+			$newLeft = $nodeLeft;
+			if ($nodeLeft >= $nextNode->{$left} || $nodeLeft <= $nextNode->{$right}) {
+				$newLeft -= $edge - $nextNode->{$left} + 1;
+			}
+			$newLeft = $nodeLeft - ($nodeLeft - $nextNode->{$left});
 
-		if ($number) {
-			$this->moveUp($node, $number);
+			$node->set($left, $newLeft);
+			$node->set($right, $newLeft + ($nodeRight - $nodeLeft));
 		}
-
-		return true;
+		
+		$node->dirty($left, false);
+		$node->dirty($right, false);
+		return $node;
 	}
 
 /**
@@ -455,7 +463,7 @@ class TreeBehavior extends Behavior {
 		while ($number-- > 0) {
 			list($nodeLeft, $nodeRight) = array_values($node->extract([$left, $right]));
 
-			if ($parentRight && ($right + 1 == $parentRight)) {
+			if ($parentRight && ($nodeRight + 1 == $parentRight)) {
 				break;
 			}
 

tests/TestCase/Model/Behavior/TreeBehaviorTest.php

@@ -168,23 +168,44 @@ class TreeBehaviorTest extends TestCase {
 		$table->addBehavior('Tree', ['scope' => ['menu' => 'main-menu']]);
 
 		// top level, wont move
-		$this->assertFalse($this->table->moveUp($table->get(1), 10));
+		$node = $this->table->moveUp($table->get(1), 10);
+		$this->assertEquals(['lft' => 1, 'rght' => 10], $node->extract(['lft', 'rght']));
 
 		// edge cases
 		$this->assertFalse($this->table->moveUp($table->get(1), 0));
-		$this->assertFalse($this->table->moveUp($table->get(1), -10));
+		$node = $this->table->moveUp($table->get(1), -10);
+		$this->assertEquals(['lft' => 1, 'rght' => 10], $node->extract(['lft', 'rght']));
 
 		// move inner node
-		$result = $table->moveUp($table->get(3), 1);
+		$node = $table->moveUp($table->get(3), 1);
 		$nodes = $table->find('children', ['for' => 1])->all();
 		$this->assertEquals([3, 4, 5, 2], $nodes->extract('id')->toArray());
-		$this->assertTrue($result);
+		$this->assertEquals(['lft' => 2, 'rght' => 7], $node->extract(['lft', 'rght']));
+		return;
+	}
 
-		// move leaf
-		$this->assertFalse($table->moveUp($table->get(5), 1));
+/**
+ * Tests moving a node with no siblings
+ *
+ * @return void
+ */
+	public function testMoveLeaf() {
+		$table = TableRegistry::get('MenuLinkTrees');
+		$table->addBehavior('Tree', ['scope' => ['menu' => 'main-menu']]);
+		$node = $table->moveUp($table->get(5), 1);
+		$this->assertEquals(['lft' => 6, 'rght' => 7], $node->extract(['lft', 'rght']));
+	}
 
-		// move to first position
-		$table->moveUp($table->get(8), true);
+/**
+ * Tests moving a node to the top
+ *
+ * @return void
+ */
+	public function testMoveTop() {
+		$table = TableRegistry::get('MenuLinkTrees');
+		$table->addBehavior('Tree', ['scope' => ['menu' => 'main-menu']]);
+		$node = $table->moveUp($table->get(8), true);
+		$this->assertEquals(['lft' => 1, 'rght' => 2], $node->extract(['lft', 'rght']));
 		$nodes = $table->find()
 			->select(['id'])
 			->where(function($exp) {