Deprecate unused jQuery UI components in favour of a suite of contrib modules

I made a start on this suite of modules:

• jQuery UI Accordion
• jQuery UI Button
• jQuery UI Checkboxradio
• jQuery UI Controlgroup
• jQuery UI Draggable
• jQuery UI Droppable
• jQuery UI Effects
• jQuery UI Menu
• jQuery UI Progressbar
• jQuery UI Selectable
• jQuery UI Selectmenu
• jQuery UI Slider
• jQuery UI Spinner
• jQuery UI Tooltip

They all depend on a core jquery_ui lib which I think could be on the 8.x branch of the existing jquery_ui module.

I think that test failed only because of one word in the description, "from drupal:9.0.0" --> "in drupal:9.0.0". Uploading a new version of that patch to see if it passes testbot now.

marking "needs review"

#7: woah, nice work!

#11:

+++ b/core/core.libraries.yml
@@ -666,6 +666,7 @@ jquery.ui.effects.scale:
+  deprecated: The "%library_id%" asset library is deprecated in drupal:8.8.0 and is removed from drupal:9.0.0. Require jQuery UI as an explicit dependency or create a pure JavaScript solution. See https://www.drupal.org/node/3064015

What does Require jQuery UI mean?

Is it core/jquery.ui? Or is it one of these contrib modules? Should this particular example not point to https://www.drupal.org/project/jquery_ui_effects now?

I wanted to get that test to pass first, and also I wasn't sure if it was a good idea since those new modules don't have a proper release yet.
Here's a patch with all the libraries @lauriii marked deprecated in https://www.drupal.org/project/drupal/issues/3051352

Updated the wording on all those libraries per your suggestion @wim.leers and added tests.

#15 works for me too :) Sorry for the unnecessary work in #14, @zrpnr!

That does seem better, and a lot simpler!
I posted a draft change record and updated the patch for core.libraries.yml and the tests.

I got the 8.x-1.x-dev branch of jquery_ui up and running, will tag a release in the next day or two.

1. +++ b/core/core.libraries.yml
   @@ -472,6 +472,7 @@ jquery.ui.accordion:
   +  deprecated: &jquery_ui_unused_deprecated The "%library_id%" asset library is deprecated in drupal:8.8.0 and is removed from drupal:9.0.0. See https://www.drupal.org/node/3067969
   
   @@ -511,6 +512,7 @@ jquery.ui.checkboxradio:
   +  deprecated: *jquery_ui_unused_deprecated
   

   👏👍

