Merge pull request #4232 from CHItA/ticket/14542

[ticket/14542] Move cron to controller

phpBB/config/default/container/services_cron.yml

@@ -3,6 +3,7 @@ services:
         class: phpbb\cron\manager
         arguments:
             - '@cron.task_collection'
+            - '@routing.helper'
             - '%core.root_path%'
             - '%core.php_ext%'
 
@@ -13,6 +14,18 @@ services:
             - '@config'
             - '@dbal.conn'
 
+    cron.controller:
+        class: phpbb\cron\controller\cron
+
+    cron.event_listener:
+        class: phpbb\cron\event\cron_runner_listener
+        arguments:
+            - '@cron.lock_db'
+            - '@cron.manager'
+            - '@request'
+        tags:
+            - { name: kernel.event_subscriber }
+
 # ----- Cron tasks -----
     cron.task_collection:
         class: phpbb\di\service_collection

phpBB/config/default/routing/cron.yml

@@ -0,0 +1,3 @@
+phpbb_cron_run:
+    path: /{cron_type}
+    defaults: { _controller: cron.controller:handle }

phpBB/config/default/routing/routing.yml

@@ -8,6 +8,10 @@
 # instantiate the 'foo_service' service and call the 'method' method.
 #
 
+phpbb_cron_routing:
+    resource: cron.yml
+    prefix: /cron
+
 phpbb_feed_routing:
     resource: feed.yml
     prefix: /feed

phpBB/cron.php

@@ -11,10 +11,11 @@
 *
 */
 
+use Symfony\Component\HttpFoundation\RedirectResponse;
+
 /**
 */
 define('IN_PHPBB', true);
-define('IN_CRON', true);
 $phpbb_root_path = (defined('PHPBB_ROOT_PATH')) ? PHPBB_ROOT_PATH : './';
 $phpEx = substr(strrchr(__FILE__, '.'), 1);
 include($phpbb_root_path . 'common.' . $phpEx);
@@ -23,62 +24,14 @@ include($phpbb_root_path . 'common.' . $phpEx);
 $user->session_begin(false);
 $auth->acl($user->data);
 
-function output_image()
-{
-	// Output transparent gif
-	header('Cache-Control: no-cache');
-	header('Content-type: image/gif');
-	header('Content-length: 43');
-
-	echo base64_decode('R0lGODlhAQABAIAAAP///wAAACH5BAEAAAAALAAAAAABAAEAAAICRAEAOw==');
-
-	// Flush here to prevent browser from showing the page as loading while
-	// running cron.
-	flush();
-}
-
-// Thanks to various fatal errors and lack of try/finally, it is quite easy to leave
-// the cron lock locked, especially when working on cron-related code.
-//
-// Attempt to alleviate the problem by doing setup outside of the lock as much as possible.
-
 $cron_type = $request->variable('cron_type', '');
 
-// Comment this line out for debugging so the page does not return an image.
+$get_params_array = $request->get_super_global(\phpbb\request\request_interface::GET);
-output_image();
 
-/* @var $cron_lock \phpbb\lock\db */
+/** @var \phpbb\controller\helper $controller_helper */
-$cron_lock = $phpbb_container->get('cron.lock_db');
+$controller_helper = $phpbb_container->get('controller.helper');
-if ($cron_lock->acquire())
+$response = new RedirectResponse(
-{
+	$controller_helper->route('phpbb_cron_run', $get_params_array),
-	/* @var $cron \phpbb\cron\manager */
+	301
-	$cron = $phpbb_container->get('cron.manager');
+);
-
+$response->send();
-	$task = $cron->find_task($cron_type);
-	if ($task)
-	{
-		/**
-		 * This event enables you to catch the task before it runs
-		 *
-		 * @event core.cron_run_before
-		 * @var	\phpbb\cron\task\wrapper	task	Current Cron task
-		 * @since 3.1.8-RC1
-		 */
-		$vars = array(
-			'task',
-		);
-		extract($phpbb_dispatcher->trigger_event('core.cron_run_before', compact($vars)));
-
-		if ($task->is_parametrized())
-		{
-			$task->parse_parameters($request);
-		}
-		if ($task->is_ready())
-		{
-			$task->run();
-		}
-	}
-	$cron_lock->release();
-}
-
-garbage_collection();

