Lazy call router when replacing old backend paths (see #4091)

Description
-----------

In #3995 we noticed the router was called for every old backend path even if it did not exist in the replaced URL. See https://github.com/contao/contao/pull/3995/files#r795530334 for the discussion.

This PR changes the behaviour to use the callback only when necessary, happily covered with some unit tests 😎

Commits
-------

c2a82f3 Lazy call router when replacing old backend paths
e3ee58d Cache old backend url if method is called multiple times
12939c3 Reset controller before tests

core-bundle/src/Framework/ContaoFramework.php

@@ -14,6 +14,7 @@
 
 use Contao\ClassLoader;
 use Contao\Config;
+use Contao\Controller;
 use Contao\CoreBundle\Exception\RedirectResponseException;
 use Contao\CoreBundle\Routing\ScopeMatcher;
 use Contao\CoreBundle\Security\Authentication\Token\TokenChecker;
@@ -114,6 +115,7 @@ public function reset(): void
             return;
         }
 
+        Controller::reset();
         Environment::reset();
         Input::resetCache();
         Input::resetUnusedGet();

core-bundle/src/Resources/contao/library/Contao/Controller.php

@@ -54,6 +54,11 @@ abstract class Controller extends System
 	 */
 	protected static $arrQueryCache = array();
 
