Automated Drupal 10 compatibility fixes

A few things here...

1. Holy cow this patch is a monster...
2. I think there were some things from the D8 -> D9 upgrade that weren't resolved.
3. Some of the tests rely on the juicebox JS library being installed -- not sure how the D.O. test runner will cope with this...?

Let's see what the test runner thinks about this patch (note - totally a manual effort here. I don't think the patch bot stands a chance with this one...

Add drupalci.yml to see if we can coerce drupalci to install the required libs...

Fix composer.json

wget isn't installed on the build container. Let's try curl instead?

....unzip isn't installed either...hopefully tar is...

Add the -L flag to curl (and run it as the proper user).

Fix web root (hopefully).

Move library to correct directory...

Know what they say, 21st time's the charm.

Hiding incomplete / red patches.

#22 is green on 9.5.x and 10.0.x -- should be good for human review!

Amazing work Luke!

With the exception of the coding standards nits this looks like it should be good to go. It also quickly obsoletes my "juicey" sandbox project. If we can move this forward within the juicebox project itself that will save a lot of duplication of effort.

Just wondering: should we create a new release specifically planning to get a release that works with Drupal 10 and that has your patches? I would love to test these on PHPstorm locally. I am not expert on version naming and numbering but maybe something in the order of 10.x-3.1 labeled as a dev release. This should also work with 9.x code but be primarily intended as code that could be used with Drupal 10.

Having a whole bunch of pending patches sitting on top of one another is not really productive. But we need to commit the patches that pass testing and fix up coding standards so that people can have something to test with. I am just about ready to upgrade my local testing site and then my live shared hosting site to Drupal 10. I'll will happily test away with the hundreds of Juicebox albums I am using.

I will email Neslee to see what he wants to do.

Great job fellas. Thanks for moving this forward. I wish I had the expertise to make this work.

@contiveros ... thanks for the encouragement. I think we are (as of 3/12/2023) a few days away from being able to apply the first round of patches. Then we'll have to assess where that gets us and have a better estimate of when we can do a dev release for Drupal 10. But we always have to be cautious about not counting our chickens before ...

Adjust support range to ^9.3 || ^10 due to service usage added in 9.3. These changes are fundamentally incompatible with versions of Drupal <= 9.2.

I will apply and test these changes locally. It will be on my 10.0.7 local system.

A question: if we adjust the support range on 8.x-3-x will this include
8.x-3.x-dev AND 8.x-3.0-alpha2 ??

Last time I checked these were identical branches. Or should we have a new release that's pegged to Drupal 10 or at least to ^9.3 || ^10 as in your patch. And warn people off from downloading it if they are below 9.3? And perhaps just leave the alpha2 release alone as the last version for anything below 10.

Off topic, but I see I've been added as a maintainer. I'll stay within the bounds of my skills, which are limited. I have some questions about tools to use, but I'll make a separate issue for it so we can keep focused on this immediate patch.

After applying patch to my local system and updating phpunit.xml to include:

" "

all the tests and all assertions ran successfully. This is on 10.0.7. With a temporary hand patched info.yml to allow the module to run under 10.

This is just an enormous enormous improvement due to the patch and a few other adjustments.

I'm going to focus on looking at PHPstan and getting PHPCS running properly so we can validate the code changes as they are completed. I think there is some "technical debt" due to several years of inactivity. PHPSTAN suggest a whole bunch of fixes ... not sure how literally we should take them.

Hey Frank.

I think at this point, it'd be safest to cut 4.x and 4.0.x branches, create a 4.0.x-dev release, change the version of this issue to target 4.0.x, re-run testing, and ultimately merge this into 4.0.x (and subsequently 4.x).

At that point, we can safely cut a 4.0.0-alpha1 release that supports ^9.3 | ^10.

I have a window tomorrow morning to do some of this "administrative" git work and get green tests running against 4.0.x, but for now it's family o'clock :-).

Enjoy Family O'clock.

I wonder at some point if we could run the tests and see the results without:

"09:03:09 sudo -u www-data mkdir -p cd ${SOURCE_DIR}/libraries && cd ${SOURCE_DIR}/libraries && sudo -u www-data curl -L https://github.com/maxstrim/juicebox/archive/refs/tags/1.5.1.tar.gz --output 1.5.1.tar.gz && sudo -u www-data tar -zxf 1.5.1.tar.gz && mv juicebox-1.5.1 juicebox"

What I'm wondering is whether by having the correct modules loaded in the test programs, we might in effect be accomplishing the same thing as getting the maxstrim programs ... the juicebox javascript .. off the Internet and loading them in the container.

