[ticket/13740] Fix itteration problems, implement class name aware collections

PHPBB3-13740

phpBB/config/installer/container/services_install_data.yml

@@ -33,7 +33,7 @@ services:
         arguments:
             - @service_container
         tags:
-            - { name: service_collection, tag: install_data_install }
+            - { name: service_collection, tag: install_data_install, class_name_aware: true }
 
     installer.module.data_install:
         class: phpbb\install\module\install_data\module

phpBB/config/installer/container/services_install_database.yml

@@ -40,7 +40,7 @@ services:
         arguments:
             - @service_container
         tags:
-            - { name: service_collection, tag: install_database_install }
+            - { name: service_collection, tag: install_database_install, class_name_aware: true }
 
     installer.module.database_install:
         class: phpbb\install\module\install_database\module

phpBB/config/installer/container/services_install_filesystem.yml

@@ -16,7 +16,7 @@ services:
         arguments:
             - @service_container
         tags:
-            - { name: service_collection, tag: install_filesystem_install }
+            - { name: service_collection, tag: install_filesystem_install, class_name_aware: true }
 
     installer.module.filesystem_install:
         class: phpbb\install\module\install_filesystem\module

phpBB/config/installer/container/services_install_finish.yml

@@ -22,7 +22,7 @@ services:
         arguments:
             - @service_container
         tags:
-            - { name: service_collection, tag: install_finish }
+            - { name: service_collection, tag: install_finish, class_name_aware: true }
 
     installer.module.finish_install:
         class: phpbb\install\module\install_filesystem\module

phpBB/config/installer/container/services_install_obtain_data.yml

@@ -53,7 +53,7 @@ services:
         arguments:
             - @service_container
         tags:
-            - { name: service_collection, tag: install_obtain_data }
+            - { name: service_collection, tag: install_obtain_data, class_name_aware: true }
 
     installer.module.obtain_data_install:
         class: phpbb\install\module\obtain_data\module

phpBB/config/installer/container/services_install_requirements.yml

@@ -22,7 +22,7 @@ services:
         arguments:
             - @service_container
         tags:
-            - { name: service_collection, tag: installer_requirements }
+            - { name: service_collection, tag: installer_requirements, class_name_aware: true }
 
 # Please note, that the name of this module is hard coded in the installer service
     installer.module.requirements_install:

phpBB/phpbb/di/pass/collection_pass.php

@@ -34,10 +34,12 @@ class collection_pass implements CompilerPassInterface
 		foreach ($container->findTaggedServiceIds('service_collection') as $id => $data)
 		{
 			$definition = $container->getDefinition($id);
+			$is_ordered_collection = (substr($definition->getClass(), -strlen('ordered_service_collection')) === 'ordered_service_collection');
+			$is_class_name_aware = (isset($data[0]['class_name_aware']) && $data[0]['class_name_aware']);
 
 			foreach ($container->findTaggedServiceIds($data[0]['tag']) as $service_id => $service_data)
 			{
-				if (substr($definition->getClass(), -strlen('ordered_service_collection')) === 'ordered_service_collection')
+				if ($is_ordered_collection)
 				{
 					$arguments = array($service_id, $service_data[0]['order']);
 				}
@@ -46,6 +48,15 @@ class collection_pass implements CompilerPassInterface
 					$arguments = array($service_id);
 				}
 
+				if ($is_class_name_aware)
+				{
+					$service_definition = $container->getDefinition($service_id);
+					$definition->addMethodCall('add_service_class', array(
+						$service_id,
+						$service_definition->getClass()
+					));
+				}
+
 				$definition->addMethodCall('add', $arguments);
 			}
 		}

phpBB/phpbb/di/service_collection.php

@@ -89,7 +89,7 @@ class service_collection extends \ArrayObject
 	* @param string	$service_id
 	* @param string	$class
 	*/
-	public function add_service_classes($service_id, $class)
+	public function add_service_class($service_id, $class)
 	{
 		$this->service_classes[$service_id] = $class;
 	}

phpBB/phpbb/install/installer.php

