[ticket/12597] Refactoring and test improving

Adding tests of return status
Refactoring code
Adding consistency in verbose mode

PHPBB3-12597

phpBB/language/en/acp/common.php

@@ -222,13 +222,13 @@ $lang = array_merge($lang, array(
 	'BACK'					=> 'Back',
 
 	'CLI_DESCRIPTION_CRON_RUN'		=> 'Runs all available cron tasks.',
-	'CLI_DESCRIPTION_CRON_RUN_ARGUMENT_1'	=> 'What task do you what to run?',
+	'CLI_DESCRIPTION_CRON_RUN_ARGUMENT_1'	=> 'Name of the task to be run',
-
 
 	'COLOUR_SWATCH'			=> 'Web-safe colour swatch',
 	'CONFIG_UPDATED'		=> 'Configuration updated successfully.',
 	'CRON_LOCK_ERROR'		=> 'Could not obtain cron lock.',
-	'CRON_NO_TASK'			=> 'No such cron task',
+	'CRON_NO_SUCH_TASK'		=> 'No such cron task',
+	'CRON_NO_TASK'			=> 'No task to be run',
 
 	'DEACTIVATE'				=> 'Deactivate',
 	'DIRECTORY_DOES_NOT_EXIST'	=> 'The entered path “%s” does not exist.',

phpBB/phpbb/console/command/cron/run.php

@@ -33,7 +33,7 @@ class run extends \phpbb\console\command\command
 	* Construct method
 	*
 	* @param \phpbb\cron\manager $cron_manager The cron manager containing
-	*							the cron tasks to be executed.
+	*		the cron tasks to be executed.
 	* @param \phpbb\lock\db $lock_db The lock for accessing database.
 	* @param \phobb\user $user The user object (used to get language information)
 	*/
@@ -68,14 +68,15 @@ class run extends \phpbb\console\command\command
 	* If the verbose option is specified, each start of a task is printed.
 	*		Otherwise there is no output.
 	* If an argument is given to the command, only the task whose name matches the 
-	*		argument will be started. If none exists, an error message is
+	*		argument will be started. If verbose option is specified,
-	*		printed and the exit status is set to 2. Verbose option does nothing in 
+	*		an info message containing the name of the task is printed.
-	*		this case.
+	* If no task matches the argument given, an error message is printed
+	*		and the exit status is set to 2.
 	*
-	* @param InputInterface $input The input stream used to get the argument
+	* @param InputInterface $input The input stream used to get the argument and verboe option.
 	* @param OutputInterface $output The output stream, used for printing verbose-mode and error information.
 	*
-	* @return int 0 if all is ok, 1 if a lock error occured and -1 if no task matching the argument was found
+	* @return int 0 if all is ok, 1 if a lock error occured and 2 if no task matching the argument was found.
 	*/
 	protected function execute(InputInterface $input, OutputInterface $output)
 	{
@@ -84,33 +85,11 @@ class run extends \phpbb\console\command\command
 			$task_name = $input->getArgument('name');
 			if ($task_name)
 			{
-				$task = $this->cron_manager->find_task($task_name);
+				return $this->run_one($input, $output, $task_name);
-				if ($task)
-				{
-					$task->run();
-					return 0;
-				}
-				else
-				{
-					$output->writeln('<error>' . $this->user->lang('CRON_NO_TASK') . '</error>');
-					return 2;
-				}
 			}
 			else
 			{
-				$run_tasks = $this->cron_manager->find_all_ready_tasks();
+				$this->run_all($input, $output);
-
-				foreach ($run_tasks as $task)
-				{
-					if ($input->getOption('verbose'))
-					{
-						$output->writeln($this->user->lang('RUNNING_TASK', $task->get_name()));
-					}
-
-					$task->run();
-				}
-				$this->lock_db->release();
-
 				return 0;
 			}
 		}
@@ -120,4 +99,71 @@ class run extends \phpbb\console\command\command
 			return 1;
 		}
 	}
