Automated Drupal 10 compatibility fixes

Beyond this, is there a Drupal 10 punchlist? How can I help?

Drupal 10 is close, any maintainer plans here?

@kevinquillen here is D10 punch list for you :)

================================================================================
ZURB Foundation 6, --
Scanned on Tue, 12/20/2022 - 16:36

FILE: web/themes/custom/zurb_foundation-3304155/inc/zurb_foundation.drush.inc

STATUS         LINE                           MESSAGE
--------------------------------------------------------------------------------
Check manually 47   Call to deprecated function drupal_get_path(). Deprecated in
                    drupal:9.3.0 and is removed from drupal:10.0.0. Use
                    Drupal\Core\Extension\ExtensionPathResolver::getPath()
                    instead.
--------------------------------------------------------------------------------

FILE: web/themes/custom/zurb_foundation-3304155/zurb_foundation.theme

STATUS         LINE                           MESSAGE
--------------------------------------------------------------------------------
Check manually 291  Call to deprecated function drupal_get_path(). Deprecated in
                    drupal:9.3.0 and is removed from drupal:10.0.0. Use
                    Drupal\Core\Extension\ExtensionPathResolver::getPath()
                    instead.
--------------------------------------------------------------------------------
Check manually 306  Call to deprecated function drupal_get_path(). Deprecated in
                    drupal:9.3.0 and is removed from drupal:10.0.0. Use
                    Drupal\Core\Extension\ExtensionPathResolver::getPath()
                    instead.
--------------------------------------------------------------------------------

FILE: web/themes/custom/zurb_foundation-3304155/zurb_foundation.libraries.yml

STATUS         LINE                           MESSAGE
--------------------------------------------------------------------------------
Check manually 0    The 'global' library is depending on a deprecated library.
                    The core/jquery.once asset library is deprecated in Drupal
                    9.3.0 and will be removed in Drupal 10.0.0. Use the
                    core/once library instead. See
                    https://www.drupal.org/node/3158256
--------------------------------------------------------------------------------
Check manually 0    The 'status_in_reveal' library is depending on a deprecated
                    library. The core/jquery.once asset library is deprecated in
                    Drupal 9.3.0 and will be removed in Drupal 10.0.0. Use the
                    core/once library instead. See
                    https://www.drupal.org/node/3158256
--------------------------------------------------------------------------------
Check manually 0    The 'alert_close' library is depending on a deprecated
                    library. The core/jquery.once asset library is deprecated in
                    Drupal 9.3.0 and will be removed in Drupal 10.0.0. Use the
                    core/once library instead. See
                    https://www.drupal.org/node/3158256
--------------------------------------------------------------------------------

FILE: web/themes/custom/zurb_foundation-3304155/zurb_foundation.info.yml

STATUS         LINE                           MESSAGE
--------------------------------------------------------------------------------
Check manually 0    Value of core_version_requirement: ^8 || ^9 is not
                    compatible with the next major version of Drupal core. See
                    https://drupal.org/node/3070687.
--------------------------------------------------------------------------------

But this isn't really resolvable if we want to keep the Drupal 8 compability, as "Drupal\Core\Extension\ExtensionPathResolver" was introduced in Drupal 9.3.x. So I advise the maintainers to create a new dev release, where we can fix the Drupal 10 compability issues and release a new version of this theme having a minimum requirement of Drupal ^9.3

@Grevil: We should have a new semver branch here to resolve this and introduce no BC issues.

Agree with #9.

Fun-fact: We now reach 7.x or 8.x :D No idea what happens with that. Never reached this limit before.

Perhaps start with 9.x or 10.x?

There is no semver on the project currently, right? Just the previous way it was done (Drupal major versions)?

That either leaves it with 1.0.x reset as other projects have done, or 9.0.0/10.0.0 (I guess?). If these changes work in both 9 and 10, I'd vote for 1.0.x reset, but I leave that to others.

Thanks @kevinquillen! Well I'm a bit unsure if composer and others will see a lower number as a newer release. Do you know about that?
So to be safe, my vote would be to have a 10.x branch referring to Drupal 10.

Are there any maintainers we should ask for that? Drupal 10 is out, so I think we should not wait too long (hours / days).

Yeah the only confusing point with 10.x is that it still works on Drupal 9, right?

@sim_1, thoughts?

Yeah the only confusing point with 10.x is that it still works on Drupal 9, right?

Well, it doesn't have to, we can also set ^10 and keep 8.x-6.x for Drupal 9.

I agree with @Anybody!

Here an updated version of the patch.

Not listed here but the stable theme was marked as deprecated, and it was deleted on D10 and this theme depends of that theme.

name: ZURB Foundation 6
description: The most advanced responsive front-end framework in the world, now in Drupal! Based on Zurb Foundation version 6.x.
core_version_requirement: ^8.7.7 || ^9 || ^10
type: theme
base theme: stable

Wondering if this can just be changed to stable9 as a base theme but I guess that will require a good amount of testing right?

I just found out that the stable theme is now a contrib theme:

https://www.drupal.org/project/stable

So, this might help to keep working this theme while the stable9 port is done.

