-
Notifications
You must be signed in to change notification settings - Fork 99
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
Bump minimum PHP requirement to 7.2 #1130
Conversation
95d94b5
to
1cc4789
Compare
build/cs
build-cs/composer.json
in root composer.json
This comment was marked as resolved.
This comment was marked as resolved.
@swissspidy Can you share your thoughts here? Also wanted to know if we are good to keep this or need to wait till 6.6 comes out. |
Let's wait until 6.6 is released. After that we can bump our minimum requirement too. Performance Lab & co. still support WordPress 6.4, FWIW. But I think it's OK to bump to PHP 7.2 regardless. Also, if you bump the PHP requirement in Composer you must also update the |
@swissspidy Meaning, bump to PHP 7.2 when WP 6.6 is released. At this time WP 6.5 will be the minimum supported version which still supports PHP 7.0, but that is fine? IMO, I'd be fine with bumping to 7.2 now even though we still support WP 6.4. Since core trunk now requires PHP 7.2, if we follow suit now we can more closely follow core in development of feature plugins. For example, there may be code in trunk we copy into our plugins and vice-versa. If we're on the same PHP version now, that will be easier. And if we force users to prematurely upgrade to PHP 7.2, then I don't think this is a bad thing at all. |
That‘s what I meant, yes. I don‘t have a strong opinion though. Core won‘t change to 7.2+ specific code right away, so code copying is probably not an issue. But if you think it‘s easier this way, we can also bump now |
I think it's better for us to err on the side of being more current than on the side of holding back, especially since it reduces our maintenance burden and improves tool compatibility. And since core is adopting 7.2 anyway and our codebase is feature plugins intended for core merge, being on whatever the next PHP version is for core makes sense to me. The only downside is not getting potential testers from PHP 7.0 and PHP 7.1. But their usage is only 1.5% and 0.9% respectively, so that seems like a non-issue. |
build-cs/composer.json
in root composer.json
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
I missed this comment. Yes and no, WordPress provides polyfilles for those. |
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Let's wait to merge this until after the 3.0.0 release (in an hour or so) because there will be a merge conflict in |
OK, merge conflicts ready to be resolved 😬 |
Co-authored-by: Pascal Birchler <pascalb@google.com>
33d0ac8
to
61b3f75
Compare
Done. |
"allow-plugins": { | ||
"dealerdirect/phpcodesniffer-composer-installer": true, | ||
"phpstan/extension-installer": true |
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 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.
composer/installers contains a Composer plugin which is currently not in your allow-plugins config. See https://getcomposer.org/allow-plugins
Do you trust "composer/installers" to execute code and wish to enable it now? (writes "allow-plugins" to composer.json) [y,n,d,?] ?
y - add package to allow-plugins in composer.json and let it run immediately
n - add package (as disallowed) to allow-plugins in composer.json to suppress further prompts
d - discard this, do not change composer.json and do not allow the plugin to run
? - print help
Do you trust "composer/installers" to execute code and wish to enable it now? (writes "allow-plugins" to composer.json) [y,n,d,?] d
I went with d
. I'm not seeing it happen now when I do composer install
. 😕
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.
This might be due to stale vendor content. I don't see if we are using this plugin so I removed it.
Summary
https://core.trac.wordpress.org/ticket/58719 is recently merged in Core which bumps the minimum PHP to 7.2. Now that we can bump to 7.2, all composer packages required by this repo are PHP 7.2+ compatible and we can safely merge root
composer.json
andbuild-cs/composer.json
.Fixes #1129
Relevant technical choices
build-cs/composer.json
in rootcomposer.json
.