Logic error in Drupal's lazy load for asset aggregation

I did a simple approach which is to ensure that the libraries that needs to be processed is sorted. This means that the order of the libraries doesn't influence the specific groups that are created, which in turn ensures as consistent behaviour.

I'm not sure if and how we should write tests for this, as the whole cycle is pretty complicated and not sure how we could write tests to actually capture this.

MR should be pointed to 11.x as latest development branch.

As far as tests go, if this is a major bug do believe we need someway to see that this is a problem and is being solved.

But appears to have test failures.

googletorp → closed merge request !9208

We've been hit by this one as well. In our case, it happens while using gin admin theme, which also has two JS files with preprocess set to FALSE, and only under a specific case being a webmaster role which has less permissions than say user 1 (so hard to reproduce as well, as I'm not totally known with the internal system)

The JS aggregation URL triggers following error: Symfony\Component\HttpKernel\Exception\BadRequestHttpException: Invalid filename. in Drupal\system\Controller\AssetControllerBase->getGroup() (line 225)

The MR in #4 fixes it for us, and the page now renders fine, so plus one from me at least.

It's close to being critical, because in our case the webmasters, while they don't get a blank screen, they couldn't edit their content as for instance CKeditor isn't loaded at all or use the media browser to select images.

Actually, I spoke to fast, while node edit page is now fine with the patch, admin/content is now broken, so it looks like the sort isn't solving everything.

Probably related, but not sure 📌 Replace custom weights with dependencies in library declarations; introduce "before" and "after" for conditional ordering Needs work

Adding 🐛 Aggregated URL hashes for assets can mismatch due to different order of library assets Active as a related issue. We definitely have an issue with the current ordering not being reliable enough, whether it's fixable without the before/after issue or not I'm less sure about, but this is a slightly different approach from the ones I've tried (and failed) previously.

I spent quite some time trying to figure out what is going on. I also saw the issue with gin theme and had some node forms which didn't work depending on which additional libraries were pulled in via the field_group module.

Originally I thought the problem was the ordering of libraries, but upon further investigation it seems the root issue is that the order of all libraries vs the ordering of only js assets isn't necessarily the same, which then produces the error @swentel mentions in #11.

I closed the MR from #4 since it doesn't necessarily fix things and made a new patch in !9219 which I think actually solves the issue.

I don't know how to do a an actual test case for this - I made a test module which has two pages, one failing and one before the fix where both are working after the fix. For some reason some functional JS unit tests are failing in the MR which introduces the test module used to demonstrate the issue, which seems really strange to me, as the introduction of a test module shouldn't make test fails, so wonder if something else is going on.

I'll try to see if I can come up with a way of testing this, but would be great if some one could take a look at things mentioned and provide feedback on the suggested fix.

Left one comment on the MR - currently it's repeating logic done earlier to remove non-js libraries from the list. Is that duplication intentional? Could we remove the unset in the second foreach maybe?

The AjaxPageState test is a real failure.

My attempt to fix this issue was in 🐛 Aggregated URL hashes for assets can mismatch due to different order of library assets Active and was clearly incomplete. If we can get this MR to green with a test that fail successfully, it might be worth looking to see whether we can revert the fix from there (but also possible that we actually need both changes).

Pushed a commit to fix the AjaxPageState failure.

functional JS unit tests are failing in the MR which introduces the test module used to demonstrate the issue

If this is ckeditor5 image tests, it's a random but frequent test failure in HEAD and unrelated.

I have done a couple of things now, to summarize what we have.