I think the best thing to do for now is adding this contrib stable theme as a dependency of this theme while a new version is created.

Nice find @gnuget!

I also agree with using the stable contrib theme, as this is also recommended in the deprecation notice in https://www.drupal.org/node/3309392!

@gnuget, if you want to help to get up to speed here, consider doing the following:

• Apply patch #19 to the issue fork.
• Set the "core_version_requirement" to ^9.3 || ^10 (See comment #7).
• Apply the new "core/once" syntax in the js files (see change record https://www.drupal.org/node/3158256).
• Add https://www.drupal.org/project/stable as a module dependency.

Hopefully the maintainer agree on a new semver branch soon, and we get this stuff resolved! :)

I think the steps @grevil outlined in #23 seem great!

As for the versioning question, my vote would be that we start by getting all the changes on the issue fork. Then we test updating on an existing d9 theme. If it's a smooth update then I don't think we need a new major version for these fixes.

If the changes aren't easily applied to d9 sites then I'm on board with a new major version and switching over to semver naming. With our current branch being 8.x-6.4, the next semver branch would be named 7.x, which I think is fine and will work with composer. But if people find that confusing we could jump up to 10x. But it doesn't have to mean that 10.x is related at all to the drupal version.

Thanks, @sim_1!

I think the original reason for a new semver branch was to avoid dropping the Drupal 8 support. But since its EOL is reached, I totally agree with you!

If @gnuget doesn't reply soon, I can implement / test the relevant changes! 🙂

Yesterday I upgraded a site to D10 which uses this theme with success.

No big changes required to #20 just add the new contrib stable theme as dependency in the composer.json of the module and adjust the core_version_requirement to ^9.3 || ^10 and that's it.

I will upload a new patch later today.

Thanks for the help.

@gnuget, not to be mean, but that is simply not true! There are 3 occurrences of the old jquery "once" syntax in code and after applying the patch from #20, the module is installable for Drupal < 9.3 which will lead to errors.

All done! Please review the provided MR! :)

Thanks @Grevil, MR!17 LGTM!

RTBC+1 - but I just reviewed the static code, didn't run a test. Someone should do that additionally to set this RTBC.

Oh I thought it was just a matter of change the library in the libraries.yml file and the rest was the same, but it is a new function, duh!

Thanks Grevil.

Should the dependency be added in the composer.json file? that would automatically download the stable theme as soon as someone include this one in their project?

Should the dependency be added in the composer.json file? that would automatically download the stable theme as soon as someone include this one in their project?

Yes, please! Good idea :)

Here a new patch adding the composer's dependency.

Also, this is how I tested it on my D9 site if someone needs a guide:

1. Make sure that the site is ready using the upgrade status module
2. Uninstall temporary the zurb_foundation theme:
   

   drush theme:uninstall zurb_foundation
   composer remove "drupal/zurb_foundation"
   

3. Upgrade the site to D10
4. Install the Composer Drupal Lenient plugin
5. Add the theme in the drupal-lenient allowed list:
   

   composer config --merge --json extra.drupal-lenient.allowed-list '["drupal/zurb_foundation"]'
   

6. Add the new stable dependency, the zurb theme and reinstall it.

   composer require 'drupal/stable:^2.0'
   composer require 'drupal/zurb_foundation:6.x-dev@dev'
   drush theme:install zurb_foundation
   

7. Finally, add this patch in the patches section and execute composer update --lock and that's it.

Thanks @gnuget!

Patches are so 2020 (for good reason :P). Please use the MR's instead. I've added the

"require": {
   "drupal/stable": "^2.0"
}

part.

Thanks for the additional information! :)

I haven't contributed formally to Drupal in a few years hahahah , this Gitlab integration is too new to me 😅

I thought that I was going to need to create a new fork of Grevil's branch and then add my commit having two MR's open, In my mind was easier to me just add a new patch :-p

Knowing that I can just add commits on top of someone else's PR is good to know, Thanks!!!!

I added this as a patch on a D9 site for testing and ran composer update drupal/zurb_foundation --with-dependencies. The patch applied cleanly (yay!), and the theme continued to build/look correct with a spot-check. However, I didn't see the contrib version of stable install. I'm wondering if that's because on D9 sites that namespace is already taken by the core version so D9 installs of this theme won't actually see that change. Can someone test on a D10 site, post their composer steps, and how it went?

Please integrate this into the distribution. Patching in composer does not seem to work.

As far as I know the dependencies are reported in the composer.json file but in the end the process that checks the dependencies is packagist.drupal.org repository.

So until this patch is committed the stable theme won’t be downloaded automatically as a dependency.

That is why the lenient composer plug-in is required to testing this, even doing the required changes in the module composer will refuse to install the module given that the requirements are read from the repository itself and not from the patches version of the module.

@sim_1, interesting, that the module is installable under d9 without the contrib "stable" module!

I always thought, that adding a contrib module to the info.yml dependencies needs to be in the form of:

dependencies:
  - stable:stable

and adding a core module dependency in the form of:

dependencies:
  - drupal:stable

But seems they share the same namespace!

@gnuget did not know about the Lenient Composer Endpoint! Thanks for sharing this information! :)