phpBB/phpbb/cron/controller/cron.php

@@ -0,0 +1,40 @@
+<?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\cron\controller;
+
+use Symfony\Component\HttpFoundation\Response;
+
+/**
+ * Controller for running cron jobs
+ */
+class cron
+{
+	/**
+	 * Handles CRON requests
+	 *
+	 * @param string $cron_type
+	 *
+	 * @return Response
+	 */
+	public function handle($cron_type)
+	{
+		$response = new Response();
+		$response->headers->set('Cache-Control', 'no-cache');
+		$response->headers->set('Content-type', 'image/gif');
+		$response->headers->set('Content-length', '43');
+		$response->setContent(base64_decode('R0lGODlhAQABAIAAAP///wAAACH5BAEAAAAALAAAAAABAAEAAAICRAEAOw=='));
+
+		return $response;
+	}
+}

phpBB/phpbb/cron/event/cron_runner_listener.php

@@ -0,0 +1,103 @@
+<?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\cron\event;
+
+use phpbb\cron\manager;
+use phpbb\lock\db;
+use phpbb\request\request_interface;
+use Symfony\Component\EventDispatcher\EventSubscriberInterface;
+use Symfony\Component\HttpKernel\KernelEvents;
+use Symfony\Component\HttpKernel\Event\PostResponseEvent;
+
+/**
+ * Event listener that executes cron tasks, after the response was served
+ */
+class cron_runner_listener implements EventSubscriberInterface
+{
+	/**
+	 * @var \phpbb\lock\db
+	 */
+	private $cron_lock;
+
+	/**
+	 * @var \phpbb\cron\manager
+	 */
+	private $cron_manager;
+
+	/**
+	 * @var \phpbb\request\request_interface
+	 */
+	private $request;
+
+	/**
+	 * Constructor
+	 *
+	 * @param db 				$lock
+	 * @param manager			$manager
+	 * @param request_interface	$request
+	 */
+	public function __construct(db $lock, manager $manager, request_interface $request)
+	{
+		$this->cron_lock	= $lock;
+		$this->cron_manager	= $manager;
+		$this->request		= $request;
+	}
+
+	/**
+	 * Runs the cron job after the response was sent
+	 *
+	 * @param PostResponseEvent	$event	The event
+	 */
+	public function on_kernel_terminate(PostResponseEvent $event)
+	{
+		$request = $event->getRequest();
+		$controller_name = $request->get('_route');
+
+		if ($controller_name !== 'phpbb_cron_run')
+		{
+			return;
+		}
+
+		$cron_type = $request->get('cron_type');
+
+		if ($this->cron_lock->acquire())
+		{
+			$task = $this->cron_manager->find_task($cron_type);
+			if ($task)
+			{
+				if ($task->is_parametrized())
+				{
+					$task->parse_parameters($this->request);
+				}
+
+				if ($task->is_ready())
+				{
+					$task->run();
+				}
+
+				$this->cron_lock->release();
+			}
+		}
+	}
+
+	/**
+	 * {@inheritdoc}
+	 */
+	static public function getSubscribedEvents()
+	{
+		return array(
+			KernelEvents::TERMINATE		=> 'on_kernel_terminate',
+		);
+	}
+}

phpBB/phpbb/cron/manager.php

@@ -13,6 +13,9 @@
 
 namespace phpbb\cron;
 
