Skip to content

[ticket/14831] Fix extension modules installation issues #4491

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Oct 24, 2016
Merged

[ticket/14831] Fix extension modules installation issues #4491

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0cc0920
[ticket/14831] Fix module migrator tool
rxu Oct 22, 2016
a02b124
[ticket/14831] Do not throw exception on the module existence checking
rxu Oct 23, 2016
51ef1ae
[ticket/14831] Add more tests, better name for $e placeholder
rxu Oct 23, 2016
849cd74
[ticket/14831] Add more tests against UCP modules
rxu Oct 23, 2016
77f1bac
[ticket/14831] Optimize code construction
rxu Oct 23, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 27 additions & 10 deletions phpBB/phpbb/db/migration/tool/module.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,12 @@ public function exists($class, $parent, $module)
$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;
}

Expand Down Expand Up @@ -197,7 +202,7 @@ public function add($class, $parent = 0, $data = array())

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'))
Expand Down Expand Up @@ -448,12 +453,11 @@ protected function get_module_info($class, $basename)
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);
Expand All @@ -469,11 +473,15 @@ protected function get_categories_list()
*
* @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;

Expand All @@ -495,7 +503,7 @@ public function get_parent_module_id($parent_id, $data = '')
{
// 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
Expand All @@ -517,7 +525,7 @@ public function get_parent_module_id($parent_id, $data = '')
$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))
Expand All @@ -535,12 +543,21 @@ public function get_parent_module_id($parent_id, $data = '')
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;
}
}
39 changes: 39 additions & 0 deletions tests/dbal/fixtures/migrator_module.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
120 changes: 115 additions & 5 deletions tests/dbal/migrator_tool_module_test.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,10 @@ public function setup()
$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',
Expand All @@ -57,7 +57,7 @@ public function exists_data()
true,
),

// Test the module
// Test the existing module
array(
'',
'ACP_MODULE',
Expand All @@ -73,17 +73,88 @@ public function exists_data()
'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
Expand Down Expand Up @@ -156,6 +227,45 @@ public function test_add()
$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()
Expand Down