- Issue created by @joelpittet
- Status changed to Needs review
over 1 year ago 12:16am 19 January 2024 - 🇨🇦Canada joelpittet Vancouver
Ideally it would be nicer to consider the include invalid cache and rebuild the asset or something...
- Status changed to Needs work
over 1 year ago 12:25am 19 January 2024 The Needs Review Queue Bot → tested this issue.
While you are making the above changes, we recommend that you convert this patch to a merge request → . Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)
- 🇨🇦Canada joelpittet Vancouver
@djsagar Why move it to 11.x-dev it's a bug, did the policy change or doesn't it get applied to the latest release's branch? I maybe missed a memo
- Status changed to Needs review
over 1 year ago 7:08pm 19 January 2024 - 🇨🇦Canada joelpittet Vancouver
Found the memo :) https://www.drupal.org/node/767608#d8-minor →
- Status changed to Needs work
over 1 year ago 7:33pm 21 January 2024 - 🇺🇸United States smustgrave
Thanks for reporting. Will need a test case showing the problem.
- 🇨🇦Canada joelpittet Vancouver
@smustgrave any hints at where tests of this nature should go or could extend?
Test Scenario:
- External Library + minified included in the URL = Fail as expected
- External Library + without minified included in the URL = Skipped
- 🇺🇸United States smustgrave
Maybe in Drupal\FunctionalTests\Asset;
Also doesn't have to be it's own test, could be an additional assertion somewhere.
- 🇺🇸United States firewaller
Does it make sense to relocate this change to `\Drupal\Core\Asset\CssCollectionOptimizerLazy::optimizeGroup` similar to the JS approach? https://git.drupalcode.org/project/drupal/-/blob/11.x/core/lib/Drupal/Co...
I am reporting a similar (as it seems) issue when I visit the Resend page of a webform.
It occurs with Claro and Gin themes. When I try with eg. the Bootstrap admin theme, the issue is not present.
These are the errors I get:
1. Only file JavaScript assets can be optimized. in Drupal\Core\Asset\JsOptimizer->optimize() (line 31 of /var/www/html/docroot/core/lib/Drupal/Core/Asset/JsOptimizer.php).
2. Symfony\Component\HttpKernel\Exception\BadRequestHttpException: Invalid filename. in Drupal\system\Controller\AssetControllerBase->getGroup() (line 224 of /var/www/html/docroot/core/modules/system/src/Controller/AssetControllerBase.php).
- 🇺🇸United States jlancaster
Error logs on a few of my sites were getting filled with warnings and I could not find culprit in backtrace. Applying this patch to core (#23) has resulted in clean logs, finally!
- First commit to issue fork.
- 🇪🇸Spain ssantaeugenia
Thanks for this great patch. I've attached the modified patch to also affect JavaScript.
- 🇩🇪Germany osopolar 🇩🇪 GER 🌐
I have the same errors in the log and now I am wondering how to fix it.
@joelpittet You mentioned that you had assets that were
type: external
ANDminified: true
and so you removed the minimized there. We have one such asset, the better_exposed_filters → library. But I am wondering whyminified: true
shouldn't be set for external libraries. There is a example of both attributes set in documentation → under minified, with the description
Whether the asset is already minified. Default: false
The asset in your example (https://cdn.ubc.ca/clf/7.0.4/css/ubc-clf-full-bw.min.css) seems to be already minified, so why not setting
minified: true
? - 🇨🇦Canada joelpittet Vancouver
@osopolar that’s a really good point — I should know better than to make assumptions about how this works. I was under the impression minified was an action to be taken rather than a descriptor. But you’re right, the docs clearly state otherwise. Looks like I’ve been shooting myself in the foot here! Thanks so much for pointing it out.
That said, I still think the page-cached asset URLs are a problem, regardless of my incorrect understanding. I’ll try to update the issue summary to reflect that.
- 🇬🇧United Kingdom catch
Yes minified describes the library, not what to do with it.
- 🇺🇸United States firewaller
At the very least the condition to check "external" should be considered in \Drupal\Core\Asset\CssCollectionOptimizerLazy::optimizeGroup calls
- 🇨🇦Canada joelpittet Vancouver
I’m digging into this because the error still appears in my logs month to month.
Here’s the asset that triggers the failure:
core/lib/Drupal/Core/Asset/CssOptimizer.php:43
Array ( [type] => external [minified] => 1 [weight] => -199.99963333333 [group] => 100 [data] => https://cdn.ubc.ca/clf/7.0.4-minimal/css/minimal-clf-full-7.0.4-bw.css [version] => 7.0.4 [media] => all [preprocess] => 1 [license] => Array ( [name] => GPL-2.0-or-later [url] => https://www.drupal.org/licensing/faq [gpl-compatible] => 1 ) )
This is coming off a request to:
/sites/default/files/css/css__xlnlyYvQDTExRxMzowYQQbU2Gkpxbvzgs_51zHi7ms.css?delta=1&include=eJxlj-EOwiAMhF-IySORwsqGFkpo56ZP79QZjfy7-y6X9iK3rKmKjYcwsCg7KGHmJvbXmACEZYRmP-KkM2Y0DaVykXRFp-AJXUyk2OyXDy9u5CaK2XoQNIEb2rIfBUp3NBMQBF3EBXG4qfXMKtqg9smz_Q8zisCE0gUTsQcyu6RULnZsSwU6Hfb9xMHgDFtXDxSHuA5-7ZJ9Iz4A2b18gw&language=en&theme=galactus_cs_ext
Which attempts to load a library from my theme:
galactus_cs_ext/clf-fw-bw
That library is no longer used. So this request seems to be coming from a cached, malicious, or stale reference, and causes the
“Only file CSS assets can be optimized”
error.
For reference, here’s how you can decode the include hash via the command line:
php -r "print_r(@gzuncompress(base64_decode(str_replace(['-', '_'], ['+', '/'], 'INCLUDE_HASH_GOES_HERE'))));"
Or you can useUrlHelper::uncompressQueryParameter()
in code.I’m not sure how to categorize this type of bug—it stems from a library that used to be used, still exists, but is no longer active in the theme. Yet the request for it persists and causes an exception rather than a graceful fallback or warning.
You might see why I confused as to the "minified" as it looked like a type: "external" is being "minified", and is thrown in a
if ($css_asset['type'] != 'file') {
The library definition is:
clf-fw-bw: version: 7.0.4 css: base: https://cdn.ubc.ca/clf/7.0.4-minimal/css/minimal-clf-full-7.0.4-bw.css: { type: external, minified: true } theme: css/clf/clf.social-icons.css: {} dependencies: - galactus_cs_ext/cdn-clf-js
Our docs suggest to me that this looks right:
https://www.drupal.org/docs/develop/theming-drupal/adding-assets-css-js-... → - 🇨🇦Canada joelpittet Vancouver
Took a stab because I noticed in debugging
JsCollectionOptimizerLazy
had an exclusion thatCssCollectionOptimizerLazy
was missing.Fixing that prevents this bug, I will see if I can write a test to prove it's fixed.
- 🇨🇦Canada joelpittet Vancouver
https://git.drupalcode.org/project/drupal/-/commit/b957382#2fb629f0e45c5... This is where the JS part got committed for reference: 🐛 Stampedes and cold cache performance issues with css/js aggregation Fixed
- 🇨🇦Canada joelpittet Vancouver
Ok wrote a test, hope the local file I faked as external is appropriate in this case. Couldn't think of what to do for an "external" CSS file but it doesn't really need to be truely external.
- 🇩🇪Germany Anybody Porta Westfalica
Thanks @joelpittet great work and test is green!
I left some final, minor comments, what do you think?
We're having these log entries day by day... - 🇨🇦Canada joelpittet Vancouver
The tests-only fail (as expected) by the way.
Thanks @Anybody for review, I put code suggestions that you can apply (might need push access to do so), but feel free to apply the suggestions or make your own in the comments you started.
- 🇺🇸United States nicxvan
I think this is ready now, I compared with the JS lazy optimizer and the logic looks the same.
I don't think bugfixes like this need CRs.
Test only fails as expected.
I took a chance on updating the title too.