Updated to 1.30.0 - PHP warning: Deprecated function: version_compare(): Passing null to parameter #1

[jenlampton]

Description of the bug

1. My site is using a Basis subtheme
2. Basis is DISABLED

I'm seeing the following warning after updating to 1.30.0

Deprecated function: version_compare(): Passing null to parameter #1 ($version1) of type string is deprecated in basis_preprocess_page() (line 69 of /var/www/html/docroot/core/themes/basis/template.php). `

I think maybe

Steps To Reproduce

To reproduce the behavior:

1. Run a subtheme of basis
2. Disable the Basis theme
3. Update to 1.30.0
4. See PHP warning on all pages

Actual behavior

PHP error

Expected behavior

No error

Additional information

Add any other information that could help, such as:
Backdrop CMS: 1.30.0
Installation profile: standard
PHP version: 8.2.24
Drupal 7 compatibility: on
Database server: 10.11.9-MariaDB-ubu2204-log
Web server: nginx/1.26.2

[indigoxela]

@jenlampton the STR seem incomplete.

I have one site with a subtheme of basis and that never showed that warning (PHP 8.1).

Whether the base theme's enabled or disabled shouldn't matter, as its template.php will be considered regardless.

Param 1 is the update_version added in an update hook.

Could it be, that this Backdrop instance is patched in any way, so crucial code is missing, or the update hook didn't run?

https://github.com/backdrop/backdrop/blob/1.x/core/themes/basis/template.php#L69

Edit: had to correct some of the text.

[jenlampton]

To circumvent this problem, I enabled Basis when I ran the update, and then disabled Basis again after the update was complete.

• schema_version is set to 1102 for the system module (so yes, updates have been run)
• The result of theme_get_setting('css_update_version'); is NULL.

[jenlampton]

I think the problem I ran into is that the update hook checks if the Basis theme is enabled before setting the value:

function system_update_1100() {
  $themes = system_list('theme');
  if (!empty($themes['basis']->status)) {

[jenlampton]

What happens when someone who's not using Basis when this update happens, wants to switch back to Basis later? When they first installed their site, Basis worked fine. But without this update, if they ever switch back to Basis they are also going to get this error if these values are only set if the theme is enabled at update-time.

Because there's no hook_enable() for themes, we can't ensure that these values are available when they are needed. I think the only safe way to have this code in Basis, is to add the setting even if the theme is disabled. :/

[indigoxela]

if (!empty($themes['basis']->status)) { ...

Oh, nice catch, didn't realize that. 👍

However, the current PR contains a problematic (additional) change (see PR comment), so this still needs work.

@jenlampton I wonder, why you decided against fixing the problem in basis_preprocess_page() with a simple isset().

[indigoxela]

What happens when someone who's not using Basis when this update happens, wants to switch back to Basis later?

I just played with that. This works properly.

Basis has a config folder. If Basis gets disabled, its config gets wiped out from the active config folder and if it's enabled again later on, the "vanilla" config is added into the active config.

[indigoxela]

I made up my mind... Back to needs testing/review. Although I find it unnecessary to add yet another update hook to fix that warning, I don't want to block anything. Obviously I didn't follow the discussions that led to the changes, that astounded me. 😁

[quicksketch]

I mentioned this a few weeks back during a dev meeting that we might be able to avoid making a basis.settings.json file if the Basis theme is disabled by providing a default in the basis.info file instead. I think that might be a little cleaner than having a config file floating around that isn't owned by any enabled module or theme.

I filed a PR at backdrop/backdrop#5010 that could use testing and confirmation if that approach might work.

My understanding is that theme_get_setting() should pull from the base theme's .info file as the very last fallback, after first checking:

• The sub-theme [theme].settings.json file
• The sub-theme .info file
• The base theme [theme].settings.json file
• The base theme .info file (settings added in this PR)

[jenlampton]

I tested @indigoxela's recommendation of simply using an isset() in the Basis template.php file to see if the theme setting exists before running version_compare(), and that also prevents the error from appearing on all pages.

I think this functionally has the same result - it does not add the class if the site was installed before the value was set.

New PR at backdrop/backdrop#5011

[jenlampton]

@quicksketch I tested your PR at backdrop/backdrop#5010 and that also prevents the error from appearing on all pages. I prefer both of these to the original replacement update hook, so I'm going to close that PR.

Nate's PR is code reviewed + tested. Marking as RTBC.