[ticket/13740] Add better progress handling, also add log messages

PHPBB3-13740

phpBB/assets/javascript/installer.js

@@ -204,13 +204,14 @@
      * @param progressLimit
      */
     function incrementFiller($progressText, $progressFiller, progressLimit) {
+        if (currentProgress >= progressLimit || currentProgress >= 100) {
+            clearInterval(progressTimer);
+            return;
+        }
+
         currentProgress++;
         $progressText.text(currentProgress + '%');
         $progressFiller.css('width', currentProgress + '%');
-
-        if (currentProgress >= progressLimit || currentProgress >= 100) {
-            clearInterval(progressTimer);
-        }
     }
 
     /**

phpBB/config/installer/container/services_install_obtain_data.yml

@@ -41,3 +41,5 @@ services:
         parent: installer.module_base
         arguments:
             - ["installer.obtain_data.obtain_admin_data", "installer.obtain_data.obtain_database_data", "installer.obtain_data.obtain_server_data", "installer.obtain_data.obtain_email_data", "installer.obtain_data.obtain_board_data", "installer.obtain_data.obtain_imagick_path"]
+            - true
+            - false

phpBB/config/installer/container/services_install_requirements.yml

@@ -18,3 +18,5 @@ services:
         parent: installer.module_base
         arguments:
             - ["installer.requirements.check_filesystem", "installer.requirements.check_server_environment"]
+            - true
+            - false

phpBB/install/exception/invalid_service_name_exception.php

@@ -0,0 +1,69 @@
+<?php
+/**
+ *
+ * This file is part of the phpBB Forum Software package.
+ *
+ * @copyright (c) phpBB Limited <https://www.phpbb.com>
+ * @license GNU General Public License, version 2 (GPL-2.0)
+ *
+ * For full copyright and license information, please see
+ * the docs/CREDITS.txt file.
+ *
+ */
+
+namespace phpbb\install\exception;
+
+class invalid_service_name_exception extends installer_exception
+{
+	/**
+	 * @var string
+	 */
+	private $params;
+
+	/**
+	 * @var string
+	 */
+	private $error;
+
+	/**
+	 * Constructor
+	 *
+	 * @param string	$error	The name of the missing installer module
+	 * @param array		$params	Additional values for message translation
+	 */
+	public function __construct($error, $params = array())
+	{
+		$this->error = $error;
+		$this->params = $params;
+	}
+
+	/**
+	 * Returns the language entry's name for the error
+	 *
+	 * @return string
+	 */
+	public function get_error()
+	{
+		return $this->error;
+	}
+
+	/**
+	 * Returns parameters for the language entry, if there is any
+	 *
+	 * @return array
+	 */
+	public function get_params()
+	{
+		return $this->params;
+	}
+
+	/**
+	 * Returns true, if there are any parameters set
+	 *
+	 * @return bool
+	 */
+	public function has_params()
+	{
+		return (sizeof($this->params) !== 0);
+	}
+}

phpBB/install/exception/module_not_found_exception.php

@@ -0,0 +1,42 @@
+<?php
+/**
+ *
+ * This file is part of the phpBB Forum Software package.
+ *
+ * @copyright (c) phpBB Limited <https://www.phpbb.com>
+ * @license GNU General Public License, version 2 (GPL-2.0)
+ *
+ * For full copyright and license information, please see
+ * the docs/CREDITS.txt file.
+ *
+ */
+
+namespace phpbb\install\exception;
+
+class module_not_found_exception extends installer_exception
+{
+	/**
+	 * @var string
+	 */
+	private $module_service_name;
+
+	/**
+	 * Constructor
+	 *
+	 * @param string	$module_service_name	The name of the missing installer module
+	 */
+	public function __construct($module_service_name)
+	{
+		$this->module_service_name = $module_service_name;
+	}
+
+	/**
+	 * Returns the missing installer module's service name
+	 *
+	 * @return string
+	 */
+	public function get_module_service_name()
+	{
+		return $this->module_service_name;
+	}
+}

phpBB/install/exception/task_not_found_exception.php

