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'); + } - if (in_array($new_parent[$this->column_item_id], $move_items)) + $current_parent = $item_data[$current_parent_id]; + unset($item_data[$current_parent_id]); + $move_items = array_keys($item_data); + + 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'); + } - if (in_array($new_parent[$this->column_item_id], $move_items)) + $item = $item_data[$item_id]; + $move_items = array_keys($item_data); + + 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); } }