Upgrade PHPCS #164
Merged
Upgrade PHPCS #164
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
matt-bernhardt
force-pushed
the
fix-phpcs
branch
2 times, most recently
from
a0fc61e to
50f0451
Compare
** Why are these changes being introduced: * We are currently using WPCS release 2.3, but the latest version is now 3.x. A number of supporting packages are implicated in this upgrade. ** Relevant ticket(s): n/a (maintenance work) ** How does this address that need: This upgrades our version of the WP coding standards to the latest release, and rebuilds the tooling and integration: * Changes the PHPCS application itself to be a plugin in this repo, rather than a dev dependency package (per their documentation). * Renames the `composer test` script to be `composer lint`, which is more in line with what this tool does. * Updates the phpcs.security.xml scoping configuration, removing a reference to the deprecated WordPress.Classes family of checks, and adding some new Modernize and Universal checks to the removal list. The purpose of this security configuration is to run _only_ the security checks, requiring no failures in order for a PR to merge. General-purpose linting, using all checks, still happens using the more generic phpcs.xml configuration as part of `composer lint`. ** Document any side effects to this change: * None
We are using PHP 8.1, but run CI on future versions of PHP as a way of protecting our upgrade pathway. In this sense, it makes sense to check all future versions of PHP, not just the next version.
The contents were already ignored, but removes the directory itself.
matt-bernhardt
force-pushed
the
fix-phpcs
branch
from
50f0451 to
cb6c912
Compare
12 tasks
djanelle-mit
approved these changes
Oct 16, 2024
There was a problem hiding this comment.
This work looks good to me!
I was able to replicate the desired output of composer lint on my local environment, and the multidev for this branch works as I'd expect. I can't think of other places for this particular example that I'd be concerned about (with my limited experience).
Should be good to merge.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The key change here is upgrading the version of the WordPress coding standards that we apply to the application. A lot changed when these standards hit the 3.x release, including support for PHP 8.x - so gettting everything in place caused follow-on changes to a handful of files.
As a side effect of this work, I've added PHP 8.3 to our list of tested language versions. It isn't clear to me yet whether we'll upgrade straight to 8.3 or not, but since it's ready we might as well check with it. When PHP 8.4 is out, we'll add that too.
To confirm these changes:
composer installto get the new tooling in place locallycomposer security, which should produce no errorscomposer lint, which should take a few minutes and flag a fair number of known issues (sadly). The key outcome is that you see flagged code, not a failure of the tool to run.While the changes here won't affect how WordPress runs, I've set up a multidev on this branch to confirm that none of these changes will impact Pantheon's build workflow. You can confirm this multidev exists in the Pantheon dashboard (I don't like to link to multidevs since these pages get crawled)
Credit to @JPrevost for writing the original documentation page I'm including here. I tweaked the wording a bit and added some links, but he wrote this a year or so ago in a branch that never ended up merging.
Developer
Stylesheets
string incremented.
Secrets
Documentation
Accessibility
our guide and
all issues introduced by these changes have been resolved or opened as new
issues (link to those issues in the Pull Request details above)
Stakeholder approval
Dependencies
YES dependencies are updated (see composer.json and composer.lock)
Code Reviewer
(not just this pull request message)