I'll quickly recheck the compatibility with Drupal 9 and check the compatibility with Drupal 10!

EDIT (ISSUE IRRELEVANT!): Regarding the "Lenient Composer Endpoint" described in the link above, the way it is described there is actually DEPRECATED and does NOT work with Drupal 10! In the deprecation notice, it even references that site, but there is no information on about the incompatibility with Drupal 10 !

Instead of adding a "lenient" repository, you should instead require the mglaman/composer-drupal-lenient composer package and add the specific modules to the "lenient allow-list" using

composer config --merge --json extra.drupal-lenient.allowed-list '["drupal/zurb_foundation"]'

I'll add this information to the docs.

Alright! So I tried testing the composer functionality, but I guess this simply isn't possible.

I thought maybe requiring the "zurb_foundation" dev version via composer, applying this issue's MR as a diff inside the composer.json "patches" section and finally updating everything using composer update drupal/zurb_foundation -W would eventually lead to composer acknowledging the zurb_foundation composer changes that came through the patch, but I guess it will check the original composer dependencies first and then apply the patch, hence it doesn't notice the newly added "drupal/stable" contrib dependency.

What I can say is, that the theme is installable in Drupal 10 after manually requiring "drupal/stable". Maybe we should follow @Anybody's idea and create a new semver branch for a Drupal 10 only version? This way, we can make sure, that requiring this module in Drupal 9 won't lead to conflicts with the core "drupal/stable" theme?

@sim_1, @kevinquillen what do you think? Should we follow the suggestion by @kevinquillen in #13 and reset this module's version to 1.0.x?

Has anyone seen other instances of using 1.x.x on other projects when they start using semver releases over the previous way?

I did some digging and couldn't find any examples of people doing that. I think it's fine just to do it. If it doesn't work we'll push a 7.x.x semver branch instead I guess.

So does it sound right that we'll create a 1.x.x-dev branch to test on first. And then merge this patch in with that if it works successfully?

Lol well I tried and it did not in fact work. I'm going to make a 7.0.x branch instead.

Ok there should now be a 7.x-dev branch to start testing off of. I know it's a little confusing because we're also supporting D7 branches. If people think we should just bump it to 10x or something we can.

@sim_1 very nice! It might be, but I think that is fine! As long as we are getting this D10 ready. :)

Alright! Everything looks good to me!

One of the maintainer needs to change the target-branch of the MR to 7.x, and then we can safely merge this! I would still suggest creating a beta release.
Furthermore, as we can only test the composer.json once there is 7.x a release, I created this follow-up issue here: #3337939: [9.x] Test: Automatic requirement of the drupal contrib stable theme through composer.json.

I also checked if the code additions from #2850823: Turn off base theme css and js in sub themes by default would have any impact on the D10 compatibility, but they don't!

Ok, I'm going to try merging. I've changed the target in gitlab. My only fear right now is that last time i did this, the automatic contrib credit thing didn't work. So fingers crossed that goes smoothly!

Ok, I think that partially worked. @gnuget I tried to give you author credit, but because @anybody was the one to make the commit on the branch I guess it overwrote what I had selected in the checkboxes? If anyone knows a more foolproof way of doing this please let me know. I now think the only way to do it consistently is making and pushing the commit manually/with the command line.
- https://www.drupal.org/docs/develop/git/git-for-drupal-project-maintaine...
- https://www.drupal.org/docs/develop/issues/issue-procedures-and-etiquett...

Ok, marking this issue fixed. I'll make a beta release and then if someone has time to work on #3337939: [9.x] Test: Automatic requirement of the drupal contrib stable theme through composer.json that would great!

Thank you very much @sim_1!

Hum, just saw this on the module page:

And now I'm sadly quite convinced this is super confusing for users. :(
So I think we "have to" create 9.x instead of 7.x and should remove the 7.x branch. Otherwise this will cause UX-trouble, I guess. (Even if it doesn't cause technical issues.)

@Grevil and me are one company, so I think we're fine with the credits. I'll pass him a beer after work ;);)
Thank you! Nice to see this committed anyway!

I think you're right. Ok, changed to a 9.0.0-beta1 release. yay! Nice working everyone getting this put together and tested!

Nice work!

Hi @sim_1

The credit appeared as expected in my profile all good from my side.

This is great news!

Thanks!!!

Great Stuff! Thanks everyone! :)

@sim_1 I got the credit as well, so all good on this end!

In my experience, the credit is given once the issue is set to "fixed". So only the credit setting on that very comment is important. Even if the credits will sometimes "reset" after the "fixed comment" is sent, it gets granted accordingly.

I want to ask on this issue since it has the most visibility: How do we feel about keeping the D8 version as a recommended release? I feel fine (maybe after another week for some more testing) removing it from the recommended release list. I don't feel that we have the overall capacity to maintain both releases and I don't think there's much of a reason to. Do others agree?

@sim_1: Totally agree!

btw, zurb_foundation development seems pretty dead? But that's a different topic / issue...

It does seem that way (Zurb co), but I think the best course of action is to get it Drupal 10 compatible and usable. That will give users a few years until Drupal 11 to change or make a decision on what to do.