Problem/Motivation

#2917655: [9.4.x only] Drop official PHP 7.3 support in Drupal 9.4 discusses the PHP 7 version support policy we will adopt for Drupal 9. However, we have agreed that the minimum PHP version supported by Drupal 9 will be at least PHP 7.2.

Proposed resolution

  • Prepare a patch to bump the minimum install and update PHP versions to 7.2 (both Drupal constants and the Composer requirement).
  • Set the recommended version to PHP 7.3. (Note: Drupal 9.0 is scheduled to be released 2020 summer, at the time of this issue, 2019 October, however, you should still be very careful and through with testing before putting 7.3 in production.)

The patch can be created and tested against 8.8.x, but will be committed to 9.0.x as soon as it opens.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Drupal 9 now requires at least PHP 7.2.3 to be installed. The suggested PHP version is 7.3 though. This requirement may still be raised higher before the release of Drupal 9.0.0.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm created an issue. See original summary.

xjm’s picture

Issue summary: View changes
Ghost of Drupal Past’s picture

https://www.drupal.org/docs/9/how-is-drupal-9-made-and-what-is-included/... says

For these reasons, we are discussing requiring a minimum of PHP 7.2 for Drupal 9. This decision will depend on disruption estimations and PHP versions required by dependencies.

This issue, however, sounds like the decision is made. Perhaps that page should be updated, then?

NaheemSays’s picture

Security support for 7.2 ends on 30 November 2020.

If the supported version for Drupal 9.0 is only bumped to php-7.2, will the minimum version need to be bumped to php-7.3 for Drupal 9.1?

Ghost of Drupal Past’s picture

As I mentioned in another issue: PHP 7.3 has a behavior change in serialize https://bugs.php.net/bug.php?id=77302 and we have seen this change cause problems with core, contrib and Symfony. I think it's worth repeating this because there is a possibility of Drupal just erroring out when it tries to read previously serialized data. The unserialized data in 7.2 and prior might be bogus but it's possible the bogosity is only visible really on a Zend Engine level and not from userspace or it might be in an unused part of the unserialized data so this often wasn't a concern where the more correct but also more strict new behavior certainly is.

This is not to say I am against PHP 7.3 or that I dislike the change made there -- it has fixed half a dozen bugs outstanding on php.net for a long while so it's obviously a great change. I am merely raising visibility to a potential problem which I have deemed important enough to postpone Smartsheet upgrading to 7.3 for a while and I am commenting here because I am hoping the community will do the hard work of automated and manual testing of PHP 7.3 with large and diverse code- and databases.

catch’s picture

This issue, however, sounds like the decision is made. Perhaps that page should be updated, then?

The decision here is that as soon as 9.x opens, we'll bump the requirement to PHP 7.2.

It does not prevent us bumping the requirement to PHP 7.3 (or even 7.4) prior to June 2020 in another issue. What it hopefully does do though is allow us to require PHP 7.2 as early as humanly possible without having to make a decision about PHP 7.3 or PHP 7.4 first, before, which is why both issues are open.

shaal’s picture

Title: Bump Drupal's minimum PHP version to 7.2 as soon as 9.0.x is branched » Bump Drupal's minimum PHP version to >= 7.2 as soon as 9.0.x is branched

Updated the title of the issue per #6 + #d9readiness slack meeting

xjm’s picture

Title: Bump Drupal's minimum PHP version to >= 7.2 as soon as 9.0.x is branched » Bump Drupal's minimum PHP version to 7.2 as soon as 9.0.x is branched

I'm changing the title of this issue back. There may be a separate issue later in the Drupal 9 development process to further increase the version requirement, but this issue is for the must-have we will do prior to alpha1. Let's keep the broader topic on #2917655: [9.4.x only] Drop official PHP 7.3 support in Drupal 9.4. Thanks!

xjm’s picture

Title: Bump Drupal's minimum PHP version to 7.2 as soon as 9.0.x is branched » Bump Drupal's minimum PHP version to 7.2 as soon as 9.0.x is branched (a higher version may be required later)
Ghost of Drupal Past’s picture

Back to the original topic, I still believe

For these reasons, we are discussing requiring a minimum of PHP 7.2 for Drupal 9. This decision will depend on disruption estimations and PHP versions required by dependencies.

Should say

For these reasons, Drupal 9 will require a minimum of PHP 7.2 or potentially a higher version. This decision will depend on disruption estimations and PHP versions required by dependencies.

xjm’s picture

Issue summary: View changes
Status: Postponed » Active

We can and should make a patch for this now as 9.0.x will be branched about one week from now! The patch can set the required version to 7.2 and the recommended version to 7.3.

