Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
jenlampton opened this issue Jan 21, 2025 · 7 comments · May be fixed by backdrop/backdrop#5002

Comments

@jenlampton
Copy link
Member

jenlampton commented Jan 21, 2025

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

@jenlampton jenlampton changed the title Updated to 1.30.0 PHP warning: Deprecated function: version_compare(): Passing null to parameter #1 Updated to 1.30.0 - PHP warning: Deprecated function: version_compare(): Passing null to parameter #1 Jan 21, 2025
@indigoxela
Copy link
Member

indigoxela commented Jan 22, 2025

@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
Copy link
Member Author

jenlampton commented Jan 22, 2025

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
Copy link
Member Author

jenlampton commented Jan 22, 2025

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 jenlampton added this to the 1.30.1 milestone Jan 22, 2025
@jenlampton
Copy link
Member Author

jenlampton commented Jan 22, 2025

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
Copy link
Member

indigoxela commented Jan 23, 2025

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
Copy link
Member

indigoxela commented Jan 23, 2025

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
Copy link
Member

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. 😁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants