[ticket/15289] Allow to configure storage from acp #4895
Conversation
rubencm
force-pushed
the
ticket/15289
branch
7 times, most recently
from
055d0b0
to
5bb393f
Compare
rubencm
force-pushed
the
ticket/15289
branch
2 times, most recently
from
9fa10cf
to
d624a48
Compare
phpBB/adm/style/acp_storage.html
Outdated
| <dt> | ||
| {% set title = 'STORAGE_ADAPTER_' ~ provider.get_name | upper ~ '_OPTION_' ~ name | upper %} | ||
| {% set description = 'STORAGE_ADAPTER_' ~ provider.get_name | upper ~ '_OPTION_' ~ name | upper ~ '_EXPLAIN' %} | ||
| <label for="">{{ lang(title) }}{{ lang('COLON') }}</label>{% if description != lang(description) %}<br /><span>{{ lang(description) }}</span>{% endif %} |
There was a problem hiding this comment.
phpBB/adm/style/acp_storage.html
Outdated
| @@ -0,0 +1,73 @@ | |||
| <!-- INCLUDE overall_header.html --> | |||
There was a problem hiding this comment.
Please use twig for the includes as well.
phpBB/develop/adjust_avatars.php
Outdated
| @@ -30,6 +30,11 @@ | |||
| die('database not up to date'); | |||
| } | |||
|
|
|||
| if (!isset($config['storage\\avatar\\config\\path']) || $config['storage\\avatar\\config\\path'] != 'phpbb\\storage\\provider\\local') | |||
| { | |||
| die('use local provider'); | |||
There was a problem hiding this comment.
Use a better message that actually explains this to user not knowing the details of the storage system.
phpBB/includes/acp/acp_storage.php
Outdated
| /** @var string */ | ||
| public $u_action; | ||
|
|
||
| public function main($id, $mode) |
There was a problem hiding this comment.
Please add docblocks to these methods.
There was a problem hiding this comment.
Is there any other module with that docblock?
There was a problem hiding this comment.
Most old acp modules don't have it but please add them when adding new methods (which you're doing in this case).
phpBB/includes/acp/acp_storage.php
Outdated
| // Check options | ||
| $new_options = $this->get_provider_options($this->get_new_provider($storage_name)); | ||
|
|
||
| foreach ($new_options as $def_k => $def_v) |
There was a problem hiding this comment.
Please don't shorten variable names like this, e.g. use $definition_value$ and $definition_key.
phpBB/includes/functions_user.php
Outdated
| $sql = 'UPDATE ' . GROUPS_TABLE . ' | ||
| SET group_avatar = \'' . $db->sql_escape($new_entry) . "' | ||
| WHERE group_id = $group_id"; | ||
| $db->sql_query($sql); | ||
| } | ||
| catch (\phpbb\storage\exception\exception $e) | ||
| { | ||
|
|
There was a problem hiding this comment.
Please add a comment to what this means (as you did in other places).
There was a problem hiding this comment.
I think i did this in other branch, but i have to rebase all
phpBB/phpbb/avatar/driver/upload.php
Outdated
|
|
||
| /** | ||
| * Before deleting an existing avatar | ||
| * | ||
| * @event core.avatar_driver_upload_delete_before | ||
| * @var string destination Destination directory where the file is going to be deleted |
There was a problem hiding this comment.
Please add a @changed to this that mentions that destination was removed.
| @@ -83,7 +83,7 @@ public function build_options($storage_name, array $definitions) | |||
| { | |||
| $options = []; | |||
|
|
|||
| foreach ($definitions as $def) | |||
| foreach (array_keys($definitions) as $def) | |||
rubencm
force-pushed
the
ticket/15289
branch
from
20e67b6
to
f607b48
Compare
|
You removed the composer-ext files. Can you please add them back? |
PHPBB3-15289
PHPBB3-15289
PHPBB3-15289
PHPBB3-15289
PHPBB3-15289
PHPBB3-15289
PHPBB3-15289
PHPBB3-15289
PHPBB3-15289
PHPBB3-15289
PHPBB3-15289
PHPBB3-15289
PHPBB3-15289
PHPBB3-15289
PHPBB3-15289
PHPBB3-15289
PHPBB3-15289
PHPBB3-15289
PHPBB3-15289
PHPBB3-15289
PHPBB3-15289
PHPBB3-15289
PHPBB3-15289
rubencm
force-pushed
the
ticket/15289
branch
from
04319a1
to
367b84b
Compare
PHPBB3-15289
phpBB/includes/acp/acp_storage.php
Outdated
| public function overview($id, $mode) | ||
| { | ||
| $form_name = 'acp_storage'; | ||
| add_form_key($form_name); |
There was a problem hiding this comment.
You're adding a form key but you're never actually checking it. This is however vital for the CSRF protection.
phpBB/language/en/acp/storage.php
Outdated
|
|
||
| // Template | ||
| 'STORAGE_TITLE' => 'Storage Settings', | ||
| 'STORAGE_TITLE_EXPLAIN' => 'Here you can change the storage.', |
There was a problem hiding this comment.
Change storage providers for the file storage types of phpBB. Choose local or remote providers to store files added to or created by phpBB.
phpBB/language/en/acp/storage.php
Outdated
| 'STORAGE_TITLE' => 'Storage Settings', | ||
| 'STORAGE_TITLE_EXPLAIN' => 'Here you can change the storage.', | ||
| 'STORAGE_SELECT' => 'Select storage', | ||
| 'STORAGE_SELECT_DESC' => 'Select an storage from the list.', |
phpBB/language/en/acp/storage.php
Outdated
| 'STORAGE_ADAPTER_LOCAL_OPTION_PATH' => 'Path', | ||
|
|
||
| // Form validation | ||
| 'STORAGE_UPDATE_SUCCESSFUL' => 'All storages were successfuly updated.', |
There was a problem hiding this comment.
All storage types were successfully updated.
phpBB/language/en/acp/storage.php
Outdated
|
|
||
| // Form validation | ||
| 'STORAGE_UPDATE_SUCCESSFUL' => 'All storages were successfuly updated.', | ||
| 'STORAGE_NO_CHANGES' => 'No changes has been made.', |
phpBB/language/en/acp/storage.php
Outdated
| // Form validation | ||
| 'STORAGE_UPDATE_SUCCESSFUL' => 'All storages were successfuly updated.', | ||
| 'STORAGE_NO_CHANGES' => 'No changes has been made.', | ||
| 'STORAGE_PROVIDER_NOT_EXISTS' => 'Provider selected for %s doesn\'t exist.', |
There was a problem hiding this comment.
Use the unicode version of the single quote: ’
phpBB/includes/acp/acp_storage.php
Outdated
|
|
||
| // Add necesary language files | ||
| $this->lang->add_lang(array('common')); | ||
| $this->lang->add_lang(array('acp/storage')); |
There was a problem hiding this comment.
I believe the whole point of the array thing is, that you can add multiple different language files at once.
Also, common is always included, isn't it?
There was a problem hiding this comment.
I dont think so, i added it here only because of L_YES and L_NO
edit: and now for the form check
There was a problem hiding this comment.
I'm preetty sure that every file adds the 'common.php' through the $user->setup() method.
phpBB/includes/acp/acp_storage.php
Outdated
| } | ||
| } | ||
|
|
||
| if (count($modified_storages)) |
There was a problem hiding this comment.
!empty($modified_storages)
There was a problem hiding this comment.
Well, is the same i think.
edit: Ok, i see, empty returns a boolean that fit better here
phpBB/includes/acp/acp_storage.php
Outdated
|
|
||
| if (count($modified_storages)) | ||
| { | ||
| if (!count($messages)) |
PHPBB3-15289
PHPBB3-15289
PHPBB3-15289
phpBB/includes/acp/acp_storage.php
Outdated
| $this->storage_collection = $phpbb_container->get('storage.storage_collection'); | ||
|
|
||
| // Add necesary language files | ||
| $this->lang->add_lang(['common', 'acp/storage']); |
There was a problem hiding this comment.
As already stated, you don't need common.
There was a problem hiding this comment.
Yes, it is working without it, but i think it wasn't
PHPBB3-15289
[ticket/15289] Allow to configure storage from acp * github.com:phpbb/phpbb: (38 commits) [ticket/15289] Add phpdoc [ticket/15289] Remove common language from acp module [ticket/15289] Check form [ticket/15289] Use empty instead of count [ticket/15289] Language fixes [ticket/15289] Add missing files [ticket/15289] Use twig syntax in variables [ticket/15289] Use lang_defined() [ticket/15289] Dont use short names [ticket/15289] Dont use short names [ticket/15289] Use Twig includes [ticket/15289] Update acp module [ticket/15289] Fix comment typo [ticket/15289] Fix show field description [ticket/15289] Update event [ticket/15289] Remove switch since there is only one mode [ticket/15289] Improve error messages [ticket/15289] Fix code style [ticket/15289] Update acp storage [ticket/15289] Update acp storage template ...
Checklist:
This depends on #4894, #4937
Tracker ticket (set the ticket ID to your ticket ID):
https://tracker.phpbb.com/browse/PHPBB3-15289