@@ -0,0 +1,42 @@
+<?php
+/**
+ *
+ * This file is part of the phpBB Forum Software package.
+ *
+ * @copyright (c) phpBB Limited <https://www.phpbb.com>
+ * @license GNU General Public License, version 2 (GPL-2.0)
+ *
+ * For full copyright and license information, please see
+ * the docs/CREDITS.txt file.
+ *
+ */
+
+namespace phpbb\install\exception;
+
+class task_not_found_exception extends installer_exception
+{
+	/**
+	 * @var string
+	 */
+	private $task_service_name;
+
+	/**
+	 * Constructor
+	 *
+	 * @param string	$task_service_name	The name of the missing installer module
+	 */
+	public function __construct($task_service_name)
+	{
+		$this->task_service_name = $task_service_name;
+	}
+
+	/**
+	 * Returns the missing installer task's service name
+	 *
+	 * @return string
+	 */
+	public function get_task_service_name()
+	{
+		return $this->task_service_name;
+	}
+}

phpBB/install/helper/config.php

@@ -232,10 +232,17 @@ class config
 
 	/**
 	 * Increments the task progress
+	 *
+	 * @param int	$increment_by	The amount to increment by
 	 */
-	public function increment_current_task_progress()
+	public function increment_current_task_progress($increment_by = 1)
 	{
-		$this->progress_data['current_task_progress']++;
+		$this->progress_data['current_task_progress'] += $increment_by;
+
+		if ($this->progress_data['current_task_progress'] > $this->progress_data['max_task_progress'])
+		{
+			$this->progress_data['current_task_progress'] = $this->progress_data['max_task_progress'];
+		}
 	}
 
 	/**

phpBB/install/installer.php

@@ -13,15 +13,24 @@
 
 namespace phpbb\install;
 
+use phpbb\install\exception\invalid_service_name_exception;
+use phpbb\install\exception\module_not_found_exception;
+use phpbb\install\exception\task_not_found_exception;
+use phpbb\install\exception\user_interaction_required_exception;
+use phpbb\install\helper\config;
+use phpbb\install\helper\iohandler\iohandler_interface;
+use Symfony\Component\DependencyInjection\ContainerInterface;
+use Symfony\Component\DependencyInjection\Exception\InvalidArgumentException;
+
 class installer
 {
 	/**
-	 * @var \Symfony\Component\DependencyInjection\ContainerInterface
+	 * @var ContainerInterface
 	 */
 	protected $container;
 
 	/**
-	 * @var \phpbb\install\helper\config
+	 * @var config
 	 */
 	protected $install_config;
 
@@ -31,17 +40,24 @@ class installer
 	protected $installer_modules;
 
 	/**
-	 * @var \phpbb\install\helper\iohandler\iohandler_interface
+	 * @var iohandler_interface
 	 */
 	protected $iohandler;
 
+	/**
+	 * Stores the number of steps that a given module has
+	 *
+	 * @var array
+	 */
+	protected $module_step_count;
+
 	/**
 	 * Constructor
 	 *
-	 * @param \phpbb\install\helper\config	$config		Installer config handler
+	 * @param config				$config		Installer config handler
-	 * @param \Symfony\Component\DependencyInjection\ContainerInterface	$container	Dependency injection container
+	 * @param ContainerInterface	$container	Dependency injection container
 	 */
