From 0a49fc471aee947d2a5392a0f486296f717c3741 Mon Sep 17 00:00:00 2001 From: Dionisio Fernandez <52870-dioni@users.noreply.drupalcode.org> Date: Mon, 16 Oct 2023 14:13:57 +0200 Subject: [PATCH 1/7] Issue #3373123: Setting 'Enforce clean and canonical URLs.' breaks CSS aggregation on multilingual Drupal 10.1.x with browser caching enabled --- src/RedirectChecker.php | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/RedirectChecker.php b/src/RedirectChecker.php index d1aa829..68c608d 100644 --- a/src/RedirectChecker.php +++ b/src/RedirectChecker.php @@ -61,6 +61,8 @@ class RedirectChecker { */ public function canRedirect(Request $request, $route_name = NULL) { $can_redirect = TRUE; + $request_uri = $request->getRequestUri(); + $assets_path = \Drupal::service('stream_wrapper_manager')->getViaUri('assets://'); if (isset($route_name)) { $route = $this->routeProvider->getRouteByName($route_name); if ($this->config->get('access_check')) { @@ -91,6 +93,9 @@ class RedirectChecker { // Do not redirect on admin paths. $can_redirect &= !(bool) $route->getOption('_admin_route'); } + elseif ($assets_path && preg_match('/^\/' . preg_quote($assets_path->basePath(), '/') . '\/(css|js)\/.*/', $request_uri)) { + $can_redirect = FALSE; + } return $can_redirect; } -- GitLab From 3decc90b1bff75a98701693dc30e14d02f5563de Mon Sep 17 00:00:00 2001 From: Marco Primitivo <5509-Bladedu@users.noreply.drupalcode.org> Date: Mon, 16 Oct 2023 14:15:32 +0200 Subject: [PATCH 2/7] Issue #3373123: Setting 'Enforce clean and canonical URLs.' breaks CSS aggregation on multilingual Drupal 10.1.x with browser caching enabled --- redirect.services.yml | 10 +++++----- src/RedirectChecker.php | 20 +++++++++++++++++--- 2 files changed, 22 insertions(+), 8 deletions(-) diff --git a/redirect.services.yml b/redirect.services.yml index 578acb0..f412154 100644 --- a/redirect.services.yml +++ b/redirect.services.yml @@ -8,17 +8,17 @@ services: - { name: backend_overridable } redirect.checker: class: Drupal\redirect\RedirectChecker - arguments: ['@config.factory', '@state', '@access_manager', '@current_user', '@router.route_provider'] + arguments: ['@config.factory', '@state', '@access_manager', '@current_user', '@router.route_provider', '@stream_wrapper_manager'] redirect.request_subscriber: class: Drupal\redirect\EventSubscriber\RedirectRequestSubscriber arguments: ['@redirect.repository', '@language_manager', '@config.factory', '@path_alias.manager', '@module_handler', '@entity_type.manager', '@redirect.checker', '@router.request_context', '@path_processor_manager'] tags: - { name: event_subscriber } redirect.settings_cache_tag: - class: Drupal\redirect\EventSubscriber\RedirectSettingsCacheTag - arguments: ['@cache_tags.invalidator'] - tags: - - { name: event_subscriber } + class: Drupal\redirect\EventSubscriber\RedirectSettingsCacheTag + arguments: ['@cache_tags.invalidator'] + tags: + - { name: event_subscriber } redirect.route_normalizer_request_subscriber: class: Drupal\redirect\EventSubscriber\RouteNormalizerRequestSubscriber arguments: ['@url_generator', '@path.matcher', '@config.factory', '@redirect.checker'] diff --git a/src/RedirectChecker.php b/src/RedirectChecker.php index 68c608d..6b9f367 100644 --- a/src/RedirectChecker.php +++ b/src/RedirectChecker.php @@ -8,6 +8,7 @@ use Drupal\Core\Session\AccountInterface; use Drupal\Core\State\StateInterface; use Drupal\Core\Routing\RouteObjectInterface; use Drupal\Core\Routing\RouteProviderInterface; +use Drupal\Core\StreamWrapper\StreamWrapperManagerInterface; use Symfony\Component\HttpFoundation\Request; /** @@ -40,12 +41,25 @@ class RedirectChecker { */ protected $routeProvider; - public function __construct(ConfigFactoryInterface $config, StateInterface $state, AccessManager $access_manager, AccountInterface $account, RouteProviderInterface $route_provider) { + /** + * @var \Drupal\Core\StreamWrapper\StreamWrapperManagerInterface + */ + private StreamWrapperManagerInterface $streamWrapperManager; + + public function __construct( + ConfigFactoryInterface $config, + StateInterface $state, + AccessManager $access_manager, + AccountInterface $account, + RouteProviderInterface $route_provider, + StreamWrapperManagerInterface $stream_wrapper_manager + ) { $this->config = $config->get('redirect.settings'); $this->accessManager = $access_manager; $this->state = $state; $this->account = $account; $this->routeProvider = $route_provider; + $this->streamWrapperManager = $stream_wrapper_manager; } /** @@ -62,7 +76,7 @@ class RedirectChecker { public function canRedirect(Request $request, $route_name = NULL) { $can_redirect = TRUE; $request_uri = $request->getRequestUri(); - $assets_path = \Drupal::service('stream_wrapper_manager')->getViaUri('assets://'); + $assetsStreamWrapper = $this->streamWrapperManager->getViaUri('assets://'); if (isset($route_name)) { $route = $this->routeProvider->getRouteByName($route_name); if ($this->config->get('access_check')) { @@ -93,7 +107,7 @@ class RedirectChecker { // Do not redirect on admin paths. $can_redirect &= !(bool) $route->getOption('_admin_route'); } - elseif ($assets_path && preg_match('/^\/' . preg_quote($assets_path->basePath(), '/') . '\/(css|js)\/.*/', $request_uri)) { + elseif ($assetsStreamWrapper && preg_match('/^\/' . preg_quote($assetsStreamWrapper->getDirectoryPath(), '/') . '\/(css|js)\/.*/', $request_uri)) { $can_redirect = FALSE; } -- GitLab From 8643b21e4fc800f5f319083770c16c4bd82c04ef Mon Sep 17 00:00:00 2001 From: Marco Primitivo <5509-Bladedu@users.noreply.drupalcode.org> Date: Mon, 16 Oct 2023 14:25:14 +0200 Subject: [PATCH 3/7] Issue #3373123: Setting 'Enforce clean and canonical URLs.' breaks CSS aggregation on multilingual Drupal 10.1.x with browser caching enabled --- src/RedirectChecker.php | 2 +- tests/src/Unit/RedirectCheckerTest.php | 73 +++++++++++++++++--------- 2 files changed, 49 insertions(+), 26 deletions(-) diff --git a/src/RedirectChecker.php b/src/RedirectChecker.php index 6b9f367..313cf14 100644 --- a/src/RedirectChecker.php +++ b/src/RedirectChecker.php @@ -76,7 +76,7 @@ class RedirectChecker { public function canRedirect(Request $request, $route_name = NULL) { $can_redirect = TRUE; $request_uri = $request->getRequestUri(); - $assetsStreamWrapper = $this->streamWrapperManager->getViaUri('assets://'); + $assetsStreamWrapper = $this->streamWrapperManager->getViaScheme('assets'); if (isset($route_name)) { $route = $this->routeProvider->getRouteByName($route_name); if ($this->config->get('access_check')) { diff --git a/tests/src/Unit/RedirectCheckerTest.php b/tests/src/Unit/RedirectCheckerTest.php index 69355fd..da83deb 100644 --- a/tests/src/Unit/RedirectCheckerTest.php +++ b/tests/src/Unit/RedirectCheckerTest.php @@ -19,7 +19,12 @@ class RedirectCheckerTest extends UnitTestCase { */ public function testCanRedirect() { - $config = ['redirect.settings' => ['ignore_admin_path' => FALSE, 'access_check' => TRUE]]; + $config = [ + 'redirect.settings' => [ + 'ignore_admin_path' => FALSE, + 'access_check' => TRUE, + ], + ]; $state = $this->createMock('Drupal\Core\State\StateInterface'); $state->expects($this->any()) @@ -30,6 +35,7 @@ class RedirectCheckerTest extends UnitTestCase { $account = $this->createMock('Drupal\Core\Session\AccountInterface'); $route_provider = $this->createMock('Drupal\Core\Routing\RouteProviderInterface'); + $stream_wrapper_manager = $this->createMock('Drupal\Core\StreamWrapper\StreamWrapperManagerInterface'); $route = new Route('/example'); $route_provider->expects($this->any()) ->method('getRouteByName') @@ -42,7 +48,7 @@ class RedirectCheckerTest extends UnitTestCase { ['allowed_route', [], $account, FALSE, TRUE], ]); - $checker = new RedirectChecker($this->getConfigFactoryStub($config), $state, $access, $account, $route_provider); + $checker = new RedirectChecker($this->getConfigFactoryStub($config), $state, $access, $account, $route_provider, $stream_wrapper_manager); // All fine - we can redirect. $request = $this->getRequestStub('index.php', 'GET'); @@ -76,7 +82,7 @@ class RedirectCheckerTest extends UnitTestCase { ->with('system.maintenance_mode') ->will($this->returnValue(TRUE)); - $checker = new RedirectChecker($this->getConfigFactoryStub($config), $state, $access, $account, $route_provider); + $checker = new RedirectChecker($this->getConfigFactoryStub($config), $state, $access, $account, $route_provider, $stream_wrapper_manager); $request = $this->getRequestStub('index.php', 'GET'); $this->assertFalse($checker->canRedirect($request), 'Cannot redirect if maintenance mode is on'); @@ -88,7 +94,7 @@ class RedirectCheckerTest extends UnitTestCase { ->with('access site in maintenance mode') ->will($this->returnValue(TRUE)); - $checker = new RedirectChecker($this->getConfigFactoryStub($config), $state, $access, $accountWithMaintenanceModeAccess, $route_provider); + $checker = new RedirectChecker($this->getConfigFactoryStub($config), $state, $access, $accountWithMaintenanceModeAccess, $route_provider, $stream_wrapper_manager); $request = $this->getRequestStub('index.php', 'GET'); $this->assertTrue($checker->canRedirect($request), 'Redirect should have worked, user has maintenance mode access.'); @@ -100,27 +106,44 @@ class RedirectCheckerTest extends UnitTestCase { ->with('system.maintenance_mode') ->will($this->returnValue(FALSE)); -// $checker = new RedirectChecker($this->getConfigFactoryStub($config), $state); -// -// $route = $this->getMockBuilder('Symfony\Component\Routing\Route') -// ->disableOriginalConstructor() -// ->getMock(); -// $route->expects($this->any()) -// ->method('getOption') -// ->with('_admin_route') -// ->will($this->returnValue('system.admin_config_search')); -// -// $request = $this->getRequestStub('index.php', 'GET', -// array(RouteObjectInterface::ROUTE_OBJECT => $route)); -// $this->assertFalse($checker->canRedirect($request), 'Cannot redirect if we are requesting a admin path'); -// -// // We are at admin path with ignore_admin_path set to TRUE. -// $config['redirect.settings']['ignore_admin_path'] = TRUE; -// $checker = new RedirectChecker($this->getConfigFactoryStub($config), $state); -// -// $request = $this->getRequestStub('index.php', 'GET', -// array(RouteObjectInterface::ROUTE_OBJECT => $route)); -// $this->assertTrue($checker->canRedirect($request), 'Can redirect a admin with ignore_admin_path set to TRUE'); + $request = $this->getRequestStub('index.php', 'GET'); + $request->expects($this->any()) + ->method('getRequestUri') + ->willReturn('/sites/default/files/css/css_test.css'); + $assetStream = $this->createMock('Drupal\Core\StreamWrapper\LocalStream'); + $assetStream->expects($this->any()) + ->method('getDirectoryPath') + ->willReturn('sites/default/files'); + + $stream_wrapper_manager->expects($this->any()) + ->method('getViaScheme') + ->with('assets') + ->willReturn($assetStream); + $checker = new RedirectChecker($this->getConfigFactoryStub($config), $state, $access, $account, $route_provider, $stream_wrapper_manager); + $this->assertFalse($checker->canRedirect($request), 'Cannot redirect if a css asset file is requested'); + + + // $checker = new RedirectChecker($this->getConfigFactoryStub($config), $state); + // + // $route = $this->getMockBuilder('Symfony\Component\Routing\Route') + // ->disableOriginalConstructor() + // ->getMock(); + // $route->expects($this->any()) + // ->method('getOption') + // ->with('_admin_route') + // ->will($this->returnValue('system.admin_config_search')); + // + // $request = $this->getRequestStub('index.php', 'GET', + // array(RouteObjectInterface::ROUTE_OBJECT => $route)); + // $this->assertFalse($checker->canRedirect($request), 'Cannot redirect if we are requesting a admin path'); + // + // // We are at admin path with ignore_admin_path set to TRUE. + // $config['redirect.settings']['ignore_admin_path'] = TRUE; + // $checker = new RedirectChecker($this->getConfigFactoryStub($config), $state); + // + // $request = $this->getRequestStub('index.php', 'GET', + // array(RouteObjectInterface::ROUTE_OBJECT => $route)); + // $this->assertTrue($checker->canRedirect($request), 'Can redirect a admin with ignore_admin_path set to TRUE'); } /** -- GitLab From b18ed99b27883d94a04b639f73f7c123d969a050 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juanjo=20L=C3=B3pez?= <8603-juanjol@users.noreply.drupalcode.org> Date: Mon, 16 Oct 2023 14:26:21 +0200 Subject: [PATCH 4/7] Issue #3373123: Setting 'Enforce clean and canonical URLs.' breaks CSS aggregation on multilingual Drupal 10.1.x with browser caching enabled --- src/RedirectChecker.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/RedirectChecker.php b/src/RedirectChecker.php index 313cf14..98b4243 100644 --- a/src/RedirectChecker.php +++ b/src/RedirectChecker.php @@ -107,7 +107,7 @@ class RedirectChecker { // Do not redirect on admin paths. $can_redirect &= !(bool) $route->getOption('_admin_route'); } - elseif ($assetsStreamWrapper && preg_match('/^\/' . preg_quote($assetsStreamWrapper->getDirectoryPath(), '/') . '\/(css|js)\/.*/', $request_uri)) { + elseif ($assetsStreamWrapper && preg_match('/^\\' . $request->getBasePath() . '\/' . preg_quote($assetsStreamWrapper->getDirectoryPath(), '/') . '\/(css|js)\/.*/', $request_uri)) { $can_redirect = FALSE; } -- GitLab From f46f8560e9c6c292a98e3b8d3b01b9364ea5d779 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juanjo=20L=C3=B3pez?= <8603-juanjol@users.noreply.drupalcode.org> Date: Mon, 16 Oct 2023 16:00:33 +0200 Subject: [PATCH 5/7] Issue #3373123: fixing tests --- src/RedirectChecker.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/RedirectChecker.php b/src/RedirectChecker.php index 98b4243..e4f22c1 100644 --- a/src/RedirectChecker.php +++ b/src/RedirectChecker.php @@ -107,7 +107,7 @@ class RedirectChecker { // Do not redirect on admin paths. $can_redirect &= !(bool) $route->getOption('_admin_route'); } - elseif ($assetsStreamWrapper && preg_match('/^\\' . $request->getBasePath() . '\/' . preg_quote($assetsStreamWrapper->getDirectoryPath(), '/') . '\/(css|js)\/.*/', $request_uri)) { + elseif ($assetsStreamWrapper && preg_match('/^' . preg_quote($request->getBasePath(), '/') . '\/' . preg_quote($assetsStreamWrapper->getDirectoryPath(), '/') . '\/(css|js)\/.*/', $request_uri)) { $can_redirect = FALSE; } -- GitLab From dd5df39a7e9dc3ac3cf90eee621e909ae1125d64 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juanjo=20L=C3=B3pez?= <8603-juanjol@users.noreply.drupalcode.org> Date: Mon, 16 Oct 2023 17:12:36 +0200 Subject: [PATCH 6/7] Issue #3373123: Replaced request uri with request pathinfo --- src/RedirectChecker.php | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/RedirectChecker.php b/src/RedirectChecker.php index e4f22c1..cc7fd97 100644 --- a/src/RedirectChecker.php +++ b/src/RedirectChecker.php @@ -75,7 +75,8 @@ class RedirectChecker { */ public function canRedirect(Request $request, $route_name = NULL) { $can_redirect = TRUE; - $request_uri = $request->getRequestUri(); + $request_path_info = $request->getPathInfo(); + $assetsStreamWrapper = $this->streamWrapperManager->getViaScheme('assets'); if (isset($route_name)) { $route = $this->routeProvider->getRouteByName($route_name); @@ -107,7 +108,7 @@ class RedirectChecker { // Do not redirect on admin paths. $can_redirect &= !(bool) $route->getOption('_admin_route'); } - elseif ($assetsStreamWrapper && preg_match('/^' . preg_quote($request->getBasePath(), '/') . '\/' . preg_quote($assetsStreamWrapper->getDirectoryPath(), '/') . '\/(css|js)\/.*/', $request_uri)) { + elseif ($assetsStreamWrapper && preg_match('/^\/' . preg_quote($assetsStreamWrapper->getDirectoryPath(), '/') . '\/(css|js)\/.*/', $request_path_info)) { $can_redirect = FALSE; } -- GitLab From 2c31f968a43da78957bd9f8509af471e30664e06 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juanjo=20L=C3=B3pez?= <8603-juanjol@users.noreply.drupalcode.org> Date: Mon, 16 Oct 2023 17:35:09 +0200 Subject: [PATCH 7/7] Issue #3373123: Fixing tests --- tests/src/Unit/RedirectCheckerTest.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/src/Unit/RedirectCheckerTest.php b/tests/src/Unit/RedirectCheckerTest.php index da83deb..719e0c2 100644 --- a/tests/src/Unit/RedirectCheckerTest.php +++ b/tests/src/Unit/RedirectCheckerTest.php @@ -108,11 +108,11 @@ class RedirectCheckerTest extends UnitTestCase { $request = $this->getRequestStub('index.php', 'GET'); $request->expects($this->any()) - ->method('getRequestUri') + ->method('getPathInfo') ->willReturn('/sites/default/files/css/css_test.css'); $assetStream = $this->createMock('Drupal\Core\StreamWrapper\LocalStream'); $assetStream->expects($this->any()) - ->method('getDirectoryPath') + ->method('getDirectoryPath') ->willReturn('sites/default/files'); $stream_wrapper_manager->expects($this->any()) -- GitLab