-
Notifications
You must be signed in to change notification settings - Fork 805
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
Codesniffer: remove PHP 7.0 and 7.1 support #40174
Conversation
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available. Follow this PR Review Process:
Still unsure? Reach out in #jetpack-developers for guidance! |
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.
Sweet exorcism! Also alphabetical ordering FTW 👍🏽
@@ -25,9 +25,6 @@ | |||
<rule ref="MediaWiki.WhiteSpace.EmptyLinesBetweenUse" /> | |||
<rule ref="MediaWiki.WhiteSpace.MultipleEmptyLines" /> | |||
<rule ref="MediaWiki.Usage.IsNull" /> | |||
<!-- Maybe in the future |
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.
Maybe in another universe :)
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.
In this case, it's redundant to another rule. :^)
These are auto-generated files. That said, I wonder why the order is different...I suspect the default on my machine vs. Brad's is different. 🤔 Edit: I tried running it after using |
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.
can you confirm the generated files on your end result in no changes in the files that are committed in this branch?
Adding LC_ALL=C.UTF-8
at the top of the script makes it consistent with what you have here for me. So let's do that to make it consistent.
@@ -9,7 +9,7 @@ DIR="$(mktemp -d "${TMPDIR%/}/codesniffer-build-compat-rulesets.XXXXXXXX")" | |||
trap 'rm -rf "$DIR"' EXIT | |||
cd "$DIR" | |||
|
|||
PHP_VERSIONS=( 7.0 7.1 7.2 7.3 7.4 8.0 8.1 8.2 8.3 ) | |||
PHP_VERSIONS=( 7.2 7.3 7.4 8.0 8.1 8.2 8.3 ) |
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.
Should we go ahead and add 8.4 right away, or save that for a followup?
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.
I was figuring a separate PR, but no strong feelings.
It's not PHPUnit that needs updating, it's phpcompatibility/php-compatibility. I'm not sure if PHP 8.0 checks still aren't finished or if the v10 release is now waiting on 8.4 checks, but either way there's no release to update to. 🙁 https://xkcd.com/2347/ |
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.
Let's do it.
As the title says, it removes PHP 7.0 and 7.1 support, and updates the tests accordingly.
We should add PHP 8+ tests, but that will probably be after updating to PHPUnit 10+.
Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
Testing instructions:
Verify CI is happy, or run tests manually:
composer test-php