+	/**
+	 * @var array
+	 */
+	private static $arrOldBePathCache = array();
+
 	/**
 	 * Find a particular template file and return its path
 	 *
@@ -1210,27 +1215,36 @@ public static function redirect($strLocation, $intStatus=303)
 	 */
 	protected static function replaceOldBePaths($strContext)
 	{
-		$router = System::getContainer()->get('router');
+		$arrCache = &self::$arrOldBePathCache;
+		$arrMapper = array
+		(
+			'contao/confirm.php'   => 'contao_backend_confirm',
+			'contao/file.php'      => 'contao_backend_file',
+			'contao/help.php'      => 'contao_backend_help',
+			'contao/index.php'     => 'contao_backend_login',
+			'contao/main.php'      => 'contao_backend',
+			'contao/page.php'      => 'contao_backend_page',
+			'contao/password.php'  => 'contao_backend_password',
+			'contao/popup.php'     => 'contao_backend_popup',
+			'contao/preview.php'   => 'contao_backend_preview',
+		);
 
-		$generate = static function ($route) use ($router)
+		$replace = static function ($matches) use ($arrMapper, &$arrCache)
 		{
-			return substr($router->generate($route), \strlen(Environment::get('path')) + 1);
+			$key = $matches[0];
+
+			if (!isset($arrCache[$key]))
+			{
+				$router = System::getContainer()->get('router');
+				$arrCache[$key] = substr($router->generate($arrMapper[$key]), \strlen(Environment::get('path')) + 1);
+			}
+
+			return $arrCache[$key];
 		};
 
-		$arrMapper = array
-		(
-			'contao/confirm.php'   => $generate('contao_backend_confirm'),
-			'contao/file.php'      => $generate('contao_backend_file'),
-			'contao/help.php'      => $generate('contao_backend_help'),
-			'contao/index.php'     => $generate('contao_backend_login'),
-			'contao/main.php'      => $generate('contao_backend'),
-			'contao/page.php'      => $generate('contao_backend_page'),
-			'contao/password.php'  => $generate('contao_backend_password'),
-			'contao/popup.php'     => $generate('contao_backend_popup'),
-			'contao/preview.php'   => $generate('contao_backend_preview'),
-		);
+		$regex = '(' . implode('|', array_map('preg_quote', array_keys($arrMapper))) . ')';
 
-		return str_replace(array_keys($arrMapper), $arrMapper, $strContext);
+		return preg_replace_callback($regex, $replace, $strContext);
 	}
 
 	/**
@@ -1435,6 +1449,12 @@ public static function loadDataContainer($strTable, $blnNoCache=false)
 		$loader->load($blnNoCache);
 	}
 
+	public static function reset()
+	{
+		self::$arrQueryCache = array();
+		self::$arrOldBePathCache = array();
+	}
+
 	/**
 	 * Redirect to a front end page
 	 *

core-bundle/tests/Contao/ControllerTest.php

@@ -13,13 +13,24 @@
 namespace Contao\CoreBundle\Tests\Contao;
 
 use Contao\Controller;
+use Contao\CoreBundle\Exception\RedirectResponseException;
 use Contao\CoreBundle\Tests\TestCase;
+use Contao\Environment;
 use Contao\System;
+use Symfony\Component\HttpFoundation\RedirectResponse;
 use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\HttpFoundation\RequestStack;
+use Symfony\Component\Routing\RouterInterface;
 
 class ControllerTest extends TestCase
 {
+    protected function setUp(): void
+    {
+        parent::setUp();
+
+        Controller::reset();
+    }
+
     public function testReturnsTheTimeZones(): void
     {
         $timeZones = System::getTimeZones();
@@ -148,4 +159,118 @@ public function testAddToUrlWithQueryString(): void
         $this->assertSame('?do=page&id=4&act=edit&foo=%2B&bar=%20&ref=cri', Controller::addToUrl('act=edit&foo=%2B&bar=%20', false));
         $this->assertSame('?do=page&key=foo&ref=cri', Controller::addToUrl('key=foo', true, ['id']));
     }
+
+    /**
+     * @dataProvider redirectProvider
+     */
+    public function testReplacesOldBePathsInRedirect(string $location, array $routes, string $expected): void
+    {
+        $router = $this->createMock(RouterInterface::class);
+        $router
+            ->expects($this->exactly(\count($routes)))
+            ->method('generate')
+            ->withConsecutive(
+                ...array_map(
+                    static function ($route) {
+                        return [$route];
+                    },
+                    $routes
+                )
+            )
+            ->willReturnOnConsecutiveCalls(
+                ...array_map(
+                    static function ($route) {
+                        return '/'.$route;
+                    },
+                    $routes
+                )
+            )
+        ;
+
+        $container = $this->getContainerWithContaoConfiguration();
+        $container->set('router', $router);
+        System::setContainer($container);
+
+        Environment::reset();
+        Environment::set('path', '');
+        Environment::set('base', '');
+
+        try {
+            Controller::redirect($location);
+        } catch (RedirectResponseException $exception) {
+            /** @var RedirectResponse $response */
+            $response = $exception->getResponse();
+
+            $this->assertInstanceOf(RedirectResponse::class, $response);
+            $this->assertSame($expected, $response->getTargetUrl());
+        }
+    }
+
+    public function redirectProvider(): \Generator
+    {
+        yield 'Never calls the router without old backend path' => [
+            'https://example.com',
+            [],
+            'https://example.com',
+        ];
+
+        yield 'Replaces multiple paths (not really expected)' => [
+            'https://example.com/contao/main.php?contao/file.php=foo',
+            ['contao_backend', 'contao_backend_file'],
+            'https://example.com/contao_backend?contao_backend_file=foo',
+        ];
+
+        $pathMap = [
+            'contao/confirm.php' => 'contao_backend_confirm',
+            'contao/file.php' => 'contao_backend_file',
+            'contao/help.php' => 'contao_backend_help',
+            'contao/index.php' => 'contao_backend_login',
+            'contao/main.php' => 'contao_backend',
+            'contao/page.php' => 'contao_backend_page',
+            'contao/password.php' => 'contao_backend_password',
+            'contao/popup.php' => 'contao_backend_popup',
+            'contao/preview.php' => 'contao_backend_preview',
+        ];
+
+        foreach ($pathMap as $old => $new) {
+            yield 'Replaces '.$old.' with '.$new.' route' => [
+                "https://example.com/$old?foo=bar",
+                [$new],
+                "https://example.com/$new?foo=bar",
+            ];
+        }
+    }
+
+    public function testCachesOldBackendPaths(): void
+    {
+        $router = $this->createMock(RouterInterface::class);
+        $router
+            ->expects($this->exactly(2))
+            ->method('generate')
+            ->withConsecutive(['contao_backend'], ['contao_backend_file'])
+            ->willReturn('/contao', '/contao/file')
+        ;
+
+        $container = $this->getContainerWithContaoConfiguration();
+        $container->set('router', $router);
+        System::setContainer($container);
+
+        Environment::reset();
+        Environment::set('path', '');
+        Environment::set('base', '');
+
+        $ref = new \ReflectionClass(Controller::class);
+        $method = $ref->getMethod('replaceOldBePaths');
+        $method->setAccessible(true);
+
+        $this->assertSame(
+            $method->invoke(null, 'This is a template with link to <a href="/contao/main.php">backend main</a> and <a href="/contao/main.php?do=articles">articles</a>'),
+            'This is a template with link to <a href="/contao">backend main</a> and <a href="/contao?do=articles">articles</a>'
+        );
+
+        $this->assertSame(
+            $method->invoke(null, 'Link to <a href="/contao/main.php">backend main</a> and <a href="/contao/file.php?x=y">files</a>'),
+            'Link to <a href="/contao">backend main</a> and <a href="/contao/file?x=y">files</a>'
+        );
+    }
 }