OEL-4511: Update to Drupal 11.3.x. by tibi2303 · Pull Request #541 · openeuropa/oe_bootstrap_theme

tibi2303 · 2026-03-03T11:17:40Z

Jira issue(s):

D8AGE-???

donquixote

some feedback.

I am still undecided if it would be acceptable to simply accept or suppress the deprecated calls. I am trying to find the discussion in drupal.org where this was mentioned.

donquixote · 2026-03-06T12:41:15Z

theme-settings.php

 * Implements hook_form_system_theme_settings_alter().
 */
 function oe_bootstrap_theme_form_system_theme_settings_alter(&$form, FormStateInterface $form_state) {
+  $theme_name = 'oe_bootstrap_theme';


That local variable seems unnecessary.

donquixote · 2026-03-06T16:18:22Z

src/ThemeSettings.php

+      $service_id = 'Drupal\Core\Extension\ThemeSettingsProvider';
+      $settings_provider = \Drupal::service($service_id);


Suggested change

$service_id = 'Drupal\Core\Extension\ThemeSettingsProvider';

$settings_provider = \Drupal::service($service_id);

$settings_provider = \Drupal::service(Drupal\Core\Extension\ThemeSettingsProvider::class);

no need for local var

use ::class so that static analysis sees the class name.

and then use class import

use Drupal\Core\Extension\ThemeSettingsProvider;

Suggested change

$service_id = 'Drupal\Core\Extension\ThemeSettingsProvider';

$settings_provider = \Drupal::service($service_id);

$settings_provider = \Drupal::service(ThemeSettingsProvider::class);

donquixote · 2026-03-06T16:19:23Z

src/ThemeSettings.php

+    if (version_compare(\Drupal::VERSION, self::THEME_SETTINGS_PROVIDER_VERSION, '>=')) {
+      $service_id = 'Drupal\Core\Extension\ThemeSettingsProvider';
+      $settings_provider = \Drupal::service($service_id);
+      return \call_user_func([$settings_provider, 'getSetting'], $setting, $theme);


why call_user_func?

Suggested change

return \call_user_func([$settings_provider, 'getSetting'], $setting, $theme);

return $settings_provider->getSetting($setting, $theme);

If you are concerned about static analysis, you can put a /** @var */ doc on the service.

donquixote · 2026-03-06T16:21:59Z

src/ThemeSettings.php

+      $service_id = 'Drupal\Core\Extension\ThemeSettingsProvider';
+      $settings_provider = \Drupal::service($service_id);
+      return \call_user_func([$settings_provider, 'getSetting'], $setting, $theme);


if you don't need the local var for the service:

Suggested change

$service_id = 'Drupal\Core\Extension\ThemeSettingsProvider';

$settings_provider = \Drupal::service($service_id);

return \call_user_func([$settings_provider, 'getSetting'], $setting, $theme);

return \Drupal::service(ThemeSettingsProvider::class)->getSetting($setting, $theme);

The local var for the service is useful only if you want to put a /** @var */ doc.

donquixote · 2026-03-06T16:23:13Z

src/ThemeSettings.php

+    $legacy_getter = 'theme_get_setting';
+    return \call_user_func($legacy_getter, $setting, $theme);


Suggested change

$legacy_getter = 'theme_get_setting';

return \call_user_func($legacy_getter, $setting, $theme);

// phpcs:ignore <deprecation code>

return theme_get_setting($setting, $theme);

not sure how that phpcs line needs to look like or if you need it at all.

donquixote · 2026-03-06T16:24:52Z

theme-settings.php

    '#title' => t('Style all tables using Bootstrap.'),
    '#description' => t('Applies Bootstrap classes to all tables rendered using this theme. Note that some table instances (such as calendars) might not render correctly when Bootstrap is applied to them.'),
-    '#default_value' => theme_get_setting('bootstrap_tables.enable'),
+    '#default_value' => ThemeSettings::get('bootstrap_tables.enable', $theme_name),


Suggested change

'#default_value' => ThemeSettings::get('bootstrap_tables.enable', $theme_name),

'#default_value' => ThemeSettings::get('bootstrap_tables.enable', 'oe_bootstrap_theme'),

But do you even need to pass the theme name at all?
Shouldn't it work without that?

Suggested change

'#default_value' => ThemeSettings::get('bootstrap_tables.enable', $theme_name),

'#default_value' => ThemeSettings::get('bootstrap_tables.enable'),

This also helps to support overriding the setting in a subtheme, which I think was the goal of theme_get_setting().

donquixote · 2026-03-06T16:34:56Z

src/ThemeSettings.php

Alternatively we could name this class DrupalCompatibility and the method themeGetSetting(), this was suggested by AI :)

The point would be to add more like this in the future.

it's a bit confusing with the existing class BackwardCompatibility, which is meant not to support old Drupal versions but to solve another kind of problem (not sure how that should work)

donquixote · 2026-03-06T16:50:53Z

src/ThemeSettings.php

+
+    $legacy_getter = 'theme_get_setting';
+    return \call_user_func($legacy_getter, $setting, $theme);
+  }


Actually, something better!
https://www.drupal.org/node/3379306
Can you try this?