-	public function __construct(\phpbb\install\helper\config $config, \Symfony\Component\DependencyInjection\ContainerInterface $container)
+	public function __construct(config $config, ContainerInterface $container)
 	{
 		$this->install_config		= $config;
 		$this->container			= $container;
@@ -66,9 +82,9 @@ class installer
 	/**
 	 * Sets input-output handler objects
 	 *
-	 * @param helper\iohandler\iohandler_interface	$iohandler
+	 * @param iohandler_interface	$iohandler
 	 */
-	public function set_iohandler(\phpbb\install\helper\iohandler\iohandler_interface $iohandler)
+	public function set_iohandler(iohandler_interface $iohandler)
 	{
 		$this->iohandler = $iohandler;
 	}
@@ -84,24 +100,44 @@ class installer
 		// Recover install progress
 		$module_index = $this->recover_progress();
 
-		// Count all tasks in the current installer modules
+		// Variable used to check if the install process have been finished
-		$task_count = 0;
-		foreach ($this->installer_modules as $name)
-		{
-			/** @var \phpbb\install\module_interface $module */
-			$module = $this->container->get($name);
-
-			$task_count += $module->get_step_count();
-		}
-
-		// Set task count
-		$this->install_config->set_task_progress_count($task_count);
-		$this->iohandler->set_task_count($task_count);
-
 		$install_finished = false;
 
+		// Flag used by exception handling, whether or not we need to flush output buffer once again
+		$flush_messages = false;
+
 		try
 		{
+			if ($this->install_config->get_task_progress_count() === 0)
+			{
+				// Count all tasks in the current installer modules
+				$step_count = 0;
+				foreach ($this->installer_modules as $index => $name)
+				{
+					try
+					{
+						/** @var \phpbb\install\module_interface $module */
+						$module = $this->container->get($name);
+					}
+					catch (InvalidArgumentException $e)
+					{
+						throw new module_not_found_exception($name);
+					}
+
+					$module_step_count = $module->get_step_count();
+					$step_count += $module_step_count;
+					$this->module_step_count[$index] = $module_step_count;
+				}
+
+				// Set task count
+				$this->install_config->set_task_progress_count($step_count);
+			}
+
+			// Set up progress information
+			$this->iohandler->set_task_count(
+				$this->install_config->get_task_progress_count()
+			);
+
 			// Run until there are available resources
 			while ($this->install_config->get_time_remaining() > 0 && $this->install_config->get_memory_remaining() > 0)
 			{
@@ -117,14 +153,26 @@ class installer
 				$this->install_config->set_active_module($module_service_name, $module_index);
 
 				// Get module from container
-				/** @var \phpbb\install\module_interface $module */
+				try
-				$module = $this->container->get($module_service_name);
+				{
+					/** @var \phpbb\install\module_interface $module */
+					$module = $this->container->get($module_service_name);
+				}
+				catch (InvalidArgumentException $e)
+				{
+					throw new module_not_found_exception($module_service_name);
+				}
 
 				$module_index++;
 
 				// Check if module should be executed
 				if (!$module->is_essential() && !$module->check_requirements())
 				{
+					$this->iohandler->add_log_message(array(
+						'SKIP_MODULE',
+						$module_service_name,
+					));
+					$this->install_config->increment_current_task_progress($this->module_step_count[$module_index - 1]);
 					continue;
 				}
 
@@ -144,10 +192,46 @@ class installer
 				// @todo: Send refresh request
 			}
 		}
-		catch (\phpbb\install\exception\user_interaction_required_exception $e)
+		catch (user_interaction_required_exception $e)
 		{
 			// @todo handle exception
 		}
+		catch (module_not_found_exception $e)
+		{
+			$this->iohandler->add_error_message('MODULE_NOT_FOUND', array(
+				'MODULE_NOT_FOUND_DESCRIPTION',
+				$e->get_module_service_name(),
+			));
+			$flush_messages = true;
+		}
+		catch (task_not_found_exception $e)
+		{
+			$this->iohandler->add_error_message('TASK_NOT_FOUND', array(
+				'TASK_NOT_FOUND_DESCRIPTION',
+				$e->get_task_service_name(),
+			));
+			$flush_messages = true;
+		}
+		catch (invalid_service_name_exception $e)
+		{
+			if ($e->has_params())
+			{
+				$msg = $e->get_params();
+				array_unshift($msg, $e->get_error());
+			}
+			else
+			{
+				$msg = $e->get_error();
+			}
+
+			$this->iohandler->add_error_message($msg);
+			$flush_messages = true;
+		}
+
+		if ($flush_messages)
+		{
+			$this->iohandler->send_response();
+		}
 
 		// Save install progress
 		$this->install_config->save_config();

phpBB/install/module/obtain_data/module.php

@@ -15,44 +15,6 @@ namespace phpbb\install\module\obtain_data;
 
 class module extends \phpbb\install\module_base
 {
-	/**
-	 * {@inheritdoc}
-	 */
-	public function run()
-	{
-		// Recover install progress
-		$task_index = $this->recover_progress();
-
-		// Run until there are available resources
-		while ($this->install_config->get_time_remaining() > 0 && $this->install_config->get_memory_remaining() > 0)
-		{
-			// Check if task exists
-			if (!isset($this->task_collection[$task_index]))
-			{
-				break;
-			}
-
-			// Recover task to be executed
-			/** @var \phpbb\install\task_interface $task */
-			$task = $this->container->get($this->task_collection[$task_index]);
-
-			// Iterate to the next task
-			$task_index++;
-
-			// Check if we can run the task
-			if (!$task->is_essential() && !$task->check_requirements())
-			{
-				continue;
-			}
-
-			$task->run();
-
-			// Log install progress
-			$current_task_index = $task_index - 1;
-			$this->install_config->set_finished_task($this->task_collection[$current_task_index], $current_task_index);
-		}
-	}
-
 	/**
 	 * {@inheritdoc}
 	 */

phpBB/install/module/requirements/module.php

@@ -34,8 +34,15 @@ class module extends \phpbb\install\module_base
 			}
 
 			// Recover task to be executed
-			/** @var \phpbb\install\task_interface $task */
+			try
-			$task = $this->container->get($this->task_collection[$task_index]);
+			{
+				/** @var \phpbb\install\task_interface $task */
+				$task = $this->container->get($this->task_collection[$task_index]);
+			}
+			catch (InvalidArgumentException $e)
+			{
+				throw new task_not_found_exception($this->task_collection[$task_index]);
+			}
 
 			// Iterate to the next task
 			$task_index++;

phpBB/install/module_base.php

@@ -13,23 +13,30 @@
 
 namespace phpbb\install;
 
+use phpbb\install\exception\invalid_service_name_exception;
+use phpbb\install\exception\task_not_found_exception;
+use phpbb\install\helper\iohandler\iohandler_interface;
+use Symfony\Component\DependencyInjection\ContainerInterface;
+use Symfony\Component\DependencyInjection\Exception\InvalidArgumentException;
+use phpbb\install\helper\config;
+
 /**
  * Base class for installer module
  */
 abstract class module_base implements module_interface
 {
 	/**
-	 * @var \Symfony\Component\DependencyInjection\ContainerInterface
+	 * @var ContainerInterface
 	 */
 	protected $container;
 
 	/**
-	 * @var \phpbb\install\helper\config
+	 * @var config
 	 */
 	protected $install_config;
 
 	/**
-	 * @var \phpbb\install\helper\iohandler\iohandler_interface
+	 * @var iohandler_interface
 	 */
 	protected $iohandler;
 
@@ -45,26 +52,33 @@ abstract class module_base implements module_interface
 	 */
 	protected $task_collection;
 
+	/**
+	 * @var bool
+	 */
+	protected $allow_progress_bar;
+
 	/**
 	 * Installer module constructor
 	 *
-	 * @param array	$tasks		array of installer tasks for installer module
+	 * @param array	$tasks				array of installer tasks for installer module
-	 * @param bool	$essential	flag that indicates if module is essential or not
+	 * @param bool	$essential			flag indicating whether the module is essential or not
+	 * @param bool	$allow_progress_bar	flag indicating whether or not to send progress information from within the module
 	 */
-	public function __construct(array $tasks, $essential = true)
+	public function __construct(array $tasks, $essential = true, $allow_progress_bar = true)
 	{
-		$this->task_collection	= $tasks;
+		$this->task_collection		= $tasks;
-		$this->is_essential		= $essential;
+		$this->is_essential			= $essential;
+		$this->allow_progress_bar	= $allow_progress_bar;
 	}
 
 	/**
 	 * Dependency getter
 	 *
-	 * @param \Symfony\Component\DependencyInjection\ContainerInterface	$container
+	 * @param ContainerInterface	$container
-	 * @param \phpbb\install\helper\config								$config
+	 * @param config								$config
-	 * @param \phpbb\install\helper\iohandler\iohandler_interface		$iohandler
+	 * @param iohandler_interface		$iohandler
 	 */
-	public function setup(\Symfony\Component\DependencyInjection\ContainerInterface $container, \phpbb\install\helper\config $config, \phpbb\install\helper\iohandler\iohandler_interface $iohandler)
+	public function setup(ContainerInterface $container, config $config, iohandler_interface $iohandler)
 	{
 		$this->container		= $container;
 		$this->install_config	= $config;
@@ -107,32 +121,55 @@ abstract class module_base implements module_interface
 			}
 
 			// Recover task to be executed
-			/** @var \phpbb\install\task_interface $task */
+			try
-			$task = $this->container->get($this->task_collection[$task_index]);
+			{
+				/** @var \phpbb\install\task_interface $task */
+				$task = $this->container->get($this->task_collection[$task_index]);
+			}
+			catch (InvalidArgumentException $e)
+			{
+				throw new task_not_found_exception($this->task_collection[$task_index]);
+			}
 
 			// Send progress information
-			$this->iohandler->set_progress(
+			if ($this->allow_progress_bar)
-				$task->get_task_lang_name(),
+			{
-				$this->install_config->get_current_task_progress()
+				$this->iohandler->set_progress(
-			);
+					$task->get_task_lang_name(),
+					$this->install_config->get_current_task_progress()
+				);
+			}
 
 			// Iterate to the next task
 			$task_index++;
-			$this->install_config->increment_current_task_progress();
 
 			// Check if we can run the task
 			if (!$task->is_essential() && !$task->check_requirements())
 			{
+				$this->iohandler->add_log_message(array(
+					'SKIP_TASK',
+					$this->task_collection[$task_index],
+				));
+				$class_name = $this->get_class_from_service_name($this->task_collection[$task_index - 1]);
+				$this->install_config->increment_current_task_progress($class_name::get_step_count());
 				continue;
 			}
 
+			if ($this->allow_progress_bar)
+			{
+				$this->install_config->increment_current_task_progress();
+			}
+
 			$task->run();
 
-			// Send progress info
+			// Send progress information
-			$this->iohandler->set_progress(
+			if ($this->allow_progress_bar)
-				$task->get_task_lang_name(),
+			{
-				$this->install_config->get_current_task_progress()
+				$this->iohandler->set_progress(
-			);
+					$task->get_task_lang_name(),
+					$this->install_config->get_current_task_progress()
+				);
+			}
 
 			$this->iohandler->send_response();
 
@@ -179,22 +216,37 @@ abstract class module_base implements module_interface
 
 		foreach ($this->task_collection as $task_service_name)
 		{
-			$task_service_name_parts = explode('.', $task_service_name);
+			$class_name = $this->get_class_from_service_name($task_service_name);
-
-			if ($task_service_name_parts[0] !== 'installer')
-			{
-				// @todo throw an exception
-			}
-
-			$class_name = '\\phpbb\\install\\module\\' . $task_service_name_parts[1] . '\\task\\' . $task_service_name_parts[2];
-			if (!class_exists($class_name))
-			{
-				// @todo throw an exception
-			}
-
 			$step_count += $class_name::get_step_count();
 		}
 
 		return $step_count;
 	}
+
+	/**
+	 * Returns the name of the class form the service name
+	 *
+	 * @param string	$task_service_name	Name of the service
+	 *
+	 * @return string	Name of the class
+	 *
+	 * @throws invalid_service_name_exception	When the service name does not meet the requirements described in task_interface
+	 */
+	protected function get_class_from_service_name($task_service_name)
+	{
+		$task_service_name_parts = explode('.', $task_service_name);
+
+		if ($task_service_name_parts[0] !== 'installer')
+		{
+			throw new invalid_service_name_exception('TASK_SERVICE_INSTALLER_MISSING');
+		}
+
+		$class_name = '\\phpbb\\install\\module\\' . $task_service_name_parts[1] . '\\task\\' . $task_service_name_parts[2];
+		if (!class_exists($class_name))
+		{
+			throw new invalid_service_name_exception('TASK_CLASS_NOT_FOUND', array($task_service_name, $class_name));
+		}
+
+		return $class_name;
+	}
 }

phpBB/language/en/install.php

@@ -249,3 +249,18 @@ $lang = array_merge($lang, array(
 	// Installer general progress messages
 	'INSTALLER_FINISHED'	=> 'The installer has finished successfully',
 ));
+
+// Installer's general messages
+$lang = array_merge($lang, array(
+	'MODULE_NOT_FOUND'				=> 'Module not found',
+	'MODULE_NOT_FOUND_DESCRIPTION'	=> 'No module is found under the service definition “%s” is not defined.',
+
+	'TASK_NOT_FOUND'				=> 'Task not found',
+	'TASK_NOT_FOUND_DESCRIPTION'	=> 'No task is found under the service definition “%s” is not defined.',
+
+	'SKIP_MODULE'	=> 'Skip “%s” module',
+	'SKIP_TASK'		=> 'Skip “%s” task',
+
+	'TASK_SERVICE_INSTALLER_MISSING'	=> 'All installer task services should start with “installer”',
+	'TASK_CLASS_NOT_FOUND'				=> 'Installer task service definition is invalid. Service name “%1$s” given, the expected class namespace is “%2$s” for that. For more information please see the documentation of task_interface.',
+));