[ticket/16891] Do not change an extension status in the middle of a request

When enabling an extension, it should be considered as not being enabled for
the entire request as if the "enabled" flag is switch during the request, the
extension will stay not loaded (same when disabling an extension).

Example when it can be an issue today : if the router is called for the first
time after enabling the extension and if the extension uses a custom route
loader (like phpbb/pages) then the router will fail because the custom route
will be found but the custom loader will not be loaded and available.

PHPBB3-16891

phpBB/config/default/container/services.yml

@@ -116,6 +116,7 @@ services:
             - '@dbal.conn'
             - '@config'
             - '@filesystem'
+            - '@router'
             - '%tables.ext%'
             - '%core.root_path%'
             - '%core.php_ext%'

phpBB/phpbb/extension/manager.php

@@ -30,9 +30,11 @@ class manager
 	protected $cache;
 	protected $php_ext;
 	protected $extensions;
+	protected $recently_changed_ext_status;
 	protected $extension_table;
 	protected $phpbb_root_path;
 	protected $cache_name;
+	protected $router;
 
 	/**
 	* Creates a manager and loads information from database
@@ -47,7 +49,7 @@ class manager
 	* @param \phpbb\cache\service $cache A cache instance or null
 	* @param string $cache_name The name of the cache variable, defaults to _ext
 	*/
-	public function __construct(ContainerInterface $container, \phpbb\db\driver\driver_interface $db, \phpbb\config\config $config, \phpbb\filesystem\filesystem_interface $filesystem, $extension_table, $phpbb_root_path, $php_ext = 'php', \phpbb\cache\service $cache = null, $cache_name = '_ext')
+	public function __construct(ContainerInterface $container, \phpbb\db\driver\driver_interface $db, \phpbb\config\config $config, \phpbb\filesystem\filesystem_interface $filesystem, \phpbb\routing\router $router, $extension_table, $phpbb_root_path, $php_ext = 'php', \phpbb\cache\service $cache = null, $cache_name = '_ext')
 	{
 		$this->cache = $cache;
 		$this->cache_name = $cache_name;
@@ -56,6 +58,7 @@ class manager
 		$this->db = $db;
 		$this->extension_table = $extension_table;
 		$this->filesystem = $filesystem;
+		$this->router = $router;
 		$this->phpbb_root_path = $phpbb_root_path;
 		$this->php_ext = $php_ext;
 
@@ -238,6 +241,12 @@ class manager
 			'ext_state'		=> serialize($state),
 		);
 
+		if ($active)
+		{
+			$this->recently_changed_ext_status[$name] = false;
+			$this->router->without_cache();
+		}
+
 		$this->update_state($name, $extension_data, $this->is_configured($name) ? 'update' : 'insert');
 
 		if ($active)
@@ -287,6 +296,12 @@ class manager
 		$state = $extension->disable_step($old_state);
 		$active = ($state !== false);
 