use Drupal\Component\Utility\DeprecationHelper; [...] return DeprecationHelper::backwardsCompatibleCall( \Drupal::VERSION, '11.3', \Drupal::service(ThemeSettingsProvider::class)->getSetting(...), theme_get_setting(...), );

donquixote · 2026-03-06T16:53:31Z

oe_bootstrap_theme.theme

+ */
+function _oe_bootstrap_theme_get_setting(string $setting): mixed {
+  return ThemeSettings::get($setting, 'oe_bootstrap_theme');
+}


Why this function, instead of calling the static method directly?

donquixote · 2026-03-06T17:06:02Z

src/DrupalCompatibility.php

+      static function () use ($setting, $theme): mixed {
+        return \theme_get_setting($setting, $theme);
+      }


we can simplify with fn (). Also use trailing comma on last arg.

Suggested change

static function () use ($setting, $theme): mixed {

return \theme_get_setting($setting, $theme);

}

fn () =>\theme_get_setting($setting, $theme),

(and same for the other one)

donquixote · 2026-03-06T17:08:43Z

src/DrupalCompatibility.php

+      static function () use ($setting, $theme): mixed {
+        $settings_provider = \Drupal::service('Drupal\Core\Extension\ThemeSettingsProvider');
+        if (is_object($settings_provider) && method_exists($settings_provider, 'getSetting')) {
+          return $settings_provider->getSetting($setting, $theme);
+        }
+
+        throw new \RuntimeException('ThemeSettingsProvider::getSetting() is not available.');
+      },


No need for all the defensive code.
If we have the correct Drupal version, it will just work. The service won't surprisingly be something we did not expect.
If it is, we will get some kind of error and we will know.
Also always use ::class, not string literal with quotes for a class name. This way an IDE can do "Find usages" etc.

Suggested change

static function () use ($setting, $theme): mixed {

$settings_provider = \Drupal::service('Drupal\Core\Extension\ThemeSettingsProvider');

if (is_object($settings_provider) && method_exists($settings_provider, 'getSetting')) {

return $settings_provider->getSetting($setting, $theme);

}

throw new \RuntimeException('ThemeSettingsProvider::getSetting() is not available.');

},

fn () => \Drupal::service(ThemeSettingsProvider::class)->getSetting($setting, $theme),

If there is any problem with phpcs or phpstan when running a different core version, we suppress.

As a general note: Defensive code is more important when we deal with user data, or with anything that could have different values or states for reasons we do not control.

donquixote · 2026-03-06T22:31:37Z

src/DrupalCompatibility.php

+      \Drupal::VERSION,
+      self::THEME_SETTINGS_PROVIDER_VERSION,
+      fn () => \Drupal::service(ThemeSettingsProvider::class)->getSetting($setting, $theme),
+      fn () =>\theme_get_setting($setting, $theme),


We need a whitespace here.
And the \ backslash is debatable, we typically don't add it.

Suggested change

fn () =>\theme_get_setting($setting, $theme),

fn () => theme_get_setting($setting, $theme),

donquixote · 2026-03-06T22:34:46Z

oe_bootstrap_theme.theme

  $extra_suggestions[] = 'views_view_table__bootstrap';

-  $responsive = theme_get_setting('bootstrap_tables.responsive');
+  $responsive = DrupalCompatibility::themeGetSetting('bootstrap_tables.responsive', 'oe_bootstrap_theme');


Again, why pass the theme name?

I assume we want the following to work:

You enable oe_whitelabel, which is a subtheme of oe_bootstrap_theme.

In oe_whitelabel, you have the same config options available (and more).

Setting values are stored separately for oe_whitelabel and oe_bootstrap_theme.

When this function is called while in oe_whitelabel, we want the setting value from oe_whitelabel to be used.

Suggested change

$responsive = DrupalCompatibility::themeGetSetting('bootstrap_tables.responsive', 'oe_bootstrap_theme');

$responsive = DrupalCompatibility::themeGetSetting('bootstrap_tables.responsive');

I don't fully remember if this is how these theme settings work, please try it out.
But there is a reason why until now we did not pass along the theme name.

donquixote · 2026-03-09T10:26:14Z

src/BackwardCompatibility.php

   */
  public static function getSetting(string $name): bool {
-    return theme_get_setting(self::PREFIX . $name) ?? TRUE;
+    return (bool) (DrupalCompatibility::themeGetSetting(self::PREFIX . $name, self::THEME) ?? TRUE);


We are still passing the theme name?

Suggested change

return (bool) (DrupalCompatibility::themeGetSetting(self::PREFIX . $name, self::THEME) ?? TRUE);

return (bool) (DrupalCompatibility::themeGetSetting(self::PREFIX . $name) ?? TRUE);

donquixote · 2026-03-09T10:26:39Z

src/BackwardCompatibility.php

+   * The theme where backward compatibility settings are stored.
+   */
+  private const THEME = 'oe_bootstrap_theme';
+


should not be needed.

donquixote · 2026-03-09T10:27:06Z

theme-settings.php

    '#title' => t('Style all tables using Bootstrap.'),
    '#description' => t('Applies Bootstrap classes to all tables rendered using this theme. Note that some table instances (such as calendars) might not render correctly when Bootstrap is applied to them.'),
-    '#default_value' => theme_get_setting('bootstrap_tables.enable'),
+    '#default_value' => DrupalCompatibility::themeGetSetting('bootstrap_tables.enable', 'oe_bootstrap_theme'),


Suggested change

'#default_value' => DrupalCompatibility::themeGetSetting('bootstrap_tables.enable', 'oe_bootstrap_theme'),

'#default_value' => DrupalCompatibility::themeGetSetting('bootstrap_tables.enable'),

donquixote · 2026-03-09T10:27:17Z

theme-settings.php

    ],
    '#empty_option' => t('Never'),
