Opened 14 months ago
Closed 12 months ago
#59161 closed defect (bug) (fixed)
Composer: update to WordPressCS 3.0.0
Reported by: | jrf | Owned by: | SergeyBiryukov |
---|---|---|---|
Milestone: | 6.4 | Priority: | normal |
Severity: | normal | Version: | 6.4 |
Component: | Build/Test Tools | Keywords: | has-patch |
Focuses: | coding-standards | Cc: |
Description (last modified by )
WordPressCS 3.0.0 has just been released. 🎉
This is an important release which makes significant changes to improve the accuracy, performance, stability and maintainability of all sniffs, as well as making WordPressCS much better at handling modern PHP.
I'd recommend for WordPress Core to upgrade to using WordPressCS 3.0.0 at the earliest convenience to benefit from the latest sniff goodies and to prevent new code issues entering the codebase.
I have a patch ready to go for this, which I will pull in a moment.
The patch consists of a number of commits to make it easier to review the changes.
There are likely some changes included which touch Gutenberg files, so a check will need to be done to see what needs to be pulled to the Gutenberg project.
This patch fixes everything but one issue: violations being flagged against the "one object declaration per file" rule.
As fixing those is more involved, those should be handled separately.
To that end, I'll be downgrading that error to a warning for the time being to allow the build to pass.
The patch also includes a full review of all PHPCS "ignore annotations" used in the WP Core codebase and removes a significant number of them.
Again: there may be some parts of these changes which need to be pulled to the Gutenberg repo.
Once the patch has been landed in WP Core, contributors/core committers will need to run composer update --with dependencies
on their local machine to pull in the latest versions of the CS dependencies.
This is also mentioned in the Make post.
Refs:
- https://github.com/WordPress/WordPress-Coding-Standards/releases/tag/3.0.0
- https://make.wordpress.org/core/2023/08/21/wordpresscs-3-0-0-is-now-available/
### Impact
This is the CS status of WP Core trunk
with the current CS dependencies:
PHP CODE SNIFFER VIOLATION SOURCE SUMMARY ----------------------------------------------------------------------------- SOURCE COUNT ----------------------------------------------------------------------------- [ ] WordPress.PHP.StrictComparisons.LooseComparison 339 [ ] WordPress.DB.PreparedSQL.InterpolatedNotPrepared 145 [ ] WordPress.DB.PreparedSQL.NotPrepared 103 [ ] WordPress.NamingConventions.ValidVariableName.VariableNotSnakeCase 39 [ ] WordPress.Files.FileName.InvalidClassFileName 9 [x] Generic.Formatting.MultipleStatementAlignment.NotSameWarning 6 ----------------------------------------------------------------------------- A TOTAL OF 641 SNIFF VIOLATIONS WERE FOUND IN 6 SOURCES ----------------------------------------------------------------------------- PHPCBF CAN FIX THE 1 MARKED SOURCES AUTOMATICALLY (6 VIOLATIONS IN TOTAL) -----------------------------------------------------------------------------
This will be the CS status of WP Core on WordPressCS 3.0.0 with the patch applied:
PHP CODE SNIFFER VIOLATION SOURCE SUMMARY ----------------------------------------------------------------------------- SOURCE COUNT ----------------------------------------------------------------------------- [x] Universal.Operators.StrictComparisons.LooseEqual 233 [ ] WordPress.DB.PreparedSQL.InterpolatedNotPrepared 145 [x] Universal.Operators.StrictComparisons.LooseNotEqual 106 [ ] WordPress.DB.PreparedSQL.NotPrepared 103 [ ] WordPress.NamingConventions.ValidVariableName.VariableNotSnakeCase 39 [ ] Generic.Files.OneObjectStructurePerFile.MultipleFound 29 [ ] WordPress.Files.FileName.InvalidClassFileName 9 ----------------------------------------------------------------------------- A TOTAL OF 664 SNIFF VIOLATIONS WERE FOUND IN 7 SOURCES ----------------------------------------------------------------------------- PHPCBF CAN FIX THE 2 MARKED SOURCES AUTOMATICALLY (339 VIOLATIONS IN TOTAL) -----------------------------------------------------------------------------
Attachments (1)
Change History (50)
This ticket was mentioned in PR #5047 on WordPress/wordpress-develop by @jrf.
14 months ago
#1
- Keywords has-patch added
This ticket was mentioned in Slack in #core-coding-standards by garyj. View the logs.
14 months ago
#3
@
14 months ago
I know that it would result in a broken build for one commit, but I wonder if this should go in in two commits. The first would be for the update to composer.json
and phpcs.xml.dist
and the second the rest of the changes. This way we could also update https://github.com/WordPress/wordpress-develop/blob/trunk/.git-blame-ignore-revs to ignore the coding standard update without ignoring the update we may want to easily trace back.
#5
@
14 months ago
@jorbin Makes sense to me 👍🏻, but I'll leave that up to whomever will commit it eventually.
Most of the actual fix commits could be committed first and then the Composer commit _after_, that way there won't be a broken build in between.
#7
@
14 months ago
Should the version of PHP used within the GitHub Actions workflows also be updated to use PHP 8.x now?
#8
follow-up:
↓ 9
@
14 months ago
@desrosj Did I forget to do that ? Oops... It doesn't have to be (as sniffs are supposed to give the same results on all supported PHP versions), but it definitely can be. If so, I suggest setting it to 'latest'
to allow setup-php
to auto-update the version (and yes, WPCS 3.0.0 works fine on PHP 8.3, so we should be good for at least a year and a half).
#9
in reply to:
↑ 8
;
follow-up:
↓ 10
@
14 months ago
Replying to jrf:
@desrosj Did I forget to do that ? Oops... It doesn't have to be (as sniffs are supposed to give the same results on all supported PHP versions), but it definitely can be. If so, I suggest setting it to
'latest'
to allowsetup-php
to auto-update the version (and yes, WPCS 3.0.0 works fine on PHP 8.3, so we should be good for at least a year and a half).
@desrosj Looking at the workflow now, but if I look at the branches
/tags
settings, I get the impression that workflow is also used with older WP (backport) releases. If so, then no, we should not change the PHP version in the workflow as older WP versions will not get the update to WPCS 3.0.0, so those may still run into PHP 8.x issues when using WPCS 2.3.0, which would break builds.
Does that make sense ?
#10
in reply to:
↑ 9
;
follow-up:
↓ 11
@
14 months ago
Replying to jrf:
@desrosj Looking at the workflow now, but if I look at the
branches
/tags
settings, I get the impression that workflow is also used with older WP (backport) releases.
It's true that it runs in older branches, but the workflows run using the source code for the workflow within that branch (unless trunk
is specifically targeted, like how the Slack workflow is called as an example). So as long as the version update is not backported to older branches, they should continue to work without issue.
latest
sounds great to me! 👍
#11
in reply to:
↑ 10
@
14 months ago
Replying to desrosj:
latest
sounds great to me! 👍
I've added a new commit to the PR which does just that.
#12
@
13 months ago
Anything I can do to move this ticket forward ? The patch will get out of date quickly if/when new issues against WPCS 3.0 get introduced into the codebase (and is now already in conflict state).
13 months ago
#13
FYI: I've recreated the PR to get it out of conflict state and to get rid of newly introduced issues. Includes one new commit (annotated in the PR description).
Would be lovely if we could get some movement on this PR to prevent having to keep recreating the PR.....
13 months ago
#23
Rebased the PR to allow for seeing the current status after a number of commits have gone in (thanks @SergeyBiryukov !).
Includes one new commit to address newly introduced issues.
#26
@
13 months ago
FYI: WordPressCS 3.0.1 has been released in the mean time, but the change included is not relevant for WP Core.
Ref: https://github.com/WordPress/WordPress-Coding-Standards/releases/tag/3.0.1
13 months ago
#28
Rebased the PR to allow for seeing the current status after a number of commits have gone in (thanks @SergeyBiryukov !).
Includes three (!) new commits to address newly introduced issues.
This ticket was mentioned in Slack in #core-committers by jrf. View the logs.
13 months ago
12 months ago
#40
@SergeyBiryukov Looks like some merge artifact crept in with that merge ? Want me to rebase instead ?
@SergeyBiryukov commented on PR #5047:
12 months ago
#41
Looks like some merge artifact crept in with that merge ?
Yeah, I tried to rebase to see what's left and move those changes to Gutenberg, but something went wrong, and I reverted it.
Want me to rebase instead ?
That would be great 🙂
@SergeyBiryukov commented on PR #5047:
12 months ago
#46
Rebased again just so we can see what's left.
@jrfnl Thanks! I have opened https://github.com/WordPress/gutenberg/pull/55615 to handle the change in wp-includes/blocks/calendar.php
.
For the change in wp-includes/class-wp-block-parser-block.php
, however, it appears that it cannot be merged as-is upstream, since Gutenberg does not have a corresponding section in phpcs.xml.dist
.
I think we can instead remove the section from WP Core's phpcs.xml.dist
file, as it does not seem necessary with the affected lines already ignored in the file itself. See 59161.wp-block-parser-block.diff. What do you think?
@SergeyBiryukov commented on PR #5047:
12 months ago
#48
I think we can instead remove the section from WP Core's
phpcs.xml.dist
file, as it does not seem necessary with the affected lines already ignored in the file itself. See 59161.wp-block-parser-block.diff.
Done in r57017. That was the last of the changes here, so I believe this PR is now fully addressed.
### CS: align equal signs for consecutive statements (pre-existing)
### PHPCS: improve organisation of the PHPCompatibility ruleset
No functional changes.
This commit:
### PHPCS: remove unnecessary directives
This commit:
node_modules
and thevendor
directory.### Composer: update CS dependencies
WordPressCS 3.0.0 has been released.
This commit updates the Composer dependencies to use the new version, including updating the underlying PHP_CodeSniffer dependency to the new minimum supported version for WPCS.
Note: the Composer PHPCS installer plugin is no longer explicitly required as it is now a dependency of WordPressCS, so we inherit the dependency automatically.
### PHPCS: improve organisation of the WordPressCS based PHPCS ruleset
No functional changes.
This commit:
### PHPCS: update the ruleset for WordPressCS 3.0.0
This commit:
WordPress-Core
ruleset..WordPress.CodeAnalysis.AssignmentInCondition
sniff has been (partially) replaced by theGeneric.CodeAnalysis.AssignmentInCondition
sniff.### PHPCS: remove the custom property for the FileName sniff
Test files should comply with the naming conventions accepted by PHPUnit, not with the WP naming conventions.
While the
WordPress.File.FileName
sniff makes a best effort to prevent throwing errors for files containing test classes, the recommended manner to deal with test files is to exclude the complete test directory from being subject to this sniff.This updates the ruleset to follow the recommendation.
### PHPCS: downgrade one new error to a warning
The
Generic.Files.OneObjectStructurePerFile
sniff enforces that there is only one OO structure declaration per file.At this time, this sniff would yield 29 errors.
By downgrading the sniff to a _warning_, the build can pass and we buy ourselves time to fix these 29 issues.
### CS: update ignore annotations for WPCS 3.0.0
### CS: remove redundant ignore annotations [1] / never needed
Remove ignore annotations which are ignoring an error which would not be thrown for that code.
Includes tidying up the format of the ignore annotation:
//
and the start of the comment.### CS: remove redundant ignore annotations [2] / not needed
Remove ignore annotations which are unnecessary due to the configuration in the
phpcs.xml.dist
ruleset already taking care of this.### CS: remove redundant ignore annotations [3] / no longer needed
Remove ignore annotations about invalid function names for deprecated functions. Those are ignored by design by WPCS since version 2.2.0.
### CS: remove redundant ignore annotations [4] / Unused WPCS rules
Remove ignore annotations related to sniffs which are not used by WP Core (like sniffs which are in the
WordPress-Extra
ruleset).### CS: remove redundant ignore annotations [5] / Non-WPCS rules
The
VariableAnalysis
standard is not used by WP Core.### CS: use pre-[in|de]crement instead of post-[in|de]crement for stand-alone statements
### CS: remove superfluous blank line(s) at end of file
### CS: remove redundant blank line(s) at end of classes
### CS: remove redundant blank line(s) at end of functions
### CS: fix spacing for spread operators
No space allowed between the operator and the variable it applies to.
### Modernize: use dirname() with the $levels parameter
PHP 7.0 introduced the
$levels
parameter to thedirname()
function, which means nested calls todirname()
are no longer needed.Ref: https://www.php.net/manual/en/function.dirname.php
### CS: one space after function keyword for closures
### CS: remove redundant semi-colons
As these are not closures/anonymous classes, these semi-colons are redundant and create an empty PHP statement.
Trac ticket: https://core.trac.wordpress.org/ticket/59161