1. Improved existing test cases in AssetResolverTest and correct a wrong assertion (different libraries, same timestamps should have 2 / 4, not 2/3). This was a result of the way libraryDiscovery was mocked
2. Create a failing unit test case where a CSS on library influence the contents of JS groups
3. Adjusted fix to ensure that new test case is now green (needed to both do sorting and re-calculation of dependencies from minimal dependencies to get a working solution

With regards to the comment in the PR, we need to double unset (explained in detail in the PR). The root issue is caused by CSS dependency changing the order or dependencies which can result in different groups. The reason why this fails is that when Drupal generates the aggregated it does so from the minimal JS only libraries.

The test cases for the fix is now all green (we got lucky with the ckeditor test case which was failing before), so I think this is ready now.

We faced the same issue and the changes from the MR seem to have fixed the problem.

Attaching the a patch for Drupal 10.3.2 backported from the MR as well since this version was also affected.

Needs a rebase for merge conflicts in the test..

Re-titling to make it clearer what the issue is.

Did a couple of things here:
1. Rebased to resolve the merge conflicts. The unit test/mocking fix implemented here was similar to the one in HEAD, but I prefer the use of withCallback() here so stuck with that from this MR.

2. Factored out the library loading/filtering logic to a protected method and calling that from getCssAssets() and getJsAssets() to reduce code duplication.

3. Both before and after #2, and also in similar attempts elsewhere, MediaStandardTest fails on vertical-tabs.js being loaded before form.js - this is because vertical-tabs.js specifies a negative weight and form.js doesn't, and weight overrides dependencies even though it shouldn't.

I tried changing the ordering logic and then stopped again. Then looked back to see why vertical-tabs.js is weighted before announce.js, and... I cannot find any reason. It was added back in #1996238: Replace hook_library_info() by *.libraries.yml file → which was a huge MR, and didn't have a weight before that.

Removing the unnecessary weight I get a green MR.

1 nitpicky comment but can't apply since it's a committer MR.

May want a 2nd thumbs up before marking. But could issue summary be updated also. Solution still seems to be TBD.

Updated the issue summary.

Rebased as I can't re-run tests and javascript tests appear to be consistently failing.

I think I can reproduce the bug manually, it silently doesn't do the thing the test is checking for (maintaining vertical tabs state when ckeditor5 buttons are moved around), which is probably something to do with the vertical tabs weight change, but there's no error at all and I'm not clear what even provides that functionality in core.

Apart from one random test failure, that results in a green run again.

Will go out on a limb and say this one is ready.

Marking since it will go through 2 committer eyes too.

I've added couple of comments to the MR that need addressing.

I think we need a CR here that notes that libraries need to ensure they list the vertical tabs as a dependency if it is one so it's not missing like it was for ckedtior.

Also think the CR should note the potential for the JS order to change if a contrib or custom extension adds a library with an impossible weight (i.e. trying to come before it's own dependencies).

I asked @catch if we could produce a warning if we determined that a library had an impossible weight and he pointed out he'd rather do 📌 Replace custom weights with dependencies in library declarations; introduce "before" and "after" for conditional ordering Needs work and deprecate weights altogether.

Added the change record.

Unfortauntely testJsAssetsOrder failed in the last run and that's definitely related.

@alexpott I think you were looking at https://git.drupalcode.org/project/drupal/-/merge_requests/9219 which is tests only - going to hide that MR.

catch → changed the visibility of the branch 3467860-11x-test to hidden.

#36 was looking at the wrong MR, but I had failed to push a commit fixing the ckeditor5 dependency from local and now that's pushed.

Oops yeah I must have been. Sorry.

Moving this back to RTBC since only trivial changes and a CR since #33.

This doesn't only affect aggregate assets, it also ensures the correct order when aggregation is off, re-titling.

Committed and pushed 0c9e4a4b8e to 11.x and 4c3bada24c to 10.4.x. Thanks!

Tagging after discussion with @xjm, @catch and @longwave

Wow, nice!

This is a solid step forwards, next up is 📌 Replace custom weights with dependencies in library declarations; introduce "before" and "after" for conditional ordering Needs work .

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

Rerolled the patch from #21 against 10.3.6.

Added the changes needed to support vertical tabs.

Views exposed filters in entity browser dialog/iframe are somehow no longer working in 10.4.x when testing on our install profile, git bisect points to this issue.

I guess there might be also an incorrect or missing library dependency, but there are no JS errors or so, pressing the submit button just reloads the page without applying the filter. maybe something related to jquery.form or views JS?

entity_browser tests on D11 are broken atm, but I did manage to at least fix previous major and set up a job for 10.4 that is also failing on various tests: https://git.drupalcode.org/project/entity_browser/-/jobs/3221250

I don't know if it's what's causing the issue, but the dependencies do seem to be incorrect:

entity_browser.common depends on drupalSettings but doesn't declare it. Other libraries depend on entity_browser.common then separately depend on drupalSettings.

Couple of other libraries declare dependencies on indirect dependencies of libraries they depend on.

Something like this (completely untested) diff:

diff --git a/entity_browser.libraries.yml b/entity_browser.libraries.yml
index bf6d05d..21e512e 100644
--- a/entity_browser.libraries.yml
+++ b/entity_browser.libraries.yml
@@ -2,9 +2,10 @@ common:
   js:
     js/entity_browser.common.js: {}
   dependencies:
-    - core/drupal.dialog.ajax
+    - core/drupalSettings
     - core/drupal
     - core/jquery
+    - core/drupal.dialog.ajax
 
 pager:
   css:
@@ -28,10 +29,7 @@ iframe:
   js:
     js/entity_browser.iframe.js: {}
   dependencies:
-    - core/drupalSettings
     - core/once
-    - core/drupal
-    - core/jquery
     - entity_browser/common
 
 iframe_selection:
@@ -47,8 +45,6 @@ entity_reference:
     - entity_browser/common
     - entity_browser/entity_list
     - core/sortable
-    - core/drupal
-    - core/jquery
 
 file_browser:
   css:

This was committed to 10.4.x so updating version.

From the CR:

If custom or contrib JavaScript is implicitly depending on vertical-tabs.js being loaded very early, this might result in regressions

more like custom, contrib, or core ;)
🐛 Block visibility settings have summary duplicated in the title Active

Yikes, while I can see the reason for this change, it should come with a big warning sign as it changes the order of assets like CSS and JS files, also the ones loaded in custom themes and modules. Especially CSS specificity which is based among others by order this will break most sites in subtle ways. I just tested this with a bigger site and I'm not sure I'll be able to identify all problems and upgrade to 10.4.

If I'm not alone in this, maybe there could be a backwards compatibility patch or setting that allows site maintainers non-breaking upgrades?

As @miiimooo predicted, this broke several of our sites that had multiple custom subthemes. Per https://www.drupal.org/project/drupal/issues/1945262 📌 Replace custom weights with dependencies in library declarations; introduce "before" and "after" for conditional ordering Needs work , I added weight to the libraries of my custom themes and it fixed the obvious issues, but we are continuing to monitor for other issues.

example fix in subtheme.libraries.yml:

global-styling:
  css:
    theme:
      css/style.css: { weight: 10 }

As miiimooo predicted, this broke several of our sites that had multiple custom subthemes. Per Replace custom weights with dependencies in library declarations; introduce "before" and "after" for conditional ordering 📌 Replace custom weights with dependencies in library declarations; introduce "before" and "after" for conditional ordering Needs work , I added weight to the libraries of my custom themes and it fixed the obvious issues, but we are continuing to monitor for other issues.

example fix in subtheme.libraries.yml:

global-styling:
  css:
    theme:
      css/style.css: { weight: 10 }

As miiimooo predicted, this broke several of our sites that had multiple custom subthemes. Per Replace custom weights with dependencies in library declarations; introduce "before" and "after" for conditional ordering 📌 Replace custom weights with dependencies in library declarations; introduce "before" and "after" for conditional ordering Needs work , I added weight to the libraries of my custom themes and it fixed the obvious issues, but we are continuing to monitor for other issues.

example fix in subtheme.libraries.yml:

global-styling:
  css:
    theme:
      css/style.css: { weight: 10 }

I found more side effects that affect Drupal core. I created https://www.drupal.org/project/drupal/issues/3498100 🐛 Duplicate summary on some admin pages using vertical tabs Active to report duplicate summaries (similar to https://www.drupal.org/project/drupal/issues/3493182 🐛 Block visibility settings have summary duplicated in the title Active ) on vertical tabs in Media, Content Block, and Taxonomy Term add/edit forms. I also provided a patch but I'm not sure the solution I proposed is the best.

This has changed the way some of our custom modules interact with contrib modules. We're using DataTables with Views and wrote custom jQuery to manipulate some data on page load. Even after adding datatables as a dependency to our custom module the code no longer functions as it did on 10.3.10 (downgraded to confirm). I'm currently debugging to see if I can add any helpful information, but I'd be grateful if anybody has any debugging tips.