Specifically warn about end dates for PHP support for old versions

I think we have to get the specific dates into 8.5 so people know they have 12 months

HEAD (committed in #2670966: Warn users of old PHP versions)

This patch

The new error message is vastly better, thanks. My concern with the first version is it didn't place a timeline on "a future Drupal release," leading to widespread panic. :) Letting people know they've gotta do something by March 2019 will cut down on that significantly.

I think this is good to go - but one tiny question - should we also link to the release schedule on d.o - can link from the will be released on March 6 2019 for example?

#9 Sure! Which URL is that you want me to link to? (I scanned both this issue and #2842431: [policy] Remove PHP 5.5, 5.6 support in Drupal 8.7, didn't find which URL you meant.) I think perhaps we don't have that URL yet because https://docs.google.com/document/d/1kQG4E2L6Ye-M0RUuz7UkTZfyUPKoI-Vd2s8Y... hasn't been updated+posted?

I was thinking https://www.drupal.org/core/release-cycle-overview, but it only deals with the next cycle

The specific date introduced here is very welcome information.

@larowlan's question is a good one. Ideally we would be able to link to this information on d.o. (would be weird that the installer knows about this but d.o. doesn't mention it). Do we have a page that mentions dates for future releases?

I only have questions/concerns with the bits of text that are not changed so probably out of scope:

- First sentence is a repeat of the actual info this is the description for. Could be dropped without loss of meaning
- "…receive Drupal updates … remains secure" reads as if running a newer PHP automagically takes care of updates, security. More like: *can* receive updates … *able* to keep secure?)

#12: I'd be happy to make those string changes if that's what @webchick + @larowlan also prefer!

Ok. Lets leave out the link to info that isnt available yet. As is this is already solid.

The date is likely correct though. Should we remove the minor version number but keep the date?

Let's do that, good plan.

On it!

The date is likely correct though. Should we remove the minor version number but keep the date?

Done.

Interdiff isn't very helpful. This is what changed:

Before

Support for this version will be  dropped in Drupal 8.7.0, which will be released on March 6, 2019.

After

Support for this version will be  dropped on March 6, 2019.

Also, should we make the textual improvements proposed in #12?

I didn't see @catch +1'ing this, but given that both @yoroy and @xjm think this is a good idea, I think he wouldn't oppose it either. So, did that too, separately, so you can still easily go back to #19.

Interdiff isn't very helpful. This is what changed:

Before

Support for this version will be dropped on March 6, 2019.

+

[…] your site continues to receive Drupal updates and remains secure.

After

+

[…] your site can receive updates and is able to remain secure.

Updated "after" screenshot in IS.

I think "Drupal" needs to be retained in one of the first two sentences - removing that reintroduces some of the ambiguity that was in the earlier patches in #2670966: Warn users of old PHP versions (where it was unclear if it was talking about Drupal security support or PHP security support).

So I suggest changing the first sentence to:

• "Drupal will drop support for this version on March 6, 2019".

Otherwise looks good, but:

• '%version' => $phpversion can be removed if it's no longer used in the string
• "can receive updates and is able to remain secure" is repetitive (since "can" and "is able to" mean the exact same thing). Could just be "can receive updates and remain secure".

Addressed #22.

I did some rewriting (well, leaving suggestions, because I can't edit it) of https://docs.google.com/document/d/1kQG4E2L6Ye-M0RUuz7UkTZfyUPKoI-Vd2s8Y... as well. Notable:

• it said "March 7" but this issue says it's March 6. Changed to March 6.
• typo+HTML fixes
• I am concerned about The end date for Drupal 8's PHP 7.0 support isn't scheduled yet, but support will continue until at least March: Imagine I'm on PHP 7.0. Or have been planning to update to PHP 7.0. Then this sounds very scary: being on PHP 7.0 may not be enough!

Other than that, I think it looks fine.

@xjm and I pushed forward the document. Both the patch and that document need review, and hopefully can be RTBC'd.

Looks good to me

Do you mean change DRUPAL_RECOMMENDED_PHP to 7.1? I think we should do that regardless.

Do you mean change DRUPAL_RECOMMENDED_PHP to 7.1? I think we should do that regardless.

This is what it's set to in HEAD:

const DRUPAL_RECOMMENDED_PHP = '7.1';

This is also what causes you to see a warning during the installation process if you're on PHP 7.0.

This is why @xjm proposed to change it to:

const DRUPAL_RECOMMENDED_PHP = '7.0';

Hmm OK. Can we hardcode 7.0 instead of using DRUPAL_MINIMUM_PHP for the message with the dates? I think we'd want to keep 7.1 recommended.

Done.

I wasn't 100% sure what was meant with So we should have one message. My understanding is that we want to keep the "warning" message we've been having so far for sites that are on PHP 5.x, but want to change it to an "info" message in case the site has at least PHP 7.0, and show it during runtime only. That's what I implemented. If my interpretation was incorrect, I'll soon find out in the comments below :)

