fix: #3536795 "Only file JavaScript/CSS assets can be optimized" errors in logs (82cfe784) · Commits · project / drupal

core/modules/system/src/Controller/AssetControllerBase.php

+9 −0

Original line number	Diff line number	Diff line
		@@ -179,6 +179,15 @@ public function deliver(Request $request, string $file_name) {
		$groups = $this->getGroups($attached_assets, $request);

		$group = $this->getGroup($groups, $request->query->get('delta'));

		// External assets, and local assets with preprocessing disabled, never
		// have an aggregate. Reject them before the hash comparison below, which
		// redirects a mismatch to the same group with a valid hash, so the retry
		// would reach the optimizer and throw.
		if ($group['type'] !== 'file' \|\| $group['preprocess'] === FALSE) {
		throw new BadRequestHttpException('The requested asset group is not aggregated.');
		}

		// Generate a hash based on the asset group, this uses the same method as
		// the collection optimizer does to create the filename, so it should match.
		$generated_hash = $this->generateHash($group);

core/modules/system/tests/modules/common_test/common_test.libraries.yml

+8 −0

Original line number	Diff line number	Diff line
		@@ -11,6 +11,14 @@ files:
		theme:
		bar.css: {}

		# Library to test CSS and JS file assets excluded from aggregation.
		no-preprocess:
		js:
		no_preprocess.js: { preprocess: false }
		css:
		theme:
		no_preprocess.css: { preprocess: false }

		# Library to test external CSS and JS file assets.
		external:
		version: 1

core/tests/Drupal/FunctionalTests/Asset/AssetOptimizationTest.php

+27 −9

Original line number	Diff line number	Diff line
		@@ -110,12 +110,20 @@ protected function doTestAggregation(array $settings): void {
		// routing, we can force the request to be served by Drupal.
		$this->assertAggregate(str_replace($this->fileAssetsPath, strtoupper($this->fileAssetsPath), $url), TRUE, 'text/css');
		$this->assertAggregate($url, FALSE, 'text/css');
		$this->assertInvalidAggregates($url);
		}

		foreach ($script_urls as $url) {
		$this->assertAggregate($url);
		$this->assertAggregate($url, FALSE);
		}

		// Check manipulating asset URL parameters.
		\Drupal::service('file_system')->deleteRecursive($this->fileAssetsPath);
		foreach ($style_urls as $url) {
		$this->assertInvalidAggregates($url);
		}

		foreach ($script_urls as $url) {
		$this->assertInvalidAggregates($url);
		}
		}
		@@ -166,8 +174,8 @@ protected function assertInvalidAggregates(string $url): void {
		return;
		}
		$session = $this->getSession();
		$session->visit($this->replaceGroupDelta($url));
		$this->assertSession()->statusCodeEquals(200);
		$session->visit($this->replaceGroupDelta($url, 100));
		$this->assertSession()->statusCodeEquals(400);

		$session->visit($this->omitTheme($url));
		$this->assertSession()->statusCodeEquals(400);
		@@ -184,7 +192,11 @@ protected function assertInvalidAggregates(string $url): void {
		$session->visit($this->replaceFileNamePrefix($url));
		$this->assertSession()->statusCodeEquals(400);

		$session->visit($this->setInvalidLibrary($url));
		// Library name must include at least one slash.
		$session->visit($this->setInvalidLibrary($url, 'abcdefghijklmnop'));
		$this->assertSession()->statusCodeEquals(400);

		$session->visit($this->setInvalidLibrary($url, 'system/llama'));
		$this->assertSession()->statusCodeEquals(200);

		// When an invalid asset hash name is given.
		@@ -200,13 +212,17 @@ protected function assertInvalidAggregates(string $url): void {
		*
		* @param string $url
		* The source URL.
		* @param int $delta
		* The delta to apply.
		*
		* @return string
		* The URL with the delta replaced.
		*/
		protected function replaceGroupDelta(string $url): string {
		protected function replaceGroupDelta(string $url, int $delta): string {
		// First replace the hash, so we don't get served the actual file on disk.
		$url = $this->replaceGroupHash($url);
		$parts = UrlHelper::parse($url);
		$parts['query']['delta'] = 100;
		$parts['query']['delta'] = $delta;
		$query = UrlHelper::buildQuery($parts['query']);
		return $this->getAbsoluteUrl($parts['path'] . '?' . $query . '#' . $parts['fragment']);
		}
		@@ -223,7 +239,7 @@ protected function replaceGroupDelta(string $url): string {
		protected function replaceGroupHash(string $url): string {
		$parts = explode('_', $url, 2);
		$hash = strtok($parts[1], '.');
		$parts[1] = str_replace($hash, 'abcdefghijklmnop', $parts[1]);
		$parts[1] = str_replace($hash, $this->randomMachineName(), $parts[1]);
		return $this->getAbsoluteUrl(implode('_', $parts));
		}

		@@ -245,16 +261,18 @@ protected function replaceFileNamePrefix(string $url): string {
		*
		* @param string $url
		* The source URL.
		* @param string $library
		* The library to add.
		*
		* @return string
		* The URL with the 'include' query set to an invalid value.
		*/
		protected function setInvalidLibrary(string $url): string {
		protected function setInvalidLibrary(string $url, string $library): string {
		// First replace the hash, so we don't get served the actual file on disk.
		$url = $this->replaceGroupHash($url);
		$parts = UrlHelper::parse($url);
		$include = explode(',', UrlHelper::uncompressQueryParameter($parts['query']['include']));
		$include[] = 'system/llama';
		$include[] = $library;
		$parts['query']['include'] = UrlHelper::compressQueryParameter(implode(',', $include));

		$query = UrlHelper::buildQuery($parts['query']);

core/tests/Drupal/KernelTests/Core/Asset/AssetControllerTest.php

0 → 100644

+209 −0

Original line number	Diff line number	Diff line
		<?php

		declare(strict_types=1);

		namespace Drupal\KernelTests\Core\Asset;

		use Drupal\Component\Utility\UrlHelper;
		use Drupal\Core\Asset\AssetResolverInterface;
		use Drupal\Core\Asset\AttachedAssets;
		use Drupal\KernelTests\KernelTestBase;
		use Drupal\system\Controller\AssetControllerBase;
		use Drupal\system\Controller\CssAssetController;
		use Drupal\system\Controller\JsAssetController;
		use Symfony\Component\HttpFoundation\Request;
		use Symfony\Component\HttpKernel\Exception\BadRequestHttpException;

		/**
		* Tests that the asset controllers reject invalid aggregate requests.
		*
		* @covers \Drupal\system\Controller\AssetControllerBase::deliver
		* @group asset
		* @runTestsInSeparateProcesses
		*/
		class AssetControllerTest extends KernelTestBase {

		/**
		* {@inheritdoc}
		*/
		protected static $modules = ['common_test', 'system'];

		/**
		* The asset resolver service.
		*/
		protected AssetResolverInterface $assetResolver;

		/**
		* {@inheritdoc}
		*/
		protected function setUp(): void {
		parent::setUp();

		$this->assetResolver = $this->container->get('asset.resolver');
		}

		/**
		* Tests that deliver() throws BadRequestHttpException for invalid requests.
		*/
		public function testDeliverBadRequests(): void {
		$language = \Drupal::languageManager()->getCurrentLanguage();
		$controller = CssAssetController::create($this->container);
		$hash = $this->randomMachineName();
		// Valid base params for assertions that must pass the early checks.
		$valid_params = [
		'delta' => 0,
		'language' => $language->getId(),
		'theme' => 'stark',
		'include' => UrlHelper::compressQueryParameter('common_test/external'),
		];

		// Missing theme.
		$this->assertDeliverThrows(
		$controller,
		Request::create('/dummy', 'GET', array_diff_key($valid_params, ['theme' => ''])),
		'css_' . $hash . '.css',
		'The theme must be passed as a query argument',
		);

		// Missing delta.
		$this->assertDeliverThrows(
		$controller,
		Request::create('/dummy', 'GET', array_diff_key($valid_params, ['delta' => ''])),
		'css_' . $hash . '.css',
		'The numeric delta must be passed as a query argument',
		);

		// Non-numeric delta.
		$this->assertDeliverThrows(
		$controller,
		Request::create('/dummy', 'GET', array_merge($valid_params, ['delta' => 'nan'])),
		'css_' . $hash . '.css',
		'The numeric delta must be passed as a query argument',
		);

		// Missing language.
		$this->assertDeliverThrows(
		$controller,
		Request::create('/dummy', 'GET', array_diff_key($valid_params, ['language' => ''])),
		'css_' . $hash . '.css',
		'The language must be passed as a query argument',
		);

		// Missing include.
		$this->assertDeliverThrows(
		$controller,
		Request::create('/dummy', 'GET', array_diff_key($valid_params, ['include' => ''])),
		'css_' . $hash . '.css',
		'The libraries to include must be passed as a query argument',
		);

		// Wrong filename prefix (JS prefix for a CSS controller).
		$this->assertDeliverThrows(
		$controller,
		Request::create('/dummy', 'GET', $valid_params),
		'js_' . $hash . '.css',
		'The filename prefix must match the file extension',
		);

		// No hash segment in filename.
		$this->assertDeliverThrows(
		$controller,
		Request::create('/dummy', 'GET', $valid_params),
		'css.css',
		'Invalid filename',
		);

		// Library name without a slash in include.
		$this->assertDeliverThrows(
		$controller,
		Request::create('/dummy', 'GET', array_merge($valid_params, [
		'include' => UrlHelper::compressQueryParameter('noslash'),
		])),
		'css_' . $hash . '.css',
		'The "noslash" library name must include at least one slash.',
		);

		// Library name without a slash in exclude.
		$this->assertDeliverThrows(
		$controller,
		Request::create('/dummy', 'GET', array_merge($valid_params, [
		'exclude' => UrlHelper::compressQueryParameter('noslash'),
		])),
		'css_' . $hash . '.css',
		'The "noslash" library name must include at least one slash.',
		);

		// Out-of-bounds delta.
		$this->assertDeliverThrows(
		$controller,
		Request::create('/dummy', 'GET', array_merge($valid_params, ['delta' => 999])),
		'css_' . $hash . '.css',
		'Invalid filename.',
		);

		// External group delta: requesting an aggregate for a non-file group.
		$css_assets = $this->assetResolver->getCssAssets(AttachedAssets::createFromRenderArray(['#attached' => ['library' => ['common_test/external']]]), FALSE, $language);
		$groups = $this->container->get('asset.css.collection_grouper')->group($css_assets);
		$external_groups = array_filter($groups, static fn($g) => $g['type'] === 'external');
		$this->assertNotEmpty($external_groups, 'common_test/external must contain an external CSS group.');
		$external_delta = array_key_first($external_groups);

		$this->assertDeliverThrows(
		$controller,
		Request::create('/dummy', 'GET', array_merge($valid_params, ['delta' => $external_delta])),
		'css_' . $hash . '.css',
		'The requested asset group is not aggregated.',
		);

		// Non-aggregated group delta: requesting an aggregate for a file group that
		// has preprocess disabled.
		$css_assets = $this->assetResolver->getCssAssets(AttachedAssets::createFromRenderArray(['#attached' => ['library' => ['common_test/no-preprocess']]]), FALSE, $language);
		$groups = $this->container->get('asset.css.collection_grouper')->group($css_assets);
		$no_preprocess_groups = array_filter($groups, static fn($g) => $g['type'] === 'file' && $g['preprocess'] === FALSE);
		$this->assertNotEmpty($no_preprocess_groups, 'common_test/no-preprocess must contain a CSS group with preprocess disabled.');
		$no_preprocess_delta = array_key_first($no_preprocess_groups);

		$this->assertDeliverThrows(
		$controller,
		Request::create('/dummy', 'GET', array_merge($valid_params, [
		'delta' => $no_preprocess_delta,
		'include' => UrlHelper::compressQueryParameter('common_test/no-preprocess'),
		])),
		'css_' . $hash . '.css',
		'The requested asset group is not aggregated.',
		);

		// Missing scope; JS-specific, since JsAssetController::getGroups() needs
		// it to distinguish header from footer assets.
		$js_controller = JsAssetController::create($this->container);
		$this->assertDeliverThrows(
		$js_controller,
		Request::create('/dummy', 'GET', $valid_params),
		'js_' . $hash . '.js',
		'The URL must have a scope query argument.',
		);
		}

		/**
		* Asserts that deliver() throws a specific BadRequestHttpException.
		*
		* @param \Drupal\system\Controller\AssetControllerBase $controller
		* The asset controller to call.
		* @param \Symfony\Component\HttpFoundation\Request $request
		* The request to deliver.
		* @param string $filename
		* The aggregate filename to request.
		* @param string $expected_message
		* The expected exception message.
		*/
		private function assertDeliverThrows(AssetControllerBase $controller, Request $request, string $filename, string $expected_message): void {
		try {
		$controller->deliver($request, $filename);
		$this->fail("Expected BadRequestHttpException: $expected_message");
		}
		catch (BadRequestHttpException $e) {
		$this->assertSame($expected_message, $e->getMessage());
		}
		}

		}