Deprecate more jQuery UI library definitions

Created contrib modules for the libraries that were not already covered by a dedicated contrib module or included in drupal.org/project/jquery_ui (mouse, widget, position, form-reset)

Added:
https://www.drupal.org/project/jquery_ui_autocomplete
https://www.drupal.org/project/jquery_ui_dialog
https://www.drupal.org/project/jquery_ui_resizable

Currently dev releases, after review and manual testing I can move to stable/promoted.

For #2 this is already partly underway in #2158943: Add a native dialog element to deprecate the jQuery UI dialog.

There's also #3079748: Deprecate jquery.ui.draggable.

It's not possible to deprecate dialog or autocomplete yet as there are not replacements for those libraries. However, the libraries they depend on can be deprecated by using the approach described in the issue summary. This makes it possible to deprecate widget, mouse, button, draggable, menu, position, and resizable. All of these either have contrib modules or - in the case of position, widget and mouse - are part of the main jquery_ui contrib module.

it is also not possible to deprecate core/jquery.ui because drupal.tabbingmanager has a dependency on it. According to a comment, this dependency is only for # Supplies the ':tabbable' pseudo selector., so this could potentially be replaced without too much headache. Tagging with "needs issue summary" for the drupal.tabbingmanager followup, and a second followup to deprecate core/jquery.ui once tabbingmanager is taken care of.

I realize I was incorrect about autocomplete and dialog -- of course those can be deprecated by using the same approach with drupal.dialog and drupal.autocomplete. It's possible drupal.tabbingmanager can even get away with using a subset of jquery.ui's assets. I'll provide an updated patch soon.

Crediting @DamienMcKenna for his work on #3113250: Deprecate jquery.ui.button.

Went the route of skipped deprecation list for jquery.ui.dialog as this is the approach being used for other deprecated jqueryui libraries.

The approach of loading assets directly was also used for jquery.ui.datepicker, a library that is already in the skipped deprecation list. This makes it possible for it to not depend on core/jquery.ui -- allowing core/jquery.ui to be deprecated and not have to be added to the skipped list.

I think we should limit the scope of this issue to just deprecating libraries. The deprecation warning could be backported to 8.9.x, if we get approval from release manager. The part where assets are being moved from library to another should be only committed to 9.0.x.

No interdiff as this isn't really building off the previous patches.

I agree with #12, which I saw just before uploading this 9.X patch that makes the asset changes so it's not necessary to add anything to the skip list.

