Problem/Motivation

In jquery_update_requirements(), the global theme key is changed and restored:

      $backup_theme_key = $GLOBALS['theme_key'];
      $GLOBALS['theme_key'] = $default_theme;
      $libraries = array($jquery, $ui);
      jquery_update_library_alter($libraries, 'system');
      $versions['theme_default']['jquery'] = $libraries['jquery']['version'];
      $versions['theme_default']['ui'] = $libraries['ui']['version'];
      $GLOBALS['theme_key'] = $backup_theme_key;

Just in case something goes wrong, let's put that in a try/catch block, so that the global variable is always restored. Who knows, some future change may mean that jquery_update_library_alter() calls drupal_http_request(), which might throw an exception.

Steps to reproduce

Proposed resolution

Put all the steps between changing and restoring in a try block, and restore in a finally block.

If something does go wrong, log an error message and return a requirements error.

Remaining tasks

User interface changes

None, unless something goes wrong in jquery_update_library_alter().

API changes

None

Data model changes

None

Issue fork jquery_update-3314003

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

benjifisher created an issue. See original summary.

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
commented
Assigned: benjifisher » Unassigned
Status: Active » Needs review

I added a MR to implement the proposed resolution.

benjifisher opened merge request !2

poker10’s picture

poker10 commented

I think this looks good! Just one question - do we need to support PHP 5.4 and lower here (like the D7 core)? Because the finally block was introduced only in the PHP 5.5. I suppose not, but just for sure.

  finally {
    $GLOBALS['theme_key'] = $backup_theme_key;
  }
benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
commented

Good question. According to PHP requirements (for Drupal 7), versions 5.3, 5.4, and 5.5 are supported but not recommended. In the PHP docs, the list of Unsupported Branches shows that support for all versions of PHP 5 ended on 2018-12-31.

I can update the MR to avoid the finally block if you want to support PHP 5.3 and 5.4.

If any of the Drupal 7 maintainers are reading this(*), I would like to point out that the doc I linked to says,

Actual minimum requirements can be located in INSTALL.TXT.

But that file lists 5.2.4 as the minimum requirement.

Also, the doc mentions #3145797: [META] Make Drupal 7 core compatible with PHP 8.0 and #3224299: [META] Make Drupal 7 core compatible with PHP 8.1. It is time to update the table.

(*) That is a joke.

mcdruid’s picture

mcdruid commented

:)

Thanks @benjifisher! I think supporting ancient versions of PHP 5 is increasingly a best efforts endeavour. For example, I'm not sure how much longer we can expect the DA to make testing environments for PHP 5.3 available etc.. and even the "traditional" arguments e.g. "but $old_version of RHEL still supports it" may be out of date now. As you say, we should revise and update that policy for all of D7.

We have #3092260: [policy, no patch] Review and update automated testing config for 7.x branch which seems a good a place as any to look at that.

Broadly though I'd say we should treat jquery_update the same as D7 core in terms of aiming for backwards compatibility.

Therefore we should avoid introducing anything we know won't work in the older PHP versions if doing so it not too onerous.

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
commented

I updated the MR, avoiding the finally block.

solideogloria’s picture

solideogloria commented
Status: Needs review » Reviewed & tested by the community

The code changes look good to me. I tested them and had no errors viewing the status report.

  • mcdruid committed cebced9 on 7.x-4.x 
    Issue #3314003 by benjifisher, poker10, solideogloria: Be careful when...
mcdruid’s picture

mcdruid commented
Status: Reviewed & tested by the community » Fixed

Thanks everybody!

mcdruid’s picture

mcdruid commented

Oh sorry, I should have committed that with @benjifisher as the author (which we don't do for core so I am out of the habit). I'll try to remember in future.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.