xjm’s picture

Issue tags: +Novice
xjm’s picture

Issue summary: View changes
xjm’s picture

I updated https://www.drupal.org/docs/9/how-drupal-9-is-made-and-what-is-included/... along the lines of @Charlie ChX Negyesi's suggestion.

xjm’s picture

Issue summary: View changes
shaal’s picture

Status: Active » Needs work
FileSize
30.24 KB

I was following #1498574: [policy, no patch] Require PHP 5.4 to make the changes, and added composer.lock updates.
Hopefully some parts of this patch are in the right direction... :)

shaal’s picture

Mixologic’s picture

+++ b/composer.lock
@@ -21,7 +21,7 @@
+                "php": ">=7.2.23",

This seems overly restrictive. Is this because there have been enough recent security releases to warrant this? I thought when we went with a version, we picked whatever distro's lowest normal shipping version was to pin it there.

In any case, the security argument would not be great, because 7.3.0 has several vulnerabilities that 7.2.23 does not, but would be acceptable with this constraint.

Perhaps >=7.2 would be adequate?

shaal’s picture

I updated it to >=7.2 instead of 7.2.23

Ghost of Drupal Past’s picture

  1. I do not see a composer.json change. What am I missing?
  2. Where can I read more about the recommendation for 7.3? Is there a compelling reason to do so now or could we postpone this? See #5 for reasons.
Gábor Hojtsy’s picture

@Charlie ChX Negyesi: if we expect 7.3 to cause problems we need to solve, is it better to start solving them as soon as the dev branch opens for Drupal 9 or postpone solving those problems to later? To me sounds like it could be better sooner?

hussainweb’s picture

Although I want to be able to use PHP 7.3 in code right now, I am also in favour of just running with PHP 7.2 for now. There are some cool features in 7.3 but not that valuable, IMHO, to worry about too much.

Also, to fix the test, please edit the file at core/assets/scaffold/files/htaccess as well.

And yes, I think the patch is missing the change to require PHP 7.2 or more.

Wim Leers’s picture

Status: Needs work » Needs review
Related issues: +#2455465: Add mod_php7 check to htaccess and remove php5 code
FileSize
1.71 KB
31.18 KB

Fixed test failure. But:

+++ b/.htaccess
@@ -27,18 +27,6 @@ AddEncoding gzip svgz
-# PHP 5, Apache 1 and 2.
-<IfModule mod_php5.c>
-  php_value assert.active                   0
-  php_flag session.auto_start               off
-  php_value mbstring.http_input             pass
-  php_value mbstring.http_output            pass
-  php_flag mbstring.encoding_translation    off
-  # PHP 5.6 has deprecated $HTTP_RAW_POST_DATA and produces warnings if this is
-  # not set.
-  php_value always_populate_raw_post_data   -1
-</IfModule>

Can we really just delete this? Shouldn't we convert this to PHP 7? Then again, it seems we've been doing fine without all this on PHP 7 for years at this point…

#2455465: Add mod_php7 check to htaccess and remove php5 code was trying to add mod_php7.c support.

This did not delete the comment just above it though:

# Most of the following PHP settings cannot be changed at runtime. See
# sites/default/default.settings.php and
# Drupal\Core\DrupalKernel::bootEnvironment() for settings that can be
# changed at runtime.

And if we go look there, we can see that

  php_value assert.active                   0

is definitely fine to remove. The session autostarting disabling we can do without too AFAICT. Then there's 3 multi-byte (unicode) string settings, which are also fine to remove AFAICT. And finally there's the odd PHP 5.6 raw POST data setting, whose need was already disproven a long time ago.

That would explain how we've been doing okay without a mod_php7.c section: none of it seems to be necessary anymore :) If so,
we can close #2455465: Add mod_php7 check to htaccess and remove php5 code too.

hussainweb’s picture

I would say it is fine to remove. Apart from all the reasons in #23, there is also the fact that this won't apply for FCGI / FPM setups anyway.

Ghost of Drupal Past’s picture

Sure, we can recommend 7.3 , I like Gábor's proposal... I guess D9 won't be in production anywhere for quite some time so we don't need to issue much warnings about the dangers of that. Perhaps by next summer we can even require it but that's a (very) different bag of hurt.

hussainweb’s picture

FileSize
126.27 KB

Another issue I thought about. What about the packages that were held back because of the older version of PHP and now can be updated because of PHP 7.2. For that, I ran composer update in a PHP 7.2.22 container and got this diff for composer files only.

