Drupal 10 compatibility fixes

please review the patch

Hello,

Thanks for the patch to solve the error as mentioned above,
I will review the patch

Before applying the patch, getting the below error when applying the bootstrap theme as the administration theme

After applying the patch, the error gets eliminated and bootstrap theme gets configurated as the administration theme

As the patch works, hence the issue status is Reviewed and tested by the community.

Thanks and regards

See core/core.services.yml

  theme.registry:
    class: Drupal\Core\Theme\Registry
    arguments: ['%app.root%', '@cache.default', '@lock', '@module_handler', '@theme_handler', '@theme.initialization', '@cache.bootstrap', '@extension.list.module']
    tags:
      - { name: needs_destruction }
    calls:
      - [setThemeManager, ['@theme.manager']]

+++ b/src/Plugin/Alter/ThemeRegistry.php
@@ -48,7 +48,9 @@ class ThemeRegistry extends Registry implements AlterInterface {
+      \Drupal::service('cache.default'),

I think cache.default should be replaced with cache.bootstrap

+++ b/src/SerializedResponse.php
@@ -160,7 +160,7 @@ class SerializedResponse extends Response {
-  public function getContent() {
+  public function getContent(): string|false {

Change out of scope here.

Better to keep all changes necessary for Drupal 10 under one issue, as #10 is not really out of scope, and the changes by the upgrade bot are useless.

Adding the D10 changes from #3286312: Automated Drupal 10 compatibility fixes, and marking both of them as @noRector to prevent the update bot from trying to fix those in the future, as they are both deprecated code that is already marked be removed in bootstrap 4.x.

The change to composer.json is because that information is redundant, and will be replaced by drupal.org packagist from the core_version_requirement line in bootstrap.info.yml, and to prevent that they start drifting apart, it is better to have only the one that actually does something.

Upon upgrading to Drupal 10, my custom theme based upon Bootstrap 3.25.0 started showing "The website encountered an unexpected error. Please try again later." and the following error in the log...

TypeError: Drupal\Core\Theme\Registry::__construct(): Argument #7 ($runtime_cache) must be of type Drupal\Core\Cache\CacheBackendInterface, string given, called in /app/web/themes/contrib/bootstrap/src/Plugin/Alter/ThemeRegistry.php on line 51 in Drupal\Core\Theme\Registry->__construct() (line 184 of /app/web/core/lib/Drupal/Core/Theme/Registry.php).

... applying the patch in #16 with cweagans/composer-patches, i.e.:

diff --git a/composer.json b/composer.json
index 0aac84e547..b05d054699 100644
--- a/composer.json
+++ b/composer.json
@@ -95,6 +95,11 @@
                 "[project-root]/pantheon.upstream.yml": false,
                 "[project-root]/.gitattributes": false
             }
+        },
+        "patches": {
+            "drupal/bootstrap": {
+                "3302607-16: Drupal 10 compatibility fixes": "https://www.drupal.org/files/issues/2022-11-30/3302607-16.patch"
+            }
         }
     },

... fixed it for me. Indeed, I can see the patch's change to \Drupal\bootstrap\Plugin\Alter\ThemeRegistry::__construct() fixes the issue. A brief glance over the rest of the code in the patch checks out... it seems clear, it conforms to coding standards, and it makes good use of the Drupal 10 API - but it does not conform to the API of Drupal 9, so we may not want to commit this to the bootstrap-8.x-3.x branch yet.

Note that this issue is filed against bootstrap-8.x-3.x, whose core version requirements are ^9.3 || ^10.

This patch makes changes to the call to parent::__construct() in \Drupal\bootstrap\Plugin\Alter\ThemeRegistry::__construct()... it turns out the API for its parent's constructor changed in 9.5.x and 10.0.x...

/*  9.3.x */ public function __construct($root, CacheBackendInterface $cache, LockBackendInterface $lock, ModuleHandlerInterface $module_handler, ThemeHandlerInterface $theme_handler, ThemeInitializationInterface $theme_initialization, $theme_name = NULL, CacheBackendInterface $runtime_cache = NULL, ModuleExtensionList $module_list = NULL) {
/*  9.4.x */ public function __construct($root, CacheBackendInterface $cache, LockBackendInterface $lock, ModuleHandlerInterface $module_handler, ThemeHandlerInterface $theme_handler, ThemeInitializationInterface $theme_initialization, $theme_name = NULL, CacheBackendInterface $runtime_cache = NULL, ModuleExtensionList $module_list = NULL) {
/*  9.5.x */ public function __construct($root, CacheBackendInterface $cache, LockBackendInterface $lock, ModuleHandlerInterface $module_handler, ThemeHandlerInterface $theme_handler, ThemeInitializationInterface $theme_initialization, $runtime_cache = NULL, $module_list = NULL, $theme_name = NULL) {
/* 10.0.x */ public function __construct($root, CacheBackendInterface $cache, LockBackendInterface $lock, ModuleHandlerInterface $module_handler, ThemeHandlerInterface $theme_handler, ThemeInitializationInterface $theme_initialization, CacheBackendInterface $runtime_cache, ModuleExtensionList $module_list, $theme_name = NULL) {

... to be fair, \Drupal\Core\Theme\Registry is marked as @internal, so this API change is allowed according to Drupal's backwards-compatibility policy.

Anyway, the change this patch makes to the call...

diff --git a/src/Plugin/Alter/ThemeRegistry.php b/src/Plugin/Alter/ThemeRegistry.php
index 9961519..fa85bc4 100644
--- a/src/Plugin/Alter/ThemeRegistry.php
+++ b/src/Plugin/Alter/ThemeRegistry.php
@@ -48,7 +48,9 @@ class ThemeRegistry extends Registry implements AlterInterface {
       \Drupal::service('module_handler'),
       \Drupal::service('theme_handler'),
       \Drupal::service('theme.initialization'),
-      $this->currentTheme->getName()
+      \Drupal::service('cache.bootstrap'),
+      \Drupal::service('extension.list.module'),
+      $this->currentTheme->getName(),
     );
     $this->setThemeManager(\Drupal::service('theme.manager'));
     $this->init();

... should work in both Drupal 9.5 and 10.0, but will break compatibility with 9.3 and 9.4.

Note this same change also introduces a trailing comma - a feature that was introduced in PHP 8.0. Bumping the minimum PHP version to 8.0 is fine for Drupal 10 because it's minimum PHP version is 8.1.0; but the minimum version of PHP for Drupal 9.3, 9.4, and 9.5 is PHP 7.3, so this change would not work for D9 users running 7.3.

Note the patch also uses another feature only present in PHP 8.0 and above - a union type for the return typehint of \Drupal\bootstrap\SerializedResponse::getContent().

So, what can we do about it? Drupal 10 users can use the method I described at the top of this comment to apply the patch in #16 for now; but I think we can improve the situation for 9.5.0 and 10.0.0 users.

Firstly, we could remove the trailing comma in \Drupal\bootstrap\Plugin\Alter\ThemeRegistry::__construct() and remove the union typehint for \Drupal\bootstrap\SerializedResponse::getContent(), to make it compatible with PHP 7.3 again.

Next, we could wrap the call to \Drupal\Core\Theme\Registry::__construct() in \Drupal\bootstrap\Plugin\Alter\ThemeRegistry::__construct() in some sort of if/then statement, i.e.: if we're running Drupal 9.5 or higher, then use the new code; otherwise use the old code. I'm running out of time to finish writing this comment, so I don't have any example code right now; but you may find this code in \Drupal\pathauto\Plugin\pathauto\AliasType\EntityAliasTypeBase::getTableInfo() in the pathauto module to be helpful.

@jcnventura I would be happy to review this again if you're able to update the patch.

Thanks for the review @mparker17.

The problem is that nothing can be done for Drupal 9. Drupal 10 uses PHP 8.1 which very much cares about return types if the parent class specifies one. And in Symfony 6, line 406 of vendor/symfony/http-foundation/Response.php states:

   public function getContent(): string|false

So the patch in #16 is a pure Drupal 10.0, solution. The code for Drupal 9.5 will need to not specify that return type, as Symfony 4.3 didn't specify a return type, and of course this means that it is not possible to support Drupal 9 and 10 with the same module version.

Note that the example you provided is fine to differ between Drupal versions, as it affects the content of a function (and we could indeed change the constructor returns).. However, I know of no way to apply a condition to the function declaration. I don't know of any way to do so short of a PHP pre-processor that would certainly require some complex support from Composer or Drupal core.

@jcnventura, ah, good point: I missed that part. I'm also not aware of a way to differ function declarations between Drupal/PHP versions.

In the meantime, I'm not sure what status to put this ticket in. The patch works great for PHP 8.1 and Drupal 10, so this isn't really "Needs work"; in fact, I would RTBC this, except that it would break things for D9, so that wouldn't be very helpful. "Needs review" could result in unnecessary noise on the ticket, as someone does what I did on Friday: review it and notice the same things. "Patch to be ported" doesn't really apply here either.

Normally, I'd suggest that we create a new branch of the project for Drupal 10; but this project has an 8.x-4.x branch already, and it seems to have a bunch of changes in it already (issue queue for 8.x-4.x).

@wundo or @shelane (and/or other maintainers), would you be open to take this opportunity to jump to semantic versioning, by creating a 4.0.x branch from the HEAD of 8.x-3.x (i.e.: so we can create a 4.0.0 release for Drupal 10), and a 5.0.x branch from the HEAD of 8.x-4.x (i.e.: so development on the current 8.x-4.x branch could continue and eventually become a 5.0.0 release?). Or would that be too confusing?

From what I can see, the 4.x branch already claims to have Drupal 10 support, so it might be that in the end, this should be closed as "Won't fix". However, the problem with that is that the 3.x version has already also been tagged as supporting Drupal 10, which is of course completely wrong.

The way ahead seems to be:

• 3.x: Revert the support for Drupal 10. Make that branch support only Drupal 9.5. (^9.5)
• 4.x: Create a release of 4.x, maybe use part of the patch in #16. Make that branch support only Drupal 10 (^10)

@jcnventura, agreed! I support this plan! Is there something I can do to help?

The super tricky part is that the 3.x version supports Bootstrap 3 and the the 4.x branch is for Bootstrap 4. Mark (the originator of this theme( never released the 4.x version and I don’t plan on supporting it. We needed this theme for Bootstrap 3 to be Drupal 10 compatible so we can first go there and then concentrate on updating to Bootstrap 5. We even plan on using an existing Bootstrap 5 theme so we can have both available in our multisite while we convert.

So, I’m not sure what to do with this one. I personally use patches applied with composer for situations like this. As a maintainer with the Bootstrap version issues, it’s hard to know the best course. If it wasn’t about Bootstrap, I would certainly tag a D10 specific release.

Thoughts?

@shelane From what I've seen recently in another project, it seems that PHP 8 is actually fine with having this kind of difference between D9 and D10's getContent() return types.. However, even if this code would make it compatible with both D9 and D10, it does raise the minimum PHP version to PHP 8(.1). I can do a trivial change to #16 to add the php requirement to composer.json, if that would be a good compromise for how to go ahead with having a 3.x version that is ready for Drupal 10.

I don’t mind raising the minimum php requirement. Those on php 7 can stay the current version. I can run tests and work on that when I’m back at work in January.

This fixes the feedback about php version.

The patch in #26 works great, allows me to run Drupal 10 + Bootstrap 3 + php8.1. Anything pending in order to tag a release with this patch?

As soon as I can. I had some major production issues to fix for my own work last week. I hope to get to this sometime this week, but I can make no guarantees.

Setting to RTBC as per #27

#26 works against the current Drupal 10.0.2 w/ php 8.2.1 Now that D10 is no longer a beta release it would be great to get this into patch committed.

#26 worked for me, thanks!

Thank you Heddn

Any chance the patch in #26 could at least be committed to 8.x-3.x-dev?

Hello,

I have found another compatibility related with the deprecated use of jquery.once in Drupal 10. I have created another issue https://www.drupal.org/project/bootstrap/issues/3336901

Thanks for that @dromansab. I think we need to handle the transition to core/once on this issue and mark #3336901: Uncaught TypeError: $(...).once is not a function with Drupal 10 as a duplicate.

There's no real D10 support while the theme continues to use jquery.once.

Indeed. This is where I could use some community help with converting that code.

Is it possible that a fix can happen against the 3.x branch? If not - shouldn't 3.x have D10 compatibility removed so it correctly shows as incompatible (and this task get bumped to the 4.x branch)?

Any thoughts about bumping this to Major priority?

I wish I knew enough about the Bootstrap/JS side to help here. I don't think I can upgrade by Bootstrap sites to D10 until this is resolved.

EDIT: Actually, there's pretty good instructions on how to fix jquery.once here. Let me have a go at it.

OK, I make NO guarantees here, but this is my first pass at revising the jquery .once() as per the instructions as described in https://www.drupal.org/node/3158256

Attached is the jquery patch against the latest 8.x-3.x dev branch, and also a revised patch that includes @heddn's patch from #26

I expect this to be broken, do NOT deploy this to production sites, but hopefully this pushes the issue forward.

Sorry, just getting lost a little in the back and forth. I've got the Response::getContent(): error in a brand new deployment of D10 with Bootstrap 3.26 on PHP 8.1.14. So I am apply Trevors's patches, but I'm getting the error:

patch --dry-run -ruN -d bootstrap < 3302607-39.patch
checking file bootstrap.info.yml
checking file composer.json
can't find file to patch at input line 29
Perhaps you should have used the -p or --strip option?
The text leading up to this was:
--------------------------
|diff --git a/js/misc/batch.js b/js/misc/batch.js
|index a424305..70af6c6 100644
|--- a/js/misc/batch.js
|+++ b/js/misc/batch.js
--------------------------
File to patch:

Am I not applying this correctly?

@geoffrsweet - just to clarify:

3302607-26.patch tried to fix issues with the composer install. However, people were also saying jquery.once is no longer supported, and will also be a D10 blocker.

I created bootstrap_jquery_once.patch as a first draft to updating jquery.once(), but it doesn't include the fixes in 3302607-26.patch.

3302607-39.patch contains both heddn's 3302607-26.patch and my jquery.once fix.

If you're talking about the bootstrap settings page at /admin/appearance/settings/bootstrap - I can access this page without error with the 3302607-39.patch patch.

EDIT: Also note the patch is against the latest 3.x-dev branch (0faeded17) of bootstrap, not the 3.26 branch. (There are some differences between 3.26 and 3.x-dev#0faeded17 in the bootstrap.libraries.yml file).

3302607-39.patch worked for me, Drupal 10.0.3.

Thank you!

I think I am dumb here but I removed the stable version of bootstrap and replaced it with the dev branch. Then reran my patch and I'm still getting file not found errors:

:$ patch --dry-run -ruN -d bootstrap < 3302607-39.patch
checking file bootstrap.info.yml
checking file composer.json
can't find file to patch at input line 29
Perhaps you should have used the -p or --strip option?
The text leading up to this was:
--------------------------
|diff --git a/js/misc/batch.js b/js/misc/batch.js
|index a424305..70af6c6 100644
|--- a/js/misc/batch.js
|+++ b/js/misc/batch.js
--------------------------
File to patch:
Skip this patch? [y] q
Skipping patch.
3 out of 3 hunks ignored
can't find file to patch at input line 57
Perhaps you should have used the -p or --strip option?
The text leading up to this was:
--------------------------
|diff --git a/js/misc/form.js b/js/misc/form.js
|index 0b09d16..9712243 100644
|--- a/js/misc/form.js
|+++ b/js/misc/form.js
--------------------------
File to patch:

I'm in /themes/contrib/ which is where composer installs bootstrap. The themes/contrib/bootstrap/js/misc/batch.js file is clearly there.

@geoffrsweet: Most of us are using composer. Maybe I'm not applying patches the way you do. This is working for me:

$ git clone --branch '8.x-3.x' https://git.drupalcode.org/project/bootstrap.git
$ cd bootstrap
$ patch -p1 < ../3302607-39.patch
patching file bootstrap.info.yml
patching file composer.json
patching file js/misc/batch.js
patching file js/misc/form.js
patching file js/misc/vertical-tabs.js
patching file js/modules/filter/filter.js
patching file js/text/text.js
patching file js/theme-settings.js
patching file src/Bootstrap.php
patching file src/Plugin/Alter/ThemeRegistry.php
patching file src/Plugin/Provider/ProviderBase.php
patching file src/SerializedResponse.php

I managed to get

patch --dry-run -p1 -d bootstrap < 3302607-39.patch

to work. Not sure what -ruN does - the documentation explicitly says to use the p1 parameter.

EDIT: I'd strongly suggest taking the time to learn to use composer for package management. It looks intimidating at first but it's going to save a ton of time updating packages and maintaining patches in the long run. It's really powerful and really good, and should be as much a part of your daily toolkit as drush. (In a "composer update && drush deploy", and you're done kind of way)

More info here.

Hi, #45 worked for me. Thanks Trevor!, but off course when I do Composer Update, I have to add patch again. So please commit. Thanks!

@Summit - you can add patches to composer.json so that it applies the patch every time you run composer install or composer update. It's super convenient. Instructions here.

Oh good grief. I'm an idiot. Yes ok, correcting the command works fine and the patch solves this for me. Also thanks for the instructions on patching. I've made those updates too.

I did two separate commits to separate the credit listings for the jQuery once and the other D10 issues. I've also updated the minimum version to be 9.5 (which I believe also requires php 8.1). I'm currently checking to see if there are any other issues that need to be included in a release. Most likely release will be on Tues, Feb 21.

Thanks! @TrevorBradley
(#39) 3302607-39.patch works for me...

can you or anyone show me a path to refactor this piece of code with new once syntax?

      var check_flags = $('header').once().find('#block-quicklinksmini .menu--quick-links-mini');
      if (check_flags.length != 0) {
        $("header > .container").once().prepend(
          "<div class='drop-down-mobile'><span class='selected'>日本語</span><ul class='menu'></ul></div>"
        );
        $("#block-quicklinksmini .menu--quick-links-mini li").once().each(function() {
          if ($(this).hasClass("menu-icon")) {
            $(this).clone(true).appendTo(".drop-down-mobile ul");
          }
        });
      }

N.B- I don't know how to find or create once-id
is it possible to refactor the code without the once-id?

I have studied some doc regarding this issue-

https://www.drupal.org/project/drupal/issues/3183149
https://www.drupal.org/project/drupal/issues/3312450
https://www.drupal.org/project/charts/issues/3261881
https://www.drupal.org/node/3158256

@Sourav_Paul: There's plenty of examples over at #3158256: Remove jQuery dependency from the once feature : https://www.drupal.org/node/3158256

The "More code change examples" talks about an alternate way to do once():

$(once('tableresponsive', 'table', context)).find(...
$(once('tableresponsive', 'table', context)).prepend(...
$(once('tableresponsive', 'table', context)).each(...

Should all work in theory.