Ensure consistent ordering when calculating library asset order

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.

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 (order of css and JS completely) is now broken, so it looks like the sort isn't solving everything.

Probably related, but not sure #1945262: Introduce "before" and "after" for conditional ordering in library definitions

Adding #3397713: [Performance regression] Starting with Drupal 10.1, some sites hit PHP for every page view due to aggregated asset URL hash mismatch from different order of asset items 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.

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 #3397713: [Performance regression] Starting with Drupal 10.1, some sites hit PHP for every page view due to aggregated asset URL hash mismatch from different order of asset items 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.

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.

I think this might provide a solution to #3397713: [Performance regression] Starting with Drupal 10.1, some sites hit PHP for every page view due to aggregated asset URL hash mismatch from different order of asset items too.

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, and there is no obvious override or interdependency between the two files, so it appears to just be bogus.

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). No error, just doesn't do it.

After some grepping, I discovered this functionality is implemented by ckeditor5 js itself, and that library does not declare a dependency on vertical tabs, even though it does in fact depend on vertical tabs. After adding the dependency that test passes, see how it looks on a full run. The problem was that the ckeditor5 js looks for vertical tabs markup which isn't there unless it runs after vertical-tabs.js.

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.

After discussion with @catch, 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 #1945262: Introduce "before" and "after" for conditional ordering in library definitions 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.

#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 #1945262: Introduce "before" and "after" for conditional ordering in library definitions.

Rerolled the patch from #21 against 10.3.6. Edited to note - this patch causes a regression with vertical tabs and should not be used, support for vertical tabs was added to the patch in #51.

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 ;)
#3493182: Block visibility settings have summary duplicated in the title

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, 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 to report duplicate summaries (similar to https://www.drupal.org/project/drupal/issues/3493182) 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.

Edit: I fixed my original issue, but I don't like how I had to do it. I added a weight of -1 to our custom js and it's now working as it did with 10.3.10.

This change broke many of our styling and javascript.
Most of them I was able to fix with a weight but on some pages, the aggregated files do not contain the required dependencies.
For example: Our theme depends on the following
dependencies:
- core/jquery
- core/drupal
- core/drupalSettings
- core/drupal.ajax
- core/drupal.dialog.ajax
- core/views.ajax

But the Drupal.behaviors.ViewsAjaxView is missing from the aggregated files.

I'm pretty sure this has caused visual problems in Claro tabledrag. See https://www.drupal.org/project/drupal/issues/3506870

Hi :)

I am not dev., I have a problem with 10.2, 10.3 or 10.4.

10.1.x is oki, but I am bloqued, if I try to update up all my Java and CSS seem bloqed on Edge and Opera (menu no work, Accordeon not deploy, CSS diseapear), seem oki with Chrome if I not use the 2 option "Agregated" in performane menu.

Thx.

++
on 10.4.6 with aggregate(js,css) turned on the number of `script` tags go down from 84 to 7 !
colorbox js is a quick example of a mission library with aggregate turned on .
same with search autocomplete.
the easiest thing to notice is scripts relevant to admin toolbar in Adminimal theme (the drop down under droplet in top left of toolbar that list actions such as `clear cache` disappears.
still looking ...

Our 10.4.6 update caused a custom module using Datatables to stop attaching our custom JS file and the Datatable JS file. We had to update our library.yml file to include weights. Same fix mentioned in #58. We have other JS that handled the upgrade just fine.

datatables:
version: VERSION
js:
js/datatables.js:
weight: -20
dependencies:
- core/jquery

jquery-table-search:
version: VERSION
css:
component:
css/commerce_reports_table.css: {}
js:
js/commerce_reports_table_search.js:
weight: -10
dependencies:
- core/jquery
- commerce_reports/datatables

Belatedly adding credits per #46.