From 195fb59b4e5d75b7204d22c18c11049b59c0ecbd Mon Sep 17 00:00:00 2001 From: Ruben Calvo Date: Sat, 25 May 2024 16:35:44 +0200 Subject: [PATCH] [ticket/15699] Fixes code review PHPBB3-15699 --- phpBB/adm/style/acp_storage.html | 12 ++-- .../default/container/services_storage.yml | 4 +- phpBB/includes/acp/acp_storage.php | 52 +++++++++++--- phpBB/phpbb/storage/adapter_factory.php | 15 ++-- phpBB/phpbb/storage/helper.php | 65 +++++++++++++++--- phpBB/phpbb/storage/state_helper.php | 68 ++++++++++++++++++- 6 files changed, 181 insertions(+), 35 deletions(-) diff --git a/phpBB/adm/style/acp_storage.html b/phpBB/adm/style/acp_storage.html index 1aedc50ce1..86c9298828 100644 --- a/phpBB/adm/style/acp_storage.html +++ b/phpBB/adm/style/acp_storage.html @@ -41,7 +41,7 @@
{{ lang('STORAGE_' ~ storage.get_name | upper ~ '_TITLE') }}
-

{{ lang('STORAGE_SELECT_DESC') }}
+

{{ lang('STORAGE_SELECT_DESC') }}
{{ lang('STORAGE_UPDATE_TYPE_CONFIG') }} diff --git a/phpBB/config/default/container/services_storage.yml b/phpBB/config/default/container/services_storage.yml index 401357861d..496c30de5d 100644 --- a/phpBB/config/default/container/services_storage.yml +++ b/phpBB/config/default/container/services_storage.yml @@ -120,7 +120,7 @@ services: class: phpbb\storage\helper arguments: - '@config' - - '@storage.provider_collection' - - '@storage.adapter_collection' - '@storage.adapter.factory' - '@storage.state_helper' + - '@storage.provider_collection' + - '@storage.adapter_collection' diff --git a/phpBB/includes/acp/acp_storage.php b/phpBB/includes/acp/acp_storage.php index 150d711672..2adf833562 100644 --- a/phpBB/includes/acp/acp_storage.php +++ b/phpBB/includes/acp/acp_storage.php @@ -123,6 +123,8 @@ class acp_storage } /** + * Method to route the request to the correct page + * * @param string $id * @param string $mode */ @@ -156,11 +158,16 @@ class acp_storage } else { - $this->settings_form($id, $mode); + $this->settings_form(); } } } + /** + * Page to update storage settings and move files + * + * @return void + */ private function update_action(): void { if (!check_link_hash($this->request->variable('hash', ''), 'acp_storage')) @@ -263,6 +270,11 @@ class acp_storage trigger_error($this->lang->lang('STORAGE_UPDATE_SUCCESSFUL') . adm_back_link($this->u_action)); } + /** + * Page that show a form with the progress bar, and a button to continue or cancel + * + * @return void + */ private function update_inprogress(): void { // Template from adm/style @@ -271,13 +283,18 @@ class acp_storage // Set page title $this->page_title = 'STORAGE_TITLE'; - $this->template->assign_vars(array( + $this->template->assign_vars([ 'U_ACTION' => $this->u_action . '&action=update&hash=' . generate_link_hash('acp_storage'), 'CONTINUE_PROGRESS' => $this->get_storage_update_progress(), - )); + ]); } - private function settings_form(string $id, string $mode): void + /** + * Main settings page, shows a form with all the storages and their configuration options + * + * @return void + */ + private function settings_form(): void { $form_key = 'acp_storage'; add_form_key($form_key); @@ -348,6 +365,12 @@ class acp_storage ]); } + /** + * When submit the settings form, check which storages have been modified + * to update only those. + * + * @return array + */ private function get_modified_storages(): array { $modified_storages = []; @@ -386,7 +409,12 @@ class acp_storage return $modified_storages; } - protected function storage_stats() + /** + * Fill template variables to show storage stats in settings page + * + * @return void + */ + protected function storage_stats(): void { // Top table with stats of each storage $storage_stats = []; @@ -435,6 +463,11 @@ class acp_storage adm_page_footer(); } + /** + * Get storage update progress to show progress bar + * + * @return array + */ protected function get_storage_update_progress(): array { $file_index = $this->state_helper->file_index(); @@ -477,7 +510,7 @@ class acp_storage * @param string $storage_name Storage name * @param array $messages Reference to messages array */ - protected function validate_data(string $storage_name, array &$messages) + protected function validate_data(string $storage_name, array &$messages): void { $storage_title = $this->lang->lang('STORAGE_' . strtoupper($storage_name) . '_TITLE'); @@ -499,6 +532,8 @@ class acp_storage return; } + $this->validate_path($storage_name, $messages); + // Check options $new_options = $this->storage_helper->get_provider_options($this->request->variable([$storage_name, 'provider'], '')); @@ -517,7 +552,7 @@ class acp_storage $messages[] = $this->lang->lang('STORAGE_FORM_TYPE_EMAIL_INCORRECT_FORMAT', $definition_title, $storage_title); } - $maxlength = isset($definition_value['max']) ? $definition_value['max'] : 255; + $maxlength = $definition_value['max'] ?? 255; if (strlen($value) > $maxlength) { $messages[] = $this->lang->lang('STORAGE_FORM_TYPE_TEXT_TOO_LONG', $definition_title, $storage_title); @@ -589,7 +624,7 @@ class acp_storage if ($this->provider_collection->get_by_class($current_provider)->get_name() == 'local' && isset($options['path'])) { - $path = $this->storage_helper->get_current_definition($storage_name, 'path'); + $path = $this->request->is_set_post('submit') ? $this->request->variable([$storage_name, 'path'], '') : $this->storage_helper->get_current_definition($storage_name, 'path'); if (empty($path)) { @@ -602,5 +637,4 @@ class acp_storage } } - } diff --git a/phpBB/phpbb/storage/adapter_factory.php b/phpBB/phpbb/storage/adapter_factory.php index 62e51f66ae..61ea879482 100644 --- a/phpBB/phpbb/storage/adapter_factory.php +++ b/phpBB/phpbb/storage/adapter_factory.php @@ -15,7 +15,6 @@ namespace phpbb\storage; use phpbb\config\config; use phpbb\di\service_collection; -use phpbb\storage\adapter\adapter_interface; use phpbb\storage\exception\storage_exception; class adapter_factory @@ -54,9 +53,9 @@ class adapter_factory * * @param string $storage_name * - * @return adapter_interface + * @return mixed */ - public function get(string $storage_name) + public function get(string $storage_name): mixed { $provider_class = $this->config['storage\\' . $storage_name . '\\provider']; $provider = $this->providers->get_by_class($provider_class); @@ -71,7 +70,15 @@ class adapter_factory return $this->get_with_options($storage_name, $options); } - public function get_with_options(string $storage_name, array $options) + /** + * Obtains a configured adapters for a given storage with custom options + * + * @param string $storage_name + * @param array $options + * + * @return mixed + */ + public function get_with_options(string $storage_name, array $options): mixed { $provider_class = $this->config['storage\\' . $storage_name . '\\provider']; $provider = $this->providers->get_by_class($provider_class); diff --git a/phpBB/phpbb/storage/helper.php b/phpBB/phpbb/storage/helper.php index de75ab337c..66921b60a7 100644 --- a/phpBB/phpbb/storage/helper.php +++ b/phpBB/phpbb/storage/helper.php @@ -21,31 +21,41 @@ class helper /** @var config */ protected $config; - /** @var service_collection */ - protected $provider_collection; - - /** @var service_collection */ - protected $adapter_collection; - /** @var adapter_factory */ protected $adapter_factory; /** @var state_helper */ protected $state_helper; - public function __construct(config $config, service_collection $provider_collection, service_collection $adapter_collection, adapter_factory $adapter_factory, state_helper $state_helper) + /** @var service_collection */ + protected $provider_collection; + + /** @var service_collection */ + protected $adapter_collection; + + /** + * Constructor + * + * @param config $config + * @param adapter_factory $adapter_factory + * @param state_helper $state_helper + * @param service_collection $provider_collection + * @param service_collection $adapter_collection + */ + public function __construct(config $config, adapter_factory $adapter_factory, state_helper $state_helper, service_collection $provider_collection, service_collection $adapter_collection) { $this->config = $config; - $this->provider_collection = $provider_collection; - $this->adapter_collection = $adapter_collection; $this->adapter_factory = $adapter_factory; $this->state_helper = $state_helper; + $this->provider_collection = $provider_collection; + $this->adapter_collection = $adapter_collection; } /** * Get adapter definitions from a provider * * @param string $provider_class Provider class + * * @return array Adapter definitions */ public function get_provider_options(string $provider_class) : array @@ -57,6 +67,7 @@ class helper * Get the current provider from config * * @param string $storage_name Storage name + * * @return string The current provider */ public function get_current_provider(string $storage_name) : string @@ -69,6 +80,7 @@ class helper * * @param string $storage_name Storage name * @param string $definition Definition + * * @return string Definition value */ public function get_current_definition(string $storage_name, string $definition) : string @@ -102,7 +114,7 @@ class helper * * @return mixed Storage adapter instance */ - public function get_new_adapter(string $storage_name) + public function get_new_adapter(string $storage_name): mixed { static $adapters = []; @@ -123,6 +135,13 @@ class helper return $adapters[$storage_name]; } + /** + * Delete configuration options for a given storage + * + * @param string $storage_name + * + * @return void + */ public function delete_storage_options(string $storage_name): void { $provider = $this->get_current_provider($storage_name); @@ -134,16 +153,41 @@ class helper } } + /** + * Set a provider in configuration for a given storage + * + * @param string $storage_name + * @param string $provider + * + * @return void + */ public function set_storage_provider(string $storage_name, string $provider): void { $this->config->set('storage\\' . $storage_name . '\\provider', $provider); } + /** + * Set storage options in configuration for a given storage + * + * @param string $storage_name + * @param string $definition + * @param string $value + * + * @return void + */ public function set_storage_definition(string $storage_name, string $definition, string $value): void { $this->config->set('storage\\' . $storage_name . '\\config\\' . $definition, $value); } + /** + * Copy a file from the current adapter to the new adapter + * + * @param $storage_name + * @param $file + * + * @return void + */ public function copy_file_to_new_adapter($storage_name, $file): void { $current_adapter = $this->get_current_adapter($storage_name); @@ -166,7 +210,6 @@ class helper */ public function update_storage_config(string $storage_name) : void { - // Remove old storage config $this->delete_storage_options($storage_name); diff --git a/phpBB/phpbb/storage/state_helper.php b/phpBB/phpbb/storage/state_helper.php index 6105dc25da..82523f0aa0 100644 --- a/phpBB/phpbb/storage/state_helper.php +++ b/phpBB/phpbb/storage/state_helper.php @@ -31,6 +31,11 @@ class state_helper /** @var service_collection */ protected $provider_collection; + /** + * @param config $config + * @param db_text $config_text + * @param service_collection $provider_collection + */ public function __construct(config $config, db_text $config_text, service_collection $provider_collection) { $this->config = $config; @@ -48,6 +53,13 @@ class state_helper return !empty(json_decode($this->config_text->get('storage_update_state'), true)); } + /** + * Get new provider for the specified storage + * + * @param string $storage_name + * + * @return string + */ public function new_provider(string $storage_name): string { $state = $this->load_state(); @@ -55,6 +67,14 @@ class state_helper return $state['storages'][$storage_name]['provider']; } + /** + * Get new definition value for the specified storage + * + * @param string $storage_name + * @param string $definition + * + * @return string + */ public function new_definition_value(string $storage_name, string $definition): string { $state = $this->load_state(); @@ -62,6 +82,11 @@ class state_helper return $state['storages'][$storage_name]['config'][$definition]; } + /** + * Get the update type + * + * @return update_type + */ public function update_type(): update_type { $state = $this->load_state(); @@ -69,6 +94,11 @@ class state_helper return update_type::from($state['update_type']); } + /** + * Get the current storage index + * + * @return int + */ public function storage_index(): int { $state = $this->load_state(); @@ -76,6 +106,13 @@ class state_helper return $state['storage_index']; } + /** + * Update the storage index + * + * @param int $storage_index + * + * @return void + */ public function set_storage_index(int $storage_index): void { $state = $this->load_state(); @@ -85,6 +122,11 @@ class state_helper $this->save_state($state); } + /** + * Get the current remove storage index + * + * @return int + */ public function remove_storage_index(): int { $state = $this->load_state(); @@ -92,6 +134,13 @@ class state_helper return $state['remove_storage_index']; } + /** + * Update the remove storage index + * + * @param int $storage_index + * + * @return void + */ public function set_remove_storage_index(int $storage_index): void { $state = $this->load_state(); @@ -101,6 +150,11 @@ class state_helper $this->save_state($state); } + /** + * Get the file index + * + * @return int + */ public function file_index(): int { $state = $this->load_state(); @@ -108,6 +162,12 @@ class state_helper return $state['file_index']; } + /** + * Set the file index + * + * @param int $file_index + * @return void + */ public function set_file_index(int $file_index): void { $state = $this->load_state(); @@ -117,6 +177,11 @@ class state_helper $this->save_state($state); } + /** + * Get the storage names to be updated + * + * @return array + */ public function storages(): array { $state = $this->load_state(); @@ -178,7 +243,7 @@ class state_helper */ public function clear_state(): void { - $this->save_state([]); + $this->save_state(); } /** @@ -210,5 +275,4 @@ class state_helper { $this->config_text->set('storage_update_state', json_encode($state, JSON_THROW_ON_ERROR)); } - }