Drop compatibility with PHP 7.2 and 7.3#22077
Conversation
Pull Request Test Coverage Report for Build d6112f48a78380ef0e30c3424c33b8a053eaa052Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
|
A merge conflict has been detected for the proposed code changes in this PR. Please resolve the conflict by either rebasing the PR or merging in changes from the base branch. |
jrfnl
left a comment
There was a problem hiding this comment.
Hi @enricobattocchi, as requested, I've reviewed this PR. Please find my review notes below.
Also needs/prerequisite ⚠️
- PR to YoastCS to update the
testVersionused by PHPCompatibility as otherwise any and all modernizations would be blocked. - Release of a new version of YoastCS containing the above PR (please co-ordinate with me as there are other changes lined up for YoastCS).
Missed references which should be updated ⚠️
composer.json: drop dependency onyoast/whipVERSIONS.json- line 16 -7.2=>7.4. (or is this updated automatically ?)
Other updates which need to be made ❗
.github/workflows/lint.yml- this workflow needs changing as the second PHP install is no longer needed (but don't just blindly remove it, certain steps need to be merged).github/workflows/test.yml- this workflow needs changing in both jobs as the second PHP install is no longer needed (but don't just blindly remove it, certain steps need to be merged)..github/workflows/finish-coveralls.yml- line 108 - the names of the builds which should potentially be carried forward need to be brought in line with the changes made in thetest.ymlworkflow.
Composer updates which can/should be made
In this PR/directly related to the version drop ✅
none
The following updates need to be made in separate PRs in a managed fashion as code changes may (will) be needed in the code using these dependencies
symfony/config 3.4.32 5.4.46 Symfony Config Component
symfony/dependency-injection 3.4.47 5.4.48 Symfony DependencyInjection Component
☝️ IIRC, this one is on @thijsoo's radar.
humbug/php-scoper 0.13.8 0.17.5 Prefixes all PHP namespaces in a file or directory.
psr/container 1.0.0 1.1.2 or 2.0.2 Common Container Interface (PHP FIG PSR-11)
The following updates can/should be made aside from this PR (unrelated to the version drop)
guzzlehttp/guzzle 7.8.1 7.9.2 Guzzle is a PHP HTTP client library
humbug/php-scoper 0.13.8 0.13.9 Prefixes all PHP namespaces in a file or directory.
league/oauth2-client 2.7.0 2.8.1 OAuth 2.0 Client Library
psr/container 1.0.0 1.1.0 Common Container Interface (PHP FIG PSR-11)
Automated tests run on PHP 7.4.0
Needed some digging in dusty archives, but I managed to find an old PHP 7.4.0 archive, installed it and I have run the tests against that version to verify that the absolute minimum supported PHP version (according to this PR) works (based on the available tests).
- Unit Tests: ✅
- WP Tests: ✅
The caveat is, of course, that test coverage is low, so this is no guarantee, only a tentative indication.
Follow up which is needed (outside the scope of this PR):
- YoastCS: decide if and if so, which new CS rules we want to start enforcing now support for PHP < 7.4 will be dropped.
- I think the PHP/WP matrix should probably get a second look, but that doesn't need to be done in this PR. Might even be better to combine this with the PR which adds PHP 8.4 to the matrix (once we can).
Hope this helps.
|
FYI: I also still found some bits and pieces of code which can be removed now. I've made some local commits for this now. These can either be added to this PR or pulled separately once this PR has been merged. |
This may need a little investigation, as I'm getting a tingly feeling that running the dependency injection + scoping on recent PHP versions may fail due to the outdated versions of the dependencies being used, so we may not be able to get rid of the extra steps yet, but in that case, we need to update the comments in the script to explain why the steps are still needed. |
885d4d0 to
8da99ec
Compare
composer.lockPackage changes
Settings · Docs · Powered by Private Packagist |
|
A merge conflict has been detected for the proposed code changes in this PR. Please resolve the conflict by either rebasing the PR or merging in changes from the base branch. |
8da99ec to
1e71cff
Compare
|
A merge conflict has been detected for the proposed code changes in this PR. Please resolve the conflict by either rebasing the PR or merging in changes from the base branch. |
776cc72 to
a8f33c0
Compare
|
A merge conflict has been detected for the proposed code changes in this PR. Please resolve the conflict by either rebasing the PR or merging in changes from the base branch. |
36ce5cd to
9091adb
Compare
|
The composer.lock diff comment has been updated to reflect new changes in this PR. |
9091adb to
56db040
Compare
|
The composer.lock diff comment has been updated to reflect new changes in this PR. |
Context
Summary
This PR can be summarized in the following changelog entry:
Relevant technical choices:
Test instructions
Test instructions for the acceptance test before the PR gets merged
This PR can be acceptance tested by following these steps:
note: the above may be tricky to test. I can see 7.3 as a choice in Local by Flywheel, but I know it's not the case for others (and I can see 7.2 is not available)

I tried on Instawp, where you have the full choice of PHP versions, but when uploading the artifact I got:
So a way could be:
Relevant test scenarios
Test instructions for QA when the code is in the RC
QA can test this PR by following these steps:
Impact check
This PR affects the following parts of the plugin, which may require extra testing:
UI changes
Other environments
[shopify-seo], added test instructions for Shopify and attached theShopifylabel to this PR.Documentation
Quality assurance
Innovation
innovationlabel.Fixes #