Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
41 changes: 16 additions & 25 deletions phpBB/includes/functions_admin.php
Original file line number Diff line number Diff line change
Expand Up @@ -3040,38 +3040,29 @@ function tidy_database()
*/
function add_permission_language()
{
global $user, $phpEx;
global $user, $phpEx, $phpbb_extension_manager;

// First of all, our own file. We need to include it as the first file because it presets all relevant variables.
$user->add_lang('acp/permissions_phpbb');
// add permission language files from extensions
$finder = $phpbb_extension_manager->get_finder();

$files_to_add = array();
$lang_files = $finder
->prefix('permissions_')
->suffix(".$phpEx")
->core_path('language/' . $user->lang_name . '/')
->extension_directory('/language/' . $user->lang_name)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is acp missing here?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So extensions can have all files in the base directory, if they want to?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And why is that useful?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it useful to require extension authors put their permissions language files under acp/ is a better question.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is also outside the scope of thi sPR.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does core path have a trailing slash but not extension directory?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I tested this out and without the trailing slash on the core_path you end up with something like this:
$lang_files = Array
(
[ext/foo/bar/language/en/permissions_foo.php] => foo/bar
[language/enacp/permissions_phpbb.php] => /
[language/enmods/permissions_mod.php] => /
)
Notice /enacp/, /enmods/ instead of /en/acp/, /en/mods/ respectively.

The finder sanitizes the 'extension_directory' and removes any trailing slashes and therefore it is not necessary. However, it seems that the '/' at the beginning of the extension_directory is unnecessary. It didn't seem to matter when I set the extension directory as 'language/...' or '/language/..'. Should I remove the beginning slash?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both inputs should be in the same format. If core_path doesn't work without the trailing slash, but the extension_directory does, that should probably be fixed (please create a bug ticket for this issue).

The leading slash should be removed.

There is also a function that should be used when core and extension dirs are the same, you just send it one path and it sets that path on both (please use it).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you could just use directory('/language/' . $user->lang_name . '/')

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't work. What we need is to set the core_path and not the core_directory. directory() sets the core_directory and extension_directory but not the core_path, which must be defined to get the matching files in the core path. As a result, it only finds the permission language files in extensions. I don't know if this was the intended behavior for the finder or not...

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, I guess I did not notice that difference before, that seems like rather poor behavior to me, but fixing that is far outside the scope of this, so I'll merge this and that issue will just have to be dealt with in the future.

->find();

// Now search in acp and mods folder for permissions_ files.
foreach (array('acp/', 'mods/') as $path)
foreach ($lang_files as $lang_file => $ext_name)
{
$dh = @opendir($user->lang_path . $user->lang_name . '/' . $path);

if ($dh)
if ($ext_name === '/')
{
while (($file = readdir($dh)) !== false)
{
if ($file !== 'permissions_phpbb.' . $phpEx && strpos($file, 'permissions_') === 0 && substr($file, -(strlen($phpEx) + 1)) === '.' . $phpEx)
{
$files_to_add[] = $path . substr($file, 0, -(strlen($phpEx) + 1));
}
}
closedir($dh);
$user->add_lang($lang_file);
}
else
{
$user->add_lang_ext($ext_name, $lang_file);
}
}

if (!sizeof($files_to_add))
{
return false;
}

$user->add_lang($files_to_add);
return true;
}

/**
Expand Down
118 changes: 118 additions & 0 deletions tests/functional/extension_permission_lang_test.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,118 @@
<?php
/**
*
* @package testing
* @copyright (c) 2012 phpBB Group
* @license http://opensource.org/licenses/gpl-2.0.php GNU General Public License v2
*
*/

/**
* @group functional
*/
class phpbb_functional_extension_permission_lang_test extends phpbb_functional_test_case
{
protected $phpbb_extension_manager;

static private $helper;

static private $copied_files = array();

static protected $fixtures = array(
'foo/bar/language/en/',
);

/**
* This should only be called once before the tests are run.
* This is used to copy the fixtures to the phpBB install
*/
static public function setUpBeforeClass()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please use the helper to create/move/delete directories and files:
https://github.com/phpbb/phpbb3/pull/1308/files#L5R29

{
global $phpbb_root_path;
parent::setUpBeforeClass();

self::$helper = new phpbb_test_case_helpers(self);

self::$copied_files = array();

if (file_exists($phpbb_root_path . 'ext/'))
{
// First, move any extensions setup on the board to a temp directory
self::$copied_files = self::$helper->copy_dir($phpbb_root_path . 'ext/', $phpbb_root_path . 'store/temp_ext/');

// Then empty the ext/ directory on the board (for accurate test cases)
self::$helper->empty_dir($phpbb_root_path . 'ext/');
}

// Copy our ext/ files from the test case to the board
self::$copied_files = array_merge(self::$copied_files, self::$helper->copy_dir(dirname(__FILE__) . '/fixtures/ext/' . $fixture, $phpbb_root_path . 'ext/' . $fixture));
}

/**
* This should only be called once after the tests are run.
* This is used to remove the fixtures from the phpBB install
*/
static public function tearDownAfterClass()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{
global $phpbb_root_path;

if (file_exists($phpbb_root_path . 'store/temp_ext/'))
{
// Copy back the board installed extensions from the temp directory
self::$helper->copy_dir($phpbb_root_path . 'store/temp_ext/', $phpbb_root_path . 'ext/');
}

// Remove all of the files we copied around (from board ext -> temp_ext, from test ext -> board ext)
self::$helper->remove_files(self::$copied_files);

if (file_exists($phpbb_root_path . 'store/temp_ext/'))
{
self::$helper->empty_dir($phpbb_root_path . 'store/temp_ext/');
}
}

public function setUp()
{
parent::setUp();

$this->get_db();

$acl_ary = array(
'auth_option' => 'u_foo',
'is_global' => 1,
);

$sql = 'INSERT INTO phpbb_acl_options ' . $this->db->sql_build_array('INSERT', $acl_ary);
$this->db->sql_query($sql);

$this->phpbb_extension_manager = $this->get_extension_manager();

$this->purge_cache();

$this->login();
$this->admin_login();
$this->add_lang('acp/permissions');
}

public function test_auto_include_permission_lang_from_extensions()
{
$this->phpbb_extension_manager->enable('foo/bar');

// User permissions
$crawler = $this->request('GET', 'adm/index.php?i=acp_permissions&icat=16&mode=setting_user_global&sid=' . $this->sid);
$this->assert_response_success();

// Select admin
$form = $crawler->selectButton($this->lang('SUBMIT'))->form();
$data = array('username[0]' => 'admin');
$form->setValues($data);
$crawler = $this->client->submit($form);
$this->assert_response_success();

// language from language/en/acp/permissions_phpbb.php
$this->assertContains('Can attach files', $crawler->filter('body')->text());

// language from ext/foo/bar/language/en/permissions_foo.php
$this->assertContains('Can view foo', $crawler->filter('body')->text());
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
<?php

// Admin Permissions
$lang = array_merge($lang, array(
'acl_u_foo' => array('lang' => 'Can view foo', 'cat' => 'misc'),
));