IS updated to reflect both possible states in the status report now (PHP 5.x vs PHP 7.0).

Further expanding the screenshots in the IS to clarify the changes.

BTW, @xjm published the PHP 5.5/5.6 EOL announcement: https://groups.drupal.org/node/518200. It was also widely tweeted: https://twitter.com/drupal/status/955699298615922693. Finally, the CR for this issue was published: https://www.drupal.org/node/2938726.

1. +++ b/core/modules/simpletest/src/InstallerTestBase.php
   @@ -210,7 +210,7 @@ protected function setUpRequirementsProblem() {
   +    if (version_compare(phpversion(), '7.0') < 0) {
   

   You used a string.

2. +++ b/core/modules/system/system.install
   @@ -189,12 +189,19 @@ function system_requirements($phase) {
   +    if (version_compare($phpversion, 7.0) < 0) {
   

   I used a float.

Both work, but we should probably make this consistent.

Now that https://groups.drupal.org/node/518200 is posted, shouldn't the message on the status report link to that (either directly, or at least indirectly via a link on https://www.drupal.org/docs/8/system-requirements/drupal-8-php-requirements)?

It would also probably be a good idea to have that (or something like it) as a front page post on the main drupal.org site, eventually.

Checked over both the proposed text and the handbook page changes; both look good to me. I made one small tweak to the language around PHP 7.2 support in 8.5.0 to make it more clear that the issue has been resolved in 8.5.0 (or at least #2923015: [PHP 7.2] Incompatible method declarations is Closed (Fixed)) vs. before it sounded more like we were kinda "targeting" support for that release. https://www.drupal.org/node/2891690/revisions/view/10813778/10818984

The PHP 5 version of the error is very easy to trigger from SimplyTest.me; verified that it displays the warning upon install:

I can't trigger the PHP 7.0 one as easily to test it, but in reading it, it seems fine.

Marking RTBC. Thanks a lot for the hard work on this, all!

Is this still needs work for #46? Not sure if #47(xjm) is saying yes to #46 or that it should go in handbook instead

Adding review credits for those who shaped the patch

Committed as 05b9f0a and pushed to 8.6.x.

Cherry-picked as d85dada and pushed to 8.5.x.

Thanks all

Follow up - this removes the duplicate "If" in an inline comment that was added by the commit in #54/#56:

-    // Warn if still on PHP 5. If If at least PHP 7.0, relax from "warning" to
+    // Warn if still on PHP 5. If at least PHP 7.0, relax from "warning" to

@TR nice catch, but please open up a new issue for that so that credit attribution works correctly. Of course feel free to post a link to the follow up here so people that worked on this can help out.

No, I'm not going to open a new issue - that will just get ignored by the people who made this mistake, and then the new issue will stay open for many years until the code gets rewritten and the patch is obsolete. (Think I'm exaggerating? I can give you a dozen examples ...) I don't give a crap about credit. Everyone makes mistakes, and the simplest thing to do is to fix it right here right now while it's still familiar to the participants.

For quick non-controversial followups I think it's fine to do it in the same issue rather than jumping through all the hoops to create a new one; I think that's one of the reasons "Fixed" issues can still be reopened (https://www.drupal.org/issue-queue/status#fixed).

The followup patch is fine; the rest of the original commit (and the documentation changes) looked good to me on a quick skim-through also.

Committed as 9b4915d and pushed to 8.6.x

Cherry-picked as 9117bdb and pushed to 8.5.x.

For quick non-controversial followups I think it's fine to do it in the same issue rather than jumping through all the hoops to create a new one

Agree

No, I'm not going to open a new issue - that will just get ignored by the people who made this mistake, and then the new issue will stay open for many years until the code gets rewritten and the patch is obsolete. (Think I'm exaggerating? I can give you a dozen examples ...) I don't give a crap about credit.

The tone here is borderline