Warn users of old PHP versions

Per #2524432: Suggest PHP 5.6 as the recommended version it would be good to at least know what debian/ubuntu are planning to do with the PHP versions on their releases that originally shipped with 5.5 - i.e. whether they'll upgrade to 5.6 or backport security fixes to 5.5. If the former then warning is quite straightforward. If the latter then we're potentially showing a false alarm to people - not that it'd be a bad thing if the update anyway.

It may be difficult to differentiate between a "safe/patched" 5.5 and an "unsafe/unpatched" 5.5. I think just noting that 5.5 has been retired is sufficient. It's only a false alarm if we make it sound like an alarm; if it's kept informational it should be safe to blanket-state.

I think just noting that 5.5 has been retired is sufficient.

Which is happening in 19 days time!

I accept a blanket statement is probably the way we are going to go.

But I feel I must stick up for the little guy,,,,,

Drupal welcomes semi-technical users/admins and long may that continue.

So, in my story, the little guy is an intern in a law firm whose boss has heard rumours about an out of date version of Drupal being a component in the Mossack Fonseca security break ( the difference between php and Drupal will unclear to both of them )

As far a I can see a smart intern should conclude that in 6 months php7.0 is the only version with active support
http://php.net/supported-versions.php
and so he/she must do whatever ( throw away a tardy distro or change hosting provider ) to get the site on php7 as soon as possible

If we say something too mild only 2 in 10 interns are going to hit the smart conclusion.

So that we have something to actually review here...

I considered encoding in automatic warnings for other PHP versions when they go out of date in the future, based on the current date. However, technically 7.0 goes out of support a few days before 5.6 does (5.6 got LTS support), and we don't know yet what the compatibility situation will be for 7.1 nor its end date, so I decided to KISS. I'm not checking the date at all because by the time 8.2 ships 5.5 is guaranteed to be unsupported anyway, so it would always be a true check.

We could easily switch from a Warning to an Info; I won't put up much of a fight either way.

The error message recommends upgrading to 7.0, because we know 8.0 runs fine on 7.0 and is even much faster, so no sense in telling people to use a slower version. :-) If someone is running on 5.6, though, there should be no message as that's still security-safe.

A well reasoned argument.

Patch looks good.

+1 from me

martin: Good to hear. Care to RTBC it? :-)

Sure I'm happy - and time is pressing

Should we do version_compare($phpversion, '5.5') < 0 instead?

Mm, yeah, that's probably better. Although it's '5.6' < 0, since anything < 5.6 that's already passed the minimum version check must be 5.5.x, which is what we're warning about.

Hehe, this means that in the installer a requirements warning is shown on PHP 5.5, which the testbot does not expect. I think that is a good thing, though, but we need to figure out a way to get this to pass.

Sending on a run on PHP 5.6 just to be sure.

LOL. Well look at that, multi-version testbot testing works as advertised. :-) Yeah, this would therefore require changing the testbot runner to 5.6 at least. There's already #2607222: [policy, no patch] Default to PHP 7 for Drupal core patch testing, if we wanted to go that far with it.

I suppose if we dropped it to an INFO notice rather than a warning, testbot would be OK with that?

Here's an alternate version that is the same, but raises an INFO rather than WARNING, just to see what Mr. Roboto says about it.

I actually think a warning would be preferable, precisely because it shows up in the installer and, thus, raises awareness.

And I think it makes sense to adapt the tests so they work on all environments. We basically just have to adapt InstallerTestBase to conditionally click the "Continue anyway" link in the installer if we're on PHP 5.5.

I'd personally agree with the stronger warning if we can make the testbot play nice, but I'm good either way. Deferring to the release managers on which way to go with it.

Did anyone look further into @catch's comment in #2?

Per #2524432: Suggest PHP 5.6 as the recommended version it would be good to at least know what debian/ubuntu are planning to do with the PHP versions on their releases that originally shipped with 5.5 - i.e. whether they'll upgrade to 5.6 or backport security fixes to 5.5. If the former then warning is quite straightforward. If the latter then we're potentially showing a false alarm to people

The current wording does read like an alarm currently, and assuming the above is the case it would be a false alarm which isn't good.

I think in that scenario it should really be more informational, e.g. a REQUIREMENT_INFO with something like "Your PHP installation is running version 5.5, which is no longer supported by the upstream PHP project (although it may be supported by other organizations). For better performance and support, upgrade to PHP 7.0 or higher."