2. +++ b/core/tests/Drupal/KernelTests/Core/Asset/LegacyLibraryDiscoveryTest.php
   @@ -0,0 +1,290 @@
   +  public function testJqueryUiAccordion() {
   ...
   +  public function testJqueryUiCheckboxradio() {
   ...
   +  public function testJqueryUiControlgroup() {
   

   Too bad we can't use setExpectedDeprecation() much like setExpectedException(), that'd allow us to massively simplify this test :)

3. Draft change record looks great — only improved nits.
4. The patch also includes jquery.ui.effects.scale, jquery.ui.effects.shake, *.size, *.slide and *.transfer. Added those to the change record. Those were the only things that were not in sync between the issue summary, the change record and the patch.

I made some small improvements to the CR including switching the order of the columns so that the "before" is more intuitively the first column.

1. However, the links to the projects in the CR seem to 404 (at least the first couple do).
2. I think it also needs a before-and-after example of the codebase changes to add the new dependency (both in the info.yml and an example of a place where the library is registered for use).
3. We shouldn't actually commit this (nor mark it RTBC) until the projects exist and have stable releases.
4. Also, I'd like manual testing of what happens when the contributed project is installed at the same time as core. We could test this with an existing project that depends on one of these libraries -- spin up a test site with the contrib module, follow the change record instructions by installing the contrib module and making the required code changes, ensure the module still works with no errors, ensure it's correctly using the contrib library and not the core one.

Thanks!

Good remarks. I should've indicated that my RTBC was conditional: I agree that this cannot be committed until the contrib projects exist and have stable releases — but I felt it was important to get release manager feedback on the patch's overall approach. I should've used the tag instead, I think?

Thank you for the comments and edits @Wim and @xjm!

1. I had a typo in the urls, these are now fixed.
2. Added an example instruction in the change record
3. I definitely agree with this, yesterday I created dev releases for all these modules, I was hoping to get someone else to look at them before creating a stable release.
4. I created a module for doing manual testing of these deprecated libraries. I copied examples off jqueryui.com and made a page for each using core asset libraries and another using contrib.

Doing manual testing led me to https://www.drupal.org/project/drupal/issues/2926155, I posted a patch there for the current core asset libraries and made the same fix in the contrib modules.

I think this is ready for another look but I also understand (from #21 and #22) it can't be RTBC until these contrib modules are approved and stable.

Sorry for the long silence here! :( This was a lower priority than some other issues, but we better get this wrapped up! Mea culpa.

RE: "manual testing led me to #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" — we got that fixed several weeks ago! 🥳

2. The change record looks great. 👍I only added a few improvements.

   But 🤔 … there is one problem in the change record. It says: Add a dependency to the module or theme's info.yml file. The former is true, the latter is not: themes cannot declare a dependency on a module — that's still a work in progress at #474684: Allow themes to declare dependencies on modules. I'd love to be able to dismiss this as an extremely rare edge case, but I don't think we can: http://grep.xnddx.ru/search?text=core%2Fjquery.ui&filename=.info.yml shows it's pretty common for themes to be overriding CSS assets in jQuery UI asset libraries and to be extending jQuery UI asset libraries with additional CSS assets.

   If it would be only libraries-extend and libraries-override in theme info.yml files and those would only have effect when the extended or overridden asset libraries actually existed, it would not be a problem.
   But \Drupal\Core\Asset\LibraryDiscoveryCollector::applyLibrariesExtend() throws InvalidLibrariesExtendSpecificationException for non-existent asset libraries that are being extended.

   I see only two ways forward:

   1. Block this on #474684: Allow themes to declare dependencies on modules, then recommend in the change record that dependencies on relevant jquery_ui* contrib modules must be declared by themes.
   2. Recommend in the change record that themes convert any libraries-extend or libraries-override declaration involving jQuery UI asset libraries to hook_library_info_alter() implementations instead.

   I think we should go with the second approach, because the first is not at all certain to succeed, and we don't want this critical issue to be blocked unnecessarily. If #474684 then still lands, we can still refine the change record.

4. I did thorough manual testing as was requested using the Deprecated Libraries Example module that @zrpnr created. (I made one pull request: https://github.com/zrpnr/deprecated_libraries/pull/1.) It works fine :) This indeed made it very easy to verify what @xjm asked to verify in #21.4 — thanks!

Keeping at Needs review, tagging Needs subsystem maintainer review and moving to theme system because we really need one and preferably multiple Theme API maintainers to chime in on point 2!

#24.2 Certainly brings up an interesting problem. The exception described there isn't actually triggered as a result of extending non-existing libraries because the library extends are processed based on existing libraries in \Drupal\Core\Asset\LibraryDiscoveryCollector::applyLibrariesExtend().

It seems like this doesn't happen for the libraries overrides either. This is due to the way library overrides are being parsed in: Drupal\Core\Asset\LibraryDiscoveryParser::applyLibrariesOverride. I can't find explicit test coverage for this anywhere but this is fairly easy to test manually.

This does bring up another problem though. At the moment themes could implement features requiring jQuery UI. Since themes cannot define dependencies on modules, there's no way for them to reliably depend on jQuery UI after we've removed jQuery UI from core. I talked to @Wim Leers and we agreed that there aren't any good solutions available to this, other than solving #474684: Allow themes to declare dependencies on modules. However, I'm fairly confident that this affects only a handful of sites. Making this issue postponed on #474684: Allow themes to declare dependencies on modules would increase the risk of having to ship Drupal 9 with most if not all jQuery UI components, therefore I recommend that we bump #474684: Allow themes to declare dependencies on modules to critical, but not make it a hard blocker to this.

I can't find explicit test coverage for this anywhere but this is fairly easy to test manually.

Perhaps to prove this we can just add explicit test coverage for this aspect here? :)

Per #25 and #27, marking Needs work for adding explicit tests that show libraries-override and libraries-extend in themes' info.yml files do not trigger exceptions or errors — they just don't have any effect in that case.

Note that explicit unit test coverage for this does exist at LibraryDiscoveryIntegrationTest::testLibrariesExtend() (asserts InvalidLibrariesExtendSpecificationException is thrown) and LibraryDiscoveryIntegrationTest::testLibrariesOverride(). But @lauriii is saying that in real world circumstances, you will never see any actual errors, because those unit tests are performing calls that the actual theme/render/asset library system never perform.

So the missing test coverage is to make a test theme that overrides a non-existent asset library and extends a non-existent asset library, asserts no errors occur, and then a module is installed that makes those non-existent asset libraries suddenly exist, now the observed behavior should be that the libraries-override and libraries-extend declarations do have an effect.

Looking at the exceptions again, it seems like the exceptions and the unit tests are for a use case where libraries-extend is extending an existing library with a non-existing library.

Example code that would trigger this exception:

libraries-extend:
  core/existing-library
    - core/non-existing-library

It's probably still worthwhile to add explicit test coverage for this, but I don't think we should block this issue on that either. We have lots of libraries-overrides overriding libraries added by modules that could be disabled and we haven't seen any bug reports related to that.

We should update the change records to explicitly include instructions to how themes should handle these deprecations.

WFM! 👍

_{(I just advocated for explicit tests to get 100% confidence. If a Theme API maintainer and frontend committer already feels 100% confident, I'm totally fine with that of course 😃)}

Opened follow-up with @zrpnr: #3078402: Follow-up to #3067251: Add additional test coverage to assets system.

• WRT #24.2.1: We have two core committers saying we don't want to block this on #474684: Allow themes to declare dependencies on modules (@lauriii in #25 and @catch in #27).
• WRT #24.2.2: I was wrong about libraries-extend and libraries-override throwing exceptions (see #25 + #28 + #29). The follow-up that we identified we needed (in #29) was filed (in #32).

So all that remained here was updating the change record. Did that: https://www.drupal.org/node/3067969/revisions/view/11543530/11552019.

I should've written more explicitly in #24.4 that I think the contrib modules are ready. Per @xjm #21, this cannot be RTBC until those modules have stable releases. I think adding those stable releases is the only remaining task here before this is RTBC.

@zrpnr, back to you!

There are now stable releases for all these contrib modules, as well as the revived jQuery UI project.

I updated the IS and the change record to include jQuery UI Tabs, which was added to this list of asset libraries after @lauriii fixed #3070369: Remove Drupal\views_ui\ViewEditForm::form dependency to jQuery UI tabs

🥳

PHP 7.1 & MySQL 5.7 Patch Failed to Apply

Rerolled #17, added test for jquery.ui.tabs deprecation.

Failed successfully! At least that shows the deprecation warnings work...
This is because of #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 which audited and fixed the jQuery UI asset library dependencies, now checkboxradio and controlgroup are dependencies of button which in turn is used by dialog.

If #2158943: Add a native dialog element to deprecate the jQuery UI dialog lands we can mark button deprecated as well, and it should be safe to ignore the warnings for checkboxradio and controlgroup in the meantime.

Added those 2 deprecation messages to DeprecationTrait::getSkippedDeprecations.

Alternatively, I could un-deprecate checkboxradio and controlgroup.

I think #41's approach of skipping those two deprecations by adding them to \Drupal\Tests\Listeners\DeprecationListenerTrait::getSkippedDeprecations() is acceptable because those asset libraries will disappear in Drupal 9 anyway. And they’re new in Drupal 8.8 (the aforementioned #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 introduced them).

In theory, we shouldn't add more "skipped deprecations", but in this case there will not be any code changes needed in Drupal 9.

Using this approach allows us to still deprecate all of the relevant asset libraries.

Lovely work on this issue. I'm so happy to see it RTBC!

I'd like to take some time to review things here and talk it over a bit with other committers, so tagging for RM review for now.

Patch failed to apply

Rerolled against 8.8.x, there were a couple new messages in getSkippedDeprecations which prevented the patch in #41 from applying.
Line numbers changed in core.libraries.yml.

Feel free to re-RTBC yourself in case of a straight rebase 😊

OK so I remember now what I wanted to ask about when I assigned this to myself a couple weeks ago: Now that we know for sure we're going to be retaining at least dialog and draggable in core for BC in 9.0.0, is it still correct for those modules to have a dependency on a contrib jQuery UI module?

Reasoning: When core forks dialog and draggable for BC in 9.0.0, we'll also need to fork the base jQuery UI API. Thus, the library will be in core anyway. Now say you also use jQuery UI Accordion. If the contrib module declares a dependency on a contrib copy of the base API, you now have two copies of it on your site (both of which will need security updates and etc.).

So, given all that, should we remove the contrib dependency from the contrib modules?

Alternately, we could retain the contrib module, but just have it declare a dependency on the core library that forks jQuery UI, instead of retaining its own copy. That's less work, and also probably more future-compatible when we get all of the remaining jQuery UI code out of core in Drupal 10.

We should also confirm what we should be doing in terms of documenting the licenses and copyrights. See #3087886: [policy, maybe patch] core/COPYRIGHT.txt is inconsistent in which libraries or copyrights are mentioned. In core, the libraries both retain the license info in their docblock and have an entry in core/COPYRIGHT.txt (even though not all asset libraries do). In the contrib modules though we don't have any such copyright info documented in the project root.

we could retain the contrib module, but just have it declare a dependency on the core library that forks jQuery UI, instead of retaining its own copy.

I posted an update on the 8.x-1.x branch of jquery_ui that removes all the js and css files from jquery_ui.libraries.yml and adds a dependency to the core asset libraries.

https://git.drupalcode.org/project/jquery_ui/commit/7f5d09ee219b849bb691...

Removed the form-reset library there too since those files are now added directly to the modules that need them, IIRC that was the only remaining difference in the contrib project.

This would be an even cleaner solution if #2905429: Library definitions containing only dependencies should be valid lands! :)

There's now a stable release for the contrib jquery_ui with the change mentioned in #52
I also posted new release for jquery_ui_checkboxradio and jquery_ui_selectmenu since they depended on a file that was previously provided in the contrib jquery_ui module.

The changes to jquery_ui look good. There was 1 small problem and @zrpnr fixed it.

Here is the 2 commits https://git.drupalcode.org/project/jquery_ui/compare/5f653b11df0c2925dcb...

also update to jquery_ui_selectmenu
https://git.drupalcode.org/project/jquery_ui_checkboxradio/commit/dc727c...

and jquery_ui_checkboxradio
https://git.drupalcode.org/project/jquery_ui_checkboxradio/commit/dc727c...

Look good. these were both just changes to not depend on jquery_ui/form-reset but pull in the form-reset-mixin-min.js file directly which is what core is doing.

We filed #3088434: Update license and copyright info to address the bit about the copyright documentation, but that doesn't need to block the core commit since it does not affect the upgrade path.

Rerolled #45 🐣

Committed to 9.0.x, 8.9.x, and 8.8.x, and published the change record. YAY! Drupal 9 JavaScript just got a lot more maintainable and future-compatible.

Just a note about the fact that the contrib modules cannot install under Drupal 9.0.x, see #3097631: Cannot install with Drupal 9.0.x