-    '#default_value' => theme_get_setting('bootstrap_tables.responsive') ?? '',
+    '#default_value' => DrupalCompatibility::themeGetSetting('bootstrap_tables.responsive', 'oe_bootstrap_theme') ?? '',


Suggested change

'#default_value' => DrupalCompatibility::themeGetSetting('bootstrap_tables.responsive', 'oe_bootstrap_theme') ?? '',

'#default_value' => DrupalCompatibility::themeGetSetting('bootstrap_tables.responsive') ?? '',

donquixote · 2026-03-09T13:00:01Z

theme-settings.php

    '#title' => t('Style all tables using Bootstrap.'),
    '#description' => t('Applies Bootstrap classes to all tables rendered using this theme. Note that some table instances (such as calendars) might not render correctly when Bootstrap is applied to them.'),
-    '#default_value' => theme_get_setting('bootstrap_tables.enable'),
+    '#default_value' => DrupalCompatibility::themeGetSetting('bootstrap_tables.enable'),


It seems we need to remove this line for correct behavior. Drupal ConfigFormBase will auto fill the setting. To be confirmed if this works in Drupal 10, I think it should.

Suggested change

'#default_value' => DrupalCompatibility::themeGetSetting('bootstrap_tables.enable'),

Compare how core themes do it, e.g. olivero/theme-settings.php

…tibility layer.

donquixote requested changes Mar 6, 2026

View reviewed changes

donquixote reviewed Mar 6, 2026

View reviewed changes

tibi2303 force-pushed the OEL-4511 branch from 3c14f40 to 2cb72c3 Compare March 6, 2026 17:16

donquixote reviewed Mar 6, 2026

View reviewed changes

OEL-4511: Update to Drupal 11.3.x.

4ac2d3f

tibi2303 force-pushed the OEL-4511 branch from 450b047 to 4ac2d3f Compare March 7, 2026 11:50

donquixote reviewed Mar 9, 2026

View reviewed changes

OEL-4511: Removed default theme param.

a946c3e

donquixote reviewed Mar 9, 2026

View reviewed changes

tibi2303 added 3 commits March 9, 2026 15:43

OEL-4511: Removed default values.

1531f61

OEL-4511: Removed unused classes.

c5d396e

OEL-4511: Avoid direct ThemeSettingsProvider class reference in compa…

110fb03

…tibility layer.

		$service_id = 'Drupal\Core\Extension\ThemeSettingsProvider';
		$settings_provider = \Drupal::service($service_id);

	$service_id = 'Drupal\Core\Extension\ThemeSettingsProvider';
	$settings_provider = \Drupal::service($service_id);
	$settings_provider = \Drupal::service(Drupal\Core\Extension\ThemeSettingsProvider::class);

	$service_id = 'Drupal\Core\Extension\ThemeSettingsProvider';
	$settings_provider = \Drupal::service($service_id);
	$settings_provider = \Drupal::service(ThemeSettingsProvider::class);

	return \call_user_func([$settings_provider, 'getSetting'], $setting, $theme);
	return $settings_provider->getSetting($setting, $theme);

		$legacy_getter = 'theme_get_setting';
		return \call_user_func($legacy_getter, $setting, $theme);

	'#default_value' => ThemeSettings::get('bootstrap_tables.enable', $theme_name),
	'#default_value' => ThemeSettings::get('bootstrap_tables.enable', 'oe_bootstrap_theme'),

	fn () =>\theme_get_setting($setting, $theme),
	fn () => theme_get_setting($setting, $theme),

	$responsive = DrupalCompatibility::themeGetSetting('bootstrap_tables.responsive', 'oe_bootstrap_theme');
	$responsive = DrupalCompatibility::themeGetSetting('bootstrap_tables.responsive');

	return (bool) (DrupalCompatibility::themeGetSetting(self::PREFIX . $name, self::THEME) ?? TRUE);
	return (bool) (DrupalCompatibility::themeGetSetting(self::PREFIX . $name) ?? TRUE);

	'#default_value' => DrupalCompatibility::themeGetSetting('bootstrap_tables.enable', 'oe_bootstrap_theme'),
	'#default_value' => DrupalCompatibility::themeGetSetting('bootstrap_tables.enable'),

Conversation

tibi2303 commented Mar 3, 2026

Uh oh!

donquixote left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

donquixote Mar 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

donquixote Mar 6, 2026 •

edited

Loading