Aggregation URL hashes should be built from normalized list of libraries

Possible culprits were ckeditor5_js_alter() and locale_js_alter() but neither appear to be the problem so far.

Seems to be triggered by Claro rather than ckeditor.

I don't know what about Claro is triggering this when Umami/Olivero don't seem to, but I do have a possible fix.

The issue is that the order of libraries that we get from the attached assets on the page (i.e. admin/content), is different to the order of libraries that we get in the aggregate controller.

This difference is because the assets on the main page are generated from the full list of attached libraries, but the assets on the controller are generated from the full list of attached libraries, transformed to the minimal subset of libraries in the 'include' query string, which is then expanded back out again to the full list of libraries. You end up with the same list of libraries, but in a different order - likely the order of both is 'correct' but different.

The easiest solution to this, is to normalize the list of libraries on the main page by also resolving that from the full list -> minimal subset -> back to full list again, then as long as that process is deterministic we end up with exactly the same list each time.

I am pretty sure that AssetOptmizationTest will catch this if we just point it at admin/content with Claro installed.

Attaching a patch, but it will need the extra test coverage as well as a comment.

Thanks. The attached patch does seem to improve the issue. I am running the Gin theme, which extends from Claro.

The flicker is pretty noticeable on pages using field group in the admin also (plus CKEditor 5) even with patch. The ckeditor dll file is 700kb - I wonder if that is adding to it.

For example this is from a vanilla content type with just a title and body field on the edit form:

init.js is from Gin, the aggregates plus the ckeditor5 dll.js file. The body field is slow to render. On pages without CKEditor it is much faster. I feel like this was not noticeable before.

@kevinquillen ckeditor5 in 10.1 has a fair number of changes from 10.0.x, so can't rule out changes in ckeditor5 itself if there's a performance difference apart from this issue.

For this issue the main thing to check is that you get files written to disk for every aggregate on a page (and cacheable html responses from those files).

Uploading a new patch which takes a slightly different approach (just sorts before getting dependencies) and resolves the test failures above for me.

With that patch, nothing loads in a node edit form - console reports:

"Uncaught TypeError: this.details.drupalGetSummary is not a function"

@kevinquillen by 'that patch' do you mean #4 or #7?

Uploading a slightly improved version of #4 which fixes the unit test but doesn't address the other two test failures yet.

Sorry, #7.

One day I'll learn how to upload a patch with a comment, but not today.

OK #4 and #11 uncovered a real bug in olivero.libraries.yml - it's not declaring its dependencies properly. This one might be green, at least StandardJavascriptTest is happier.

What's the minimal set of conditions where this would be occurring? I was trying to think if we could write integration tests for this.

Olivero bug was introduced, or not entirely fixed at least, in #3231416: Olivero should not expect its default menu blocks to always exist → .

@lauriii that's the bit I'm not sure about yet, I'm pretty sure it's about ordering of libraries that don't have dependencies on each other, or are at the same level in the dependency tree - so the order doesn't 'matter' except it does when we're comparing hashes.

New patch which should fix the Olivero nightwatch test hopefully - moving dependencies around a bit more.

Here's an attempt at a test-only functional test with Umami, unfortunately I can't get it to fail yet.

The header changes are because my local ddev install's nginx header differ from DrupalCI, so trying to find assertions that work for both.

Alright figured out why the test wasn't failing - the JavaScript assertions in AssetOptimizationTest never get reached, because we were making http requests before collecting the script tags, which meant we were trying to collect script tags from a CSS aggregate...

Patch now refactors AssetOptimizationTest so it tests everything it should, subclasses it into AssetOptimizationTestUmami and fails in the right place.

Fixing cs error and more code we can remove from the new test.

Re-titling to better explain what the bug is.

Needs CS fix