+++ b/core/tests/Drupal/Tests/Listeners/DeprecationListenerTrait.php
@@ -186,6 +186,7 @@ public static function getSkippedDeprecations() {
+      'The "core/jquery.ui.dialog" asset library is deprecated in drupal:8.8.0 and is removed from drupal:9.0.0. See https://www.drupal.org/node/3067969',

Can this be removed too?

Re: #14
core/jquery.ui.dialog is in the skip list due to a direct call to that library in \Drupal\views_ui\ViewEditForm:
$form['#attached']['library'][] = 'core/jquery.ui.dialog';

The issue summary mentions two different approaches to addressing this. I saw pros and cons to both approaches, but adding the skip-list option to the patch seemed like a quick way to surface it for discussion. I'm also curious if changing the views form to use core/drupal.dialog would work of if it may conflict with dialog.views.js, and that's why the jQueryUI library was attached directly.

This patch removes jquery.ui.dialog from the skip list since #3113556: Change Views UI to use drupal.dialog instead of jquery.ui.dialog has landed. Also realized that it was neccessary to refactor some library overrides in Claro and Seven as they are preventing the loading of CSS assets that now come from different libraries.

Also removing needs followup - created the tabbingmanager issue a few weeks back #3113649: Remove drupal.tabbingmanager's jQueryUI dependency

So, overall, this seems like a good idea and is probably what we should have done in the first place.

The one thing I'm not totally sure of though is removing the library definitions from Drupal 9 so late relative to when they're being deprecated. Maybe we should deprecate them for Drupal 10 instead? We could still backport it to 8.9.x (like we are doing for theme functions) to make noise at people if they do use them, but without actually having a last-minute BC break.

I'm somewhat on the fence; there's also obvious value in doing this before 9.0. Just not sure we should given that 8.8 should have been the cutoff (and furthermore already was for 3/4 the libraries).

Something to take into account here might be that solving these deprecations is quite easy. Modules relying on jQuery UI only have to define dependency on the contrib modules and change their libraries to depend on the contrib project libraries instead of core.

It's beta deadline if it's 9.0.x.

I'm still leaning toward just changing the deprecation to Drupal 10 though.

It makes sense to move the library dependency to the modules and out of core. My only concern would be with people that relatively new to Drupal theming and require jQuery from Core on their libraries.yml file. It might be difficult to debug sudden breaks.

Moving to 9.1.x since this was beta-deadline. We'll deprecate these in D9 for D10 removal. :)

@jedgar1mx, agreed! That was my feeling too. The goal of the continuous upgrade path is to give people at least one minor release to prepare for the BC breaks that come with the major release.

Working on the test fails, which exposed the need to provide additional asset loading weights now that the order for some assets are no longer enforced via dependencies:

Had to add several explicit asset weights which had previously not been needed since dependencies: always load first in a library.

#29

Do we want to leave a gap between these weights just in case someone wanted to alter the definition to add a file in-between?

That crossed my mind as well. My thought was adjacent weight values would best reproduce the behavior that was present when these assets were loaded as dependencies, where weight-specific flexibility was not available, just a guarantee that the dependency would be available before the library assets. This also best replicates how jQuery UI works when used with AMD instead of splitting it into Drupal libraries, which doesn't offer this level of control.

If this doesn't seem like a problem and would be considered in-scope/commitable, I certainly have no objection to providing users more flexibility.

Would be good to have a change record explaining the implications of this change including changes needed for library override implementations.

I have some concerns that this could cause regressions to sites that are explicitly overriding or extending libraries that will no longer be loaded. Based on the tests in #3064017: Create a means to mark an asset library as deprecated in a *.libraries.yml file, I'm not sure if we display a warning in the case where deprecated library is being overridden or extended without it being loaded. Fixing that could be a separate issue, but we should probably try to take these use cases into account on the change record and release notes. I'm not sure if that issue should be blocker to this.

The change record looks better. Can we still get a follow-up for the runtime deprecation warning?

1. I'm still concerned about this breaking sites. At the moment, both Seven and Claro are extending core/jquery.ui which is not being loaded anymore because it's not dependency of the autocomplete or dialog libraries. As a result, styles for both of these are broken in both themes. This is something we were not able to catch with our testing because we don't show deprecation warnings for libraries-override and libraries-extend. After this, I'm pretty convinced we should postpone this issue on #3163500: Detect when libraries-override or libraries-extend is used on deprecated library.

2. +++ b/core/core.libraries.yml
   @@ -259,10 +320,33 @@ drupal.tabbingmanager:
   +    # The remaining JavaScript assets previously came from core/jquery.ui, a
   +    # deprecated library.
   +    # @todo replace with solution found in https://drupal.org/node/3113649
   ...
   +  # All CSS assets previously came from core/jquery.ui, a deprecated library.
   +  # @todo replace with solution found in https://drupal.org/node/3113649
   

   Could we add this type of documentation to all libraries that have jQuery UI libraries added.

3. +++ b/core/core.libraries.yml
   @@ -430,7 +514,7 @@ jquery.ui:
   -    assets/vendor/jquery.ui/ui/version-min.js: { weight: -11, minified: true }
   +    assets/vendor/jquery.ui/ui/version-min.js: { weight: -12, minified: true }
   

   Why are we changing the weight of this asset?

4. +++ b/core/core.libraries.yml
   @@ -459,14 +544,15 @@ jquery.ui.autocomplete:
   +    assets/vendor/jquery.ui/ui/widgets/checkboxradio-min.js: {  weight: -7, minified: true }
   +    assets/vendor/jquery.ui/ui/widgets/controlgroup-min.js: {  weight: -7, minified: true }
   +    assets/vendor/jquery.ui/ui/widgets/button-min.js: {  weight: -7, minified: true }
   
   @@ -521,29 +617,32 @@ jquery.ui.menu:
   +    assets/vendor/jquery.ui/ui/ie-min.js: {  weight: -11, minified: true }
   ...
   +    assets/vendor/jquery.ui/ui/widgets/resizable-min.js: {  weight: -8, minified: true }
   
   @@ -551,14 +650,16 @@ jquery.ui.resizable:
   +    assets/vendor/jquery.ui/ui/widget-min.js: {  weight: -10, minified: true }
   

   Nit: extra space before weight property

I have just fixed the extra spaces before the weight property as suggested in #41.
The other points of #41 still needs to be addressed.

Triggering the test bots for review.

Back to NW for changes suggested in #41

Re-rolled the patch in #45... Retaining the status as "Needs work" for the changes suggested in #41. Thanks!

#41.1 Once #3163500: Detect when libraries-override or libraries-extend is used on deprecated library landed, errors were exposed. These have been addressed by having the themes extend non-deprecated libraries that include assets that the extension is indended to be paired with.

#41.2 Good call, added explanatory docs to all non-jquery/UI libraries importing jqueryUI assets
#41.3 The asset weight is changed because the assets with -11 weights require it to be present to work properly, confirmed by seeing the define() calls in a given asset. Some tests were failing until the weight was changed in some libraries. Not every library needed this weight adjustment for the tests to pass, but they were all changed for consistency and the code itself expecting it to be available.

I started looking at the asset weights and there are still some instances where a dependency doesn't have lower weight. I think we should go through all of the weights and ensure that weights always have lower weight. We should also ensure that the weights are consistent across different libraries because each file is only loaded once. Based on Drupal\Core\Asset\AssetResolver::getJsAssets the assets are sorted after duplicates are removed.

I charted it out and used that to determine the dependency relationships

There are six groups

GROUP 1: [-7] dialog
GROUP 2: [-8] button
GROUP 3: [-9] checkboxradio, resizable, draggable
GROUP 4. [-10]  autocomplete, controlgroup, form-reset-mixin, labels,  mouse, menu, tabbable
GROUP 5. [-11] data, disable-selection, escape-selector, form, focusable, ie, jquery1-7, keycode, plugin, position, safe-active-element, safe-blur, scroll-parent, unique-id, widget
GROUP 6. [-12] version

The weights relative to one another should be fine now. It's possible the overall weights can be moved closer to 0, I don't recall why those specific high/low weights were chosen when the patch was added 6mos ago. There may have been a good reason, but I'm not sure.

Thank you for creating that chart @bnjmnm! It clarifies the weights a lot. We should probably also apply weights on CSS files. Some elements could receive rules from multiple files, and in those scenarios the weight could have an impact how the element is rendered.

Weights are now applied to CSS as well.

I can’t help but feel that we need some tests here because the only way to test that the patch is correct would be to write some test code to load different variations of the libraries and ensure that they are loaded in correct order. Automating that would be more reliable and would at the same time ensure there aren't regressions.

Agree with #56 that tests are needed, so I added some.

Updated CS issues.

The current set of tests won’t work without the other changes in the patch, but I see the value in an additional test that confirms consistent losing before/after. I can add that in a day or two. If anyone is eager to see it sooner and wants to work on it, it’ll help if they can assign the issue to themselves so there isn’t overlap.

Added a new test within the new test Class - testAssetLoadingUnchanged, this is the only test that the test-only patch runs. It checks to make sure the same assets are loaded before and after the refactoring in this issue. The before/after tests do not include load order as there's not a 1:1 relationship. With the dependency approach, files are present within that dependency before any tasks begin, so the load order of individual files is less important. With the individual file approach here, weights have to be assigned more deliberately, based on the findings in #52. testLibraryAssetLoadingOrder()
and testSameAssetSameWeight() cover the explicitly set weights that were previously unnecessary due to dependencies dictating load order.

1. +++ b/core/modules/system/tests/src/Functional/Libraries/JqueryUiLibraryAssetsTest.php
   @@ -0,0 +1,752 @@
   + * Tests the loading of jQueryUI CSS and JS assets.
   

   Nit: I think the correct spelling is jQuery UI.

2. +++ b/core/modules/system/tests/src/Functional/Libraries/JqueryUiLibraryAssetsTest.php
   @@ -0,0 +1,752 @@
   +  public function testLibraryAssetLoadingOrder($library) {
   ...
   +  public function testAssetLoadingUnchanged($library, array $expected_css, array $expected_js) {
   

   Is this testing something that isn't covered by ::testAssetLoadingUnchanged()?

3. +++ b/core/modules/system/tests/src/Functional/Libraries/JqueryUiLibraryAssetsTest.php
   @@ -0,0 +1,752 @@
   +    $this->assertEmpty(array_diff($js_loaded_by_page, $expected_js), print_r(array_diff($js_loaded_by_page, $expected_js), 1));
   

   Couldn't this be simplified to $this->assertSame($js_loaded_by_page, $expected_js);?

#65(1) nit squashed
#65(2-3) These are both due to that specific test being designed to work in a test-only patch, as the other tests in the class will not work without the changes here. This makes it possible to confirm the same assets load before/after the changes. The assets load in a different order so assertSame() can't be used. The orders won't be identical as prior to the changes here, individual jQuery UI assets didn't need to have weights assigned to them as declaring their library as a dependency provided availability assurances that aren't there when loading every file individually.

I revised+added some comments in hopes of making this a little clearer.

+++ b/core/modules/system/tests/src/Functional/Libraries/JqueryUiLibraryAssetsTest.php
@@ -0,0 +1,760 @@
+    // assertEmpty() is used instead of assertSame() because this test is
+    // test is designed to pass before and after the jQuery UI asset changes in
+    // http://drupal.org/node/3113400. These changes alter the loading order of
+    // these assets, so we can only compare the contents of the array, not the
+    // order.

I'm wondering if should split this into two issues: one where we add weights that result with the same loading order (this issue) and another issue where we make changes to the loading order based on on #52.

I'm wondering if should split this into two issues: one where we add weights that result with the same loading order (this issue) and another issue where we make changes to the loading order based on on

Separating these would result in functionality-breaking JavaScript errors until the second issue was completed. This is why changes to the weights were introduced in #28. Among other things, every js asset in core/jquery.ui: has a weight of -11, yet every js asset requires ersion-min.js to work properly. This is not an issue when core/jquery.ui: is declared as a dependency of another jquery ui library. When a library that previously depended on core/jquery.ui: loads those assets as individual files, the weights become necessary to avoid "version is not defined" errors.

Discussed this with @lauriii and clarified why this can't be broken into separate issues.

Currently core/jquery.ui has all JavaScript assets configured with a weight of -11. Despite all being confgured with the same weight, some of these assets depend on other ones within the library to function. Giving them all the same weight is not a problem when core/jquery.ui is loaded as a dependency, the assets are not invoked until the full library loads. When the files of core/jquery.ui are added directly, specific weights have to be assigned, or else errors will occur due to files such as the invoking of widget-min.js before version-min.js (which it depends on) has loaded.

This patch adds an additional test which will be explained in this summary of all tests added.

• testProperlySetWeights Compares the asset weights configured in core.libraries.yml to the loading order requirements identified in #52
• testSameAssetSameWeight() Is necessary since multiple libraries now include the same jQueryUI files. This confirms that each file inclusion is set to the same weight.
• testLibraryAssetLoadingOrder Confirms that the configured weights of assets result in the expected loading order on an actual page, regardless of the library/libraries loaded on the page.
• testAssetLoadingUnchanged() is the one test that can be run as a test-only patch. It confirms that the changes in this issue did not alter the files loaded for a given library/libraries. It only checks for the presence of these assets as the order they are loaded in can't be compared due to the need to add more specific weights when loading assets directly instead of within a library dependency.

we can also use floats for weights, keeping all the files weights between -12 (excluded) and -11. That way other scripts can still work based on the assumption the library has a -11 weight.

I'm not too happy about adding that many weights, we tried to get rid of them as much as possible before #1945262: Replace custom weights with dependencies in library declarations; introduce "before" and "after" for conditional ordering

Good call on the floats! This implements it.

It's so nice to finally see that deprecated message on all those jQuery UI libraries.

Amazing work tracing out the weights and mapping the dependencies @bnjmnm!
The chart you made helped me understand how you picked the various weights: 11.9, 11.8 etc.
This reminds me of the detailed dives in #2926155: Follow-up to #2809427: update from jQuery UI 1.11.4 to 1.12.1 forgot to list some new JS files in *.libraries.yml.

The load order on a page is now changed from 9.2.x, but I didn't notice any changes in how autocomplete or dialog functioned.
I think this is related to what you are saying in #66 and #68.
In the network panel I can see version-min loading first now.

Using floats per #70 is a good idea

+1 to this, I also think the use of floats really helps here to "group" all these assets.

Why did you inline all the dependencies for core/jquery.ui.dialog but not do the same for other libraries that use core/jquery.ui.widget or core/jquery.ui.position?
Dependencies were left unchanged in jquery.ui.autocomplete and jquery.ui.menu.
I guess it doesn't make a difference since those libraries are no longer dependents of anything else in core?

Checked over drupal.autocomplete and drupal.dialog and they include all the files from all the previous dependencies.

I just found a couple small nits, this looks great otherwise.

  +++ b/core/tests/Drupal/KernelTests/Core/Asset/LibraryDiscoveryIntegrationTest.php
  @@ -146,7 +146,7 @@ public function testLibrariesOverrideOtherAssetLibraryNames() {
  -    $this->assertAssetInLibrary('//my-server/my_theme/css/jquery_ui.css', 'core', 'jquery.ui', 'css');
  +    $this->assertAssetInLibrary('//my-server/my_theme/css/overridden.js', 'core', 'drupal.displace', 'js');

Since you're using a js file here for the override, for consistency the path should be changed from my_theme/css to my_theme/js as well.

  +++ b/core/tests/Drupal/KernelTests/Core/Asset/LibraryDiscoveryIntegrationTest.php
  @@ -146,7 +146,7 @@ public function testLibrariesOverrideOtherAssetLibraryNames() {
  -    $this->assertAssetInLibrary('//my-server/my_theme/css/jquery_ui.css', 'core', 'jquery.ui', 'css');
  +    $this->assertAssetInLibrary('//my-server/my_theme/css/overridden.js', 'core', 'drupal.displace', 'js');

Both in the test and in test_theme.info.yml.

  +++ b/core/core.libraries.yml
  @@ -265,10 +338,36 @@ drupal.tabbingmanager:
  +    # deprecated library. These assets are used to provide the :tabbable psuedo

spelling: should be "pseudo"

Addresses #73

Looking good.

One minor thing is that the order or files is "upside down" compared to the loading order on the page. -11.9 is "lighter" than -11.4 so -11.9 should be at the top of the list and -11.4 should be at the bottom of the list.

On the page the order is correct, it's just for maintenance sake. Given that the next time we touch those lines it's to remove them I'm not worried about it.

Patch will conflict with #3113649: Remove drupal.tabbingmanager's jQueryUI dependency.

1. +++ b/core/core.libraries.yml
   @@ -428,143 +527,161 @@ jquery.ui:
   -jquery.ui.draggable:
   -  version: *jquery_ui_version
   -  license: *jquery_ui_license
   

   Do we not need to leave a deprecated library in to be removed in Drupal 10? The issue summary says it is deprecated but AFAICS we're now removing it. Also shouldn't jquery.ui.draggable be part of the tests.

2. +++ b/core/modules/system/tests/src/Functional/Libraries/JqueryUiLibraryAssetsTest.php
   @@ -0,0 +1,843 @@
   +  public function trimFilePath($path) {
   

   Can be protected - not part of any public test API.

3. +++ b/core/modules/system/tests/src/Functional/Libraries/JqueryUiLibraryAssetsTest.php
   @@ -0,0 +1,843 @@
   +   * @dataProvider providerTestAssetLoading
   ...
   +   * @dataProvider providerTestAssetLoading
   

   Dataprovider in a BrowserTestBase - I've checked locally how long this takes for me 1.75 minutes... I guess that's okay.

4. +++ b/core/modules/system/tests/src/Functional/Libraries/JqueryUiLibraryAssetsTest.php
   @@ -0,0 +1,843 @@
   +   * that loaded prior to the the deprecation of all remaining cire jQuery UI
   

   cire = core?

5. +++ b/core/modules/system/tests/src/Functional/Libraries/JqueryUiLibraryAssetsTest.php
   @@ -0,0 +1,843 @@
   +    $neccessary_loading_order = [
   ...
   +    foreach ($neccessary_loading_order as $stage => $assets) {
   

   Spelling: necessary

Addresses #76
Item #1 correctly pointed out that draggable shouldn't be removed. I could see how that snuck by based on what accompanied it in the dfiff, but that was unintentional and has been taken care of.

I think jquery.ui.draggable needs to be added to JqueryUiLibraryAssetsTest - no?

Draggable doesn’t need to be added to the test as it didn’t require converting library its dependencies to direct file inclusion. The dependencies-to-files refactoring is only necessary for libraries still used by core so they could avoid declaring deprecated libraries as dependencies.
The test does cover libraries that previously depended on core/jquery.ui.draggable that now consume that library via direct file requests. The library itself is now unused as a result, so it only require a deprecation message.

@bnjmnm but then we have no test to prove we've not removed it be accident. And with the changes here if we break it in some subtle way we'll never know.

#80 makes a good point why those other libraries need coverage. They don't need tests as extensive as the ones in JqueryUiLibraryAssetsTest since they aren't being refactored, just deprecated, so those were put in a lower-overhead Kernel test. I also moved the JqueryUiLibraryAssetsTest out of system so it was in the same neighborhood as the added test.

+++ b/core/modules/system/tests/modules/jqueryui_library_assets_test/src/Controller/JqueryUiTestAssetsController.php
diff --git a/core/modules/system/tests/src/Kernel/Extension/ModuleHandlerTest.php b/core/modules/system/tests/src/Kernel/Extension/ModuleHandlerTest.php
deleted file mode 100644

deleted file mode 100644
index ff40c1b5e2..0000000000

index ff40c1b5e2..0000000000
--- a/core/modules/system/tests/src/Kernel/Extension/ModuleHandlerTest.php

--- a/core/modules/system/tests/src/Kernel/Extension/ModuleHandlerTest.php
+++ /dev/null

+++ /dev/null
+++ /dev/null
@@ -1,345 +0,0 @@

@@ -1,345 +0,0 @@
-<?php
-
-namespace Drupal\Tests\system\Kernel\Extension;
-

This looks like it got deleted accidentally.

Please review!

Addressing #82 and working from #81

This was RTBC in #75, the problems pointed out in #76 are now fixed:

1. jquery.ui.draggable is restored
2. JqueryUiLibraryAssetsTest::trimFilePath is now protected instead of public
3. JqueryUiLibraryAssetsTest is still slow 😅
4. and 5, spelling errors fixed, cspell didn't catch any new ones in 84

The actual changes from #77 to #81 are:
moving core/modules/system/tests/src/Functional/Libraries/JqueryUiLibraryAssetsTest.php to core/tests/Drupal/FunctionalTests/Libraries/JqueryUiLibraryAssetsTest.php
(no code changes in JqueryUiLibraryAssetsTest except for updating the namespace)

and adding core/tests/Drupal/KernelTests/Core/Asset/DeprecatedJqueryUiAssetsTest.php
This new test has hashes for all 10 jQuery UI library definitions so a change to any of those will cause it to fail.

#82 caught a mistakenly removed file, #84 restored it.

looks good to me!

1. +++ b/core/core.libraries.yml
   @@ -428,82 +527,105 @@ jquery.ui:
   +  deprecated: &jquery_ui_unused_deprecated The "%library_id%" asset library is deprecated in drupal:9.2.0 and is removed from drupal:10.0.0. See https://www.drupal.org/node/3067969
   

   We should make supported releases on the replacement modules. That allows people to start updating their code to rely on those modules after this has been committed. We should also update all of the module descriptions and https://www.drupal.org/node/3067969 to include the libraries deprecated here.

2. +++ b/core/modules/system/tests/modules/jqueryui_library_assets_test/jqueryui_library_assets_test.info.yml
   @@ -0,0 +1,5 @@
   +name: 'JqueryUI Library assets test'
   ...
   +description: 'Tests jquery UI library asset loading'
   
   +++ b/core/modules/system/tests/modules/jqueryui_library_assets_test/src/Controller/JqueryUiTestAssetsController.php
   @@ -0,0 +1,42 @@
   +      '#markup' => 'I am a page for testing jqueryUI asset loading order.',
   

   Nit: the name should be written consistently as jQuery UI

Patch addresses #86.2

I created supported releases of the new modules, and updated the related modules list on every module other than jquery_ui and jquery_ui_datepicker, which I'm not a maintainer of. For those I created issues for them to update the list:
https://www.drupal.org/project/jquery_ui_datepicker/issues/3186323
https://www.drupal.org/project/jquery_ui/issues/3186319

Updated those two project pages and added you as a maintainer @bnjmnm

+++ b/core/modules/system/tests/modules/jqueryui_library_assets_test/jqueryui_library_assets_test.info.yml
@@ -0,0 +1,5 @@
+description: 'Tests jquery UI library asset loading'

+++ b/core/modules/system/tests/modules/jqueryui_library_assets_test/src/Controller/JqueryUiTestAssetsController.php
@@ -0,0 +1,42 @@
+      '#markup' => 'I am a page for testing jqueryUI asset loading order.',

These 2 lines are still not showing the correct capitalization asked for in #86

+++ b/core/tests/Drupal/FunctionalTests/Libraries/JqueryUiLibraryAssetsTest.php
@@ -0,0 +1,843 @@
+      $this->assertTrue($found, "A jquery UI file: $file was loaded by the page, but is not taken into account by the test.");

found one more.

Patch still applies cleanly to 8.2.x, should be good to RTBC after these small nits are addressed.

Thanks @zrpnr!

Once the pandemic is over I'll look into getting professional help for my bad jQuery UI capitalization habits 🙂

My name is @bnjmnm and I have a problem with correctly capitalizing the letters in jQuery UI.

Don't be too hard on yourself @bnjmnm ! Those are tough to search for :)

I looked again and those last 2 you fixed in #91 were the only ones I noticed.
I think this is good to RTBC but I'll wait for the tests to finish.

Putting back to RTBC

Committed 42da50d and pushed to 9.2.x. Thanks!

Unfortunately I have to revert because this has broken HEAD - see https://dispatcher.drupalci.org/job/drupal8_core_regression_tests/32220/... for example.

It appears that something about the library info is non-determinate so that different systems can generate different hashes... unfortunately I've not managed to reproduce this fail locally on any environment (php 7.3, 7.4. or 8.0 and sqlite or mysql) so I've got no choice but to revert :(

I've also tried with and without the pecl yaml extension.

Let's see what drupalci can show us...

DiDn't get that right... should have checked the docs first.

Third time lucky lol

So it's the floats... locally for me they look something like this in the serialize string d:188.2... but on DrupalCI they look like... d:188.19999999999998863131622783839702606201171875

The problem is probably that locally you have serialize_precision set to the default value of -1 whereas DrupalCI has it set to 100.

Great catch @lauriii

We can set this in the test to fix this. Also we probably want to find out why CI has that set to 100. But that can take place separately.

+++ b/core/tests/Drupal/KernelTests/Core/Asset/DeprecatedJqueryUiAssetsTest.php
@@ -0,0 +1,58 @@
+  /**
+   * The library discovery service.
+   *
+   * @var \Drupal\Core\Asset\LibraryDiscoveryInterface
+   */
+  protected $libraryDiscovery;
+
+  /**
+   * {@inheritdoc}
+   */
+  protected function setUp(): void {
+    parent::setUp();
+
+    $this->libraryDiscovery = $this->container->get('library.discovery');
+  }

Attaching services to test classes has caused enough issues for it to be bad practice. If a container rebuild happens during the test they get out of date.

Fixed this in the attached patch.

Checked interdiffs in #104 and #105 and both look good for me. ✌️

Committed 1c20159 and pushed to 9.2.x. Thanks!

Second time lucky.

Had a chat with @lauriii earlier today about this issue and other jQuery UI deprecations, and it sounds like we might have underestimated the disruption a bit. Reopening and tagging so I can follow up with @catch about this. Thanks!

This also definitely needs to be in the release notes, so tagging accordingly. Also updating the release note to make it clear that the contrib modules are only a temporary fix.

This was released in the 9.2.0-beta so would be too late to revert I think either way.

The 1c20159 commit/update seems to have broken integration with Gutenberg #3219569: Stuck loading Gutenberg (JS errors) when using Gutenberg with Drupal 9.2, I tried to isolate the bug into a repeatable test module, but I'm not as familiar with the Drupal library/weight architecture and code.

From my investigation into the issue, the bug is definitely related to the weight/asset order of the library, and is only made apparent because the Gutenberg module introduces a lot of JavaScript files in its dependency tree which causes the weight of the AssetResolver::getJsAssets() to shift significantly in some manner because count($javascript) is much larger than what is typically expected from core Drupal site installs.