[ticket/15699] Fixes code review

PHPBB3-15699

phpBB/adm/style/acp_storage.html

@@ -41,7 +41,7 @@
 		<fieldset>
 			<legend>{{ lang('STORAGE_' ~ storage.get_name | upper ~ '_TITLE') }}</legend>
 			<dl>
-				<dt><label for="{{ storage.get_name }}">{{ lang('STORAGE_SELECT') }}{{ lang('COLON') }}</label><br /><span>{{ lang('STORAGE_SELECT_DESC') }}</span></dt>
+				<dt><label for="{{ storage.get_name }}">{{ lang('STORAGE_SELECT') ~ lang('COLON') }}</label><br /><span>{{ lang('STORAGE_SELECT_DESC') }}</span></dt>
 				<dd>
 					<select id="{{ storage.get_name }}" name="{{ storage.get_name }}[provider]" data-togglable-settings="true">
 						{% for provider in PROVIDERS %}
@@ -65,7 +65,7 @@
 							<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>{{ lang(title) }}{{ lang('COLON') }}</label>
+								<label>{{ lang(title) ~ lang('COLON') }}</label>
 								{% if lang_defined(description) %}
 									<br /><span>{{ lang(description) }}</span>
 								{% endif %}
@@ -82,8 +82,7 @@
 									{% set buttons = [] %}
 
 									{% for button in options.buttons %}
-										{% set checked = button.value == input_value %}
+										{% set new_button = button | merge({"name": input_name, "checked": button.value == input_value}) %}
-										{% set new_button = button | merge({"name": input_name, "checked": checked}) %}
 										{% set buttons = buttons | merge([new_button]) %}
 									{% endfor %}
 
@@ -92,8 +91,7 @@
 									{% set select_options = [] %}
 
 									{% for option in options.options %}
-										{% set selected = option.value == input_value %}
+										{% set new_option = option | merge({"selected": option.value == input_value}) %}
-										{% set new_option = option | merge({"selected": selected}) %}
 										{% set select_options = select_options | merge([new_option]) %}
 									{% endfor %}
 
@@ -109,7 +107,7 @@
 
 	<fieldset>
 		<dl>
-			<dt><label for="update_type">{{ lang('STORAGE_UPDATE_TYPE') }}{{ lang('COLON') }}</label></dt>
+			<dt><label for="update_type">{{ lang('STORAGE_UPDATE_TYPE') ~ lang('COLON') }}</label></dt>
 			<dd>
 				<label><input id="update_type" class="radio" name="update_type" value="{{ STORAGE_UPDATE_TYPE_CONFIG }}" checked="checked" type="radio"> {{ lang('STORAGE_UPDATE_TYPE_CONFIG') }}</label>
 				<label><input class="radio" name="update_type" value="{{ STORAGE_UPDATE_TYPE_COPY }}" type="radio"> {{ lang('STORAGE_UPDATE_TYPE_COPY') }}</label>

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'

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
 		}
 	}
 
-
 }

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);

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);
 

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));
 	}
-
 }