Problem/Motivation

Currently, using a PHP version between \Drupal::MINIMUM_PHP and \Drupal::MINIMUM_SUPPORTED_PHP leads to:

  • A requirements error on installation
  • A requirements error on the site status report
  • A requirements warning on update

In #2917655: [9.4.x only] Drop official PHP 7.3 support in Drupal 9.4, it's been raised that the distinction between installation and update is somewhat artificial, because it's quite common to have a deployment workflow that involves installing from default config, even for site updates.

Proposed resolution

Change the behavior of \Drupal::MINIMUM_SUPPORTED_PHP to raise a warning on installation, rather than an error.

Remaining tasks

#3261602: Display installation warnings in the quick-start command is a followup to make this work properly with the quick-start command.

User interface changes

Instead of an error, sites installing on versions between MINIMUM_PHP and MINIMUM_SUPPORTED_PHP will see the following warning:

They can then advance to the next screen of the installer by scrolling to the end of the page and click a small "continue anyway" link.

API changes

As per title.

Data model changes

N/A

Release notes snippet

The installer will now display a warning rather than an error if the PHP version is between \Drupal::MINIMUM_PHP and \Drupal::MINIMUM_SUPPORTED_PHP. No existing sites will be affected by this change since \Drupal::MINIMUM_PHP and \Drupal::MINIMUM_SUPPORTED_PHP have had the same value since Drupal 8.8.0; however, some development or testing workflows from Drupal 8.6 or 8.7 might be affected. (More information.)

Issue fork drupal-3265325

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

xjm created an issue. See original summary.

xjm opened merge request !1832

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Active » Needs review
xjm’s picture

xjm
she/her
Primary language English
commented

So, turns out, most of the test failures are actually regressions in either tests or HEAD when MINIMUM_SUPPORTED_PHP is used at all. One is easy to fix, but the other two look like they might actually be core bugs, so I'm going to split off an issue for each.

xjm’s picture

xjm
she/her
Primary language English
commented

I've opened these three issues for the specific failures, since a couple are a bit tricky:

  1. #3265376: Fix UpdateScriptTest when MINIMUM_SUPPORTED_PHP is used
  2. #3265377: Fix LocaleTranslatedSchemaDefinitionTest when MINIMUM_SUPPORTED_PHP is used
  3. #3265378: Fix NoPreExistingSchemaUpdateTest when MINIMUM_SUPPORTED_PHP is used

Meanwhile, we can skip those tests here and focus on actually addressing the scope here.

xjm’s picture

xjm
she/her
Primary language English
commented
Related issues: +#3261602: Display installation warnings in the quick-start command

New approach, skipping out of scope tests.

The QuickStartTest method that fails it to prove there's an error on installation on old PHP versions. But this issue is changing that behavior. So, it makes sense to just remove the test for the old behavior. Right now, nothing happens, so there's nothing to test.

Currently the quick-start command does not display installer warnings, only errors. #3261602: Display installation warnings in the quick-start command is the issue to fix that, and it should add the equivalent test coverage once it's working.

xjm’s picture

xjm
she/her
Primary language English
commented
Issue summary: View changes
Status: Needs review » Postponed

Postponing on:

  1. #3265376: Fix UpdateScriptTest when MINIMUM_SUPPORTED_PHP is used
  2. #3265377: Fix LocaleTranslatedSchemaDefinitionTest when MINIMUM_SUPPORTED_PHP is used
  3. #3265378: Fix NoPreExistingSchemaUpdateTest when MINIMUM_SUPPORTED_PHP is used
xjm’s picture

xjm
she/her
Primary language English
commented
Issue summary: View changes
xjm’s picture

xjm
she/her
Primary language English
commented
Related issues: +#3039287: Implement changes required to remove support for PHP 5.5 and 5.6

Good thing we kept that API around. Thank you, past selves.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Postponed » Needs review

The three blockers are in, and this is working now, so this is ready for review. Edit: Oops, #3265377: Fix LocaleTranslatedSchemaDefinitionTest when MINIMUM_SUPPORTED_PHP is used is just RTBC, but soon enough anyway.

xjm’s picture

xjm
she/her
Primary language English
commented

The test failure in https://www.drupal.org/pift-ci-job/2324415 was due to HEAD being broken, so this is still ready for review. I can't seem to queue a retest ATM (the link is missing from the d.o UI).

xjm’s picture

xjm
she/her
Primary language English
commented
Issue summary: View changes
daffie’s picture

daffie commented
Status: Needs review » Needs work

Back to NW for my question on the MR and the needed CR.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Posted review on MR. I think this does deserve a release note snippet because it is a significant change that could disrupt people's workflows, especially for quick-start's new behavior.

xjm’s picture

xjm
she/her
Primary language English
commented
Issue tags: +Needs release note

Fair enough about the release note. I was leaving it for the parent issue, but we need to make sure not to leave this part out of it.