+		if (!$active)
+		{
+			$this->recently_changed_ext_status[$name] = true;
+			$this->router->without_cache();
+		}
+
 		$extension_data = array(
 			'ext_active'	=> $active,
 			'ext_state'		=> serialize($state),
@@ -499,6 +514,13 @@ class manager
 	*/
 	public function is_enabled($name)
 	{
+		// The extension has just been enabled and so is not loaded. When asking if it is enabled or
+		// not we should answer no to stay consistent with the status at the beginning of the request.
+		if (isset($this->recently_changed_ext_status[$name]))
+		{
+			return $this->recently_changed_ext_status[$name];
+		}
+
 		return isset($this->extensions[$name]['ext_active']) && $this->extensions[$name]['ext_active'];
 	}
 
@@ -510,6 +532,13 @@ class manager
 	*/
 	public function is_disabled($name)
 	{
+		// The extension has just been disabled and so is still loaded. When asking if it is disabled or
+		// not we should answer yes to stay consistent with the status at the beginning of the request.
+		if (isset($this->recently_changed_ext_status[$name]))
+		{
+			return $this->recently_changed_ext_status[$name];
+		}
+
 		return isset($this->extensions[$name]['ext_active']) && !$this->extensions[$name]['ext_active'];
 	}
 

phpBB/phpbb/routing/router.php

@@ -80,6 +80,11 @@ class router implements RouterInterface
 	 */
 	protected $cache_dir;
 
+	/**
+	 * @var bool
+	 */
+	protected $use_cache;
+
 	/**
 	 * Construct method
 	 *
@@ -97,6 +102,7 @@ class router implements RouterInterface
 		$this->php_ext				= $php_ext;
 		$this->context				= new RequestContext();
 		$this->cache_dir			= $cache_dir;
+		$this->use_cache			= true;
 	}
 
 	/**
@@ -176,6 +182,22 @@ class router implements RouterInterface
 		return $this->get_matcher()->match($pathinfo);
 	}
 
+	/**
+	 * Enables the use of a cached URL generator and matcher
+	 */
+	public function with_cache()
+	{
+		$this->use_cache = true;
+	}
+
+	/**
+	 * Disables the use of a cached URL generator and matcher
+	 */
+	public function without_cache()
+	{
+		$this->use_cache = false;
+	}
+
 	/**
 	 * Gets the UrlMatcher instance associated with this Router.
 	 *
@@ -198,6 +220,12 @@ class router implements RouterInterface
 	 */
 	protected function create_dumped_url_matcher()
 	{
+		if (!$this->use_cache)
+		{
+			$this->create_new_url_matcher();
+			return;
+		}
+
 		try
 		{
 			$cache = new ConfigCache("{$this->cache_dir}url_matcher.{$this->php_ext}", defined('DEBUG'));
@@ -253,6 +281,12 @@ class router implements RouterInterface
 	 */
 	protected function create_dumped_url_generator()
 	{
+		if (!$this->use_cache)
+		{
+			$this->create_new_url_generator();
+			return;
+		}
+
 		try
 		{
 			$cache = new ConfigCache("{$this->cache_dir}url_generator.{$this->php_ext}", defined('DEBUG'));

tests/dbal/migrator_test.php

@@ -80,6 +80,7 @@ class phpbb_dbal_migrator_test extends phpbb_database_test_case
 			$this->db,
 			$this->config,
 			new phpbb\filesystem\filesystem(),
+			new phpbb_mock_dummy_router(),
 			'phpbb_ext',
 			__DIR__ . '/../../phpBB/',
 			'php',

tests/extension/manager_test.php

@@ -30,6 +30,7 @@ class phpbb_extension_manager_test extends phpbb_database_test_case
 	{
 		parent::setUp();
 
+		$this->db = null;
 		$this->extension_manager = $this->create_extension_manager();
 	}
 
@@ -95,7 +96,12 @@ class phpbb_extension_manager_test extends phpbb_database_test_case
 
 		$this->assertEquals(array('vendor2/foo'), array_keys($this->extension_manager->all_enabled()));
 		$this->extension_manager->enable('vendor2/bar');
-		$this->assertEquals(array('vendor2/bar', 'vendor2/foo'), array_keys($this->extension_manager->all_enabled()));
+
+		// We should not display the extension as being enabled in the same request
+		$this->assertEquals(array('vendor2/foo'), array_keys($this->extension_manager->all_enabled()));
+		// With a different request we should see the extension as being disabled
+		$this->assertEquals(array('vendor2/bar', 'vendor2/foo'), array_keys($this->create_extension_manager()->all_enabled()));
+
 		$this->assertEquals(array('vendor/moo', 'vendor2/bar', 'vendor2/foo'), array_keys($this->extension_manager->all_configured()));
 
 		$this->assertEquals(4, vendor2\bar\ext::$state);
@@ -119,7 +125,12 @@ class phpbb_extension_manager_test extends phpbb_database_test_case
 
 		$this->assertEquals(array('vendor2/foo'), array_keys($this->extension_manager->all_enabled()));
 		$this->extension_manager->disable('vendor2/foo');
-		$this->assertEquals(array(), array_keys($this->extension_manager->all_enabled()));
+
+		// We should still display the extension as being enabled in the current request
+		$this->assertEquals(array('vendor2/foo'), array_keys($this->extension_manager->all_enabled()));
+		// With a different request we should see the extension as being disabled
+		$this->assertEquals(array(), array_keys($this->create_extension_manager()->all_enabled()));
+
 		$this->assertEquals(array('vendor/moo', 'vendor2/foo'), array_keys($this->extension_manager->all_configured()));
 
 		$this->assertTrue(vendor2\foo\ext::$disabled);
@@ -147,7 +158,6 @@ class phpbb_extension_manager_test extends phpbb_database_test_case
 
 	protected function create_extension_manager($with_cache = true)
 	{
-
 		$config = new \phpbb\config\config(array('version' => PHPBB_VERSION));
 		$db = $this->new_dbal();
 		$factory = new \phpbb\db\tools\factory();
@@ -177,6 +187,7 @@ class phpbb_extension_manager_test extends phpbb_database_test_case
 			$db,
 			$config,
 			new \phpbb\filesystem\filesystem(),
+			new phpbb_mock_dummy_router(),
 			'phpbb_ext',
 			__DIR__ . '/',
 			$php_ext,

tests/extension/metadata_manager_test.php

@@ -98,6 +98,7 @@ class phpbb_extension_metadata_manager_test extends phpbb_database_test_case
 			$this->db,
 			$this->config,
 			new \phpbb\filesystem\filesystem(),
+			new phpbb_mock_dummy_router(),
 			'phpbb_ext',
 			$this->phpbb_root_path,
 			$this->phpEx,

tests/mock/dummy_router.php

@@ -0,0 +1,31 @@
+<?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.
+*
+*/
+
+class phpbb_mock_dummy_router extends phpbb_mock_router
+{
+	public function __construct()
+	{
+	}
+
+	public function get_matcher()
+	{
+		$this->create_new_url_matcher();
+		return parent::get_matcher();
+	}
+
+	public function get_generator()
+	{
+		$this->create_new_url_generator();
+		return parent::get_generator();
+	}
+}

tests/mock/extension_manager.php

@@ -11,6 +11,7 @@
 *
 */
 
+
 class phpbb_mock_extension_manager extends \phpbb\extension\manager
 {
 	public function __construct($phpbb_root_path, $extensions = array(), $container = null)
@@ -25,5 +26,6 @@ class phpbb_mock_extension_manager extends \phpbb\extension\manager
 		$this->container = $container;
 		$this->config = new \phpbb\config\config(array());
 		$this->user = new \phpbb\user($lang,'\phpbb\datetime');
+		$this->router = new phpbb_mock_dummy_router();
 	}
 }

tests/test_framework/phpbb_functional_test_case.php

@@ -261,6 +261,7 @@ class phpbb_functional_test_case extends phpbb_test_case
 			$db,
 			$config,
 			new phpbb\filesystem\filesystem(),
+			new phpbb_mock_dummy_router(),
 			self::$config['table_prefix'] . 'ext',
 			__DIR__ . '/',
 			$phpEx,