+
+	/*
+	* Executes the command in the case when no argument was given.
+	*
+	* If verbose mode is set, an info message will be printed if their is no task to
+	*		be run, or else for each starting task.
+	*
+	* @see execute
+	* @param InputInterface $input The input stream used to get the argument and verboe option.
+	* @param OutputInterface $output The output stream, used for printing verbose-mode and error information.
+	* @return null
+	*/
+	private function run_all(InputInterface $input, OutputInterface $output)
+	{
+		$run_tasks = $this->cron_manager->find_all_ready_tasks();
+
+		if ($run_tasks) {
+			foreach ($run_tasks as $task)
+			{
+				if ($input->getOption('verbose'))
+				{
+					$output->writeln('<info>' . $this->user->lang('RUNNING_TASK', $task->get_name()) . '</info>');
+				}
+
+				$task->run();
+			}
+		}
+		else
+		{
+			$output->writeln('<info>' . $this->user->lang('CRON_NO_TASK') . '</info>');
+		}
+		$this->lock_db->release();
+	}
+
+	/*
+	* Executes the command in the case where an argument is given.
+	*
+	* If their is a task whose name matches the argument, it is run and 0 is returned.
+	*		and if verbose mode is set, print an info message with the name of the task.
+	* If there is no task matching $task_name, the function prints an error message
+	*		and returns with status 2.
+	*
+	* @see execute
+	* @param string $task_name The name of the task that should be run.
+	* @param InputInterface $input The input stream used to get the argument and verboe option.
+	* @param OutputInterface $output The output stream, used for printing verbose-mode and error information.
+	* @return int 0 if all is well, 2 if no task matches $task_name.
+	*/
+	private function run_one(InputInterface $input, OutputInterface $output, $task_name)
+	{
+		$task = $this->cron_manager->find_task($task_name);
+		if ($task)
+		{
+			if ($input->getOption('verbose'))
+			{
+				$output->writeln('<info>' . $this->user->lang('RUNNING_TASK', $task_nameœ) . '</info>');
+			}
+
+			$task->run();
+			return 0;
+		}
+		else
+		{
+			$output->writeln('<error>' . $this->user->lang('CRON_NO_SUCH_TASK') . '</error>');
+			return 2;
+		}
+	}
 }

tests/console/cron/run_test.php

@@ -56,56 +56,74 @@ class phpbb_console_command_cron_run_test extends phpbb_database_test_case
 	public function test_normal_use()
 	{
 		$command_tester = $this->get_command_tester();
-		$command_tester->execute(array('command' => $this->command_name));
+		$exit_status = $command_tester->execute(array('command' => $this->command_name));
 
 		$this->assertSame('', $command_tester->getDisplay());
 		$this->assertSame(true, $this->task->executed);
+		$this->assertSame(0, $exit_status);
 	}
 
 	public function test_verbose_mode()
 	{
 		$command_tester = $this->get_command_tester();
-		$command_tester->execute(array('command' => $this->command_name, '--verbose' => true));
+		$exit_status = $command_tester->execute(array('command' => $this->command_name, '--verbose' => true));
 
 		$this->assertContains('RUNNING_TASK', $command_tester->getDisplay());
 		$this->assertSame(true, $this->task->executed);
+		$this->assertSame(0, $exit_status);
 	}
 
 	public function test_error_lock()
 	{
 		$this->lock->acquire();
 		$command_tester = $this->get_command_tester();
-		$command_tester->execute(array('command' => $this->command_name));
+		$exit_status = $command_tester->execute(array('command' => $this->command_name));
 
 		$this->assertContains('CRON_LOCK_ERROR', $command_tester->getDisplay());
 		$this->assertSame(false, $this->task->executed);
+		$this->assertSame(1, $exit_status);
+	}
+
+	public function test_no_task()
+	{
+		$tasks = array(
+		);
+		$this->cron_manager = new \phpbb\cron\manager($tasks, $phpbb_root_path, $pathEx);
+		$command_tester = $this->get_command_tester();
+		$exit_status = $command_tester->execute(array('command' => $this->command_name));
+
+		$this->assertContains('CRON_NO_TASK', $command_tester->getDisplay());
+		$this->assertSame(0, $exit_status);
 	}
 
 	public function test_arg_valid()
 	{
 		$command_tester = $this->get_command_tester();
-		$command_tester->execute(array('command' => $this->command_name, 'name' => 'phpbb_cron_task_simple'));
+		$exit_status = $command_tester->execute(array('command' => $this->command_name, 'name' => 'phpbb_cron_task_simple'));
 
 		$this->assertSame('', $command_tester->getDisplay());
 		$this->assertSame(true, $this->task->executed);
+		$this->assertSame(0, $exit_status);
 	}
 
 	public function test_arg_invalid()
 	{
 		$command_tester = $this->get_command_tester();
-		$command_tester->execute(array('command' => $this->command_name, 'name' => 'foo'));
+		$exit_status = $command_tester->execute(array('command' => $this->command_name, 'name' => 'foo'));
 
-		$this->assertContains('CRON_NO_TASK', $command_tester->getDisplay());
+		$this->assertContains('CRON_NO_SUCH_TASK', $command_tester->getDisplay());
 		$this->assertSame(false, $this->task->executed);
+		$this->assertSame(2, $exit_status);
 	}
 
 	public function test_arg_valid_verbose()
 	{
 		$command_tester = $this->get_command_tester();
-		$command_tester->execute(array('command' => $this->command_name, 'name' => 'phpbb_cron_task_simple', '--verbose' => true));
+		$exit_status = $command_tester->execute(array('command' => $this->command_name, 'name' => 'phpbb_cron_task_simple', '--verbose' => true));
 
-		$this->assertSame('', $command_tester->getDisplay());
+		$this->assertContains('RUNNING_TASK', $command_tester->getDisplay());
 		$this->assertSame(true, $this->task->executed);
+		$this->assertSame(0, $exit_status);
 	}
 
 	public function get_command_tester()