xjm’s picture

xjm
she/her
Primary language English
commented
Issue summary: View changes
xjm’s picture

xjm
she/her
Primary language English
commented
Status: Needs work » Needs review

Okay, all feedback has been addressed. The issue still needs a change record and such, which I will work on later, but for now it could use another round of code review. Thanks!

daffie’s picture

daffie commented
Status: Needs review » Needs work

The MR looks good to me and for me is RTBC.

I think we need a CR for the fact that during installation, when the PHP version is below MINIMUM_SUPPORTED_PHP, a REQUIREMENT_WARNING is given as before it was a REQUIREMENT_ERROR. With that is also an explanation necessary about the difference between MINIMUM_SUPPORTED_PHP, MINIMUM_PHP and RECOMMENDED_PHP and what there current values are.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Did another review round. Mostly nits, but one more serious thing: I'd feel safer if we had explicit $this->fail() statements for places that "should never be reached anyway".

xjm’s picture

xjm
she/her
Primary language English
commented
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs change record, -Needs release note

Thanks for the reviews.

I've addressed all the recent feedback with further improvements to the documentation. I think changes to the existing coverage of updated tests or changing how InstallerTestBase works are out of scope for the issue.

Change record added:
https://www.drupal.org/node/3267954

I also added a release note, but I'm really not sure it's necessary. No supported versions of Drupal will be affected because MINIMUM_PHP and MINIMUM_SUPPORTED_PHP have had equal values in all releases since 8.8.0. There might be older testing or deployment workflows out there that could be affected, but only if they were written specifically in 2019 (between 8.6.0 and 8.8.0). It can have a release note for now and it might get edited away when we coauthor the release notes for 10.0.0-beta1. ;)

Thanks!

xjm’s picture

xjm
she/her
Primary language English
commented
Issue summary: View changes
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community

Change record looks good. My concerns/confusion have been addressed. Thanks!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed e8b2bec983 to 10.0.x and 3e6b51d96d to 9.4.x. Thanks!

diff --git a/core/modules/system/tests/src/Functional/System/PhpRequirementTest.php b/core/modules/system/tests/src/Functional/System/PhpRequirementTest.php
index 46a76d45f4..af57919408 100644
--- a/core/modules/system/tests/src/Functional/System/PhpRequirementTest.php
+++ b/core/modules/system/tests/src/Functional/System/PhpRequirementTest.php
@@ -59,22 +59,22 @@ public function testStatusPage() {
     // the minimum supported PHP.
     if (version_compare($phpversion, \Drupal::MINIMUM_SUPPORTED_PHP) < 0) {
       $this->assertErrorSummaries(['PHP']);
-      $this->assertSession()->pageTextContains('Your PHP installation is too old. Drupal requires at least PHP ' . \DRUPAL::MINIMUM_SUPPORTED_PHP);
+      $this->assertSession()->pageTextContains('Your PHP installation is too old. Drupal requires at least PHP ' . \Drupal::MINIMUM_SUPPORTED_PHP);
     }
     // Otherwise, there should be no error.
     else {
-      $this->assertSession()->pageTextNotContains('Your PHP installation is too old. Drupal requires at least PHP ' . \DRUPAL::MINIMUM_SUPPORTED_PHP);
+      $this->assertSession()->pageTextNotContains('Your PHP installation is too old. Drupal requires at least PHP ' . \Drupal::MINIMUM_SUPPORTED_PHP);
       $this->assertSession()->pageTextNotContains('Errors found');
     }
 
     // There should be an informational message if the PHP version is below the
     // recommended version.
     if (version_compare($phpversion, \Drupal::RECOMMENDED_PHP) < 0) {
-      $this->assertSession()->pageTextContains('It is recommended to upgrade to PHP version ' . \DRUPAL::RECOMMENDED_PHP . ' or higher');
+      $this->assertSession()->pageTextContains('It is recommended to upgrade to PHP version ' . \Drupal::RECOMMENDED_PHP . ' or higher');
     }
     // Otherwise, the message should not be there.
     else {
-      $this->assertSession()->pageTextNotContains('It is recommended to upgrade to PHP version ' . \DRUPAL::RECOMMENDED_PHP . ' or higher');
+      $this->assertSession()->pageTextNotContains('It is recommended to upgrade to PHP version ' . \Drupal::RECOMMENDED_PHP . ' or higher');
     }
   }

Fixed class case on commit - caught by PHPStan running on D10 FTW.

  • alexpott committed e8b2bec on 10.0.x 
    Issue #3265325 by xjm, Wim Leers, daffie: Raise a warning instead of an...

  • alexpott committed 3e6b51d on 9.4.x 
    Issue #3265325 by xjm, Wim Leers, daffie: Raise a warning instead of an...

Status: Fixed » Closed (fixed)

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

xjm closed merge request !1832