-
Notifications
You must be signed in to change notification settings - Fork 3.1k
#62622 Bump minimum supported PHP version to 7.4 #9181
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: trunk
Are you sure you want to change the base?
Conversation
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a couple of spots where we should document changes, otherwise LGTM
Co-authored-by: Aaron Jorbin <aaronjorbin@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Can't wait to get rid of that local environment code working around available MySQL authentication plugins.
I only have one thought to consider around which point version of 7.4 to actually use.
| * @global string $required_php_version | ||
| */ | ||
| $required_php_version = '7.2.24'; | ||
| $required_php_version = '7.4'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a specific bug fix version of 7.4 that we'd like to make a requirement? It seems that 7.2.24 was used based on usage numbers.
I've created a similar spreadsheet to the one used for the previous bump. Some observations:
- The "5% or less" criteria is met using
7.4.27. - 7.4.x only went up to 7.4.33 so that may not be the best.
7.4.3has 1.23% for some reason.- The only other versions with greater than 1% are
7.4.33and7.4.30. - No other versions have more than a half percent (0.5%) usage.
I don't know that we'll find a point release with a specific change that we want required, and the last 7.4.x release included security fixes, so technically all others are potentially insecure.
There's a few possible landing spots:
7.4.22would mean just over 1% of all WordPress sites would be dropped.7.4.16would mean just 3% of sites using 7.4.x would be dropped.7.4.27would mean less than 5% of PHP sites using PHP 7.4.x would be dropped.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a practical benefit to using a point release as the minimum instead of 7.4.0?
When the minimum was bumped to 5.6 in Core-46594 we ended up on 5.6.20 because of the small fractional difference between that and 5.6.0. When it was bumped to 7.0 in Core-57345 we didn't bother looking at usage for point releases and just went with 7.0.
My preference would be to not introduce an unnecessary point release restriction if it means sites that are on an older point release of 7.4 aren't able to update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only benefit I can think of is that we ensure certain bug fix are present or specific vulnerabilities are not.
However, I don't know that there is anything specific we want to target in this case. I don't feel strongly about including a particular point release, but just wanted to discuss it because we have used one in the past.
# Conflicts: # .github/workflows/local-docker-environment.yml # .github/workflows/phpunit-tests.yml # .github/workflows/upgrade-testing.yml # src/wp-includes/compat.php
Trac ticket: https://core.trac.wordpress.org/ticket/62622