Problem/Motivation

#2842431: [policy] Remove PHP 5.5, 5.6 support in Drupal 8.7 outlined the policy for dropping support for PHP 5.5 and 5.6, but there are some details that were never implemented as part of that issue. This issue aims to resolve those.

Proposed resolution

C&P from @xjm's comment in the original issue:

  1. New sites will not be able to install on PHP 5.5 or 5.6.
  2. An error will appear on the status report for sites on PHP 5.5 and 5.6.
  3. Sites on PHP 5 will still be able to run Drupal updates, but a warning will be presented on the update screen.
  4. The 8.8.x branch will not be tested against PHP 5.
  5. We will update to PHPUnit 6.
  6. If possible, the 8.7.x branch will have one nightly test against PHP 5.6 (with PHPUnit 4).
  7. If a commit breaks the 8.7 nightly test on PHP 5.6, it may be reverted, but site owners should expect bugs and fatal errors if they do not update to PHP 7
  8. On PHP 5.6 only, the Migrate test suite should be skipped.

Remaining tasks

None.

User interface changes

If Drupal is attempted to be installed on PHP versions below PHP 7.0.8, the installer stops and does not allow to continue.

For existing Drupal sites below PHP version 7.0.8, a warning is displayed in status report about incompatibility.

API changes

The Drupal\simpletest\InstallerTestBase::continueOnExpectedWarnings, Drupal\FunctionalTests\Installer\InstallerTestBase::continueOnExpectedWarnings methods were refactored and moved to Drupal\Tests\RequirementsPageTrait.

Data model changes

None.

Release notes snippet

PHP stopped supporting version 5.5 on 21 Jul 2016, two years and eight months ago and version 5.6 on 31 Dec 2018. Drupal's end of support for PHP 5.5 and 5.6 was announced in January 2018.

Therefore, new Drupal 8 installations now require PHP 7.0.8. For now, existing sites can still run and be updated on PHP 5.5.9 or higher, but will display a warning. Note that Drupal security updates will begin requiring PHP 7 as early as Drupal 8.8.0 (December 2019), so all users are advised to update to at least PHP 7.0.8 now.

CommentFileSizeAuthor
#89 interdiff.txt2.09 KBeffulgentsia
#89 3039287-89.patch32.72 KBeffulgentsia
#78 interdiff.txt6.88 KBeffulgentsia
#78 3039287-78.patch31.95 KBeffulgentsia
#69 interdiff-3039287-65-69.txt3.42 KBbendeguz.csirmaz
#69 3039287-69.patch30.43 KBbendeguz.csirmaz
#66 interdiff-3039287-55-65.txt20.21 KBbendeguz.csirmaz
#66 3039287-65.patch32.47 KBbendeguz.csirmaz
#55 interdiff-3039287-49-55.txt4.56 KBbendeguz.csirmaz
#55 3039287-55.patch29.04 KBbendeguz.csirmaz
#49 interdiff-3039287-45-49.txt9.2 KBbendeguz.csirmaz
#49 3039287-49.patch28.94 KBbendeguz.csirmaz
#45 interdiff-3039287-43-45.txt1.45 KBbendeguz.csirmaz
#45 3039287-45.patch32.86 KBbendeguz.csirmaz
#43 interdiff-3039287-41-43.txt3.42 KBbendeguz.csirmaz
#43 3039287-43.patch32.06 KBbendeguz.csirmaz
#42 interdiff-3039287-40-41.txt1.46 KBbendeguz.csirmaz
#41 3039287-41.patch29.88 KBbendeguz.csirmaz
#40 interdiff-3039287-39-40.txt752 bytesbendeguz.csirmaz
#40 3039287-40.patch29.76 KBbendeguz.csirmaz
#39 interdiff-3039287-38-39.txt5.25 KBbendeguz.csirmaz
#39 3039287-39.patch29.73 KBbendeguz.csirmaz
#38 interdiff-3039287-33-38.txt18.7 KBbendeguz.csirmaz
#38 3039287-38.patch24.8 KBbendeguz.csirmaz
#33 interdiff-3039287-32-33.txt9.95 KBbendeguz.csirmaz
#33 3039287-33.patch24.21 KBbendeguz.csirmaz
#32 3039287-32.patch17.45 KBbendeguz.csirmaz
#32 interdiff-3039287-29-32.txt9.97 KBbendeguz.csirmaz
#29 3039287-29.patch13.12 KBbendeguz.csirmaz
#29 interdiff-3039287-27-29.txt2.81 KBbendeguz.csirmaz
#27 interdiff-3039287-26-27.txt1.75 KBbendeguz.csirmaz
#27 3039287-27.patch10.31 KBbendeguz.csirmaz
#26 3039287-26.patch10.28 KBbendeguz.csirmaz
#26 interdiff-3039287-24-26.txt3.43 KBbendeguz.csirmaz
#24 interdiff-3039287-23-24.txt714 bytesbendeguz.csirmaz
#24 3039287-24.patch8.59 KBbendeguz.csirmaz
#23 interdiff-3039287-22-23.txt849 bytesbendeguz.csirmaz
#23 3039287-23.patch8.56 KBbendeguz.csirmaz
#22 interdiff-3039287-19-22.txt1.99 KBbendeguz.csirmaz
#22 3039287-22.patch7.73 KBbendeguz.csirmaz
#20 3039287-20.patch9.07 KBbendeguz.csirmaz
#20 interdiff-3039287-19-20.txt1016 bytesbendeguz.csirmaz
#19 3039287-19.patch8.08 KBbendeguz.csirmaz
#19 interdiff-3039287-18-19.txt1.12 KBbendeguz.csirmaz
#18 interdiff-3039287-10-18.txt3.19 KBbendeguz.csirmaz
#18 3039287-18.patch7.97 KBbendeguz.csirmaz
#10 interdiff-3039287-4-10.txt753 bytesbendeguz.csirmaz
#10 3039287-10.patch5.13 KBbendeguz.csirmaz
#4 interdiff-3039287-3-4.txt2.47 KBbendeguz.csirmaz
#4 3039287-4.patch4.75 KBbendeguz.csirmaz
#3 3039287-3.patch2.28 KBbendeguz.csirmaz
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

balsama created an issue. See original summary.

xjm’s picture

Priority: Normal » Critical

Thanks @balsama!

Promoting to critical because this is an essential security and maintainability step for this release.

bendeguz.csirmaz’s picture

Changing DRUPAL_MINIMUM_PHP to 7.0.0.

bendeguz.csirmaz’s picture

Removing the warning message about support for < PHP 7 is getting dropped.

Krzysztof Domański’s picture

Berdir’s picture

Should this be a meta issue maybe, e.g. the phpunit/composer stuff probably makes sense to do separately.

jibran’s picture

xjm’s picture

Issue summary: View changes
Status: Active » Needs work

Thanks everyone!

#2908079: Move some of the bootstrap.inc PHP-related constants to \Drupal and deprecate the old versions will not block this issue; this is an 8.7 critical whereas that is a 8.8 would-be-good-to-do-but-not-major-or-anything. :)

So one important aspect of this change that's different from how we've dropped support before:

  • When the user tries to install Drupal on PHP 5, they should get a requirements error.
  • However, when the user tries to update core, they should see a warning that PHP 5 is no longer supported and they must upgrade to PHP ASAP to continue receiving updates.

Marking NW for that. Thank you!

xjm’s picture

Issue tags: +Needs tests

We also need browser and update test coverage for what's described in #8; there should be some foundational work for that in #2670966: Warn users of old PHP versions.

