Merge branch '3.1.x' into 3.2.x

* 3.1.x: [ticket/14831] Optimize code construction [ticket/14831] Add more tests against UCP modules [ticket/14831] Add more tests, better name for $e placeholder [ticket/14831] Do not throw exception on the module existence checking [ticket/14831] Fix module migrator tool
author: Tristan Darricau <github@nicofuma.fr> 2016-10-24 22:24:26 +0200
committer: Tristan Darricau <github@nicofuma.fr> 2016-10-24 22:24:26 +0200
commit: b13910ee45fc5c2985d00b2a8166489b2fc58bec (patch)
tree: 6fe77e3bbdd90b7178af5aba2471f8a0341d0e0a
parent: 52b1a9d4fa877bc6633ec53c905bd4dfaddbbf8d (diff)
parent: 163292bc1188efdd623a1270a1aedf7e9b281801 (diff)
download: forums-b13910ee45fc5c2985d00b2a8166489b2fc58bec.tar
forums-b13910ee45fc5c2985d00b2a8166489b2fc58bec.tar.gz
forums-b13910ee45fc5c2985d00b2a8166489b2fc58bec.tar.bz2
forums-b13910ee45fc5c2985d00b2a8166489b2fc58bec.tar.xz
forums-b13910ee45fc5c2985d00b2a8166489b2fc58bec.zip
3 files changed, 181 insertions, 15 deletions
diff --git a/phpBB/phpbb/db/migration/tool/module.php b/phpBB/phpbb/db/migration/tool/module.php
index a7dffbb7f2..b47c426110 100644
--- a/phpBB/phpbb/db/migration/tool/module.php
+++ b/phpBB/phpbb/db/migration/tool/module.php
@@ -97,7 +97,12 @@ class module implements \phpbb\db\migration\tool\tool_interface
 		$parent_sql = '';
 		if ($parent !== false)
 		{
-			$parent = $this->get_parent_module_id($parent, $module);
+			$parent = $this->get_parent_module_id($parent, $module, false);
+			if ($parent === false)
+			{
+				return false;
+			}
+
 			$parent_sql = 'AND parent_id = ' . (int) $parent;
 		}
 
@@ -205,7 +210,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']);
 		}
 
 		$module_data = array(
@@ -431,12 +436,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);
@@ -452,11 +456,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
+		$exception = false;
+
 		// Allow '' to be sent as 0
 		$parent_id = $parent_id ?: 0;
 
@@ -478,7 +486,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);
+					$exception = new \phpbb\db\migration\exception('MODULE_NOT_EXIST', $parent_id);
 				break;
 
 				// Return the module id
@@ -500,7 +508,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']);
+							$exception = new \phpbb\db\migration\exception('PARENT_MODULE_FIND_ERROR', $data['parent_id']);
 						}
 					}
 					else if (!empty($data) && !is_array($data))
@@ -518,12 +526,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);
+						$exception = new \phpbb\db\migration\exception('MODULE_EXIST_MULTIPLE', $parent_id);
 					}
 				break;
 			}
 		}
 
+		if ($exception !== false)
+		{
+			if ($throw_exception)
+			{
+				throw $exception;
+			}
+			return false;
+		}
+
 		return $parent_id;
 	}
 }
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 @@
 			<value></value>
 			<value></value>
 		</row>
+		<row>
+			<value>7</value>
+			<value>1</value>
+			<value>1</value>
+			<value></value>
+			<value>ucp</value>
+			<value>0</value>
+			<value>13</value>
+			<value>18</value>
+			<value>UCP_MAIN_CAT</value>
+			<value></value>
+			<value></value>
+		</row>
+		<row>
+			<value>8</value>
+			<value>1</value>
+			<value>1</value>
+			<value>ucp_subcat</value>
+			<value>ucp</value>
+			<value>7</value>
+			<value>14</value>
+			<value>17</value>
+			<value>UCP_SUBCATEGORY</value>
+			<value>ucp_test</value>
+			<value></value>
+		</row>
+		<row>
+			<value>9</value>
+			<value>1</value>
+			<value>1</value>
+			<value>ucp_module</value>
+			<value>ucp</value>
+			<value>8</value>
+			<value>15</value>
+			<value>16</value>
+			<value>UCP_MODULE</value>
+			<value>ucp_module_test</value>
+			<value></value>
+		</row>
 	</table>
 </dataset>
diff --git a/tests/dbal/migrator_tool_module_test.php b/tests/dbal/migrator_tool_module_test.php
index 1744b7e92d..29b21166b6 100644
--- a/tests/dbal/migrator_tool_module_test.php
+++ b/tests/dbal/migrator_tool_module_test.php
@@ -45,10 +45,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, $module_manager, $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',
@@ -60,7 +60,7 @@ class phpbb_dbal_migrator_tool_module_test extends phpbb_database_test_case
 				true,
 			),
 
-			// Test the module
+			// Test the existing module
 			array(
 				'',
 				'ACP_MODULE',
@@ -76,17 +76,88 @@ 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)
+	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
@@ -159,6 +230,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()
author	Tristan Darricau <github@nicofuma.fr>	2016-10-24 22:24:26 +0200
committer	Tristan Darricau <github@nicofuma.fr>	2016-10-24 22:24:26 +0200
commit	b13910ee45fc5c2985d00b2a8166489b2fc58bec (patch)
tree	6fe77e3bbdd90b7178af5aba2471f8a0341d0e0a
parent	52b1a9d4fa877bc6633ec53c905bd4dfaddbbf8d (diff)
parent	163292bc1188efdd623a1270a1aedf7e9b281801 (diff)
download	forums-b13910ee45fc5c2985d00b2a8166489b2fc58bec.tar forums-b13910ee45fc5c2985d00b2a8166489b2fc58bec.tar.gz forums-b13910ee45fc5c2985d00b2a8166489b2fc58bec.tar.bz2 forums-b13910ee45fc5c2985d00b2a8166489b2fc58bec.tar.xz forums-b13910ee45fc5c2985d00b2a8166489b2fc58bec.zip