Apart from the updates, most notably, the package ircmaxell/password-compat was removed. I don't know why we even have this as this is for forward compatibility for PHP >= 5.3.7. The functions already exist in PHP 5.5.

Wim Leers’s picture

Issue tags: +Needs followup

#26: I think we should not do that in this issue, that'll make this issue much harder to review. I agree it's an important follow-up though. I suspect an issue for this already exists?

catch’s picture

Yes this issue should just bump the minimum PHP, upgrading packages should also happen asap, just not here.

Not sure exactly which issue is the correct one, but should be a sub-issue of #3009213: [META] Update / reconsider PHP dependencies for Drupal 9.

hussainweb’s picture

Status: Needs review » Needs work

That's fair. Then I think we still need to fix the composer.json in the patch (there isn't one) and an updated composer.lock. I could be wrong but I don't see how we got the modified composer.lock as in the patch. Was it manually edited?

hussainweb’s picture

After travelling through the rabbit hole of that META issue, I didn't find an appropriate issue for this specific case. So I just created #3085456: Remove ircmaxell/password-compat. I think this got missed out because we tend to do changes piecemeal one package at a time. There are issues for updates for other packages.

xjm’s picture

Issue tags: -Needs followup

Correct, this issue is only for the PHP version. Many dependency packaces need special handling and will be reviewed on a case-by-case basis. This just unblocks that whole chain of issues (which already has many filed for specific dependencies, i.e. Symfony 4.4, Twig 2, etc.)

Thanks everyone for working on this so quickly!

xjm’s picture

Version: 9.x-dev » 9.0.x-dev

The 9.0.x branch will open for development soon, and the placeholder 9.x branch should no longer be used. Only issues that require a new major version should be filed against 9.0.x (for example, removing deprecated code or updating dependency major versions). New developments and disruptive changes that are allowed in a minor version should be filed against 8.9.x, and significant new features will be moved to 9.1.x at committer discretion. For more information see the Allowed changes during the Drupal 8 and 9 release cycles and the Drupal 9.0.0 release plan.

mikelutz’s picture

Status: Needs work » Needs review
Issue tags: -Novice
FileSize
6.52 KB

I'm not sure why this patch is changing the vendor php dependencies in the lock file, I don't think we need to touch those. We do need to update the dependency in our composer.json though.

No interdiff due to too many conflicts.

heddn’s picture

Status: Needs review » Needs work
+++ b/composer.lock
@@ -678,7 +678,7 @@
+                "php": ">=7.2",

Please base on 7.2.3 or above. See https://bugs.php.net/bug.php?id=75938 and #3053798: Checker: PHP version is lower than supported version. We've blacklisted 7.2.0-7.2.2 in automatic_updates for just this reason. I don't think using a slightly higher than 7.2.0 version is a hardship as there still are 20ish minor releases to pick from since 28 Feb 2018.

mikelutz’s picture

Status: Needs work » Needs review
FileSize
4.23 KB
6.54 KB

Sounds good to me.

mikelutz’s picture

I forgot about all the component composer.json files..

mglaman’s picture

With the bump, shouldn't we remove "paragonie/random_compat": "^1.0|^2.0|^9.99.99", from core/composer.json? it's around for 5.x polyfill.

Mixologic’s picture

@mglaman: #3058116: Remove paragonie/random_compat as a top-level dependency from 8.8.x removed it already, but is showing up because its a dep of a couple other deps.