bendeguz.csirmaz’s picture

Changing error to warning when the phase is update.

Unfortunately I got stuck with an error and I'm not sure how to progress with this issue.

Steps to reproduce
- Install a site with PHP 5.6.37
- Apply this patch OR just change DRUPAL_MINIMUM_PHP to 7.0.0
- Go to update.php
- The warning is shown as expected
- Refresh the site
- White screen of death, "Error: Uncaught Error: Call to undefined function system_rebuild_module_data() in {...}/core/includes/common.inc:1126"

catch’s picture

system_rebuild_module_data() is on its way out, so it might be worth checking if this patch works alongside #2926068: Deprecate system_rebuild_module_data() and remove usages in core.

Wim Leers’s picture

- Refresh the site

Is that just "reload the site in the browser" or is it /rebuild.php or is it apachectl -k restart?

hampercm’s picture

I was able to reproduce the WSOD on PHP 5.6.40, using the steps given by @bendeguz.csirmaz in #10.

One detail that I see differently is that the WSOD that occurs when refreshing the browser tab actually produces this PHP error:

Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException: You have requested a non-existent service "access_check.db_update". in /Users/chris.hamper/Sites/drupal/vendor/symfony/dependency-injection/ContainerBuilder.php on line 1053 #0 /Users/chris.hamper/Sites/drupal/vendor/symfony/dependency-injection/ContainerBuilder.php(619): Symfony\Component\DependencyInjection\ContainerBuilder->getDefinition('access_check.db...')
#1 /Users/chris.hamper/Sites/drupal/vendor/symfony/dependency-injection/ContainerBuilder.php(587): Symfony\Component\DependencyInjection\ContainerBuilder->doGet('access_check.db...', 1)
#2 /Users/chris.hamper/Sites/drupal/core/lib/Drupal/Core/Update/UpdateKernel.php(187): Symfony\Component\DependencyInjection\ContainerBuilder->get('access_check.db...')
#3 /Users/chris.hamper/Sites/drupal/core/lib/Drupal/Core/Update/UpdateKernel.php(102): Drupal\Core\Update\UpdateKernel->handleAccess(Object(Symfony\Component\HttpFoundation\Request), Object(Drupal\Core\DependencyInjection\ContainerBuilder))
#4 /Users/chris.hamper/Sites/drupal/core/lib/Drupal/Core/Update/UpdateKernel.php(76): Drupal\Core\Update\UpdateKernel->handleRaw(Object(Symfony\Component\HttpFoundation\Request))
#5 /Users/chris.hamper/Sites/drupal/update.php(28): Drupal\Core\Update\UpdateKernel->handle(Object(Symfony\Component\HttpFoundation\Request))
#6 {main}

The error related to system_rebuild_module_data() occurs when I attempt to run drush cr to fix the issue.

hampercm’s picture

To make matters worse, loading update.php, even once, running on php 5.6.40 seems to hose the site install in a way that is not obvious to fix. Browsing to any pages on the site begins to result in WSODs, with errors like:

Uncaught PHP Exception InvalidArgumentException: "No check has been registered for access_check.permission" at /Users/chris.hamper/Sites/drupal/core/lib/Drupal/Core/Access/CheckProvider.php line 97

Reverting the patch and attempting to perform a dr cr produces the error:

PHP Fatal error: Call to undefined function system_rebuild_module_data() in /Users/chris.hamper/Sites/drupal/core/includes/common.inc on line 1136

xjm’s picture

Hmmm. So the access_check.db_update service is specific to update.php:

grep -r "access_check\.db_update" *
core/lib/Drupal/Core/Update/UpdateKernel.php:    $db_update_access = $this->getContainer()->get('access_check.db_update');
core/modules/system/system.services.yml:  access_check.db_update:

Route definition adds the _access_system_update route requirement:

# Note: This route just exists for generating URLs, the dedicated
# frontcontroller is used if the URL is accessed.
system.db_update:
  path: '/update.php/{op}'
  defaults:
    op: 'info'
  requirements:
    _access_system_update: 'TRUE'
  options:
    default_url_options:
      path_processing: FALSE

