3 files changed, 141 insertions, 60 deletions
diff --git a/phpBB/includes/nestedset/base.php b/phpBB/includes/nestedset/base.php
index bb7acca0fb..f3bdfe1c7d 100644
--- a/phpBB/includes/nestedset/base.php
+++ b/phpBB/includes/nestedset/base.php
@@ -141,7 +141,7 @@ abstract class phpbb_nestedset_base implements phpbb_nestedset_interface
 	/**
 	* @inheritdoc
 	*/
-	public function move(array $item, $delta)
+	public function move($item_id, $delta)
 	{
 		if ($delta == 0)
 		{
@@ -156,6 +156,21 @@ abstract class phpbb_nestedset_base implements phpbb_nestedset_interface
 		$action = ($delta > 0) ? 'move_up' : 'move_down';
 		$delta = abs($delta);
 
+		// Keep $this->get_sql_where() here, to ensure we are in the right tree.
+		$sql = 'SELECT *
+			FROM ' . $this->table_name . '
+			WHERE ' . $this->column_item_id . ' = ' . (int) $item_id . '
+				' . $this->get_sql_where();
+		$result = $this->db->sql_query_limit($sql, $delta);
+		$item = $this->db->sql_fetchrow($result);
+		$this->db->sql_freeresult($result);
+
+		if (!$item)
+		{
+			$this->lock->release();
+			throw new phpbb_nestedset_exception($this->message_prefix . 'INVALID_ITEM');
+		}
+
 		/**
 		* Fetch all the siblings between the item's current spot
 		* and where we want to move it to. If there are less than $delta
@@ -248,37 +263,60 @@ abstract class phpbb_nestedset_base implements phpbb_nestedset_interface
 	/**
 	* @inheritdoc
 	*/
-	public function move_down(array $item)
+	public function move_down($item_id)
 	{
-		return $this->move($item, -1);
+		return $this->move($item_id, -1);
 	}
 
 	/**
 	* @inheritdoc
 	*/
-	public function move_up(array $item)
+	public function move_up($item_id)
 	{
-		return $this->move($item, 1);
+		return $this->move($item_id, 1);
 	}
 
 	/**
 	* @inheritdoc
 	*/
-	public function move_children(array $current_parent, array $new_parent)
+	public function move_children($current_parent_id, $new_parent_id)
 	{
-		if (($current_parent[$this->column_right_id] - $current_parent[$this->column_left_id]) <= 1 || !$current_parent[$this->column_item_id] || $current_parent[$this->column_item_id] == $new_parent[$this->column_item_id])
+		$current_parent_id = (int) $current_parent_id;
+		$new_parent_id = (int) $new_parent_id;
+
+		if ($current_parent_id == $new_parent_id)
 		{
 			return false;
 		}
 
+		if (!$current_parent_id)
+		{
+			throw new phpbb_nestedset_exception($this->message_prefix . 'INVALID_ITEM');
+		}
+
 		if (!$this->lock->acquire())
 		{
 			throw new phpbb_nestedset_exception($this->message_prefix . 'LOCK_FAILED_ACQUIRE');
 		}
 
-		$move_items = array_keys($this->get_branch_data((int) $current_parent[$this->column_item_id], 'children', true, false));
+		$item_data = $this->get_branch_data($current_parent_id, 'children');
+		if (!isset($item_data[$current_parent_id]))
+		{
+			$this->lock->release();
+			throw new phpbb_nestedset_exception($this->message_prefix . 'INVALID_ITEM');
+		}
+
+		$current_parent = $item_data[$current_parent_id];
+		unset($item_data[$current_parent_id]);
+		$move_items = array_keys($item_data);
 
-		if (in_array($new_parent[$this->column_item_id], $move_items))
+		if (($current_parent[$this->column_right_id] - $current_parent[$this->column_left_id]) <= 1)
+		{
+			$this->lock->release();
+			return false;
+		}
+
+		if (in_array($new_parent_id, $move_items))
 		{
 			$this->lock->release();
 			throw new phpbb_nestedset_exception($this->message_prefix . 'INVALID_PARENT');
@@ -291,12 +329,12 @@ abstract class phpbb_nestedset_base implements phpbb_nestedset_interface
 
 		$this->remove_subset($move_items, $current_parent, false, true);
 
-		if ($new_parent[$this->column_item_id])
+		if ($new_parent_id)
 		{
 			// Retrieve new-parent again, it may have been changed...
 			$sql = 'SELECT *
 				FROM ' . $this->table_name . '
-				WHERE ' . $this->column_item_id . ' = ' . (int) $new_parent[$this->column_item_id];
+				WHERE ' . $this->column_item_id . ' = ' . $new_parent_id;
 			$result = $this->db->sql_query($sql);
 			$new_parent = $this->db->sql_fetchrow($result);
 			$this->db->sql_freeresult($result);
@@ -335,7 +373,7 @@ abstract class phpbb_nestedset_base implements phpbb_nestedset_interface
 		$sql = 'UPDATE ' . $this->table_name . '
 			SET ' . $this->column_left_id . ' = ' . $this->column_left_id . $diff . ',
 				' . $this->column_right_id . ' = ' . $this->column_right_id . $diff . ',
-				' . $this->column_parent_id . ' = ' . $this->db->sql_case($this->column_parent_id . ' = ' . (int) $current_parent[$this->column_item_id], (int) $new_parent[$this->column_item_id], $this->column_parent_id) . ',
+				' . $this->column_parent_id . ' = ' . $this->db->sql_case($this->column_parent_id . ' = ' . $current_parent_id, $new_parent_id, $this->column_parent_id) . ',
 				' . $this->column_item_parents . " = ''
 			WHERE " . $this->db->sql_in_set($this->column_item_id, $move_items) . '
 				' . $this->get_sql_where('AND');
@@ -350,16 +388,37 @@ abstract class phpbb_nestedset_base implements phpbb_nestedset_interface
 	/**
 	* @inheritdoc
 	*/
-	public function set_parent(array $item, array $new_parent)
+	public function set_parent($item_id, $new_parent_id)
 	{
+		$item_id = (int) $item_id;
+		$new_parent_id = (int) $new_parent_id;
+
+		if ($item_id == $new_parent_id)
+		{
+			return false;
+		}
+
+		if (!$item_id)
+		{
+			throw new phpbb_nestedset_exception($this->message_prefix . 'INVALID_ITEM');
+		}
+
 		if (!$this->lock->acquire())
 		{
 			throw new phpbb_nestedset_exception($this->message_prefix . 'LOCK_FAILED_ACQUIRE');
 		}
 
-		$move_items = array_keys($this->get_branch_data((int) $item[$this->column_item_id], 'children'));
+		$item_data = $this->get_branch_data($item_id, 'children');
+		if (!isset($item_data[$item_id]))
+		{
+			$this->lock->release();
+			throw new phpbb_nestedset_exception($this->message_prefix . 'INVALID_ITEM');
+		}
+
+		$item = $item_data[$item_id];
+		$move_items = array_keys($item_data);
 
-		if (in_array($new_parent[$this->column_item_id], $move_items))
+		if (in_array($new_parent_id, $move_items))
 		{
 			$this->lock->release();
 			throw new phpbb_nestedset_exception($this->message_prefix . 'INVALID_PARENT');
@@ -372,12 +431,12 @@ abstract class phpbb_nestedset_base implements phpbb_nestedset_interface
 
 		$this->remove_subset($move_items, $item, false, true);
 
-		if ($new_parent[$this->column_item_id])
+		if ($new_parent_id)
 		{
 			// Retrieve new-parent again, it may have been changed...
 			$sql = 'SELECT *
 				FROM ' . $this->table_name . '
-				WHERE ' . $this->column_item_id . ' = ' . (int) $new_parent[$this->column_item_id];
+				WHERE ' . $this->column_item_id . ' = ' . $new_parent_id;
 			$result = $this->db->sql_query($sql);
 			$new_parent = $this->db->sql_fetchrow($result);
 			$this->db->sql_freeresult($result);
@@ -416,7 +475,7 @@ abstract class phpbb_nestedset_base implements phpbb_nestedset_interface
 		$sql = 'UPDATE ' . $this->table_name . '
 			SET ' . $this->column_left_id . ' = ' . $this->column_left_id . $diff . ',
 				' . $this->column_right_id . ' = ' . $this->column_right_id . $diff . ',
-				' . $this->column_parent_id . ' = ' . $this->db->sql_case($this->column_item_id . ' = ' . (int) $item[$this->column_item_id], $new_parent[$this->column_item_id], $this->column_parent_id) . ',
+				' . $this->column_parent_id . ' = ' . $this->db->sql_case($this->column_item_id . ' = ' . $item_id, $new_parent_id, $this->column_parent_id) . ',
 				' . $this->column_item_parents . " = ''
 			WHERE " . $this->db->sql_in_set($this->column_item_id, $move_items) . '
 				' . $this->get_sql_where('AND');
diff --git a/phpBB/includes/nestedset/interface.php b/phpBB/includes/nestedset/interface.php
index 1a6b09f975..aedd57821a 100644
--- a/phpBB/includes/nestedset/interface.php
+++ b/phpBB/includes/nestedset/interface.php
@@ -56,45 +56,45 @@ interface phpbb_nestedset_interface
 	/**
 	* Move an item by a given delta
 	*
-	* @param array	$item	The item to be moved
-	* @param int	$delta	Number of steps to move this item, < 0 => down, > 0 => up
+	* @param int	$item_id	The item to be moved
+	* @param int	$delta		Number of steps to move this item, < 0 => down, > 0 => up
 	* @return bool True if the item was moved
 	*/
-	public function move(array $item, $delta);
+	public function move($item_id, $delta);
 
 	/**
 	* Move an item down by 1
 	*
-	* @param array	$item	The item to be moved
+	* @param int	$item_id	The item to be moved
 	* @return bool True if the item was moved
 	*/
-	public function move_down(array $item);
+	public function move_down($item_id);
 
 	/**
 	* Move an item up by 1
 	*
-	* @param array	$item	The item to be moved
+	* @param int	$item_id	The item to be moved
 	* @return bool True if the item was moved
 	*/
-	public function move_up(array $item);
+	public function move_up($item_id);
 
 	/**
 	* Moves all children of one item to another item
 	*
-	* @param array	$current_parent		The current parent item
-	* @param array	$new_parent			The new parent item
+	* @param int	$current_parent_id	The current parent item
+	* @param int	$new_parent_id		The new parent item
 	* @return bool True if any items where moved
 	*/
-	public function move_children(array $current_parent, array $new_parent);
+	public function move_children($current_parent_id, $new_parent_id);
 
 	/**
 	* Set the parent item
 	*
-	* @param array	$item		The item to be moved
-	* @param array	$new_parent	The new parent item
+	* @param int	$item_id			The item to be moved
+	* @param int	$new_parent_id		The new parent item
 	* @return bool True if the parent was set successfully
 	*/
-	public function set_parent(array $item, array $new_parent);
+	public function set_parent($item, $new_parent_id);
 
 	/**
 	* Get branch of the item
diff --git a/tests/nestedset/set_forum_move_test.php b/tests/nestedset/set_forum_move_test.php
index b90eaa12ad..9a19d18476 100644
--- a/tests/nestedset/set_forum_move_test.php
+++ b/tests/nestedset/set_forum_move_test.php
@@ -120,7 +120,7 @@ class phpbb_tests_nestedset_set_forum_move_test extends phpbb_tests_nestedset_se
 	*/
 	public function test_move($explain, $forum_id, $delta, $expected_moved, $expected)
 	{
-		$this->assertEquals($expected_moved, $this->set->move($this->forum_data[$forum_id], $delta));
+		$this->assertEquals($expected_moved, $this->set->move($forum_id, $delta));
 
 		$result = $this->db->sql_query("SELECT forum_id, parent_id, left_id, right_id, forum_parents
 			FROM phpbb_forums
@@ -167,7 +167,7 @@ class phpbb_tests_nestedset_set_forum_move_test extends phpbb_tests_nestedset_se
 	*/
 	public function test_move_down($explain, $forum_id, $expected_moved, $expected)
 	{
-		$this->assertEquals($expected_moved, $this->set->move_down($this->forum_data[$forum_id]));
+		$this->assertEquals($expected_moved, $this->set->move_down($forum_id));
 
 		$result = $this->db->sql_query("SELECT forum_id, parent_id, left_id, right_id, forum_parents
 			FROM phpbb_forums
@@ -214,7 +214,7 @@ class phpbb_tests_nestedset_set_forum_move_test extends phpbb_tests_nestedset_se
 	*/
 	public function test_move_up($explain, $forum_id, $expected_moved, $expected)
 	{
-		$this->assertEquals($expected_moved, $this->set->move_up($this->forum_data[$forum_id]));
+		$this->assertEquals($expected_moved, $this->set->move_up($forum_id));
 
 		$result = $this->db->sql_query("SELECT forum_id, parent_id, left_id, right_id, forum_parents
 			FROM phpbb_forums
@@ -257,22 +257,6 @@ class phpbb_tests_nestedset_set_forum_move_test extends phpbb_tests_nestedset_se
 				array('forum_id' => 10, 'parent_id' => 9, 'left_id' => 17, 'right_id' => 18, 'forum_parents' => ''),
 				array('forum_id' => 11, 'parent_id' => 7, 'left_id' => 20, 'right_id' => 21, 'forum_parents' => ''),
 			)),
-			array('Parent is 0',
-				0, 1, false, array(
-				array('forum_id' => 1, 'parent_id' => 0, 'left_id' => 1, 'right_id' => 6, 'forum_parents' => ''),
-				array('forum_id' => 2, 'parent_id' => 1, 'left_id' => 2, 'right_id' => 3, 'forum_parents' => ''),
-				array('forum_id' => 3, 'parent_id' => 1, 'left_id' => 4, 'right_id' => 5, 'forum_parents' => ''),
-
-				array('forum_id' => 4, 'parent_id' => 0, 'left_id' => 7, 'right_id' => 12, 'forum_parents' => ''),
-				array('forum_id' => 5, 'parent_id' => 4, 'left_id' => 8, 'right_id' => 11, 'forum_parents' => ''),
-				array('forum_id' => 6, 'parent_id' => 5, 'left_id' => 9, 'right_id' => 10, 'forum_parents' => ''),
-
-				array('forum_id' => 7, 'parent_id' => 0, 'left_id' => 13, 'right_id' => 22, 'forum_parents' => ''),
-				array('forum_id' => 8, 'parent_id' => 7, 'left_id' => 14, 'right_id' => 15, 'forum_parents' => ''),
-				array('forum_id' => 9, 'parent_id' => 7, 'left_id' => 16, 'right_id' => 19, 'forum_parents' => ''),
-				array('forum_id' => 10, 'parent_id' => 9, 'left_id' => 17, 'right_id' => 18, 'forum_parents' => ''),
-				array('forum_id' => 11, 'parent_id' => 7, 'left_id' => 20, 'right_id' => 21, 'forum_parents' => ''),
-			)),
 			array('Move single child up',
 				5, 1, true, array(
 				array('forum_id' => 1, 'parent_id' => 0, 'left_id' => 1, 'right_id' => 8, 'forum_parents' => ''),
@@ -380,7 +364,7 @@ class phpbb_tests_nestedset_set_forum_move_test extends phpbb_tests_nestedset_se
 	*/
 	public function test_move_children($explain, $forum_id, $target_id, $expected_moved, $expected)
 	{
-		$this->assertEquals($expected_moved, $this->set->move_children($this->forum_data[$forum_id], $this->forum_data[$target_id]));
+		$this->assertEquals($expected_moved, $this->set->move_children($forum_id, $target_id));
 
 		$result = $this->db->sql_query("SELECT forum_id, parent_id, left_id, right_id, forum_parents
 			FROM phpbb_forums
@@ -388,7 +372,26 @@ class phpbb_tests_nestedset_set_forum_move_test extends phpbb_tests_nestedset_se
 		$this->assertEquals($expected, $this->db->sql_fetchrowset($result));
 	}
 
-	public function move_children_throws_data()
+	public function move_children_throws_item_data()
+	{
+		return array(
+			array('Item 0 does not exist', 0, 5),
+			array('Item does not exist', 200, 5),
+		);
+	}
+
+	/**
+	* @dataProvider move_children_throws_item_data
+	*
+	* @expectedException			phpbb_nestedset_exception
+	* @expectedExceptionMessage		FORUM_NESTEDSET_INVALID_ITEM
+	*/
+	public function test_move_children_throws_item($explain, $forum_id, $target_id)
+	{
+		$this->set->move_children($forum_id, $target_id);
+	}
+
+	public function move_children_throws_parent_data()
 	{
 		return array(
 			array('New parent is child', 4, 5),
@@ -398,14 +401,14 @@ class phpbb_tests_nestedset_set_forum_move_test extends phpbb_tests_nestedset_se
 	}
 
 	/**
-	* @dataProvider move_children_throws_data
+	* @dataProvider move_children_throws_parent_data
 	*
 	* @expectedException			phpbb_nestedset_exception
 	* @expectedExceptionMessage		FORUM_NESTEDSET_INVALID_PARENT
 	*/
-	public function test_move_children_throws($explain, $forum_id, $target_id)
+	public function test_move_children_throws_parent($explain, $forum_id, $target_id)
 	{
-		$this->set->move_children($this->forum_data[$forum_id], $this->forum_data[$target_id]);
+		$this->set->move_children($forum_id, $target_id);
 	}
 
 	public function set_parent_data()
@@ -517,7 +520,7 @@ class phpbb_tests_nestedset_set_forum_move_test extends phpbb_tests_nestedset_se
 	*/
 	public function test_set_parent($explain, $forum_id, $target_id, $expected_moved, $expected)
 	{
-		$this->assertEquals($expected_moved, $this->set->set_parent($this->forum_data[$forum_id], $this->forum_data[$target_id]));
+		$this->assertEquals($expected_moved, $this->set->set_parent($forum_id, $target_id));
 
 		$result = $this->db->sql_query("SELECT forum_id, parent_id, left_id, right_id, forum_parents
 			FROM phpbb_forums
@@ -525,7 +528,26 @@ class phpbb_tests_nestedset_set_forum_move_test extends phpbb_tests_nestedset_se
 		$this->assertEquals($expected, $this->db->sql_fetchrowset($result));
 	}
 
-	public function set_parent_throws_data()
+	public function set_parent_throws_item_data()
+	{
+		return array(
+			array('Item 0 does not exist', 0, 5),
+			array('Item does not exist', 200, 5),
+		);
+	}
+
+	/**
+	* @dataProvider set_parent_throws_item_data
+	*
+	* @expectedException			phpbb_nestedset_exception
+	* @expectedExceptionMessage		FORUM_NESTEDSET_INVALID_ITEM
+	*/
+	public function test_set_parent_throws_item($explain, $forum_id, $target_id)
+	{
+		$this->set->set_parent($forum_id, $target_id);
+	}
+
+	public function set_parent_throws_parent_data()
 	{
 		return array(
 			array('New parent is child', 4, 5),
@@ -535,13 +557,13 @@ class phpbb_tests_nestedset_set_forum_move_test extends phpbb_tests_nestedset_se
 	}
 
 	/**
-	* @dataProvider set_parent_throws_data
+	* @dataProvider set_parent_throws_parent_data
 	*
 	* @expectedException			phpbb_nestedset_exception
 	* @expectedExceptionMessage		FORUM_NESTEDSET_INVALID_PARENT
 	*/
-	public function test_set_parent_throws($explain, $forum_id, $target_id)
+	public function test_set_parent_throws_parent($explain, $forum_id, $target_id)
 	{
-		$this->set->set_parent($this->forum_data[$forum_id], $this->forum_data[$target_id]);
+		$this->set->set_parent($forum_id, $target_id);
 	}
 }