Prepare Field Group for Drupal 9

1. +++ b/composer.json
   @@ -5,6 +5,6 @@
        "minimum-stability": "dev",
        "require": {
   -        "drupal/core": "^8 || ^9"
   +        "drupal/core": "^8.8 || ^9"
        }
    }
   

   I have noticed that adding/changing a composer.json means that things are not installed, because the way this works is that the composer.json is first installed, then patched and then the module is composer-require'd again from that checkout, and everything that's not in there is lost.

   Try adding the dependencies to composer.json *too* (so in both places)

2. +++ b/field_group.libraries.yml
   @@ -26,7 +26,7 @@ formatter.accordion:
        formatters/accordion/accordion.js: {}
      dependencies:
   -    - core/jquery.ui.accordion
   +    - jquery_ui_accordion/accordion
   

   tricky. this is the correct solution, but I guess a better long-term solution would be to find a different way to implement this that doesn't require the basically unsupported jquery.ui. It has been removed from core for a reason.

   Will let the maintainers decide on that. One option would be to put it in a new optional submodule only only require jquery_ui_accordion from that, or even a separate project, but that might need a new major version?

3. +++ b/tests/src/Functional/FieldGroupWithoutFieldUiTest.php
   @@ -15,7 +15,7 @@ class FieldGroupWithoutFieldUiTest extends BrowserTestBase {
       */
   -  public static $modules = ['field_group', 'block'];
   +  public static $modules = ['field_group', 'block', 'jquery_ui_accordion'];
    
   

   a browser test shouldn't need this as it should respect module dependencies.

Ok, added the dependency to composer.json to see if that helps. If we're going down that path then this will also require an update function to enable those new dependencies.

I've also fixed some #pre_render deprecation messages, not sure if it makes sense to add the new one on \Drupal\field_group\FormatterHelper.

Also improved the assert count stuff, some expected counts were off and I used assertCount() now as some of the count() placements were wrong. Most of those could probably be rewritten to much nicer CSS selectors now, but that's a bit more work.

Forgot to make the new callback static.

There's also field_group_form_process() (and field_group_form_pre_render but that seems a BC layer this point). And then paragraphs, profile and some other modules will need to be updated as well to support that callback.

Some more progress, converting the form process stuff. Probably makes more sense to make that a separate class/service for these callbacks.

I guess a better long-term solution would be to find a different way to implement this that doesn't require the basically unsupported jquery.ui. It has been removed from core for a reason.

Agree with Berdir in #5 on that one. I don't think we should add a dependency here that is deprecated already. Looks like Drupal core is moving to vanilla Javascript. I suggest we take this route as well.

> I suggest we take this route as well.

It's very simple. Doing takes a lot more time than than just adding the extra module dependency. If someone steps up and rewrites the JS, great. If not, then we have the fallback for now so we can update to D9, that's exactly why those legacy contrib projects were created.

Rewriting the JS would take to much time indeed. I'd rather want to unblock DS and Paragraphs so those projects can keep on preparing for D9, so the current approach makes sense to me. I guess the only thing that's left is an upgrade path. I'll roll a patch for that today to see if DS is unblocked then already too.

Maybe we can also already depend on 8.9 and release a new version of field group the day Drupal 8.9 comes out?
I have the same thing for Display Suite, so I'm definitely available that day to push releases anyway :)

Ok, this patch adds the upgrade path and sets dependency for core on 8.9

Great: tests are green for D8 and D9 and I can also confirm DS tests work too now for D9 when I apply this patch for Field group.

So I'm +1 personally :)

I've pinged Zuuperman for his opinion as I don't want to make this decision on my own, but I'm very much in favor of it.
We can always open a follow-up issue to rewrite the JS, and even try to get that in before D8.9 comes out, but unblocking other projects is much more valuable at the moment IMHO.

1. +++ b/composer.json
   @@ -5,7 +5,7 @@
        "require": {
   -        "drupal/core": "^8.8 || ^9",
   +        "drupal/core": "^8.9 || ^9",
            "drupal/jquery_ui_accordion": "^1.0"
   

   Why is this necessary? 8.9 will come out on the same day as 9.0, 8.8 should have everything we need here?

2. +++ b/field_group.install
   @@ -11,3 +11,14 @@
   + */
   +function field_group_update_8302() {
   +  // Enable the jQuery UI accordion module if needed.
   +  if (!\Drupal::moduleHandler()->moduleExists('jquery_ui_accordion')) {
   +    \Drupal::service('module_installer')->install(['jquery_ui_accordion'], FALSE);
   +    return t('The "jquery_ui_accordion" module has been installed.');
   +  }
   +}
    
   

   Wondering if you want to check that the new dependency is present before trying to enable it and abort the update with an exception if not? not everyone uses composer yet and would get the dependency automatically.

Why is this necessary? 8.9 will come out on the same day as 9.0, 8.8 should have everything we need here?

I've had breaking releases with DS where I removed deprecations which didn't work anymore then with the previous core version because the interface wasn't available yet for instance (#3075509: 3.x-dev broken on Drupal 8.6 after Deprecation of ConfigurablePluginInterface comes to mind). So I guess I just wanted to be conservative :)

However, if everything is fine, that I'm ok with putting that on 8.8.

.. new dependency ...

Oh right, good call. Can we fix that by adding a hook_requirements() ? Or can an update hook function return FALSE to signal that something is missing?

FWIW, there is one downside to adding the dependency "for now". Module dependencies are awkward to get rid of again, because if you remove it in an update then it will still be installed on all existing sites but composer will remove the code unless a site depends on it.

One option that would however require a bit more work for both the patch and users is to move that feature into a submodule and only make that depend on jquery_ui_accordion, making it just a suggestion/optional dependency. Then users will have to require the new module themself (the update function could optionally enable the modules if it's present). Advantage is that the dependency is then explicitly registered in the project composer.json and users won't just lose it if someone ever rewrites things to not depend on it.

Crosspost. Yeah, this should only require 8.8. I prefer running tests by default against the current stable version (8.8), to avoid accidentally depending on something that isn't possible yet but 8.9 doesn't have any new deprecations or so anyway, to avoid that projects need to depend on it to be D9 compatible.

And yes, you could do a requirements hook with op = update error, drush will only show that as a message that can be skipped but the UI should prevent you from actually updating then.

One option that would however require a bit more work for both the patch and users is to move that feature into a submodule

Hmmm .. tricky as well. Sooo, how about we implement field_group_library_info_alter() and switch the library depending on what you have installed? That allows to

• not set an explicit dependency on jquery_ui_accordion in composer.json
• just write a hook_requirements to warn about the deprecation and tell to install jquery_ui_accordion for D9
• refactor js and when that's done keep hook_requirements to tell that jquery_ui_accordion may be uninstalled (unless some other dependency needs it of course)
• just enable it in the hook_update now if it's available

We'd probably need to add test-dependencies then in info file, but I can live with that.

That sounds good, then users will only be required to actively do something when they update to Drupal 9 and won't be surprised with a regular update on Drupal 8, where (almost) nobody reads release notes :)

> We'd probably need to add test-dependencies then in info file, but I can live with that.

You want to add it to require-dev in comoser.json instead, because that also works in patches, but yeah.

New patch.

• Core dep on 8.8
• added hook_requirements
• hook_update only installs if it's available
• swap library for D9

If this works, I'm actually going to commit this as it's a good middle ground. There's no pressure for refactoring and contrib and work projects don't have to follow field group closely for future releases - unless they override the default formatter js, but hey, than you should really read release notes :)

Looks good to me. Testing the patch in our install profile now.

Last patch seems like a good solution for me. Better than requiring the dependency for everyone, as accordion groups are probably only used on < 10% of the sites. It gives us the time to think about a replacement for the javascript without switching to a submodule.

Ok cool, I'll wait for a ping from Berdir and then commit :)

This looks good to me as well, however I would ask if you could make a release before this patch makes it in? That'd give folks using older versions of D8 one last update before field_group requires 8.8. 8.7 is still supported in core, and for various reasons customers may be stuck on other older versions of d8.

I think this is fine, seems works for us but we don't actually use accordions :)

Whether or not to do a release is up to the maintainers, but from a quick look at the commits since 3.0 2 month ago, there doesn't really seem anything there that would be worth a release.

I'm fine with another release before we commit this, but I'll let Zuuperman decide on that because I follow the queue less frequently - usually blocking issues ;) I'd love to get DS further by the beginning of next week, so there's at least some time to commit other issues first if needed and push a release then.

I doubt we need another release. All commits between 3.0 and now are code cleanups. So no big bugfixes or new features are waiting for a release.

Conflict in composer.json.

committed and pushed, thanks all!

Thanks!

Paragraphs had a first proper test run against D9 and is almost passing now, including the field group integration test: #3119813: Fixing new/remaining Drupal 9 test fails .

Just updated field_group to b925d0b and it breaks "updb -y":

>  [notice] Update started: field_group_update_8302
>  [error]  The module jquery_ui_accordion does not exist.
>  [error]  Update failed: field_group_update_8302
 [error]  Update aborted by: field_group_update_8302
 [error]  Finished performing updates.

I think the problem is that:

if (\Drupal::service('extension.list.module')->getName('jquery_ui_accordion')) {

throws an exception rather than true/false, and this exception then breaks the updater.

This patch fixes the problem, though perhaps not in the best way possible:

 function field_group_update_8302() {
+  try {
+    // Enables the jQuery UI accordion module if it exists.
+    if (\Drupal::service('extension.list.module')->getName('jquery_ui_accordion')) {
+      \Drupal::service('module_installer')->install(['jquery_ui_accordion'], FALSE);
+      return t('The "jquery_ui_accordion" module has been installed.');
+    }
+  }
+  catch (\Exception $e) {
+    return
+      t('If you want to use the Field Group accordion formatter, you will need to install the <a href=":link" target="_blank">jQuery UI Accordion</a> module.',
+        [':link' => 'https://www.drupal.org/project/jquery_ui_accordion']);
   }
 }

Ah damn! :)

This patch fixes the problem, though perhaps not in the best way possible:

I don't think there's another way, so looks fine to me.

committed and pushed, thanks!

Thank you for this. I am facing the issue field groups: Render #pre_render callbacks must be methods of a class that implements \Drupal\Core\Security\TrustedCallbackInterface or be an anonymous function. The callback was Drupal\field_group\Element\VerticalTabs::preRenderGroup. Support for this callback implementation is deprecated in 8.8.0 and will be removed in Drupal 9.0.0. See https://www.drupal.org/node/2966725 on 3.0 and it seems the fix went in as part of this issue.

I created #3154304: Remove jQuery UI Accordion dependency and warning for removing jQuery UI dependency completely.