And here's the relevant hunk in the update.php front controller:

    /** @var \Drupal\system\Access\DbUpdateAccessCheck $db_update_access */
    $db_update_access = $this
      ->getContainer()
      ->get('access_check.db_update');
    if (!Settings::get('update_free_access', FALSE) && !$db_update_access
      ->access($account)
      ->isAllowed()) {
      throw new AccessDeniedHttpException('In order to run update.php you need to either have "Administer software updates" permission or have set $settings[\'update_free_access\'] in your settings.php.');

Is this reproducible only with the patch provided, or is this happening in HEAD? Edit: Chris said it was only with the patch, which is a relief.

xjm’s picture

+++ b/core/includes/bootstrap.inc
@@ -25,7 +25,7 @@
-const DRUPAL_MINIMUM_PHP = '5.5.9';
+const DRUPAL_MINIMUM_PHP = '7.0.0';

My suggestion is to exclude changing the constant for now, and temporarily hardcode a check for PHP 7.0 in the installer, to see whether that makes update.php work here. It sounds like the whole service container might be broken following the update, which makes me think something during the update is croaking.

If that turns out to solve the issue, then we can step through it in a debugger and see what needs to be changed.

I think we might end up defining a constant like DRUPAL_MINIMUM_UPDATE_PHP or something, and use that explicitly anywhere that might be accessed outside of the Drupal installer. (Or maybe instead it's a new constant DRUPAL_MINIMUM_INSTALL_PHP that's only used in the core installer.)

bendeguz.csirmaz’s picture

#12
@Wim Leers
Yes, it's just a "reload the site in the browser" .

#16
@xjm
Thanks! It turns out the problem is the extension lists that use DRUPAL_MINIMUM_PHP as a default value for the 'php' key when reading info files.

Specifically, ModuleExtensionList causes this exception.

  /**
   * {@inheritdoc}
   */
  protected $defaults = [
    'dependencies' => [],
    'description' => '',
    'package' => 'Other',
    'version' => NULL,
    'php' => DRUPAL_MINIMUM_PHP,
  ];
bendeguz.csirmaz’s picture

Adding a new constant for the default 'php' key values.

bendeguz.csirmaz’s picture

bendeguz.csirmaz’s picture

@xjm, I was thinking about skipping browser tests when the installation fails due to the version error.

xjm’s picture

Thanks @bendeguz.csirmaz. I think #20 is too general; we should not skip tests. We need those test results in order to find out when we're introducing a different regression only on those environments. Instead, maybe we need to allow Drupal to be installed on the minimum update PHP version only in tests where the test environment is between the minimum update PHP and the minimum install PHP, with a @todo + followup to remove the override once we drop update support.

Then, while the workaround is in place, we might need a one-off dedicated test specifically for the installer when the install PHP version is the "real" one for the branch.

Finally, update tests might need a step similar to InstallerTestBase::setUpRequirementsProblem() to ensure that we continue the update on the expected warnings on the allowed update PHP version.

bendeguz.csirmaz’s picture

bendeguz.csirmaz’s picture

Removing code from InstallerTestBase::setUpRequirementsProblem() as it is no longer needed. This should fix the failures in #22.

bendeguz.csirmaz’s picture

Consider the DRUPAL_DEV_SITE_PATH environment variable when installing too. This should fix the error in #23.

xjm’s picture

Yay, green tests! 🎉 Thanks @bendeguz.csirmaz.

+++ b/core/modules/system/system.install
@@ -186,24 +186,24 @@ function system_requirements($phase) {
+  // @todo Remove once update support is dropped.
+  if (drupal_valid_test_ua() || getenv('DRUPAL_DEV_SITE_PATH')) {

I think we'll actually keep this codepath around in the future, because we'll want to use the same update-but-not-install pattern around for the next time we drop support for a PHP version. So this comment can actually be replaced with something like:

Allow test installations of Drupal on the minimum update PHP.

The hardcoded getenv() check surprised me. It seems like something we should have explicit API for. However, I wasn't able to find anything more when grepping, and clearly it works since those tests are green!

bendeguz.csirmaz’s picture

Adding a test for the warning on the update page. Small refactor.

bendeguz.csirmaz’s picture

bendeguz.csirmaz’s picture

For the record, in #26 & #27 I changed system_requirements() to always show a requirement warning for updates when the PHP version is smaller than DRUPAL_MINIMUM_PHP.
#24 didn't show the warning when it was a test site.

bendeguz.csirmaz’s picture

Adding a "continueOnExpectedWarnings" to UpdatePathTestBase.
This should fix the tests extending this class, but the function is duplicated.

larowlan’s picture

+++ b/core/includes/bootstrap.inc
@@ -25,7 +25,15 @@
+const DRUPAL_MINIMUM_UPDATE_PHP = '5.5.9';

lets not add a new global constants, let's put it on a class/object from the get-go

amateescu’s picture

bendeguz.csirmaz’s picture

- creating a new trait for skipping the PHP warning
- allow other warnings on update.php besides the PHP warning in system_requirements
- fixing UpdateScriptTest

bendeguz.csirmaz’s picture

- fixing the rest of the tests
- deleting the new test from #26
- fixing a typo

xjm’s picture

Thanks everyone!

I disagree with #30, I think. We should not stealth-add the constant in a different place than the others before the general deprecation of all of them is done in #2908079: Move some of the bootstrap.inc PHP-related constants to \Drupal and deprecate the old versions. We can simply add this constant to the pile of things we deprecate there; it's not too costly.

+++ b/core/tests/Drupal/Tests/UpdateUiTrait.php
@@ -0,0 +1,61 @@
+ * Provides UI helper methods for update.php.
+ */
+trait UpdateUiTrait {

I think this trait is misnamed since it also includes a method we use in install tests. We should also make the name a little more specific. Maybe something like ExpectedRequirementsErrorTrait or something?

xjm’s picture

  1. +++ b/core/tests/Drupal/Tests/UpdateUiTrait.php
    @@ -0,0 +1,61 @@
    +  protected function updateRequirementsProblem() {
    

    Maybe we could give this a more clear name, like continueOnExpectedUpdateWarnings()?

    Also let's use the trait in InstallerTestBase (with any small refactors needed) so the code is in one place.

    Thanks!

  2. +++ b/core/tests/Drupal/Tests/UpdateUiTrait.php
    @@ -0,0 +1,61 @@
    +    if (version_compare(phpversion(), '7.0') < 0) {
    

    Here, I think we can use the constant now rather than hardcoding 7.0. (That was a special behavior when we were treating PHP 5 differently than other old versions.)

catch’s picture

Apart from xjm's outstanding review notes this looks pretty good to me. Agreed on adding the global constant for now and moving everything together.

alexpott’s picture

  1. +++ b/core/modules/system/system.install
    @@ -186,24 +186,29 @@ function system_requirements($phase) {
    +  if (version_compare($phpversion, DRUPAL_MINIMUM_UPDATE_PHP) < 0) {
    +    $requirements['php']['description'] = t('Your PHP installation is too old. Drupal requires at least PHP %version.', ['%version' => DRUPAL_MINIMUM_UPDATE_PHP]);
         $requirements['php']['severity'] = REQUIREMENT_ERROR;
    -    // If PHP is old, it's not safe to continue with the requirements check.
         return $requirements;
       }
    

    The comment about the early return still looks relevant.

  2. +++ b/core/modules/system/system.install
    @@ -186,24 +186,29 @@ function system_requirements($phase) {
    +    // Allow test installations of Drupal on the minimum update PHP.
    +    if ($phase === 'update') {
    

    This comment goes with the elseif - I think we need to explain why we only have a warning on the update.

  3. +++ b/core/tests/Drupal/FunctionalTests/Installer/InstallerTestBase.php
    @@ -244,12 +244,7 @@ protected function setUpSettings() {
       protected function setUpRequirementsProblem() {
    -    // By default, skip the "recommended PHP version" warning on older test
    -    // environments. This allows the installer to be tested consistently on
    -    // both recommended PHP versions and older (but still supported) versions.
    -    if (version_compare(phpversion(), '7.0') < 0) {
    -      $this->continueOnExpectedWarnings(['PHP']);
    -    }
    +    // Do nothing.
       }
    

    This bit is interesting. As in we can definitely remove \Drupal\FunctionalTests\Installer\InstallerExistingConfigSyncDirectoryProfileHookInstall::setUpRequirementsProblem() now. And it's an open question about whether we can remove this method, the call to it and \Drupal\FunctionalTests\Installer\InstallerTestBase::continueOnExpectedWarnings() as this is all used code now - at least in core. I think it is worth considering whether we should deprecate \Drupal\FunctionalTests\Installer\InstallerTestBase::continueOnExpectedWarnings() here because now this is untested so we'll not know to make changes to it is we change something that affects it. Another option is to used the trait here to provide BC and leave the setUpRequirementsProblem override to do nothing.

bendeguz.csirmaz’s picture

Refactored the trait so it can be used in Drupal\simpletest\InstallerTestBase and Drupal\FunctionalTests\Installer\InstallerTestBase too. I'd like to see if the existing tests pass first.

bendeguz.csirmaz’s picture

bendeguz.csirmaz’s picture

One thing I forgot in 38 & 39 is it always tries to click the continue button, even if the test is not on the "requirements problem" page.

bendeguz.csirmaz’s picture

bendeguz.csirmaz’s picture

FileSize
1.46 KB
bendeguz.csirmaz’s picture

Adding the test for the installer. I'm not sure if this is the best solution... it skips 2 tests on older PHP versions. We could introduce a new environment variable to allow the site install (the DRUPAL_DEV_SITE_PATH is not enough).

xjm’s picture

Status: Needs work » Needs review
bendeguz.csirmaz’s picture

effulgentsia’s picture

+++ b/core/includes/bootstrap.inc
@@ -25,7 +25,15 @@
+/**
+ * Updates cannot be run on versions of PHP older than this version.
+ *
+ * @todo Move this to an appropriate autoloadable class. See
+ *   https://www.drupal.org/project/drupal/issues/2908079
+ */
+const DRUPAL_MINIMUM_UPDATE_PHP = '5.5.9';

Ok, I think I get what this constant is saying.

+++ b/core/lib/Drupal/Core/Extension/ModuleExtensionList.php
@@ -28,7 +28,7 @@ class ModuleExtensionList extends ExtensionList {
-    'php' => DRUPAL_MINIMUM_PHP,
+    'php' => DRUPAL_MINIMUM_UPDATE_PHP,
...
+++ b/core/lib/Drupal/Core/Extension/ThemeExtensionList.php
@@ -48,7 +48,7 @@ class ThemeExtensionList extends ExtensionList {
-    'php' => DRUPAL_MINIMUM_PHP,
+    'php' => DRUPAL_MINIMUM_UPDATE_PHP,

But this is saying that not only can you run updates on DRUPAL_MINIMUM_UPDATE_PHP, but you can also install new modules/themes on DRUPAL_MINIMUM_UPDATE_PHP. Is that desired? If so, should the constant be renamed and documented as such? Or do we consider installing new modules/themes as part of being able to update a site?

bendeguz.csirmaz’s picture

@effulgentsia
Yes, to update a site you need to be able to the install modules.

effulgentsia’s picture

Status: Needs review » Needs work

To address #46/#47, @xjm suggested to rename DRUPAL_MINIMUM_UPDATE_PHP to DRUPAL_MINIMUM_PHP and to rename DRUPAL_MINIMUM_PHP to DRUPAL_MINIMUM_INSTALL_PHP. I like that!

bendeguz.csirmaz’s picture

effulgentsia’s picture

  1. +++ b/core/modules/system/system.install
    @@ -192,18 +192,25 @@ function system_requirements($phase) {
    +  if (version_compare($phpversion, DRUPAL_MINIMUM_INSTALL_PHP) < 0) {
    +    if ($phase === 'update') {
    +      $requirements['php']['description'] = t('Your PHP installation is too old. Drupal requires at least PHP %version.', ['%version' => DRUPAL_MINIMUM_INSTALL_PHP]);
    +      // Only show a warning, so sites running on an older version of PHP are
    +      // still able to run Drupal updates.
           $requirements['php']['severity'] = REQUIREMENT_WARNING;
         }
    +    // Allow test installations of Drupal on older PHP versions.
    +    elseif (!drupal_valid_test_ua()) {
    +      $requirements['php']['description'] = t('Your PHP installation is too old. Drupal requires at least PHP %version.', ['%version' => DRUPAL_MINIMUM_INSTALL_PHP]);
    +      $requirements['php']['severity'] = REQUIREMENT_ERROR;
    

    This looks good. However, what runs right above this is:

    if (version_compare($phpversion, DRUPAL_MINIMUM_PHP) < 0) {
        $requirements['php']['description'] = t('Your PHP installation is too old. Drupal requires at least PHP %version.', ['%version' => DRUPAL_MINIMUM_PHP]);
        $requirements['php']['severity'] = REQUIREMENT_ERROR;
      }
    

    So if someone tries installing with PHP 5.4, we first tell them that we require at least 5.5.9. Then, if they update to 5.5.9 and try installing again, we tell them we require at least 7.0.0. I think it makes sense to also change the description of this first one (just the description, not the if statement) to '%version' => DRUPAL_MINIMUM_INSTALL_PHP. So if they try installing on 5.4, we tell them they need 7.0.0 right away.

    The question then becomes whether to do that for all phases, or whether to toggle between DRUPAL_MINIMUM_PHP and DRUPAL_MINIMUM_INSTALL_PHP depending on the phase. I think it's okay to do it for all phases. If they're doing anything on PHP 5.4, I think it's ok if we tell them they need 7.0.0 (rather than telling them that they can get away with 5.5.9). But I'm willing to be convinced otherwise.

  2. +++ b/core/modules/system/system.install
    @@ -192,18 +192,25 @@ function system_requirements($phase) {
    +      // If PHP is old, it's not safe to continue with the requirements check.
    +      return $requirements;
    

    Why? This is running if they're on something between 5.5.9 and 7.0.0. What's unsafe (or undesired) about continuing with other requirements checks? In fact, for the 'runtime' phase, wouldn't we want to display other requirements violations in the status report too? And even for the 'install' phase, why not display the other requirements violations?

xjm’s picture

Why? This is running if they're on something between 5.5.9 and 7.0.0. What's unsafe (or undesired) about continuing with other requirements checks? In fact, for the 'runtime' phase, wouldn't we want to display other requirements violations in the status report too? And even for the 'install' phase, why not display the other requirements violations?

Looks like this check is copied from earlier in the function. You can just see the end of it in the context lines above:

+++ b/core/modules/system/system.install
@@ -192,18 +192,25 @@ function system_requirements($phase) {
     // If PHP is old, it's not safe to continue with the requirements check.
     return $requirements;

This dates way back to #807622: Fatal error calling sys_get_temp_dir() in installer - pHP 5.2.0 and earlier, which was fixed before the release of Drupal 7 (!).

It's also been obsolete anyway since #722974: Install script returns a blank page for PHP4, PHP 5.1 (should display *something* instead), which was changed well before Drupal 8's release: When we bumped the required PHP version to PHP 5.3 during Drupal 8's development, we changed the way we define constants from define() to const. When we did that, it started causing syntax errors on those older versions, which broke the installer entirely and prevented anything from being reported to the user. However, it turned out it didn't work anyway, so at some point pre-release we added this to core/install.php...

// Exit early if running an incompatible PHP version to avoid fatal errors.                                   
// The minimum version is specified explicitly, as DRUPAL_MINIMUM_PHP is not                                  
// yet available. It is defined in bootstrap.inc, but it is not possible to                                   
// load that file yet as it would cause a fatal error on older versions of PHP.                               
if (version_compare(PHP_VERSION, '5.5.9') < 0) {
  print 'Your PHP installation is too old. Drupal requires at least PHP 5.5.9. See the <a href="https://www.drupal.org/requirements">system requirements</a> page for more information.';
  exit;
}

...So it looks like that's another place we need to fix. That, or we could consider removing the hunk entirely (because Drupal 8 has never supported PHP 5.3 and 5.3 itself has been EOL for 5 years). Edit: Although, it could still be useful if/when we start adopting PHP 7-only syntax, so maybe we should keep it, and just clean up the later logic.

xjm’s picture

Following up on #51, I think we should also add a bit to the docblocks of DRUPAL_MINIMUM_PHP and DRUPAL_MINIMUM_INSTALL_PHP indicating that the line in the installer should also be updated whenever the version constants are.

effulgentsia’s picture

+++ b/core/tests/Drupal/Tests/RequirementsPageTrait.php
@@ -0,0 +1,73 @@
+    if (strpos($this->getTextContent(), 'Requirements problem') === FALSE) {
+      return;
+    }

This means that the test passes if it doesn't return requirements problems, even in a situation where it should. Can we make this more robust? Like perhaps assert that there are requirements problems when the PHP version is less than DRUPAL_MINIMUM_INSTALL_PHP, and that there aren't when it's higher?

jibran’s picture

+++ b/core/includes/bootstrap.inc
@@ -28,6 +28,14 @@
+const DRUPAL_MINIMUM_INSTALL_PHP = '7.0.0';

This should be 7.0.8 as per composer.json and https://packagist.org/packages/symfony/http-foundation#v3.4.23 require section.

bendeguz.csirmaz’s picture

#50.1, #50.2, #51, #52, #53, #54
Fixed!

#50.2, #51
@xjm
Yes, I think we should keep exiting early in install.php. The early return from #50.2 can be removed I think.

bendeguz.csirmaz’s picture

Status: Needs work » Needs review
alexpott’s picture

  1. +++ b/core/tests/Drupal/FunctionalTests/Installer/InstallerTestBase.php
    @@ -286,32 +284,8 @@ protected function refreshVariables() {
       protected function continueOnExpectedWarnings($expected_warnings = []) {
    
    +++ b/core/tests/Drupal/Tests/RequirementsPageTrait.php
    @@ -0,0 +1,62 @@
    +  protected function continueOnExpectedWarnings($link_label) {
    

    I'm not sure about these having different signatures. Are we passing in the link label for translations? I think it might be best if the trait had the same signature as the old function and we passed in ['PHP'] everywhere. That way we could remove InstallerTestBase::continueOnExpectedWarnings().

  2. +++ b/core/modules/simpletest/src/InstallerTestBase.php
    @@ -213,12 +216,7 @@ protected function setUpSettings() {
       protected function setUpRequirementsProblem() {
    -    // By default, skip the "recommended PHP version" warning on older test
    -    // environments. This allows the installer to be tested consistently on
    -    // both recommended PHP versions and older (but still supported) versions.
    -    if (version_compare(phpversion(), '7.0') < 0) {
    -      $this->continueOnExpectedWarnings(['PHP']);
    -    }
    +    // Do nothing.
       }
    
    +++ b/core/tests/Drupal/FunctionalTests/Installer/InstallerTestBase.php
    @@ -244,12 +247,7 @@ protected function setUpSettings() {
       protected function setUpRequirementsProblem() {
    -    // By default, skip the "recommended PHP version" warning on older test
    -    // environments. This allows the installer to be tested consistently on
    -    // both recommended PHP versions and older (but still supported) versions.
    -    if (version_compare(phpversion(), '7.0') < 0) {
    -      $this->continueOnExpectedWarnings(['PHP']);
    -    }
    +    // Do nothing.
       }
    

    How can we remove this code...

    +++ b/core/modules/system/system.install
    @@ -187,23 +187,28 @@ function system_requirements($phase) {
    +    // Allow test installations of Drupal on older PHP versions.
    +    elseif (!drupal_valid_test_ua()) {
    

    Ah this is why. Hmmm. I'm not sure on one hand how do we test this code then - this makes it untestable. On the other it feels a reasonable approach as it makes everything a bit simpler. One concern I have is what happens when we have to do something similar in future that generates warning again for the installer. Another concern is that in #8 / #9 the needs tests was added for exactly this situation.

  3. +++ b/core/modules/system/system.install
    @@ -187,23 +187,28 @@ function system_requirements($phase) {
       if (version_compare($phpversion, DRUPAL_MINIMUM_PHP) < 0) {
    -    $requirements['php']['description'] = t('Your PHP installation is too old. Drupal requires at least PHP %version.', ['%version' => DRUPAL_MINIMUM_PHP]);
    +    $requirements['php']['description'] = t('Your PHP installation is too old. Drupal requires at least PHP %version.', ['%version' => DRUPAL_MINIMUM_INSTALL_PHP]);
         $requirements['php']['severity'] = REQUIREMENT_ERROR;
         // If PHP is old, it's not safe to continue with the requirements check.
         return $requirements;
       }
    -  if ((version_compare($phpversion, DRUPAL_RECOMMENDED_PHP) < 0) && ($phase === 'install' || $phase === 'runtime')) {
    -    // Warn if still on PHP 5. If at least PHP 7.0, relax from "warning" to
    -    // "info", and show it at runtime only, to not scare users while installing.
    -    if (version_compare($phpversion, '7.0') < 0) {
    -      $requirements['php']['description'] = t('Drupal will drop support for this version on March 6, 2019. Upgrade to PHP version %recommended or higher to ensure your site can receive updates and remain secure. See <a href="http://php.net/supported-versions.php">PHP\'s version support documentation</a> and the <a href=":php_requirements">Drupal 8 PHP requirements handbook page</a> for more information.', ['%recommended' => DRUPAL_RECOMMENDED_PHP, ':php_requirements' => 'https://www.drupal.org/docs/8/system-requirements/php']);
    +  if (version_compare($phpversion, DRUPAL_MINIMUM_INSTALL_PHP) < 0) {
    +    if ($phase === 'update') {
    +      $requirements['php']['description'] = t('Your PHP installation is too old. Drupal requires at least PHP %version.', ['%version' => DRUPAL_MINIMUM_INSTALL_PHP]);
    +      // Only show a warning, so sites running on an older version of PHP are
    +      // still able to run Drupal updates.
           $requirements['php']['severity'] = REQUIREMENT_WARNING;
         }
    -    else {
    -      if ($phase === 'runtime') {
    -        $requirements['php']['description'] = t('It is recommended to upgrade to PHP version %recommended or higher for the best ongoing support.  See <a href="http://php.net/supported-versions.php">PHP\'s version support documentation</a> and the <a href=":php_requirements">Drupal 8 PHP requirements handbook page</a> for more information.', ['%recommended' => DRUPAL_RECOMMENDED_PHP, ':php_requirements' => 'https://www.drupal.org/docs/8/system-requirements/php']);
    -        $requirements['php']['severity'] = REQUIREMENT_INFO;
    -      }
    +    // Allow test installations of Drupal on older PHP versions.
    +    elseif (!drupal_valid_test_ua()) {
    +      $requirements['php']['description'] = t('Your PHP installation is too old. Drupal requires at least PHP %version.', ['%version' => DRUPAL_MINIMUM_INSTALL_PHP]);
    +      $requirements['php']['severity'] = REQUIREMENT_ERROR;
    +    }
    +  }
    +  if (version_compare($phpversion, DRUPAL_RECOMMENDED_PHP) < 0) {
    +    if ($phase === 'runtime') {
    +      $requirements['php']['description'] = t('It is recommended to upgrade to PHP version %recommended or higher for the best ongoing support.  See <a href="http://php.net/supported-versions.php">PHP\'s version support documentation</a> and the <a href=":php_requirements">Drupal 8 PHP requirements handbook page</a> for more information.', ['%recommended' => DRUPAL_RECOMMENDED_PHP, ':php_requirements' => 'https://www.drupal.org/docs/8/system-requirements/php']);
    +      $requirements['php']['severity'] = REQUIREMENT_INFO;
         }
       }
    

    I think this code can be simplified a bit and fixed. There's a bug in that when the PHP version is 5.6 and you look at runtime you'll not get an error - you'll only get an informational about updating to the recommended version.

    I think something like:

      if (version_compare($phpversion, DRUPAL_MINIMUM_INSTALL_PHP) < 0) {
        $requirements['php']['description'] = t('Your PHP installation is too old. Drupal requires at least PHP %version. It is recommended to upgrade to PHP version %recommended or higher for the best ongoing support. See <a href="http://php.net/supported-versions.php">PHP\'s version support documentation</a> and the <a href=":php_requirements">Drupal 8 PHP requirements handbook page</a> for more information.',
          [
            '%version' => DRUPAL_MINIMUM_INSTALL_PHP,
            '%recommended' => DRUPAL_RECOMMENDED_PHP,
            ':php_requirements' => 'https://www.drupal.org/docs/8/system-requirements/php',
          ]
        );
        $requirements['php']['severity'] = REQUIREMENT_ERROR;
    
        // If PHP is old, it's not safe to continue with the requirements check.
        if (version_compare($phpversion, DRUPAL_MINIMUM_PHP) < 0) {
          return $requirements;
        }
        // Only show a warning during updates, so sites running on an older version
        // of PHP are still able to run them.
        elseif ($phase === 'update') {
          $requirements['php']['severity'] = REQUIREMENT_WARNING;
        }
        // Allow test installations of Drupal on older PHP versions.
        elseif (drupal_valid_test_ua()) {
          $requirements['php']['severity'] = REQUIREMENT_INFO;
        }
      }
      elseif ($phase === 'runtime' && version_compare($phpversion, DRUPAL_RECOMMENDED_PHP) < 0) {
        $requirements['php']['description'] = t('It is recommended to upgrade to PHP version %recommended or higher for the best ongoing support.  See <a href="http://php.net/supported-versions.php">PHP\'s version support documentation</a> and the <a href=":php_requirements">Drupal 8 PHP requirements handbook page</a> for more information.', ['%recommended' => DRUPAL_RECOMMENDED_PHP, ':php_requirements' => 'https://www.drupal.org/docs/8/system-requirements/php']);
        $requirements['php']['severity'] = REQUIREMENT_INFO;
      }
    

    Is better because:

    • We have less copies of the same string to maintain.
    • We always error for < DRUPAL_MINIMUM_INSTALL_PHP at non-test runtime.
    • We recommend the recommend version for < DRUPAL_MINIMUM_INSTALL_PHP during updates and point to the requirements page too
bendeguz.csirmaz’s picture

#57.1
@alexpott
The idea is to use this trait on both the update and install requirements pages (#35).
On the update requirements page the continue button is called 'try again', on the install requirements page it is 'continue anyway'.
Creating a new method for update requirements and hardcoding the labels might be a better solution.

alexpott’s picture

@bendeguz.csirmaz but we never hit the install requirements page now because of the drupal_valid_test_ua() check - right?

bendeguz.csirmaz’s picture

#57.2
@alexpott
I agree. I managed to add a test for this in #43, but it's an ugly workaround.
We have to allow test installs on older versions (#21). The solution I think is to somehow continue on this specific InstallerException when installing test sites.

bendeguz.csirmaz’s picture

#59 Yes.

bendeguz.csirmaz’s picture

#57.3, #61

Hmmm... Yes, you're right, an info level error seems like a good solution. We hit the requirements page, but it doesn't prevent test sites from installing.

alexpott’s picture

Maybe

    // Allow test installations of Drupal on older PHP versions.
    elseif (drupal_valid_test_ua()) {

could be

    // Allow test installations of Drupal on older PHP versions.
    elseif ($phase === 'install' && drupal_valid_test_ua()) {

Because then like re-installs we'll get an error on the requirements page at runtime when PHP > DRUPAL_MINIMUM_PHP but < DRUPAL_MINIMUM_INSTALL_PHP

That said I'm not a huge fan of DRUPAL_MINIMUM_INSTALL_PHP and DRUPAL_MINIMUM_PHP and supporting updates on PHP 5 in 8.7.x and greater. We now have extended security support for 8.6.x so I'm not sure why this is necessary anymore.

catch’s picture

For me it still makes sense to separate the install/update requirement. Otherwise we allow people to install on a PHP version one week, but error when they try to update two weeks later. If we tie the two together to the same release, we're always introducing a hard cliff between minors.

While we have extended security support for minor releases that would allow them to update to a security release, we'd not be able to point to which release to update to on update.php where we make this check without a lot of work. So we'd just be showing an error with a link to a docs page or something. There are still people who have control over their code base but not their hosting, such as some university hosting setups.

alexpott’s picture

+++ b/core/modules/system/system.install
@@ -186,26 +186,34 @@ function system_requirements($phase) {
+    // Allow test installations of Drupal on older PHP versions.
+    elseif ($phase === 'install' && drupal_valid_test_ua()) {
+      $requirements['php']['severity'] = REQUIREMENT_INFO;
     }

+++ b/core/tests/Drupal/FunctionalTests/Installer/InstallerTestBase.php
@@ -244,11 +247,13 @@ protected function setUpSettings() {
+    // Assert an informational is shown on older test environments.
+    if (version_compare(phpversion(), DRUPAL_MINIMUM_INSTALL_PHP) < 0) {

I think InstallerTestBase::setUpRequirementsProblem() still is unnecessary because we make it only REQUIREMENT_INFO

alexpott’s picture

@catch but at what point do we change the requirement in core/composer.json? If we are going to remove PHP5 support during Drupal 8's lifetime at some point we need to update that. And surely that's the point we should remove PHP 5 testing and install and update support. For example if you are using a configuration install based method for bringing up development environments or installing staging or feature test environments then this two different minimum PHPs is going to be really confusing and awkward.

effulgentsia’s picture

@alexpott: Re #68, I think there are 3 things that we'll probably want to synchronize into the same release:

1) Updating (non-dev) vendor dependencies to versions that have a PHP 7 minimum.
2) Updating our own core/composer.json to have a PHP 7 minimum.
3) Updating DRUPAL_MINIMUM_PHP to a PHP 7 minimum.

We could theoretically do 1) without doing 2) and 3). Especially if it's a library that doesn't actually have any PHP 5 breaks in a critical path. However, even if we choose to do all 3 of those together, it's too late to do them for 8.7, so we're talking about in the 8.8.x branch at whatever point in time that a release manager approves that.

Given that we're not doing any of the above for 8.7, the question then becomes: is it valuable to prevent people from installing new 8.7 sites on PHP 5, as well as is it valuable to provide extra warnings (during update.php and in the status report) for people who update an existing site to 8.7? If so, how would you suggest doing all that without introducing a new constant?

dawehner’s picture

I do have a question. It can happen that someone needs to re-install a site, using an existing DB dump. Many people are using tools like https://www.drupal.org/project/backup_migrate for this.

What's the expected workflow for someone on php 5.5/5.6 then?

  • Create the settings.php / files directory etc. manually
  • Import the DB dump and adopt the settings.php to point to that

?

I wonder whether it would be worth documenting this case, maybe in a change record for this issue.

. Especially if it's a library that doesn't actually have any PHP 5 breaks in a critical path.

This feels really risky for two reasons:

  • The library could then add a php 7 based code in a minor release, potentially even in a security related update
  • We don't know which classes people might use. If we have a dependency on a library we need to support all the code, as a module could use a class drupal core doesn't usetg
+++ b/core/modules/system/system.install
@@ -186,26 +186,34 @@ function system_requirements($phase) {
+  if (version_compare($phpversion, DRUPAL_MINIMUM_INSTALL_PHP) < 0) {
+    $requirements['php']['description'] = t('Your PHP installation is too old. Drupal requires at least PHP %version. It is recommended to upgrade to PHP version %recommended or higher for the best ongoing support. See <a href="http://php.net/supported-versions.php">PHP\'s version support documentation</a> and the <a href=":php_requirements">Drupal 8 PHP requirements handbook page</a> for more information.',
+      [
+        '%version' => DRUPAL_MINIMUM_INSTALL_PHP,
+        '%recommended' => DRUPAL_RECOMMENDED_PHP,
+        ':php_requirements' => 'https://www.drupal.org/docs/8/system-requirements/php',
+      ]
+    );

It seems worth mentioning on https://www.drupal.org/docs/8/system-requirements/php-requirements that update is still supported but not the install.

catch’s picture

#70 is exactly what I mean with this patch allowing things to be phased. Even there are arguments for doing everything at once, by default we have not done that for any of the previous times we've dropped support for a PHP version. Or in other words the patch allows us to be more aggressive with when we drop support, not less, relative to the status quo of never actually getting around to it.

catch’s picture

It seems worth mentioning on https://www.drupal.org/docs/8/system-requirements/php-requirements that update is still supported but not the install.

For me no. We don't actually support people updating existing sites, the patch just allows us to not actively prevent people from doing this. The PHP version will still be untested etc.

alexpott’s picture

I'm still super confused by this approach. The moment a library is PHP7 only and has a spaceship operator or something and is used in our update process our best effort support is broken. We have been placing warning in people's status reports since we announced dropping PHP5 support. If we're arguing that it is too late to properly drop PHP5 support for 8.7.x fine I can buy that that and I can also buy that we should have made the following code warn on update too

  if ((version_compare($phpversion, DRUPAL_RECOMMENDED_PHP) < 0) && ($phase === 'install' || $phase === 'runtime')) {
    // Warn if still on PHP 5. If at least PHP 7.0, relax from "warning" to
    // "info", and show it at runtime only, to not scare users while installing.
    if (version_compare($phpversion, '7.0') < 0) {
      $requirements['php']['description'] = t('Drupal will drop support for this version on March 6, 2019. Upgrade to PHP version %recommended or higher to ensure your site can receive updates and remain secure. See <a href="http://php.net/supported-versions.php">PHP\'s version support documentation</a> and the <a href=":php_requirements">Drupal 8 PHP requirements handbook page</a> for more information.', ['%recommended' => DRUPAL_RECOMMENDED_PHP, ':php_requirements' => 'https://www.drupal.org/docs/8/system-requirements/php']);
      $requirements['php']['severity'] = REQUIREMENT_WARNING;
    }
    else {
      if ($phase === 'runtime') {
        $requirements['php']['description'] = t('It is recommended to upgrade to PHP version %recommended or higher for the best ongoing support.  See <a href="http://php.net/supported-versions.php">PHP\'s version support documentation</a> and the <a href=":php_requirements">Drupal 8 PHP requirements handbook page</a> for more information.', ['%recommended' => DRUPAL_RECOMMENDED_PHP, ':php_requirements' => 'https://www.drupal.org/docs/8/system-requirements/php']);
        $requirements['php']['severity'] = REQUIREMENT_INFO;
      }
    }
  }

And I also understand that we might want to introduce a DRUPAL_FUTURE_MINIMUM_PHP and continue to issue a warning on update/install/runtime if you are under it. At the moment this is hardcoded to be if (version_compare($phpversion, '7.0') < 0) { in several places.

catch’s picture

The moment a library is PHP7 only and has a spaceship operator or something and is used in our update process our best effort support is broken.

Right, so we haven't updated libraries to require PHP 7 in 8.7.x, because we've been running tests against PHP 5 versions during development. This has drawbacks in that we've had to hold back some library updates, but it also avoids having to change patches we backport to 8.6.x and 8.5.x too just because of syntax changes or similar in Drupal code.

However assuming we do start to upgrade to libraries requiring PHP 7 in 8.8.x, we could also increase the update minimum PHP in that version to match. When we drop support for PHP 7.0, the infrastructure is here to prevent new installs prior to upgrading PHP dependencies etc. all over again. We don't actually have a proper policy for dropping PHP version support, which is partly why it's taken so long to drop PHP 5.5 and PHP 5.6, this issue is a symptom of that lack of policy, not its cause.

It's unlikely that we'll update a library dependency in 8.7.x to require PHP 7 from this point forward (i.e. post beta), but if we did so, it would most likely be due to a highly critical security release. In that case, unless the library update causes a fatal error on every request or something, the upgraded site on PHP 5.6 is likely to work better than a hacked one (or one stuck in a limbo of a new code base and updates not having been run).

alexpott’s picture

It's unlikely that we'll update a library dependency in 8.7.x to require PHP 7 from this point forward (i.e. post beta), but if we did so, it would most likely be due to a highly critical security release. In that case, unless the library update causes a fatal error on every request or something, the upgraded site on PHP 5.6 is likely to work better than a hacked one (or one stuck in a limbo of a new code base and updates not having been run).

But that's what I don't understand. Is this site not better to be on 8.6.x which is still security supported and has PHP 5 support. Yes their urgency to get to support PHP7 is still there but at least they are not broken.

catch’s picture

But that's what I don't understand. Is this site not better to be on 8.6.x which is still security supported and has PHP 5 support. Yes their urgency to get to support PHP7 is still there but at least they are not broken.

Yes, but if they don't realise that 8.6 security support is available and better for them, they could end up trying to update to 8.7 anyway.

Additionally 8.7 is supported for a year, and the highly critical security release might not happen until 8.8.1, so if they take advantage of the longer security support at that point, they'll be better off than staying on a completely unsupported 8.6 (which might only be a month out of support by that point).

effulgentsia’s picture

We don't actually support people updating existing sites, the patch just allows us to not actively prevent people from doing this.

Based on this statement, here's a patch that renames the new constant from DRUPAL_MINIMUM_INSTALL_PHP to DRUPAL_MINIMUM_SUPPORTED_PHP. I also added some more code docs (but did not change any user-facing text) to help clarify the difference between DRUPAL_MINIMUM_SUPPORTED_PHP and DRUPAL_MINIMUM_PHP.

I don't think that really addresses @alexpott's deepest concerns with this patch, but maybe it helps a little?

effulgentsia’s picture

Is this site not better to be on 8.6.x which is still security supported and has PHP 5 support.

I think this is @alexpott's main concern with this patch (as opposed to simply upping DRUPAL_MINIMUM_PHP to 7.0.8 in 8.7.x).

To answer that, I don't see how a site could possibly be better off on 8.6 than on 8.7. So long as we're running any PHP 5 tests on 8.6, why wouldn't we also run them on 8.7? If a vendor library issues an SA and only fixes it in a PHP 7 compatible version, then that affects 8.6 just as much as it affects 8.7. If we backport the fix to the version that's in 8.6, we can also backport it to the version that's in 8.7. If a PHP 5 backport is not feasible, then potentially we update the library in a way that breaks on PHP 5. But if that's the situation we're in, it applies equally to 8.6.

One could potentially argue that a site is equally well off on 8.6 as on 8.7 (for the duration that 8.6 is still security supported), but that's not true for several reasons:

  • The extended security support of the previous minor applies to core only. Contrib modules are not required to remain compatible with the previous core minor. If a site is forced into staying on 8.6, it might therefore expose them to unpatched disclosed security vulnerabilities in the contrib modules that they use.
  • The purpose of the 6 month overlapping window (where both 8.6 and 8.7 are security supported) is to give people a window of time in which they have the choice of when to make time in their schedule to do all the Drupal / website work required to perform the update. But if they're forced into staying on 8.6 until after their IT department updates to PHP 7, then that window gets shortened. By allowing updates to 8.7, the site administrators can update on their schedule, without being hard-blocked on their IT department.

One could still argue that the above benefits aren't big enough to justify us doing the work of this patch, or creating confusion around the difference between "this is supported" vs. "it's not supported, but you can still do it, and we'll try to be courteous about not breaking your stuff without a really good reason". As far as the work of this patch goes, that's already pretty much done, so I don't think that should factor into the decision. As far as the confusion about what we mean by supported vs. still-functional and whether there's enough value in keeping unsupported things functional, I think that's a release management decision, so tagging for that.

tedbow’s picture

I am new to this patch so I might be missing something but...

  1. +++ b/core/tests/Drupal/Tests/RequirementsPageTrait.php
    @@ -0,0 +1,75 @@
    +  protected function continueOnExpectedWarnings($expected_warnings = []) {
    

    I don't think method is actually called from anywhere anymore.
    I search my code base after apply the patch this declaration is the only match I find.

  2. +++ b/core/tests/Drupal/Tests/RequirementsPageTrait.php
    @@ -0,0 +1,75 @@
    +  protected function assertWarningSummaries(array $warning_summaries) {
    ...
    +    for ($i = 0; $i <= count($warning_summaries); $i++) {
    +      $selectors[] = 'h3#warning' . implode('', array_fill(0, $i + 1, '+details')) . ' summary';
    +    }
    

    If continueOnExpectedWarnings() indeed can be removed then assertWarningSummaries in only called once with the argument ['PHP']. if that is case then we don't need the logic around more selectors, correct?

catch’s picture

As far as the confusion about what we mean by supported vs. still-functional and whether there's enough value in keeping unsupported things functional, I think that's a release management decision, so tagging for that.

I think both release managers (i.e. me and xjm) are agreed we need the flexibility to differentiate between these two things - although as above I see this as an implementation detail of the update system rather than any official extra level of support.

effulgentsia’s picture

Status: Needs review » Needs work

Needs work for #80. Other than that, given #81, I think this is very close. Possibly committable after #80 is fixed.

plach’s picture

Minor nit:

+++ b/core/includes/bootstrap.inc
@@ -18,15 +18,36 @@
+ *   that Drupal no longer supports this PHP version.

What about changing this to "their PHP version", at first sight I thought the comment was referring to the PHP version of the constant being documented.

tedbow’s picture

Status: Needs work » Needs review
+++ b/core/modules/simpletest/src/InstallerTestBase.php
@@ -244,44 +242,4 @@ protected function refreshVariables() {
-  protected function continueOnExpectedWarnings($expected_warnings = []) {

Ok I was wrong in #80.

Since basically we moved continueOnExpectedWarnings from directly being in \Drupal\FunctionalTests\Installer\InstallerTestBase to being in a trait we can't actually remove it from core. We would have to deprecate them since our BC policy says

Test traits and abstract base classes should generally use deprecation where possible rather than breaking backwards compatibility, but they are still considered internal API and may change if necessary.

So even though we aren't using it anymore distro probably are.

So that would be out of scope for this issue. Same for assertWarningSummaries(). And we can't the method signature just because are using it for just 1 warning.

Sorry for the false alarm.

The patch looks good to me. I looked over the test changes.

pandaski’s picture

The patch looks good to me. Also, test in 8.8.x

alexpott’s picture

It's not just security. It's having DRUPAL_MINIMUM_PHP and thereby our composer.json be at PHP less than what we're running tests on and declaring we support. Even with regular PHP 5 tests we've broken PHP 5 loads of times now that patch testing is done on PHP 7 by default. I feel stupid because I don't understand how having code for allowing updates on PHP 5 and not testing it will lead to a good user experience. Additionally, there are so many workflows that companies have that start from an install - eg the backup and migrate flow, config install etc.. that having different minimum PHPs for update and install feels like inviting trouble.

alexpott’s picture

So here's another thing. If we don't change our minimum PHP in composer.json then people will be able to composer install drupal 8.7 on PHP 5 but hey you're not allowed to install it. If we do change on 8.7 then no one will be able to update their code on PHP 5 even though with this patch they would be able to run updates.

pandaski’s picture

And this section is quite confused to the site user

  // Suggest to update to at least 5.5.21 or 5.6.5 for disabling multiple
  // statements.
  if (($phase === 'install' || \Drupal::database()->driver() === 'mysql') && !SystemRequirements::phpVersionWithPdoDisallowMultipleStatements($phpversion)) {
    $requirements['php_multiple_statement'] = [
      'title' => t('PHP (multiple statement disabling)'),
      'value' => $phpversion_label,
      'description' => t('PHP versions higher than 5.6.5 or 5.5.21 provide built-in SQL injection protection for mysql databases. It is recommended to update.'),
      'severity' => REQUIREMENT_INFO,
    ];
  }
effulgentsia’s picture

effulgentsia’s picture

Issue tags: -Needs tests

The "Needs tests" tag was added in #9 and referenced again in #57.2. From what I can tell, the two required test cases from #8 are in this patch.

  1. +++ b/core/tests/Drupal/Tests/Core/Command/QuickStartTest.php
    @@ -132,10 +136,48 @@ public function testQuickStartCommand() {
    +  public function testPhpRequirement() {
    

    This implements #8.1.

  2. +++ b/core/tests/Drupal/Tests/RequirementsPageTrait.php
    @@ -0,0 +1,79 @@
    +  protected function updateRequirementsProblem() {
    +    // Assert a warning is shown on older test environments.
    +    if (version_compare(phpversion(), DRUPAL_MINIMUM_SUPPORTED_PHP) < 0) {
    +      $this->assertNoText('Errors found');
    +      $this->assertWarningSummaries(['PHP']);
    

    This implements #8.2.

Therefore, removing the "Needs tests" tag.

effulgentsia’s picture

Crediting people who've contributed through reviews or significant comments. Thank you!

effulgentsia’s picture

Earlier this evening, I discussed this issue at length with @alexpott, given his concerns in #86 and other comments. He continues to believe that we should either fully drop PHP 5 support (by changing DRUPAL_MINIMUM_PHP and our composer.json) in Drupal 8.7, or wait and not do that until Drupal 8.8. He remains skeptical about what anyone really gains by a halfway step of allowing updates but not installs and is concerned about the downsides of such a halfway step (as he expressed in #86 and #87). However, he also accepts that it's a release management decision, so given #81, he told me he's ok with it going in despite his concerns.

  • effulgentsia committed c087641 on 8.8.x
    Issue #3039287 by bendeguz.csirmaz, effulgentsia, xjm, alexpott, catch,...

  • effulgentsia committed 472d262 on 8.7.x
    Issue #3039287 by bendeguz.csirmaz, effulgentsia, xjm, alexpott, catch,...
effulgentsia’s picture

Status: Needs review » Fixed

The interdiffs in #78 and #89 include some not entirely trivial decisions, so given that, this should normally exclude me from RTBC'ing or committing this patch. However, because we're only half a day or so from the beta release, I'm breaking protocol, and pushed this to 8.8.x and 8.7.x.

I'm ok with this, because #84 and #85 provided some additional eyes on this (thank you!), and I think it's better to have a few extra hours where this is in the branches getting whatever additional accidental testing comes from that.

Any committer is free to revert this if they disagree with my decision. Or, we can open a followup to discuss or refine anything that still needs that.

Thanks again for everyone's contribution to this!

Gábor Hojtsy’s picture

Let's document this :)

Gábor Hojtsy’s picture

Issue tags: +Needs screenshots

Also screenshots would help people (in the change record at least) to see how they can identify if something is wrong.

Gábor Hojtsy’s picture

Issue summary: View changes
Gábor Hojtsy’s picture

Has all the missing parts now.

Status: Fixed » Closed (fixed)

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

pfrenssen’s picture

I'm having serious trouble with some of my contributed projects because of the way this has been implemented.

I am co-maintainer of the Behat Drupal Extension project. I am starting to implement PHP 7 only features on a number of modules that support Behat which are intended to be released for Drupal 8.8. It is a very frustrating experience to get tests up and running for them. Behat tests are running against an installed Drupal. I cannot do that any more for Drupal 8.7 on PHP 5.6, even though users are still allowed to use my software.

Like @alexpott mentioned in #87 we now have a declaration in composer.json that says that Drupal 8.7 is compatible with PHP 5.6, but this is not correct. It does not resolve into a working package (meaning a package in which I can install Drupal so I can run my tests on them).

The minimum PHP version that is declared in composer.json should not be considered as some kind of suggestion. This is intended to be read by automated systems and result in a working system under all circumstances.

pfrenssen’s picture

I think the only proper approach to this is to allow to install Drupal 8.7.x on PHP 5.6 but show a warning. Then at least we are honoring the minimum PHP version compatibility as stipulated in composer.json.