Also this issue kind of seems like a duplicate of the one mentioned above (#2524432: Suggest PHP 5.6 as the recommended version) which also has a patch... not sure it really makes sense to keep both open separately?

Ubuntu 12.04 LTS was just updated to 5.3.10-1ubuntu3.24 and will continue to be supported through Apr-2017.

Ubuntu 14.04 LTS was just updated to 5.5.9+dfsg-1ubuntu4.19 and will continue to be supported through Apr-2019.

Upgrading the PHP version within an LTS release would risk breaking things — the exact opposite of what LTS is supposed to do.

I agree with David_Rothstein.

At this point I'm half tempted to deprecate both issues and have a notice-level message that recommends people upgrade all the way to PHP 7. Uptake has been good, performance is great, stability is fine, and it will be the only version getting bugfix support in less than a month.

I closed #2524432: Suggest PHP 5.6 as the recommended version as a duplicate.

I would suggest a less alarming wording, but I do agree we should start recommending PHP 7 soon. #2524432: Suggest PHP 5.6 as the recommended version has some added constants for a "Recommended version" that might be worth adopting rather than hardcoding this warning.

Does #23 constitute enough of a review to remove the "Needs release manager review" tag?

I'd also go for less alarming wording - if you're on an old LTS you're still getting security support from the distro. Otherwise agreed with the direction of the issue, so I think that counts as three +1s from the release side.

I see others, suggesting that the wording is a little strong

#33 and #23 #19

For me the wording is fine.

No offense to the people maintaining distros with security minded updates but I see that support as a less than the support offered by the original PHP developers.

The number of eyes looking at the code will be less, and those eyes with have less familiarity with the original intent of the original code.

In short when a version is done, it is time to move on

Thanks @martin107.

Two committers have asked for the wording to be changed, so the issue is NW for that.

@martin07 if we want strong wording, we can make it an install error and drop official support for 5.5 entirely, just not in this issue which is only about notifying people.

The 'warning' error level was entirely correct for the current warning, but that can't be done without a larger change to tests and more friction in the installer for real people.

A fair point.

Well, here we are again. I've softened the wording a bit and used a new constant to specify the recommended version, which I've set at 7.0.0. That's easy to tweak to whatever the RMs decide it should be if they want a later patch release of 7.0. It's still a notice-level message.

If this wording isn't acceptable, RMs/committers please suggest something and I'll reroll.

The patch is trivial enough I didn't bother with an interdiff, but it's just a wording change.

+++ b/core/modules/system/system.install
@@ -178,6 +178,11 @@ function system_requirements($phase) {
+  if (version_compare($phpversion, DRUPAL_RECOMMENDED_PHP) < 0) {
+    $requirements['php']['description'] = t('Your PHP installation is running version %version, which is no longer receiving support from the PHP development team. We recommend upgrading to version %recommended or higher.', ['%version' => DRUPAL_MINIMUM_PHP, '%recommended' => DRUPAL_RECOMMENDED_PHP]);

$phpversion is retrieved trough phpversion(), but %version shown in the message is retrieved through DRUPAL_MINIMUM_PHP.

I think %version should be $phpversion and not DRUPAL_MINIMUM_PHP, right?

+++ b/core/modules/system/system.install
@@ -178,6 +178,11 @@ function system_requirements($phase) {
+    $requirements['php']['description'] = t('Your PHP installation is running version %version, which is no longer receiving support from the PHP development team. We recommend upgrading to version %recommended or higher.', ['%version' => DRUPAL_MINIMUM_PHP, '%recommended' => DRUPAL_RECOMMENDED_PHP]);

Isn't PHP 5.6 receiving security support until 31 Dec 2018. Which is actually longer than PHP 7.0.0 (https://secure.php.net/supported-versions.php)

I'm very much in favour of recommending PHP 7 but I don't think we need to mis-inform people who are PHP 5.6.

Here's an approach that does not mis-inform people but still always recommends.

@hchonov is correct in #39. Fixed in latest patch.

+++ b/core/modules/system/system.install
@@ -177,11 +177,19 @@ function system_requirements($phase) {
-    $requirements['php'] = array(
+    $requirements['php_mysql'] = array(

This is required because the way this was added resulting in overwriting the new messages.

+++ b/core/modules/system/system.install
@@ -177,11 +177,19 @@ function system_requirements($phase) {
+  elseif (version_compare($phpversion, DRUPAL_MINIMUM_SECURE_PHP) < 0) {
+    $requirements['php']['description'] = t('Your PHP installation is running version %version, which is no longer receiving security support from the PHP development team. We recommend upgrading to version %recommended or higher.', ['%version' => $phpversion, '%recommended' => DRUPAL_RECOMMENDED_PHP]);

In this case where the php version is lower than the minimum secure version I would say the message should be saying that an update is recommended to the minimum secure but better the to the drupal recommend php version - I mean the message should currently contain as suggestions PHP 5.6 as the minimum secure version and PHP 7 as the currently recommended version. What do you think?

@hchonov I think we should only recommend one version. Doing two recommendations feels overly complex.

@alexpott, then probably on 5.5 we should recommend 5.6 and higher and on 5.6 recommend 7 and higher, otherwise if on 5.5 we recommend directly 7 it sounds like we do not support 5.6. I mean it is usual to recommend the next supported version and higher and not skip versions in between.

@hchonov I don't follow that logic to be honest. If we want to recommend PHP 7 then that's what we should do.

Ok, I get it - we only want to recommend 7 :).

The patch looks good now.

The patch source code includes http://php.net/supported-versions.php (which I've found a highly useful page), could that be visible to users?

e.g. wrap an href tag around "no longer receiving (security) security support" in the warnings.

@wturrell, you are right, but actually I don't think that this is necessary as this is mainly what we recommend for Drupal installations.

Committed/pushed to 8.3.x, thanks!

So #14 / #15 / #17 of this issue was never actually resolved, i.e. the installer now (correctly) shows a warning on PHP 5.5 forcing people to explicitly click "continue anyway" if they want to install nevertheless. This is a good thing, except that our installer tests do not account for this behavior.

Therefore the test suite currently fails on PHP 5.5 (see https://www.drupal.org/pift-ci-job/574483 for proof, I just uploaded a quasi-empty patch). This was not caught as the testbots now default to 5.6.

As the fix is presumably pretty self-contained into one of the installer test base classes, I personally think handling this in a follow-up should be fine, as this doesn't hinder core development unless PHP 5.5 specific bugs are found which I don't think is that likely. Alternatively this would have to be rolled back.

No that's just a whoops, had we tested on all versions this wouldn't have got to/stayed RTBC in the first place. Reverted. We can make it a warning, but in another issue and with the requisite test alterations.

+    $requirements['php']['description'] = t('Your PHP installation is running version %version, which is no longer receiving security support from the PHP development team. We recommend upgrading to version %recommended or higher.', ['%version' => $phpversion, '%recommended' => DRUPAL_RECOMMENDED_PHP]);

1. Do we really want to overwrite the other description that tells people how to enable phpinfo()? See code about 10-15 lines above this:
   

       $requirements['php'] = array(
   ....
         'description' => t('The phpinfo() function has been disabled for security reasons. To see your server\'s phpinfo() information, change your PHP settings or contact your server administrator. For more information, <a href=":phpinfo">Enabling and disabling phpinfo()</a> handbook page.', array(':phpinfo' => 'https://www.drupal.org/node/243993')),
         'severity' => REQUIREMENT_INFO,
       );
   

2. I think it's incomplete (and too scary) to mention that PHP doesn't provide security support without mentioning that others do. It's also a little long. I suggest changing it to simply say "... which is no longer supported by the PHP development team".
3. In the last sentence, who is "We"? I suggest changing it to "Version %recommended or higher is recommended."

+ $requirements['php']['description'] = t('Your PHP installation is running version %version. We recommend upgrading to version %recommended or higher.', ['%version' => $phpversion, '%recommended' => DRUPAL_RECOMMENDED_PHP]);

Some similar comments as above, and also - this is missing any reason for the recommendation. I think this (and the above) should link to https://www.drupal.org/requirements/php or similar like the earlier patch in #2524432: Suggest PHP 5.6 as the recommended version did, since that is a place where the recommendation could be fleshed out.

I have done the above mentioned changes, Please take a look

1. index 0000000..94a25f7
   --- /dev/null
   
   --- /dev/null
   +++ b/.idea/vcs.xml
   
   +++ b/.idea/vcs.xml
   @@ -0,0 +1,6 @@
   
   @@ -0,0 +1,6 @@
   +<?xml version="1.0" encoding="UTF-8"?>
   +<project version="4">
   +  <component name="VcsDirectoryMappings">
   +    <mapping directory="$PROJECT_DIR$" vcs="Git" />
   +  </component>
   +</project>
   \ No newline at end of file
   

   Oops. :-)

2. +++ b/core/modules/system/system.install
   @@ -177,11 +177,19 @@ function system_requirements($phase) {
        // If PHP is old, it's not safe to continue with the requirements check.
        return $requirements;
      }
   +  elseif (version_compare($phpversion, DRUPAL_MINIMUM_SECURE_PHP) < 0) {
   +    $requirements['php']['description'] = t('Your PHP installation is running version %version, which is no longer supported by the PHP development team.  Version %recommended or higher.', ['%version' => $phpversion, '%recommended' => DRUPAL_RECOMMENDED_PHP]);
   +    $requirements['php']['severity'] = REQUIREMENT_WARNING;
   +  }
   +  elseif (version_compare($phpversion, DRUPAL_RECOMMENDED_PHP) < 0) {
   +    $requirements['php']['description'] = t('Your PHP installation is running version %version. We recommend upgrading to version %recommended or higher.', ['%version' => $phpversion, '%recommended' => DRUPAL_RECOMMENDED_PHP]);
   

   Why not just make the PHP version a separate block, php_version, so there's no clobbering of the phpinfo message either way? If both are problems then, well, both are problems.

Addressing items from #57.

I added tests for PHP 5.5 and 7.0. We have to check what happens in all cases.

+++ b/core/modules/system/system.install
@@ -172,16 +172,33 @@ function system_requirements($phase) {
+      'value' => t('Your PHP installation is running version %version, which is no longer supported by the PHP development team.  Version %recommended or higher.', ['%version' => $phpversion, '%recommended' => DRUPAL_RECOMMENDED_PHP]),

Extra space before "Version %recommended or higher.". Also "Version %recommended or higher." doesn't sound. Maybe "We recommend upgrading to version %recommended or higher." as in the last elseif(...)?

Yes, I think it should be "Version %recommended or higher is recommended" (like I suggested above). And similar wording is still needed for the DRUPAL_RECOMMENDED_PHP sentence too.

Also this is still a REQUIREMENT_WARNING, so it will probably fail tests...

I don't think it makes much sense to have two separate lines in the status report about the PHP version -- too much duplication. However to avoid clobbering messages we could take the earlier one and move that to a separate status report block. I.e. this one:

    $requirements['php'] = array(
      'title' => t('PHP'),
      'value' => $phpversion_label,
      'description' => t('The phpinfo() function has been disabled for security reasons. To see your server\'s phpinfo() information, change your PHP settings or contact your server administrator. For more information, <a href=":phpinfo">Enabling and disabling phpinfo()</a> handbook page.', array(':phpinfo' => 'https://www.drupal.org/node/243993')),
      'severity' => REQUIREMENT_INFO,
    ); 

(Since it doesn't have anything to do with the PHP version.) Doing it that way would also get rid of the clobbered message.

I would also rename DRUPAL_MINIMUM_SECURE_PHP to DRUPAL_MINIMUM_SUPPORTED_PHP since we're not actually claiming it's insecure.

OK. Fixing.

Here we go.

EDIT: I kept the WARNING and changed InstallTestBase as it was suggested in #17. A warning is better.
EDIT2: Also I think this should not wait for 8.4.x.

Hm. InstallerTestBase is not prepared for this.

@todo Fix also UpdateTestBase

In general, issues like this should be targeted for 8.4.x now, but in this case, I agree that this change should also be added for 8.3.0.

I'd even add this in RC.

Sounds like this needs work for #68.

PHP 5.5 reached end of life 10 months ago. Should this issue maybe be changed to dropping support for it in 8.4.x instead?

In #2874909: Update Symfony components to 3.3.* it was discovered some newer packages depend on PHP 5.6, so I agree with #72, we need to look at making 5.6 a minimum requirement.

  Problem 1
    - Installation request for doctrine/annotations v1.4.0 -> satisfiable by doctrine/annotations[v1.4.0].
    - doctrine/annotations v1.4.0 requires php ^5.6 || ^7.0 -> your PHP version (5.5.38) does not satisfy that requirement.
  Problem 2
    - Installation request for doctrine/collections v1.4.0 -> satisfiable by doctrine/collections[v1.4.0].
    - doctrine/collections v1.4.0 requires php ^5.6 || ^7.0 -> your PHP version (5.5.38) does not satisfy that requirement.
  Problem 3
    - Installation request for doctrine/common v2.7.2 -> satisfiable by doctrine/common[v2.7.2].
    - doctrine/common v2.7.2 requires php ~5.6|~7.0 -> your PHP version (5.5.38) does not satisfy that requirement.
  Problem 4
    - Installation request for zendframework/zend-feed 2.8.0 -> satisfiable by zendframework/zend-feed[2.8.0].
    - zendframework/zend-feed 2.8.0 requires php ^5.6 || ^7.0 -> your PHP version (5.5.38) does not satisfy that requirement.
  Problem 5
    - Installation request for zendframework/zend-stdlib 3.1.0 -> satisfiable by zendframework/zend-stdlib[3.1.0].
    - zendframework/zend-stdlib 3.1.0 requires php ^5.6 || ^7.0 -> your PHP version (5.5.38) does not satisfy that requirement.

https://www.drupal.org/pift-ci-job/697487

Running composer update in Drupal Core will now pull down some doctrine libraries which require PHP 7. We really need to take this more seriously. I've already hit this with 8.3.x in multiple client projects.

Can we get this in 8.4?

@timmillwood, see #68. I would definitely like to see this in 8.4.x (it's marked as an RC target) but the patch tests need to be fixed first.

Actually dropping support for a version of PHP is out of scope here; the issue for that is #2842431: [policy] Remove PHP 5.5, 5.6 support in Drupal 8.7.

1. +++ b/core/includes/bootstrap.inc
   @@ -24,6 +24,18 @@
    const DRUPAL_MINIMUM_PHP = '5.5.9';
    
    /**
   + * Minimum version of PHP receiving security updates.
   + *
   + * @see https://secure.php.net/supported-versions.php
   + */
   +const DRUPAL_MINIMUM_SUPPORTED_PHP = '5.6';
   +
   +/**
   + * Minimum recommended version of PHP.
   + */
   +const DRUPAL_RECOMMENDED_PHP = '7.0.0';
   

   The constant names here need work. I wouldn't understand that DRUPAL_MINIMUM_PHP is different from DRUPAL_MINIMUM_SUPPORTED_PHP or what the difference between them is. I'm not sure what to name the middle constant.

2. +++ b/core/modules/system/system.install
   @@ -157,35 +157,42 @@ function system_requirements($phase) {
   -      'severity' => REQUIREMENT_INFO,
   +      'severity' => REQUIREMENT_WARNING,
   

   Changing this seems out of scope for the issue.

+++ b/core/modules/simpletest/src/InstallerTestBase.php
@@ -218,4 +244,29 @@ protected function refreshVariables() {
+      $text = t('Your PHP installation is running version %version, which is no longer supported by the PHP development team. We strongly recommend upgrading to version %recommended or higher.', ['%version' => phpversion(), '%recommended' => DRUPAL_RECOMMENDED_PHP]);

Rather than saying "the PHP development team" which could be confusing, I think we should link http://php.net/supported-versions.php. Like:
...%version, which is <a href="http://php.net/supported-versions.php">no longer supported</a>.

Also it needs to dig which PHP 7.0.x version is minimally usable for core, iirr there was a lot of segfaults in first releases

My bad for suggesting DRUPAL_MINIMUM_SUPPORTED_PHP - I wasn't thinking about the fact that the existing constant had a similar name. Maybe it could be something like DRUPAL_MINIMUM_UPSTREAM_SUPPORTED_PHP instead.

Linking to http://php.net/supported-versions.php is good, but linking to it without commentary is somewhat misleading because it doesn't tell the whole story of PHP security support. Could use something like the text in #19, or if that's too wordy then maybe just something like ....which is no longer supported by the <a href="http://php.net/supported-versions.php">original developers</a>.

Also it needs to dig which PHP 7.0.x version is minimally usable for core, iirr there was a lot of segfaults in first releases

The feature set of PHP 7.0 is also incomplete and prevents the usage of some of its most important improvements in practice (for example it lacks the void return type and nullable types).

Symfony 4 which will be released next week is requiring PHP 7.1.3 as a minimum version. They probably chose this version because it fixes Bug #73989 which was discovered by running the Symfony test suite.

I think we can take the same approach. We can pick the lowest version of PHP 7.1.x which passes our entire test suite.

We're now in a situation where this very important information for developers has been blocked for a year, which means that those PHP versions are a year closer to EOL without getting this very important information.

We can discuss splitting warnings about unsupported versions in the future and the specific policy in other issues, but let's please get this in as soon as possible to start warning site owners about the upcoming EOLs for several versions. Attached strips the patch down to its minimum parts (it will probably need some of the test fixes again, but it no longer applied).

Setting to 8.4.x. We want to backport this as soon as possible to start informing site owners.

Okay that was a bit more than I was expecting and also the testbot is apparently using some version of 7.0 rather than 7.1. I should not have queued so many environments... Anyway, working on re-adding some of @claudiu.cristea's test improvements but trying to keep it to the minimum scope.

Attached fixes the update tests (and update.php) by only raising the warning at install and runtime. We don't need to annoy people every single time they update Drupal; that's counterproductive.

+++ b/core/includes/bootstrap.inc
@@ -22,6 +22,11 @@
+const DRUPAL_RECOMMENDED_PHP = '7.4.0';

lol, this was how I was testing it locally.

Fixed in attached.

If you accidentally try to upload a patch with a disallowed extension to d.o, it fails silently.

Missed one.

Attached fixes an issue with the CSS selectors being tied to a specific CSS class (which won't necessarily be present for other distributions), so that the expected warnings are matched correctly.

#97 would still skip past other warnings on old PHP versions. Attached fixes that so that tests will fail if there is a different warning.

+++ b/core/includes/bootstrap.inc
@@ -22,6 +22,11 @@
+const DRUPAL_RECOMMENDED_PHP = '7.4.0';

Why this version?
SF 4 will require 7.1.3 https://symfony.com/doc/master/reference/requirements.html

@andypost, see #86 -- it's just from testing something that definitely isn't released yet. :)

Attached is the actual patch.

@xjm I mean to set that to 7.1.3 instead of 7.1.0 - the same #80 tells

Thanks @dawehner and @andypost for the reviews!

I'll address point #102.1 shortly. I think #103 makes sense as well because of our close dependency on Symfony, so I'll update that as well.

For #102.2, this is actually necessary to make the tests pass. It's not moving code in the file -- the diff is misleading. It's splitting up an existing method that incorrectly spanned three installer steps, so that our new test step can correctly happen between them.

Here we go.

Here's what it looks like (using an artificially high PHP version):

Should we call them expected or allowed warnings? The parameter and the function name kinda disagree ...

Good point; I went back and forth between failing when an expected warning wasn't in the list and eventually decided that we should to avoid accidentally hiding things. If we want to allow but not expect certain warnings we can always override the method. So I'll rename the parameter to match.

I'll roll a patch addressing #108.

1. +  elseif ((version_compare($phpversion, DRUPAL_RECOMMENDED_PHP) < 0) && ($phase === 'install' || $phase === 'runtime')) {
   +    $requirements['php']['description'] = t('Your PHP installation is running version %version. It is recommended to upgrade to version %recommended or higher to ensure your site remains secure and continues to receive updates. See the <a href="http://php.net/supported-versions.php">PHP version support documentation</a> and the <a href=":php_requirements">PHP requirements handbook page</a> for more information.', ['%version' => $phpversion, '%recommended' => DRUPAL_RECOMMENDED_PHP, ':php_requirements' => 'https://www.drupal.org/docs/8/system-requirements/php']);
   

   This is still going to clobber the "phpinfo is disabled" text (see #55, point 1). The patch in #68 is the most recent which had a fix for that, but it was lost in subsequent patches.

2. + * Sites installing Drupal on PHP versions lower than this will see a warning
   + * message, but Drupal can still be installed.
   + */
   +const DRUPAL_RECOMMENDED_PHP = '7.1.3';
   

   This I don't understand. If a site is running PHP 7.0.26, for example (currently an up-to-date, secure, and supported release), there is going to be a warning message telling it to update to PHP 7.1.3 "to ensure your site remains secure", even though PHP 7.1.3 actually has known security vulnerabilities and PHP 7.0.26 doesn't?

   Drupal cannot be the "Update Manager" for the PHP project - it is too complicated. If there is a Drupal-specific reason to warn people about the PHP version they are using, that's OK - but otherwise it shouldn't be a warning message and shouldn't mention security.

3. Two links is maybe information overload... Perhaps there should be one link to https://www.drupal.org/docs/8/system-requirements/php, and the link to http://php.net/supported-versions.php could be on that drupal.org page instead (where it could also be presented in the proper context). Actually, this might even be useful to display for sites that are running a completely up-to-date version of PHP, since it won't be up-to-date forever.

This is still going to clobber the "phpinfo is disabled" text (see #55, point 1). The patch in #68 is the most recent which had a fix for that, but it was lost in subsequent patches.

I deliberately excluded this because it looked to be out of scope -- this already happens in HEAD, because unlike the SQL multiple statement warning, it comes before the PHP requirements check rather than after.

I can see this both ways. So, in order to not have the phpinfo() message shown when you can't install Drupal at all, but to still allow it to be shown next to the warning, I've moved the whole thing down after the version check as well as changed the key. I'm just trying to keep the scope small so this can land.

Drupal cannot be the "Update Manager" for the PHP project - it is too complicated. If there is a Drupal-specific reason to warn people about the PHP version they are using, that's OK - but otherwise it shouldn't be a warning message and shouldn't mention security.

I think it is important to mention security, because if you stay on an old PHP version after we completely drop support, you can't get Drupal security updates for it. Not because PHP might be insecure, but because Drupal will eventually disclose a vulnerability that you can't update for. So I am going to leave the message as-is. We could bump the recommended PHP version to 7.1.7 instead of 7.1.3, though, since that looks to be the PHP security release that patched a bunch of sec issues.

I think linking the PHP release cycle support directly is also important, because otherwise the information is buried. I also don't want to retain a copy of it on a handbook page that won't be updated regularly. People should be aware of PHP's own support cycles and make decisions based on that. If they're on an Ubuntu LTS or whatever other distro that backports sec fixes, then that's up to them to determine.

Attached raises the minimum recommended version to 7.1.7 and makes the change to the phpinfo() section described above. To make the message a little clearer, I changed it to "PHP's version support documentation" to distinguish it from ours. But I feel strongly that the link should be there and that we need to mention security support in the warning, so I did not change that.

Needs to leave the variable definition where it was though.

Okay, so #113.1 includes a misunderstanding of how the phpinfo() thing was used in system_requirements(), and it's related to why I excluded that change in the first place. Plus, HEAD already "clobbers" the message if you're on a version of PHP that is less than 5.5.21 or 5.6.5. So it's also sort of out of scope to change that hunk for that reason; the patch just slightly expands the scope of old versions for which the phpinfo() was "clobbered".

The purpose of the phpinfo() bit was to display a link to the person's phpinfo() in the section about their PHP version if it is enabled, as a description for supported PHP versions, a nicety for them to inspect their PHP configuration. So I would have just put this in an else for a fully supported PHP version after the warning, which is the same as just having it before.

And the message about it being disabled was just an apology for not displaying a link to it. But that's silly; we shouldn't apologize for something that's a more secure configuration, nor encourage people to turn it back on. That's all out of scope here, though, so I've filed #2927318: Status report shouldn't apologize for phpinfo() being disabled nor encourage people to enable it (edit: corrected the link) and restored it to its previous location, with a comment explaining why it's there (and not elsewhere) with a @todo.

Just a typo in the added comment.

#119 looks solid to me. There's no point recommending PHP < 7.1 given PHP 7.2 was just released today, and this has no effect outside of the status report.

1. +++ b/core/modules/simpletest/src/InstallerTestBase.php
   @@ -195,6 +198,23 @@ protected function setUpSettings() {
   +    if (version_compare(phpversion(), DRUPAL_RECOMMENDED_PHP) < 0) {
   

   It feels like we could have a static method for this somewhere, follow up of course. Probably makes sense after #2909472: Add value objects to represent the return of hook_requirements because we'd have some classes to put it on.

2. +++ b/core/modules/simpletest/src/InstallerTestBase.php
   @@ -218,4 +238,42 @@ protected function refreshVariables() {
   +      $selectors[] = 'h3#warning' . implode('', array_fill(0, $i + 1, '+details')) . ' summary';
   

   This feels very brittle. Can we make the requirements warnings output data-warning-id attributes or something reliable? Happy for that to be a follow-up + @todo.

3. +++ b/core/modules/system/src/Tests/Installer/DistributionProfileTranslationQueryTest.php
   @@ -64,6 +64,27 @@ protected function visitInstaller() {
   +  protected function setUpLanguage() {
   ...
   +  protected function setUpProfile() {
   
   @@ -80,21 +101,8 @@ protected function visitInstaller() {
   -  protected function setUpLanguage() {
   ...
   -  protected function setUpProfile() {
   
   +++ b/core/modules/system/src/Tests/Installer/DistributionProfileTranslationTest.php
   @@ -59,7 +59,27 @@ protected function visitInstaller() {
   +  protected function setUpLanguage() {
   ...
   +  protected function setUpProfile() {
   
   @@ -76,22 +96,10 @@ protected function visitInstaller() {
   -  protected function setUpLanguage() {
   ...
   -  protected function setUpProfile() {
   

   the diff here is weird. are we just reordering the methods? If so, out of scope?

4. +++ b/core/modules/system/system.install
   @@ -157,9 +157,16 @@ function system_requirements($phase) {
   +  // longer supported by Drupal, or that will drop support in the near future.
   

   nit: the 'or that will drop support in the near future' is a bit fragmented

5. +++ b/core/modules/system/system.install
   @@ -176,18 +185,21 @@ function system_requirements($phase) {
   -
   

   out of scope?

6. +++ b/core/modules/system/system.install
   @@ -176,18 +185,21 @@ function system_requirements($phase) {
   +  elseif ((version_compare($phpversion, DRUPAL_RECOMMENDED_PHP) < 0) && ($phase === 'install' || $phase === 'runtime')) {
   

   should just be if. The previous if returns early, so no need for elseif

7. +++ b/core/modules/system/system.install
   @@ -176,18 +185,21 @@ function system_requirements($phase) {
   +    $requirements['php']['description'] = t('Your PHP installation is running version %version. It is recommended to upgrade to version %recommended or higher to ensure your site remains secure and continues to receive updates. See <a href="http://php.net/supported-versions.php">PHP\'s version support documentation</a> and the <a href=":php_requirements">PHP requirements handbook page</a> for more information.', ['%version' => $phpversion, '%recommended' => DRUPAL_RECOMMENDED_PHP, ':php_requirements' => 'https://www.drupal.org/docs/8/system-requirements/php']);
   

   I'm happy with this statement, if we add a @todo and follow up to make it indicate when we intend to drop support i.e. after we reach consensus in #2842431: [policy] Remove PHP 5.5, 5.6 support in Drupal 8.7. At present it is ambiguous.

   Something like

   Your PHP installation is running version %version. It is recommended to upgrade to version %recommended or higher to ensure your site remains secure and continues to receive updates. Support for %version will be dropped in Drupal 8.7.0. Updating in advance will ensure your server is able to run future versions of Drupal 8.
   

Yeah #121.3 is a diff artifact due to how the test was (incorrectly) written before (see #102 and the reply #104 about it).

+1 to having a followup to making the message more specific and clear once we have the exact timeline; I was planning to do that anyway but an explicit @todo is better.

Updating to address #121.

1. Filed #2927339: Provide a static method for checking PHP versions.
2. Filed #2927345: Add data attributes to installation requirements screen for more reliable testing and added an @todo.
3. Not fixed because this is actually needed for tests to pass; as mentioned, it only appears that the methods are being "moved" because a different method is being split between methods that are (currently) before and after.
4. Tried to make the wording a little better.
5. Fixed.
6. Changed as suggested, although I had picked elseif on purpose despite the early return because I thought it made the code more explicit and it's the same number of code paths under each condition either way.
7. Filed #2927344: Specifically warn about end dates for PHP support for old versions and added an @todo.

1. I'm happy with this statement, if we add a @todo and follow up to make it indicate when we intend to drop support i.e. after we reach consensus in #2842431: [policy] Remove PHP 5.5, 5.6 support in Drupal 8.6. At present it is ambiguous.

   I think it's more than ambiguous; I literally read it as being about something completely different (PHP security support, not Drupal support). If this is supposed to be about Drupal, the wording should say that. Personally I'd just postpone this on the above issue entirely... but if not, I recommend that this patch say something more like the following:

   Your PHP installation is running version %version. Support for this version will be dropped in a future Drupal release. Although the timing of that change has not been announced yet, it is recommended to upgrade to version %recommended or higher in advance, to ensure your site remains secure and continues to receive Drupal updates. See the <a href=":php_requirements">PHP requirements handbook page</a> for more information.
   

2. I think linking the PHP release cycle support directly is also important, because otherwise the information is buried. I also don't want to retain a copy of it on a handbook page that won't be updated regularly. People should be aware of PHP's own support cycles and make decisions based on that. If they're on an Ubuntu LTS or whatever other distro that backports sec fixes, then that's up to them to determine.

   I wasn't suggested copying that info to a handbook page; I was suggesting linking to it on the handbook page. In order to know that there's even a possibility their PHP version is supported by someone else, most people probably need information telling them that - and the handbook page is where that can occur, and where the link to PHP's support cycle can be included in the context of that discussion.

3. Okay, so #113.1 includes a misunderstanding of how the phpinfo() thing was used in system_requirements()....
   The purpose of the phpinfo() bit was to display a link to the person's phpinfo() in the section about their PHP version if it is enabled, as a description for supported PHP versions, a nicety for them to inspect their PHP configuration. So I would have just put this in an else for a fully supported PHP version after the warning, which is the same as just having it before.

   Why wouldn't the message about a disabled phpinfo() also be relevant for someone running, say, PHP 7.0? (There may be another more important message to show them too, but that doesn't mean they shouldn't see both.) That said, #68 may not have been the right fix (it probably put the text in a non-ideal place on the screen) and I think you are right that there may be a better way to fix all this via #2927318: Status report shouldn't apologize for phpinfo() being disabled nor encourage people to enable it and perhaps it can just be fixed there.

4. +    // If the site is running a recommended PHP version and has the phpinfo()
   +    // function enabled, add a link to the phpinfo() to the description of the
   +    // PHP version.
   

   This comment is incorrect - the code adds the link even when the site is not running a recommended PHP version. I would just remove this comment entirely (I don't think it adds anything that isn't apparent from the code itself).

We could bump the recommended PHP version to 7.1.7 instead of 7.1.3

So while I agree about recommending 7.1 over 7.0, can we check for PHP < 7.1.0 and not show the point releases in the UI just literally encourage people to get on PHP 7.1? Most people aren't going to install a specific PHP 7.1 version, they'll just use whatever's in package management, and this will then save on updating it until we're ready to recommend PHP 7.2 (probably not sooner than the 8.6 or 8.7 cycle).

Thanks @catch and @David_Rothstein.

For #124.1, I was kind of struggling with the fact that I think we're not supposed to hardcode the word "Drupal" in the UI for distros or other sites that have a reason to not surface that the site is using Drupal. However, I checked just now and system_requirements() already has the word "Drupal" in a couple places, so I think you're right and we can make the sentence clearer.

I do still want to include the direct link to PHP's schedule page though so that people can go "Oh damn, PHP totally changed their release cycles from how it was for PHP 4 and 5". Basically, our handbook page is long and a lot of words, but the PHP release cycle page is short and has a picture. So I want people to look at both. But we can also add a short section to our handbook page about PHP's support vs. various Linux distros' support, etc.

This comment is incorrect - the code adds the link even when the site is not running a recommended PHP version. I would just remove this comment entirely (I don't think it adds anything that isn't apparent from the code itself).

Hm, but the link is overridden if it's not a recommended PHP version... but happy to remove the comment since it felt out of scope to me. I was just trying to address your concern about this bit. But the followup is suffiicent I think.

For #125, I do think we should include a minimum point release, because if they somehow are on or update to 7.1.2 when Symfony requires 7.1.3, and then have to update PHP again when we update Symfony, that's going to make a ton of extra work for them and make them really annoyed with us. And we might as well tell them to update to a more secure version. I can change it back to 7.1.3 if we're more worried about hosts on 7.1.4 that don't have those sec updates, but I do think it should be at least 7.1.3 specifically since this message is supposed to warn sites when their installations might not be future-compatible.

I'll take another stab at the warning message without avoiding the word "Drupal".

Okay yeah Symfony 3.4 (the LTS) is supported until 2021 and it's only Symfony 4 that requires 7.1.3. So we don't need to worry for a long time about their bump to 7.1.3 and I've updated the constant to just be 7.1 for now. Attached also tries to make the message clearer.

Reuploading the correct patch and interdiff.

Looks good to me!

Thank you @xjm! Is is till assigned to you because you have planned changes, or are we good to RTBC?

I guess this is what Tim meant :)

Didn't mean to leave assigned. I think this is good to go.

To address concerns about (e.g.) people on Ubuntu 16.04 who can't update to 18.04 because it's not released yet, I added this section to our PHP requirements handbook page that explains what "recommended" means:
https://www.drupal.org/docs/8/system-requirements/php#php_recommended

Ticking credit boxes for reviewers. Thank you!

Pushed this to 8.5.x and cherry picked to 8.4.x. Seems like good timing considering that PHP 7.0 dropped out of active support and into security-only support today.

My one reservation about this warning is that for users of Ubuntu 16.04 (PHP 7.0) who don't want to install 3rd party PPAs, the best thing for them to do is to ignore this new warning, wait until July when Ubuntu 18.04.1 is released, and upgrade to that. I think it's a shame for us to be flagging something as a warning when the best course of action for the person viewing the warning is to ignore it. That trains people to ignore warnings, which isn't good.

However, @xjm pointed out that the course of action isn't merely to wait till July, but to start making plans (evaluating whether to update to 18.04 or whether to use a 3rd party PPA to update the PHP build, getting organizational approval to take whatever action is best, scheduling the time in which to do that work, etc.), in which case seeing the warning now is potentially helpful. I think the text in https://www.drupal.org/docs/8/system-requirements/php#php_recommended addresses that.

If anyone disagrees with that reasoning though, please open a new issue, where we can discuss how to refine under what conditions to show the warning or refine the text of it. Thanks!

Thanks @effulgentsia.

Tagging to also explain this change in both the patch and minor release notes, since site owners might want more context about this warning when they update.

@larowlan, @webchick, and I discussed this today and decided to revert it for 8.4.x only since the warning is a bit alarming and should probably be vetted by the UX team. We might even want to fix how the installer looks... @webchick also pointed out that people might read "a future release" as "in one month or so" rather than "in one year or so" so it's probably better to do #2927344: Specifically warn about end dates for PHP support for old versions first for the patch release version of the warning.

One thing we discussed is warning on the status report only in 8.4.x, but on the installer (with specific dates) once those are set. We can probably have this back in 8.4.x after that.

Oh, forgot to mention, we also discussed making it a warning on the status report only in the next patch release, and then letting it upgrade to a warning for 8.5.0 with what's in 8.5.x HEAD currently (plus, hopefully by then, specific dates).

What's left here, cos this crit looks fixed in 8.5

We actually ended up with the followup here: #2927344: Specifically warn about end dates for PHP support for old versions.

Help welcome over there, thanks!