➜  drupal_8 git:(8.9.x) ✗ composer why -t paragonie/random_compat   
paragonie/random_compat v9.99.99 PHP 5.x polyfill for random_bytes() and random_int() from PHP 7
└──symfony/polyfill-php70 v1.12.0 (requires paragonie/random_compat ~1.0|~2.0|~9.99)
   ├──symfony/http-foundation v3.4.32 (requires symfony/polyfill-php70 ~1.6)
   │  ├──asm89/stack-cors 1.2.0 (requires symfony/http-foundation ~2.7|~3.0|~4.0)
   │  │  └──drupal/core 8.9.x-dev (requires asm89/stack-cors ^1.1)
   │  │     └──drupal/drupal 8.9.x-dev (requires drupal/core self.version)
   │  ├──drupal/core 8.9.x-dev (requires symfony/http-foundation ~3.4.27)
   │  │  └──drupal/drupal 8.9.x-dev (requires drupal/core self.version)
   │  ├──stack/builder v1.0.5 (requires symfony/http-foundation ~2.1|~3.0|~4.0)
   │  │  └──drupal/core 8.9.x-dev (requires stack/builder ^1.0)
   │  │     └──drupal/drupal 8.9.x-dev (requires drupal/core self.version)
   │  ├──symfony/http-kernel v3.4.32 (requires symfony/http-foundation ~3.4.12|~4.0.12|^4.1.1)
   │  │  ├──asm89/stack-cors 1.2.0 (requires symfony/http-kernel ~2.7|~3.0|~4.0)
   │  │  │  └──drupal/core 8.9.x-dev (requires asm89/stack-cors ^1.1)
   │  │  │     └──drupal/drupal 8.9.x-dev (requires drupal/core self.version)
   │  │  ├──drupal/core 8.9.x-dev (requires symfony/http-kernel ~3.4.14)
   │  │  │  └──drupal/drupal 8.9.x-dev (requires drupal/core self.version)
   │  │  ├──stack/builder v1.0.5 (requires symfony/http-kernel ~2.1|~3.0|~4.0)
   │  │  │  └──drupal/core 8.9.x-dev (requires stack/builder ^1.0)
   │  │  │     └──drupal/drupal 8.9.x-dev (requires drupal/core self.version)
   │  │  └──symfony-cmf/routing 1.4.1 (requires symfony/http-kernel ^2.2|3.*)
   │  │     └──drupal/core 8.9.x-dev (requires symfony-cmf/routing ^1.4)
   │  │        └──drupal/drupal 8.9.x-dev (requires drupal/core self.version)
   │  └──symfony/psr-http-message-bridge v1.1.2 (requires symfony/http-foundation ^2.3.42 || ^3.4 || ^4.0)
   │     └──drupal/core 8.9.x-dev (requires symfony/psr-http-message-bridge ^1.1.2)
   │        └──drupal/drupal 8.9.x-dev (requires drupal/core self.version)
   └──symfony/lock v3.4.32 (requires symfony/polyfill-php70 ~1.0)
      └──drupal/drupal 8.9.x-dev (requires (for development) symfony/lock ~3.4.0)
mikelutz’s picture

I think (per #31) that this issue is specifically about changing the php version. There is a lot of dependency management upcoming around that and the SF4 switch, but as I understand it, this blocks everything else that needs to happen on the 9.0.x branch.

But correct me if I misunderstood.

mglaman’s picture

I totally forgot Symfony is including it because we're not on v4, ignore me!

mikelutz’s picture

but but but... if someone could RTBC THIS issue, then we could be. There's a patch for that.. :-p

catch’s picture

Status: Needs review » Reviewed & tested by the community

Patch is straightforward, let's unblock Symfony 4.

amateescu’s picture

+++ b/.htaccess
@@ -22,23 +22,6 @@ DirectoryIndex index.php index.html index.htm
-# PHP 5, Apache 1 and 2.

There's another issue for removing this PHP 5 stuff: #3075999: Remove php5 from htaccess writer

We should either not do it in this patch since it's not really related to updating the requirement from 7.0.8 to 7.2.3, or close the other issue as a duplicate :)

catch’s picture

Status: Reviewed & tested by the community » Needs work

Let's take it out of this patch, #3075999: Remove php5 from htaccess writer could still land in 8.8/8.9.

mikelutz’s picture

Status: Needs work » Needs review
FileSize
16.35 KB
2.01 KB

OKay

catch’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

alexpott’s picture

Updating issue credit for comments that have helped this issue get to rtbc.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 68a659f and pushed to 9.0.x. Thanks!

  • alexpott committed 68a659f on 9.0.x
    Issue #3079791 by mikelutz, shaal, Wim Leers, hussainweb, xjm, catch,...
Ghost of Drupal Past’s picture

Issue summary: View changes

I am not going to kvetch over the 7.3 recommendation for 9.0 that makes sense but I do feel the necessity to put a warning in the IS for people who would get carried away by the shiny :)

xjm’s picture

Status: Fixed » Needs work
Issue tags: +9.0.0 release notes, +Needs change record, +Needs release note

This definitely needs a CR and a release note. And to go in the release notes. We'll want to collate a whole specific picture of how 9.0.x is different from 8.9.x in ways that aren't simply straightforward Drupal API deprecations.

Gábor Hojtsy’s picture

Issue summary: View changes
Issue tags: -Needs release note

Added this release notes snippet:

Drupal 9 now requires at least PHP 7.2.3 to be installed. The suggested PHP version is 7.3 though. This requirement may still be raised higher before the release of Drupal 9.0.0.

I don't think there is more in the patch or in the future plans to say yet :)

Change record still needed.

Gábor Hojtsy’s picture

Status: Needs work » Fixed
Issue tags: -Needs change record

Added https://www.drupal.org/node/3089166 as a change record too.

Status: Fixed » Closed (fixed)

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