That is the only way I can explain why the test programs would work on PHPstorm. I think you made a LOT of corrections in the first patch file that worked ... especially in the USE statements at the tops of the programs and that the rest of the code did not have a chance of working without having that done. Last week I had one of the test programs (I honestly forget which) that was failing and when I added some additional modules in:

  protected static $modules = [
    'node',
    'text',
    'field',
    'image',
    'editor',
    'juicebox',
    'views',
    'contextual',
    'juicebox_mimic_article',
    'juicebox_test_views',
  ];

All of a sudden the tests and assertions passed. I believe it's the juicebox module itself that loads the javascript.

I will have to study the intricacies of version naming. I haven't done much testing on my 10.0.7 system running Juicebox 8-x-3.x with a modified info.yml. I am seeing some log messages such as:

Location	http://ndrupal/web/contextual/render
Referrer	http://ndrupal/web/admin/reports
Message	Error: Call to undefined method Drupal\Core\Config\Entity\ConfigEntityType::isSubclassOf() in Drupal\juicebox\Plugin\Derivative\JuiceboxConfFieldContextualLinks->getDerivativeDefinitions() (line 62 of D:\webpage\compg\web\modules\contrib\juicebox\src\Plugin\Derivative\JuiceboxConfFieldContextualLinks.php

but I realize we need to be more systematic about tracking these down.

re #32 - it looks like isSubclassOf was removed in Drupal 10 ([#2842808]). Seems easy enough to fix, just change isSubclassOf to entityClassImplements.

Now targeting 4.0.x and uploaded a new patch/interdiff that resolves #32.

Let's see how the test runner fares against the new automated test configuration for 4.0.x, fingers crossed!

Sure enough, it now looks like we now have our contextual links back in D9 + D10.

I'm going to commit #34 to 4.0.x.

Since this is a new branch, merging this will do no harm to existing 8.x-3.x users and it'll open us up to making smaller, more controlled and incremental changes. Smaller the scope, smaller the risk :-)

I modified the patch relating to JuiceBoxFieldContextualLinks.php so the patched function reads:

  public function getDerivativeDefinitions($base_plugin_definition) {
    // We need a contextual link defined for each entity type (that may contain
    // a Juicebox gallery) in order to provide a link to the relevant edit
    // display screen. These link definitions must be unique because the related
    // route to the edit display screen is different for each entity type.
    foreach ($this->entityTypeManager->getDefinitions() as $entity_type_id => $entity_type) {
      // Only fieldable entity are candidates.
      if ($entity_type->entityClassImplements('\Drupal\Core\Entity\ContentEntityInterface'))  {
        $this->derivatives['juicebox.conf_field_' . $entity_type_id]['title'] = $this->t('Configure galleries of this field instance');
        $this->derivatives['juicebox.conf_field_' . $entity_type_id]['route_name'] = 'entity.entity_view_display.' . $entity_type_id . '.view_mode';
        $this->derivatives['juicebox.conf_field_' . $entity_type_id]['group'] = 'juicebox_conf_field_' . $entity_type_id;
  }
     }
    return parent::getDerivativeDefinitions($base_plugin_definition);
  }

eliminating what seems to be an unused variable.

I have been spelunking around my Wampserver local test 10.0.7 system for a half hour trying as much Juicebox stuff as I can. Not seeing any errors kick off ... though I do see "curl" related errors due to Wampserver not having the right certificates set up. Addressing that is no a priority right now.

Time to do some file comparisons between 4.0.x-dev and my local juicebox code so we can be on synchronized wave lengths and I can contribute patches according to Drupal git guidelines. I don't know how to test for contextual links. I will have to spelunk around to even see what they are.

Did file comparison yesterday and it looks like we have things pretty well in synch. between 4.0.x-dev and what I've been testing (and have running) locally on Drupal 10. I got bogged down with my local git, unfortunately.

I noticed that:

protected $defaultTheme = 'stark';

is only in the JuiceBoxTestCase program in the tests directory. I read up on a couple of issue queue discussions as well as the documentation and looked at tests in some of the core modules as well as the devel module. It's not clear to me whether we need it in all the programs or not. Devel leaves it out in some cases. The core file module includes it in all. @Luke knows better than I whether it is needed.

At the bottom of JuiceBoxCaseTestBase we have " return $this->drupalPost('contextual/render', 'application/json', $post, ['query' => ['destination' => $current_path]]);" where I believe drupalPost is deprecated and we may need submitForm.

On an unrelated note, I was reading through the Project page. That's going to need some loving. We still reference the need to get the separate libraries module. This is true prior to 8.3 but no longer is applicable. All library discovery is done right in Juicebox now and will stay that way in Drupal 10.

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

Luke.Leber → closed merge request !7