From 0cc092073405166a0da72009823256a00389d0b8 Mon Sep 17 00:00:00 2001 From: rxu Date: Sat, 22 Oct 2016 21:40:21 +0700 Subject: [PATCH 1/5] [ticket/14831] Fix module migrator tool PHPBB3-14831 --- phpBB/phpbb/db/migration/tool/module.php | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/phpBB/phpbb/db/migration/tool/module.php b/phpBB/phpbb/db/migration/tool/module.php index 6d5378e35f..d28d6153b9 100644 --- a/phpBB/phpbb/db/migration/tool/module.php +++ b/phpBB/phpbb/db/migration/tool/module.php @@ -197,7 +197,7 @@ class module implements \phpbb\db\migration\tool\tool_interface if ($this->exists($class, $parent, $data['module_langname'])) { - throw new \phpbb\db\migration\exception('MODULE_EXISTS', $module_id); + throw new \phpbb\db\migration\exception('MODULE_EXISTS', $data['module_langname']); } if (!class_exists('acp_modules')) @@ -448,12 +448,11 @@ class module implements \phpbb\db\migration\tool\tool_interface protected function get_categories_list() { // Select the top level categories - // and 2nd level [sub]categories which exist for ACP only + // and 2nd level [sub]categories $sql = 'SELECT m2.module_id, m2.module_langname FROM ' . $this->modules_table . ' m1, ' . $this->modules_table . " m2 WHERE m1.parent_id = 0 - AND (m1.module_id = m2.module_id - OR m2.module_class = 'acp' AND m2.parent_id = m1.module_id) + AND (m1.module_id = m2.module_id OR m2.parent_id = m1.module_id) ORDER BY m1.module_id, m2.module_id ASC"; $result = $this->db->sql_query($sql); From a02b124dd02f8c5a2d0c60820f6563311e983a4c Mon Sep 17 00:00:00 2001 From: rxu Date: Sun, 23 Oct 2016 21:17:41 +0700 Subject: [PATCH 2/5] [ticket/14831] Do not throw exception on the module existence checking PHPBB3-14831 --- phpBB/phpbb/db/migration/tool/module.php | 34 +++++++++++++++++++----- 1 file changed, 27 insertions(+), 7 deletions(-) diff --git a/phpBB/phpbb/db/migration/tool/module.php b/phpBB/phpbb/db/migration/tool/module.php index d28d6153b9..e8f2eaf80b 100644 --- a/phpBB/phpbb/db/migration/tool/module.php +++ b/phpBB/phpbb/db/migration/tool/module.php @@ -90,8 +90,15 @@ class module implements \phpbb\db\migration\tool\tool_interface $parent_sql = ''; if ($parent !== false) { - $parent = $this->get_parent_module_id($parent, $module); - $parent_sql = 'AND parent_id = ' . (int) $parent; + $parent = $this->get_parent_module_id($parent, $module, false); + if ($parent !== false) + { + $parent_sql = 'AND parent_id = ' . (int) $parent; + } + else + { + return false; + } } $sql = 'SELECT module_id @@ -468,11 +475,15 @@ class module implements \phpbb\db\migration\tool\tool_interface * * @param string|int $parent_id The parent module_id|module_langname * @param int|string|array $data The module_id, module_langname for existance checking or module data array for adding - * @return int The parent module_id + * @param bool $throw_exception The flag indicating if exception should be thrown on error + * @return mixed The int parent module_id or false * @throws \phpbb\db\migration\exception */ - public function get_parent_module_id($parent_id, $data = '') + public function get_parent_module_id($parent_id, $data = '', $throw_exception = true) { + // Initialize exception object placeholder + $e = false; + // Allow '' to be sent as 0 $parent_id = $parent_id ?: 0; @@ -494,7 +505,7 @@ class module implements \phpbb\db\migration\tool\tool_interface { // No parent with the given module_langname exist case 0: - throw new \phpbb\db\migration\exception('MODULE_NOT_EXIST', $parent_id); + $e = new \phpbb\db\migration\exception('MODULE_NOT_EXIST', $parent_id); break; // Return the module id @@ -516,7 +527,7 @@ class module implements \phpbb\db\migration\tool\tool_interface $parent_id = (int) $this->db->sql_fetchfield('parent_id'); if (!$parent_id) { - throw new \phpbb\db\migration\exception('PARENT_MODULE_FIND_ERROR', $data['parent_id']); + $e = new \phpbb\db\migration\exception('PARENT_MODULE_FIND_ERROR', $data['parent_id']); } } else if (!empty($data) && !is_array($data)) @@ -534,12 +545,21 @@ class module implements \phpbb\db\migration\tool\tool_interface else { //Unable to get the parent module id, throwing an exception - throw new \phpbb\db\migration\exception('MODULE_EXIST_MULTIPLE', $parent_id); + $e = new \phpbb\db\migration\exception('MODULE_EXIST_MULTIPLE', $parent_id); } break; } } + if ($e !== false) + { + if ($throw_exception) + { + throw $e; + } + return false; + } + return $parent_id; } } From 51ef1ae346c08863d188e723ba41720e744a2fbd Mon Sep 17 00:00:00 2001 From: rxu Date: Sun, 23 Oct 2016 22:44:47 +0700 Subject: [PATCH 3/5] [ticket/14831] Add more tests, better name for $e placeholder PHPBB3-14831 --- phpBB/phpbb/db/migration/tool/module.php | 12 ++--- tests/dbal/migrator_tool_module_test.php | 59 ++++++++++++++++++++++-- 2 files changed, 61 insertions(+), 10 deletions(-) diff --git a/phpBB/phpbb/db/migration/tool/module.php b/phpBB/phpbb/db/migration/tool/module.php index e8f2eaf80b..a543fd5cab 100644 --- a/phpBB/phpbb/db/migration/tool/module.php +++ b/phpBB/phpbb/db/migration/tool/module.php @@ -482,7 +482,7 @@ class module implements \phpbb\db\migration\tool\tool_interface public function get_parent_module_id($parent_id, $data = '', $throw_exception = true) { // Initialize exception object placeholder - $e = false; + $exception = false; // Allow '' to be sent as 0 $parent_id = $parent_id ?: 0; @@ -505,7 +505,7 @@ class module implements \phpbb\db\migration\tool\tool_interface { // No parent with the given module_langname exist case 0: - $e = new \phpbb\db\migration\exception('MODULE_NOT_EXIST', $parent_id); + $exception = new \phpbb\db\migration\exception('MODULE_NOT_EXIST', $parent_id); break; // Return the module id @@ -527,7 +527,7 @@ class module implements \phpbb\db\migration\tool\tool_interface $parent_id = (int) $this->db->sql_fetchfield('parent_id'); if (!$parent_id) { - $e = new \phpbb\db\migration\exception('PARENT_MODULE_FIND_ERROR', $data['parent_id']); + $exception = new \phpbb\db\migration\exception('PARENT_MODULE_FIND_ERROR', $data['parent_id']); } } else if (!empty($data) && !is_array($data)) @@ -545,17 +545,17 @@ class module implements \phpbb\db\migration\tool\tool_interface else { //Unable to get the parent module id, throwing an exception - $e = new \phpbb\db\migration\exception('MODULE_EXIST_MULTIPLE', $parent_id); + $exception = new \phpbb\db\migration\exception('MODULE_EXIST_MULTIPLE', $parent_id); } break; } } - if ($e !== false) + if ($exception !== false) { if ($throw_exception) { - throw $e; + throw $exception; } return false; } diff --git a/tests/dbal/migrator_tool_module_test.php b/tests/dbal/migrator_tool_module_test.php index 49dff8b929..462b521f1a 100644 --- a/tests/dbal/migrator_tool_module_test.php +++ b/tests/dbal/migrator_tool_module_test.php @@ -42,10 +42,10 @@ class phpbb_dbal_migrator_tool_module_test extends phpbb_database_test_case $this->tool = new \phpbb\db\migration\tool\module($this->db, $this->cache, $this->user, $phpbb_root_path, $phpEx, 'phpbb_modules'); } - public function exists_data() + public function exists_data_acp() { return array( - // Test the category + // Test the existing category array( '', 'ACP_CAT', @@ -57,7 +57,7 @@ class phpbb_dbal_migrator_tool_module_test extends phpbb_database_test_case true, ), - // Test the module + // Test the existing module array( '', 'ACP_MODULE', @@ -73,11 +73,23 @@ class phpbb_dbal_migrator_tool_module_test extends phpbb_database_test_case 'ACP_MODULE', true, ), + + // Test for non-existant modules + array( + '', + 'ACP_NON_EXISTANT_CAT', + false, + ), + array( + 'ACP_CAT', + 'ACP_NON_EXISTANT_MODULE', + false, + ), ); } /** - * @dataProvider exists_data + * @dataProvider exists_data_acp */ public function test_exists($parent, $module, $expected) { @@ -156,6 +168,45 @@ class phpbb_dbal_migrator_tool_module_test extends phpbb_database_test_case $this->fail($e); } $this->assertEquals(true, $this->tool->exists('acp', 'ACP_FORUM_BASED_PERMISSIONS', 'ACP_NEW_PERMISSIONS_MODULE')); + + // Test adding UCP modules + // Test adding new UCP category + try + { + $this->tool->add('ucp', 0, 'UCP_NEW_CAT'); + } + catch (Exception $e) + { + $this->fail($e); + } + $this->assertEquals(true, $this->tool->exists('ucp', 0, 'UCP_NEW_CAT')); + + // Test adding new UCP subcategory + try + { + $this->tool->add('ucp', 'UCP_NEW_CAT', 'UCP_NEW_SUBCAT'); + } + catch (Exception $e) + { + $this->fail($e); + } + $this->assertEquals(true, $this->tool->exists('ucp', 'UCP_NEW_CAT', 'UCP_NEW_SUBCAT')); + + // Test adding new UCP module + try + { + $this->tool->add('ucp', 'UCP_NEW_SUBCAT', array( + 'module_basename' => 'ucp_new_module', + 'module_langname' => 'UCP_NEW_MODULE', + 'module_mode' => 'ucp_test', + 'module_auth' => '', + )); + } + catch (Exception $e) + { + $this->fail($e); + } + $this->assertEquals(true, $this->tool->exists('ucp', 'UCP_NEW_SUBCAT', 'UCP_NEW_MODULE')); } public function test_remove() From 849cd74700bc1425245ae9a8c5ec5f126d7a61ff Mon Sep 17 00:00:00 2001 From: rxu Date: Sun, 23 Oct 2016 23:33:03 +0700 Subject: [PATCH 4/5] [ticket/14831] Add more tests against UCP modules PHPBB3-14831 --- tests/dbal/fixtures/migrator_module.xml | 39 +++++++++++++++ tests/dbal/migrator_tool_module_test.php | 61 +++++++++++++++++++++++- 2 files changed, 99 insertions(+), 1 deletion(-) diff --git a/tests/dbal/fixtures/migrator_module.xml b/tests/dbal/fixtures/migrator_module.xml index e172d7a145..e85c43ee25 100644 --- a/tests/dbal/fixtures/migrator_module.xml +++ b/tests/dbal/fixtures/migrator_module.xml @@ -90,5 +90,44 @@ + + 7 + 1 + 1 + + ucp + 0 + 13 + 18 + UCP_MAIN_CAT + + + + + 8 + 1 + 1 + ucp_subcat + ucp + 7 + 14 + 17 + UCP_SUBCATEGORY + ucp_test + + + + 9 + 1 + 1 + ucp_module + ucp + 8 + 15 + 16 + UCP_MODULE + ucp_module_test + + diff --git a/tests/dbal/migrator_tool_module_test.php b/tests/dbal/migrator_tool_module_test.php index 462b521f1a..bbe543f347 100644 --- a/tests/dbal/migrator_tool_module_test.php +++ b/tests/dbal/migrator_tool_module_test.php @@ -91,11 +91,70 @@ class phpbb_dbal_migrator_tool_module_test extends phpbb_database_test_case /** * @dataProvider exists_data_acp */ - public function test_exists($parent, $module, $expected) + public function test_exists_acp($parent, $module, $expected) { $this->assertEquals($expected, $this->tool->exists('acp', $parent, $module)); } + public function exists_data_ucp() + { + return array( + // Test the existing category + array( + '', + 'UCP_MAIN_CAT', + true, + ), + array( + 0, + 'UCP_MAIN_CAT', + true, + ), + + // Test the existing module + array( + '', + 'UCP_SUBCATEGORY', + false, + ), + array( + false, + 'UCP_SUBCATEGORY', + true, + ), + array( + 'UCP_MAIN_CAT', + 'UCP_SUBCATEGORY', + true, + ), + array( + 'UCP_SUBCATEGORY', + 'UCP_MODULE', + true, + ), + + // Test for non-existant modules + array( + '', + 'UCP_NON_EXISTANT_CAT', + false, + ), + array( + 'UCP_MAIN_CAT', + 'UCP_NON_EXISTANT_MODULE', + false, + ), + ); + } + + /** + * @dataProvider exists_data_ucp + */ + public function test_exists_ucp($parent, $module, $expected) + { + $this->assertEquals($expected, $this->tool->exists('ucp', $parent, $module)); + } + public function test_add() { try From 77f1bac64bd00a581f71dccbb20b532ff1ec2434 Mon Sep 17 00:00:00 2001 From: rxu Date: Mon, 24 Oct 2016 01:41:24 +0700 Subject: [PATCH 5/5] [ticket/14831] Optimize code construction PHPBB3-14831 --- phpBB/phpbb/db/migration/tool/module.php | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/phpBB/phpbb/db/migration/tool/module.php b/phpBB/phpbb/db/migration/tool/module.php index a543fd5cab..7ea7d1dac1 100644 --- a/phpBB/phpbb/db/migration/tool/module.php +++ b/phpBB/phpbb/db/migration/tool/module.php @@ -91,14 +91,12 @@ class module implements \phpbb\db\migration\tool\tool_interface if ($parent !== false) { $parent = $this->get_parent_module_id($parent, $module, false); - if ($parent !== false) - { - $parent_sql = 'AND parent_id = ' . (int) $parent; - } - else + if ($parent === false) { return false; } + + $parent_sql = 'AND parent_id = ' . (int) $parent; } $sql = 'SELECT module_id