1. +++ b/core/lib/Drupal/Core/Asset/AssetResolver.php
   @@ -101,8 +101,12 @@ public function __construct(LibraryDiscoveryInterface $library_discovery, Librar
   +    if ($libraries) {
   +      $libraries = $this->libraryDependencyResolver->getMinimalRepresentativeSubset($libraries);
   

   needs comment

2. +++ b/core/tests/Drupal/FunctionalTests/Asset/AssetOptimizationTest.php
   @@ -56,6 +56,15 @@ public function testAssetAggregation(): void {
   +  protected function requestPage() {
   

   could use :void return type

3. +++ b/core/tests/Drupal/FunctionalTests/Asset/AssetOptimizationTest.php
   @@ -147,6 +151,9 @@ protected function assertAggregate(string $url, bool $from_php = TRUE): void {
   +    if (!str_contains($this->fileAssetsPath, $url)) {
   +      return;
   

   needs comment

Addressing #19.

The last submitted patch, 20: 3370930-20-test-only.patch, failed testing. View results →

The last submitted patch, 20: 3370930-20.patch, failed testing. View results →

Let's make the assertion version agnostic..

It looks like the reason for the changes in dependencies within Olivero are because Drupal.olivero.isDesktopNav() exists within navigation-utils.js, so this file needs to be loaded first.

At first glance the code changes look good. I'm going to do some additional testing to verify functionality.

Tested everything out in Olivero and it works great!

Note that in Kevin's original bug report in Slack, he also states

Also having a lot of trouble getting custom theme css to load new versions without incrementing 'version' every refresh. The only change today was going from 10.0.9 -> 10.1.0.

This issue doesn't seem to be fixed. When I look at the aggregate hashes, and then clear caches (via the UI), the filenames or hashes do not change. I expect them to change to force a new version of the aggregate to download. Is this expected?

Ahh that last one is 🐛 Ensure that edge caches are busted on deployments for css/js aggregates Fixed , I think we need to do something about it, but it's not related to the bug here.

Based on the issue summary I was getting random errors in the logs. With the patch though I still got error. See screenshots above.

@smustgrave OK those errors are weird, but also it looks like you have css and js aggregation off - they need to be enabled to trigger the bug here.

Patch in #23 vastly improves performance and now seeing very little flicker if any, even with CKEditor on the page.

Will lean on the other reviews then. Maybe not sure what I'm looking for.

Moving back to needs review, I think the errors in #29 are probably an environment issue.

I validated the issue in Drupal 11.x in Firefox 114.0.2 and Chrome Version 114.0.5735.198 using DDEV by following the steps to reproduce in the IS, and found 2 JS assets' response headers included x-generator: Drupal 11 (https://www.drupal.org), as described in the issue summary problem. (Screenshots attached.)

To test the patch, I applied the patch, cleared the cache on the Performance page, and returned to the Add article page. I opened the devtools' Network tab, and refreshed the page 3 times. I inspected the response headers for each aggregated CSS and JS asset, and did NOT find x-generator: Drupal 11 (https://www.drupal.org) as before.

I navigated to other admin and front-facing pages on the Umami site, and inspected response headers for aggregated CSS and JS assets. Where I found x-generator: Drupal 11 (https://www.drupal.org), I refreshed the page, and noticed it was then gone. (With the patch applied.)

As far as I can tell, this patch in #23 addresses the problem. Setting to RTBC.

I found this issue via #bugsmash, where catch requested reviews and testing, so adding the Bug Smash Initiative tag.

+++ b/core/tests/Drupal/FunctionalTests/Asset/AssetOptimizationTest.php
@@ -56,6 +56,15 @@ public function testAssetAggregation(): void {
+  protected function requestPage(): void {

I think it might be useful to be able to pass a list of paths to test. Now you would have to create a new test class for each page. I don't think it's needed for this issue but might be easier to add it now than later. Thoughts?

@lauriii at the moment it's not just the paths but also the install profile and user that's different. I'm not sure we'd get much from testing different paths in the same install profile with the same user for what this test is doing. Overall would prefer to do 📌 Integration test for AssetResolver + LibraryDependencyResolver Active (which I've just opened) so that we can hopefully test for this case at a much lower level than installing Umami.

Fair enough. 👍

Committed 9a66ffc and pushed to 11.x. Thanks! Also cherry-picked to 10.1.x.

Automatically closed - issue fixed for 2 weeks with no activity.

Is it possible that this also affects css at a simliar functionality?
When I aggregate css files not all styles are used.

I don't think this is completely resolved. I encountered the same issue when viewing a webform submission. The order of the assets is slightly different causing the hashes to be different. Opened 🐛 Aggregated URL hashes for assets can mismatch due to different order of library assets Active .