+use phpbb\cron\task\wrapper;
+use phpbb\routing\helper;
+
 /**
 * Cron manager class.
 *
@@ -20,6 +23,11 @@ namespace phpbb\cron;
 */
 class manager
 {
+	/**
+	 * @var helper
+	 */
+	protected $routing_helper;
+
 	/**
 	* Set of \phpbb\cron\task\wrapper objects.
 	* Array holding all tasks that have been found.
@@ -28,18 +36,27 @@ class manager
 	*/
 	protected $tasks = array();
 
+	/**
+	 * @var string
+	 */
 	protected $phpbb_root_path;
+
+	/**
+	 * @var string
+	 */
 	protected $php_ext;
 
 	/**
 	* Constructor. Loads all available tasks.
 	*
 	* @param array|\Traversable $tasks Provides an iterable set of task names
+	* @param helper $routing_helper Routing helper
 	* @param string $phpbb_root_path Relative path to phpBB root
 	* @param string $php_ext PHP file extension
 	*/
-	public function __construct($tasks, $phpbb_root_path, $php_ext)
+	public function __construct($tasks, helper $routing_helper, $phpbb_root_path, $php_ext)
 	{
+		$this->routing_helper = $routing_helper;
 		$this->phpbb_root_path = $phpbb_root_path;
 		$this->php_ext = $php_ext;
 
@@ -142,6 +159,6 @@ class manager
 	*/
 	public function wrap_task(\phpbb\cron\task\task $task)
 	{
-		return new \phpbb\cron\task\wrapper($task, $this->phpbb_root_path, $this->php_ext);
+		return new wrapper($task, $this->routing_helper, $this->phpbb_root_path, $this->php_ext);
 	}
 }

phpBB/phpbb/cron/task/wrapper.php

@@ -13,14 +13,32 @@
 
 namespace phpbb\cron\task;
 
+use phpbb\routing\helper;
+
 /**
 * Cron task wrapper class.
 * Enhances cron tasks with convenience methods that work identically for all tasks.
 */
 class wrapper
 {
+	/**
+	 * @var helper
+	 */
+	protected $routing_helper;
+
+	/**
+	 * @var task
+	 */
 	protected $task;
+
+	/**
+	 * @var string
+	 */
 	protected $phpbb_root_path;
+
+	/**
+	 * @var string
+	 */
 	protected $php_ext;
 
 	/**
@@ -28,13 +46,15 @@ class wrapper
 	*
 	* Wraps a task $task, which must implement cron_task interface.
 	*
-	* @param \phpbb\cron\task\task $task The cron task to wrap.
+	* @param task	$task				The cron task to wrap.
+	* @param helper	$routing_helper		Routing helper for route generation
 	* @param string	$phpbb_root_path	Relative path to phpBB root
 	* @param string	$php_ext			PHP file extension
 	*/
-	public function __construct(\phpbb\cron\task\task $task, $phpbb_root_path, $php_ext)
+	public function __construct(task $task, helper $routing_helper, $phpbb_root_path, $php_ext)
 	{
 		$this->task = $task;
+		$this->routing_helper = $routing_helper;
 		$this->phpbb_root_path = $phpbb_root_path;
 		$this->php_ext = $php_ext;
 	}
@@ -49,7 +69,7 @@ class wrapper
 	*/
 	public function is_parametrized()
 	{
-		return $this->task instanceof \phpbb\cron\task\parametrized;
+		return $this->task instanceof parametrized;
 	}
 
 	/**
@@ -76,22 +96,13 @@ class wrapper
 	*/
 	public function get_url()
 	{
-		$name = $this->get_name();
+		$params['cron_type'] = $this->get_name();
 		if ($this->is_parametrized())
 		{
-			$params = $this->task->get_parameters();
+			$params = array_merge($params, $this->task->get_parameters());
-			$extra = '';
-			foreach ($params as $key => $value)
-			{
-				$extra .= '&' . $key . '=' . urlencode($value);
 		}
-		}
+
-		else
+		return $this->routing_helper->route('phpbb_cron_run', $params);
-		{
-			$extra = '';
-		}
-		$url = append_sid($this->phpbb_root_path . 'cron.' . $this->php_ext, 'cron_type=' . $name . $extra);
-		return $url;
 	}
 
 	/**

tests/console/cron/cron_list_test.php

@@ -74,7 +74,35 @@ class phpbb_console_command_cron_list_test extends phpbb_test_case
 			$task->set_name('command' . $i);
 			$i++;
 		}
-		$this->cron_manager = new \phpbb\cron\manager($tasks, $phpbb_root_path, $pathEx);
+
+		$mock_config = new \phpbb\config\config(array(
+			'force_server_vars' => false,
+			'enable_mod_rewrite' => '',
+		));
+
+		$mock_router = $this->getMockBuilder('\phpbb\routing\router')
+			->setMethods(array('setContext', 'generate'))
+			->disableOriginalConstructor()
+			->getMock();
+		$mock_router->method('setContext')
+			->willReturn(true);
+		$mock_router->method('generate')
+			->willReturn('foobar');
+
+		$request = new \phpbb\request\request();
+		$request->enable_super_globals();
+
+		$routing_helper = new \phpbb\routing\helper(
+			$mock_config,
+			$mock_router,
+			new \phpbb\symfony_request($request),
+			$request,
+			new \phpbb\filesystem\filesystem(),
+			$phpbb_root_path,
+			$pathEx
+		);
+
+		$this->cron_manager = new \phpbb\cron\manager($tasks, $routing_helper, $phpbb_root_path, $pathEx);
 	}
 
 	public function get_command_tester()

tests/console/cron/run_test.php

@@ -50,7 +50,35 @@ class phpbb_console_command_cron_run_test extends phpbb_database_test_case
 		$tasks = array(
 			$this->task,
 		);
-		$this->cron_manager = new \phpbb\cron\manager($tasks, $phpbb_root_path, $phbEx);
+
+		$mock_config = new \phpbb\config\config(array(
+			'force_server_vars' => false,
+			'enable_mod_rewrite' => '',
+		));
+
+		$mock_router = $this->getMockBuilder('\phpbb\routing\router')
+			->setMethods(array('setContext', 'generate'))
+			->disableOriginalConstructor()
+			->getMock();
+		$mock_router->method('setContext')
+			->willReturn(true);
+		$mock_router->method('generate')
+			->willReturn('foobar');
+
+		$request = new \phpbb\request\request();
+		$request->enable_super_globals();
+
+		$routing_helper = new \phpbb\routing\helper(
+			$mock_config,
+			$mock_router,
+			new \phpbb\symfony_request($request),
+			$request,
+			new \phpbb\filesystem\filesystem(),
+			$phpbb_root_path,
+			$phpEx
+		);
+
+		$this->cron_manager = new \phpbb\cron\manager($tasks, $routing_helper, $phpbb_root_path, $phpEx);
 
 		$this->assertSame('0', $config['cron_lock']);
 	}
@@ -96,7 +124,35 @@ class phpbb_console_command_cron_run_test extends phpbb_database_test_case
 	{
 		$tasks = array(
 		);
-		$this->cron_manager = new \phpbb\cron\manager($tasks, $phpbb_root_path, $phpEx);
+
+		$mock_config = new \phpbb\config\config(array(
+			'force_server_vars' => false,
+			'enable_mod_rewrite' => '',
+		));
+
+		$mock_router = $this->getMockBuilder('\phpbb\routing\router')
+			->setMethods(array('setContext', 'generate'))
+			->disableOriginalConstructor()
+			->getMock();
+		$mock_router->method('setContext')
+			->willReturn(true);
+		$mock_router->method('generate')
+			->willReturn('foobar');
+
+		$request = new \phpbb\request\request();
+		$request->enable_super_globals();
+
+		$routing_helper = new \phpbb\routing\helper(
+			$mock_config,
+			$mock_router,
+			new \phpbb\symfony_request($request),
+			$request,
+			new \phpbb\filesystem\filesystem(),
+			$phpbb_root_path,
+			$phpEx
+		);
+
+		$this->cron_manager = new \phpbb\cron\manager($tasks, $routing_helper, $phpbb_root_path, $phpEx);
 		$command_tester = $this->get_command_tester();
 		$exit_status = $command_tester->execute(array('command' => $this->command_name));
 
@@ -109,7 +165,35 @@ class phpbb_console_command_cron_run_test extends phpbb_database_test_case
 	{
 		$tasks = array(
 		);
-		$this->cron_manager = new \phpbb\cron\manager($tasks, $phpbb_root_path, $phpEx);
+
+		$mock_config = new \phpbb\config\config(array(
+			'force_server_vars' => false,
+			'enable_mod_rewrite' => '',
+		));
+
+		$mock_router = $this->getMockBuilder('\phpbb\routing\router')
+			->setMethods(array('setContext', 'generate'))
+			->disableOriginalConstructor()
+			->getMock();
+		$mock_router->method('setContext')
+			->willReturn(true);
+		$mock_router->method('generate')
+			->willReturn('foobar');
+
+		$request = new \phpbb\request\request();
+		$request->enable_super_globals();
+
+		$routing_helper = new \phpbb\routing\helper(
+			$mock_config,
+			$mock_router,
+			new \phpbb\symfony_request($request),
+			$request,
+			new \phpbb\filesystem\filesystem(),
+			$phpbb_root_path,
+			$phpEx
+		);
+
+		$this->cron_manager = new \phpbb\cron\manager($tasks, $routing_helper, $phpbb_root_path, $phpEx);
 		$command_tester = $this->get_command_tester();
 		$exit_status = $command_tester->execute(array('command' => $this->command_name, '--verbose' => true));
 

tests/cron/manager_test.php

@@ -75,6 +75,33 @@ class phpbb_cron_manager_test extends \phpbb_test_case
 	{
 		global $phpbb_root_path, $phpEx;
 
-		return new \phpbb\cron\manager($tasks, $phpbb_root_path, $phpEx);
+		$mock_config = new \phpbb\config\config(array(
+			'force_server_vars' => false,
+			'enable_mod_rewrite' => '',
+		));
+
+		$mock_router = $this->getMockBuilder('\phpbb\routing\router')
+			->setMethods(array('setContext', 'generate'))
+			->disableOriginalConstructor()
+			->getMock();
+		$mock_router->method('setContext')
+			->willReturn(true);
+		$mock_router->method('generate')
+			->willReturn('foobar');
+
+		$request = new \phpbb\request\request();
+		$request->enable_super_globals();
+
+		$routing_helper = new \phpbb\routing\helper(
+			$mock_config,
+			$mock_router,
+			new \phpbb\symfony_request($request),
+			$request,
+			new \phpbb\filesystem\filesystem(),
+			$phpbb_root_path,
+			$phpEx
+		);
+
+		return new \phpbb\cron\manager($tasks, $routing_helper, $phpbb_root_path, $phpEx);
 	}
 }

tests/functional/controllers_compatibility_test.php

@@ -37,6 +37,13 @@ class phpbb_functional_controllers_compatibility_test extends phpbb_functional_t
 		$this->assert301('feed.php?t=1', 'app.php/feed/topic/1');
 	}
 
+	public function test_cron_compatibility()
+	{
+		$this->assert301('cron.php?cron_type=foo', 'app.php/cron/foo');
+		$this->assert301('cron.php?cron_type=foo&bar=foobar', 'app.php/cron/foo?bar=foobar');
+		$this->assert301('cron.php?cron_type=foo&bar=foobar&who=me', 'app.php/cron/foo?bar=foobar&who=me');
+	}
+
 	protected function assert301($from, $to)
 	{
 		self::$client->followRedirects(false);
@@ -44,6 +51,7 @@ class phpbb_functional_controllers_compatibility_test extends phpbb_functional_t
 
 		// Fix sid issues
 		$location = self::$client->getResponse()->getHeader('Location');
+		$location = str_replace('&', '&', $location);
 		$location = preg_replace('#sid=[^&]+(&(amp;)?)?#', '', $location);
 		if (substr($location, -1) === '?')
 		{

tests/functional/prune_shadow_topic_test.php

@@ -130,7 +130,16 @@ class phpbb_functional_prune_shadow_topic_test extends phpbb_functional_test_cas
 
 		$crawler = self::request('GET', "viewforum.php?f={$this->data['forums']['Prune Shadow']}&sid={$this->sid}");
 		$this->assertNotEmpty($crawler->filter('img')->last()->attr('src'));
-		self::request('GET', "cron.php?cron_type=cron.task.core.prune_shadow_topics&f={$this->data['forums']['Prune Shadow']}&sid={$this->sid}", array(), false);
+		self::request('GET', "app.php/cron/cron.task.core.prune_shadow_topics?f={$this->data['forums']['Prune Shadow']}&sid={$this->sid}", array(), false);
+
+		// Try to ensure that the cron can actually run before we start to wait for it
+		sleep(1);
+		$cron_lock = new \phpbb\lock\db('cron_lock', new \phpbb\config\db($this->db, new \phpbb\cache\driver\dummy(), 'phpbb_config'), $this->db);
+		while (!$cron_lock->acquire())
+		{
+			// do nothing
+		}
+ 		$cron_lock->release();
 
 		$this->assert_forum_details($this->data['forums']['Prune Shadow'], array(
 			'forum_posts_approved'		=> 0,