@@ -137,9 +137,9 @@ class installer
 			foreach ($this->installer_modules as $name => $module)
 			{
 				// Skip forward until the current task is reached
-				if (!empty($task_name) && !$module_found)
+				if (!$module_found)
 				{
-					if ($module_name === $name)
+					if ($module_name === $name || empty($module_name))
 					{
 						$module_found = true;
 					}
@@ -245,7 +245,7 @@ class installer
 	/**
 	 * Recover install progress
 	 *
-	 * @return int	Index of the next installer module to execute
+	 * @return string	Index of the next installer module to execute
 	 */
 	protected function recover_progress()
 	{

phpBB/phpbb/install/module/requirements/module.php

@@ -13,6 +13,7 @@
 
 namespace phpbb\install\module\requirements;
 
+use phpbb\install\exception\resource_limit_reached_exception;
 use phpbb\install\exception\user_interaction_required_exception;
 use Symfony\Component\DependencyInjection\Exception\InvalidArgumentException;
 
@@ -23,30 +24,38 @@ class module extends \phpbb\install\module_base
 		$tests_passed = true;
 
 		// Recover install progress
-		$task_index = 0;
+		$task_name = $this->recover_progress();
+		$task_found = false;
 
+		/**
+		 * @var string							$name	ID of the service
+		 * @var \phpbb\install\task_interface	$task	Task object
+		 */
+		foreach ($this->task_collection as $name => $task)
+		{
 			// Run until there are available resources
-		while ($this->install_config->get_time_remaining() > 0 && $this->install_config->get_memory_remaining() > 0)
+			if ($this->install_config->get_time_remaining() <= 0 && $this->install_config->get_memory_remaining() <= 0)
 			{
-			// Check if task exists
+				throw new resource_limit_reached_exception();
-			if (!isset($this->task_collection[$task_index]))
-			{
-				break;
 			}
 
-			// Recover task to be executed
+			// Skip forward until the next task is reached
-			try
+			if (!$task_found)
 			{
-				/** @var \phpbb\install\task_interface $task */
+				if ($name === $task_name || empty($task_name))
-				$task = $this->container->get($this->task_collection[$task_index]);
-			}
-			catch (InvalidArgumentException $e)
 				{
-				throw new task_not_found_exception($this->task_collection[$task_index]);
+					$task_found = true;
-			}
 
-			// Iterate to the next task
+					if ($name === $task_name)
-			$task_index++;
+					{
+						continue;
+					}
+				}
+				else
+				{
+					continue;
+				}
+			}
 
 			// Check if we can run the task
 			if (!$task->is_essential() && !$task->check_requirements())
@@ -54,10 +63,18 @@ class module extends \phpbb\install\module_base
 				continue;
 			}
 
+			if ($this->allow_progress_bar)
+			{
+				$this->install_config->increment_current_task_progress();
+			}
+
 			$test_result = $task->run();
 			$tests_passed = ($tests_passed) ? $test_result : false;
 		}
 
+		// Module finished, so clear task progress
+		$this->install_config->set_finished_task('');
+
 		// Check if tests have failed
 		if (!$tests_passed)
 		{
@@ -74,10 +91,6 @@ class module extends \phpbb\install\module_base
 			$this->iohandler->send_response();
 			throw new user_interaction_required_exception();
 		}
-
-		// Log install progress
-		$current_task_index = $task_index - 1;
-		$this->install_config->set_finished_task($this->task_collection[$current_task_index], $current_task_index);
 	}
 
 	/**

phpBB/phpbb/install/module_base.php

@@ -14,7 +14,6 @@
 namespace phpbb\install;
 
 use phpbb\di\ordered_service_collection;
-use phpbb\install\exception\invalid_service_name_exception;
 use phpbb\install\exception\resource_limit_reached_exception;
 use phpbb\install\helper\config;
 use phpbb\install\helper\iohandler\iohandler_interface;
@@ -53,6 +52,11 @@ abstract class module_base implements module_interface
 	 */
 	protected $task_collection;
 
+	/**
+	 * @var array
+	 */
+	protected $task_step_count;
+
 	/**
 	 * @var bool
 	 */
@@ -111,7 +115,7 @@ abstract class module_base implements module_interface
 	{
 		// Recover install progress
 		$task_name = $this->recover_progress();
-		$name_found = false;
+		$task_found = false;
 
 		/**
 		 * @var string							$name	ID of the service
@@ -126,15 +130,22 @@ abstract class module_base implements module_interface
 			}
 
 			// Skip forward until the next task is reached
-			if (!empty($task_name) && !$name_found)
+			if (!$task_found)
 			{
+				if ($name === $task_name || empty($task_name))
+				{
+					$task_found = true;
+
 					if ($name === $task_name)
 					{
-					$name_found = true;
-				}
-
 						continue;
 					}
+				}
+				else
+				{
+					continue;
+				}
+			}
 
 			// Send progress information
 			if ($this->allow_progress_bar)
@@ -153,18 +164,22 @@ abstract class module_base implements module_interface
 					$name,
 				));
 
-				$class_name = $this->get_class_from_service_name($name);
+				$this->install_config->increment_current_task_progress($this->task_step_count[$name]);
-				$this->install_config->increment_current_task_progress($class_name::get_step_count());
 				continue;
 			}
 
 			if ($this->allow_progress_bar)
 			{
+				// Only increment progress by one, as if a task has more than one steps
+				// then that should be incremented in the task itself
 				$this->install_config->increment_current_task_progress();
 			}
 
 			$task->run();
 
+			// Log install progress
+			$this->install_config->set_finished_task($name);
+
 			// Send progress information
 			if ($this->allow_progress_bar)
 			{
@@ -175,9 +190,6 @@ abstract class module_base implements module_interface
 			}
 
 			$this->iohandler->send_response();
-
-			// Log install progress
-			$this->install_config->set_finished_task($name);
 		}
 
 		// Module finished, so clear task progress
@@ -187,7 +199,7 @@ abstract class module_base implements module_interface
 	/**
 	 * Returns the next task's index
 	 *
-	 * @return int	index of the array element of the next task
+	 * @return string	index of the array element of the next task
 	 */
 	protected function recover_progress()
 	{
@@ -200,43 +212,16 @@ abstract class module_base implements module_interface
 	 */
 	public function get_step_count()
 	{
-		$step_count = 0;
+		$task_step_count = 0;
+		$task_class_names = $this->task_collection->get_service_classes();
 
-		/** @todo:	Fix this
+		foreach ($task_class_names as $name => $task_class)
-		foreach ($this->task_collection as $task_service_name)
 		{
-			$class_name = $this->get_class_from_service_name($task_service_name);
+			$step_count = $task_class::get_step_count();
-			$step_count += $class_name::get_step_count();
+			$task_step_count += $step_count;
-		}
+			$this->task_step_count[$name] = $step_count;
-		*/
-
-		return $step_count;
 		}
 
-	/**
+		return $task_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/phpbb/install/task_interface.php

@@ -15,10 +15,6 @@ namespace phpbb\install;
 
 /**
  * Interface for installer tasks
- *
- * Note: The task service ID must match up with the namespace and class name.
- * For example: if your task is located at \phpbb\install\module\module_name\task\task_name
- * then the service ID must be installer.module_name.task_name.
  */
 interface task_interface
 {