Make WordPress Core

Opened 8 months ago

Closed 6 months ago

#59161 closed defect (bug) (fixed)

Composer: update to WordPressCS 3.0.0

Reported by: jrf's profile jrf Owned by: sergeybiryukov's profile 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 jrf)

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:

### 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)

59161.wp-block-parser-block.diff (962 bytes) - added by SergeyBiryukov 6 months ago.

Download all attachments as: .zip

Change History (50)

This ticket was mentioned in PR #5047 on WordPress/wordpress-develop by @jrf.


8 months ago
#1

  • Keywords has-patch added

### CS: align equal signs for consecutive statements (pre-existing)

### PHPCS: improve organisation of the PHPCompatibility ruleset

No functional changes.

This commit:

  • Adds section headers to the ruleset file.
  • Organizes all directives in their respective sections.

### PHPCS: remove unnecessary directives

This commit:

  • Removes the unnecessary exclusion patterns for the node_modules and the vendor directory.

As this ruleset only scans the src directory, those directories would never be scanned anyway.

  • Removes the selective excludes related to the Random Compat package.

This package was removed in WP 6.3, so these excludes are no longer necessary.

### 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:

  • Adds section headers to the ruleset file.
  • Organizes all directives in their respective sections.

### PHPCS: update the ruleset for WordPressCS 3.0.0

This commit:

  • Raises the memory limit to be on the safe side as WPCS 3.0.0 contains a lot more sniffs.
  • Removes explicit inclusions of extra rules, which have now been added to the WordPress-Core ruleset..
  • Updates property names for select sniffs.
  • Updates one exclusion - the WordPress.CodeAnalysis.AssignmentInCondition sniff has been (partially) replaced by the Generic.CodeAnalysis.AssignmentInCondition sniff.
  • Adds one new exclusion.

### 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:

  • Customary one space between the // and the start of the comment.
  • There should be no spaces in the comma-separated sniff list.

### 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 the dirname() function, which means nested calls to dirname() 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

This ticket was mentioned in Slack in #core-coding-standards by garyj. View the logs.


8 months ago

#3 @jorbin
8 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.

#4 @jrf
8 months ago

  • Description modified (diff)

#5 @jrf
8 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.

[EDIT]
The Composer commit + ruleset + the ignore annotation changes ? Being able to track those will probably be a good thing.

Last edited 8 months ago by jrf (previous) (diff)

#6 @SergeyBiryukov
8 months ago

  • Owner set to SergeyBiryukov
  • Status changed from new to accepted

#7 @desrosj
8 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: @jrf
8 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: @jrf
8 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 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).

@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: @desrosj
8 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 @jrf
8 months ago

Replying to desrosj:

latest sounds great to me! 👍

I've added a new commit to the PR which does just that.

#12 @jrf
8 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).

@jrf commented on PR #5047:


8 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.....

#14 @SergeyBiryukov
8 months ago

In 56514:

Code Modernization: Rename parameters that use reserved keywords in wp-includes/functions.php.

While using reserved PHP keywords as parameter name labels is allowed, in the context of function calls using named parameters in PHP 8.0+, this will easily lead to confusion. To avoid that, it is recommended not to use reserved keywords as function parameter names.

This commit renames the $class parameter to $class_name in _deprecated_class().

Follow-up to [54929], [56467].

Props jrf.
See #59161.

#15 @SergeyBiryukov
8 months ago

In 56521:

Coding Standards: Correct equals sign alignment in various files.

This resolves a few WPCS warnings:

Equals sign not aligned with surrounding statements

so that the output of composer format is clean.

Follow-up to [56276], [56302].

Props jrf.
See #59161, #58831.

#16 @SergeyBiryukov
8 months ago

In 56536:

Coding Standards: Remove superfluous blank lines at the end of various files.

Note: This is enforced by WPCS 3.0.0.

Props jrf.
See #59161, #58831.

#17 @SergeyBiryukov
7 months ago

In 56547:

Coding Standards: Remove superfluous blank lines at the end of various classes.

Note: This is enforced by WPCS 3.0.0.

Follow-up to [56536].

Props jrf.
See #59161, #58831.

#18 @SergeyBiryukov
7 months ago

In 56548:

Coding Standards: Remove superfluous blank lines at the end of various functions.

Note: This is enforced by WPCS 3.0.0.

Follow-up to [56536], [56547].

Props jrf.
See #59161, #58831.

#19 @SergeyBiryukov
7 months ago

In 56549:

Coding Standards: Use pre-increment/decrement for stand-alone statements.

Note: This is enforced by WPCS 3.0.0:

  1. There should be no space between an increment/decrement operator and the variable it applies to.
  2. Pre-increment/decrement should be favoured over post-increment/decrement for stand-alone statements. “Pre” will in/decrement and then return, “post” will return and then in/decrement. Using the “pre” version is slightly more performant and can prevent future bugs when code gets moved around.

References:

Props jrf.
See #59161, #58831.

#20 @SergeyBiryukov
7 months ago

In 56551:

Coding Standards: Correct spacing for spread operators.

No space allowed between the operator and the variable it applies to.

Note: This is enforced by WPCS 3.0.0.

Follow-up to [46133].

Props jrf.
See #59161, #58831.

#21 @SergeyBiryukov
7 months ago

In 56552:

Code Modernization: Use dirname() with the $levels parameter.

PHP 7.0 introduced the $levels parameter to the dirname() function, which means nested calls to dirname() are no longer needed.

Note: This is enforced by WPCS 3.0.0.

Reference: PHP Manual: dirname().

Follow-up to [56141].

Props jrf.
See #59161, #58831.

#22 @SergeyBiryukov
7 months ago

In 56559:

Coding Standards: Include one space after function keyword for closures.

Note: This is enforced by WPCS 3.0.0.

Reference: WPCS: PR #2328 Core: properly check formatting of function declaration statements.

Props jrf.
See #59161, #58831.

@jrf commented on PR #5047:


7 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.

#24 @SergeyBiryukov
7 months ago

In 56593:

Coding Standards: Improve organization of the WPCS-based PHPCS ruleset.

This commit:

  • Adds section headers to the ruleset file.
  • Organizes all directives in their respective sections.

No functional changes.

Props jrf.
See #59161.

#25 @SergeyBiryukov
7 months ago

In 56594:

Coding Standards: 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.

Follow-up to [42346], [45607].

Props jrf.
See #59161.

#26 @jrf
7 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

#27 @SergeyBiryukov
7 months ago

In 56598:

Coding Standards: Fix a few newly introduced WPCS issues.

Follow-up to [56515], [56557], [56560].

Props jrf.
See #59161, #58831.

@jrf commented on PR #5047:


7 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.

#29 @SergeyBiryukov
7 months ago

In 56680:

Coding Standards: Fix a few newly introduced WPCS issues.

Follow-up to [56570], [56573], [56589], [56604], [56612], [56620], [56629], [56631], [56638], [56642], [56644], [56649].

Props jrf.
See #59161, #58831.

#30 @SergeyBiryukov
7 months ago

In 56692:

Coding Standards: Fix a few newly introduced WPCS issues.

Follow-up to [56683], [56689].

See #59161, #58831.

#31 @SergeyBiryukov
7 months ago

In 56695:

Coding Standards: Upgrade WPCS to version 3.0.0.

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.

WordPressCS 3.0.0 contains breaking changes, both for people using ignore annotations, people maintaining custom rulesets, as well as for sniff developers who maintain a custom PHPCS standard based on WordPressCS.

If you are an end-user or maintain a custom WordPressCS based ruleset, please start by reading the Upgrade Guide to WordPressCS 3.0.0 for ruleset maintainers which lists the most important changes and contains a step by step guide for upgrading.

If you are a maintainer of an external standard based on WordPressCS and any of your custom sniffs are based on or extend WordPressCS sniffs, please read the Upgrade Guide to WordPressCS 3.0.0 for Developers.

In all cases, please read the complete changelog carefully before you upgrade.

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 WPCS, so the dependency is inherited automatically.
  • Updates the ruleset for WPCS 3.0.0. This includes:
    • Raising the memory limit to be on the safe side as WPCS 3.0.0 contains a lot more sniffs.
    • Removing explicit inclusions of extra rules, which have now been added to the WordPress-Core ruleset..
    • Updating property names for select sniffs.
    • Updating one exclusion — the WordPress.CodeAnalysis.AssignmentInCondition sniff has been (partially) replaced by the Generic.CodeAnalysis.AssignmentInCondition sniff.
    • Adding one new exclusion.
  • Downgrades 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 the issues can be fixed in due time. For now, the test directory will be excluded until the issues are fixed (as the test directory CS run does not allow for warnings).
  • Updates ignore annotations for WPCS 3.0.0.

Reference: WPCS 3.0.0 release notes.

Follow-up to [43571], [44574], [45600], [47927].

Props jrf, jorbin, desrosj.
See #59161.

#32 @SergeyBiryukov
7 months ago

In 56696:

Build/Test Tools: Use the latest PHP version for the coding standards workflow.

PHPCS can now be run on the latest PHP version as all known PHP 8.x compatibility issues (in WPCS) have been fixed.

Follow-up to [49162], [56695].

Props jrf, desrosj.
See #59161.

This ticket was mentioned in Slack in #core-committers by jrf. View the logs.


7 months ago

#34 @SergeyBiryukov
7 months ago

In 56738:

Coding Standards: Remove redundant ignore annotations.

This removes ignore annotations which are ignoring an error which would not be thrown for that code.

Includes tidying up the format of the ignore annotation:

  • Customary one space between the // and the start of the comment.
  • There should be no spaces in the comma-separated sniff list.

Follow-up to [45607], [47185], [49200], [53152].

Props jrf.
See #59161.

#35 @SergeyBiryukov
7 months ago

In 56741:

Coding Standards: Remove temporary exclusion from the PHPCS ruleset.

Now that the block-library package is updated for WP 6.4, this is no longer necessary.

Related PR from Gutenberg repository:

Follow-up to [56695], [56710].

See #59161.

#36 @SergeyBiryukov
7 months ago

In 56743:

Coding Standards: Remove redundant ignore annotations, take 2.

This removes ignore annotations which are unnecessary due to the configuration in the phpcs.xml.dist ruleset already taking care of this.

Follow-up to [45611], [50146], [50148], [50586], [50822], [56738].

Props jrf.
See #59161.

#37 @SergeyBiryukov
7 months ago

In 56751:

Coding Standards: Remove redundant ignore annotations, take 3.

This removes ignore annotations about invalid function names for deprecated functions. Those are ignored by design in WPCS since version 2.2.0.

Follow-up to [45580], [47612], [47927].

Props jrf.
See #59161.

#38 @SergeyBiryukov
7 months ago

In 56752:

Coding Standards: Remove redundant ignore annotations, take 4.

This removes ignore annotations related to sniffs which are not used by WP Core (like sniffs in the WordPress-Extra ruleset).

Follow-up to [48072], [51003], [55204], [56714].

Props jrf.
See #59161.

#39 @SergeyBiryukov
7 months ago

In 56753:

Coding Standards: Remove redundant ignore annotations, take 5.

The VariableAnalysis standard is not used by WP Core.

Follow-up to [50958], [51003], [52049], [52051], [52069], [53072], [54132], [55132], [56363], [56738], [56743], [56751], [56752].

Props jrf.
See #59161.

@jrf commented on PR #5047:


7 months ago
#40

@SergeyBiryukov Looks like some merge artifact crept in with that merge ? Want me to rebase instead ?

@SergeyBiryukov commented on PR #5047:


7 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 🙂

@jrf commented on PR #5047:


7 months ago
#42

Want me to rebase instead ?

That would be great 🙂

Done!

#43 @SergeyBiryukov
7 months ago

In 56798:

Coding Standards: Correct alignment in wp-admin/user-edit.php.

This resolves a WPCS warning:

Array double arrow not aligned correctly;
expected 1 space(s) between "'type'" and double arrow, but found 15.

Follow-up to [56570], [56680].

Props jrf.
See #59161, #58831.

#44 @SergeyBiryukov
6 months ago

In 56799:

Coding Standards: Upgrade WPCS to version 3.0.1.

In WordPressCS 3.0.0, the functionality of the WordPress.Security.EscapeOutput sniff was updated to report unescaped message parameters passed to exceptions created in throw statements. This specific violation now has a separate error code: ExceptionNotEscaped. This will allow users to ignore or exclude that specific error code.

The error code(s) for other escaping issues flagged by the sniff remain unchanged.

References:

Follow-up to [56695].

Props jrf, bjorsch, dawidurbanski.
See #59161.

@jrf commented on PR #5047:


6 months ago
#45

Rebased again just so we can see what's left.

@SergeyBiryukov commented on PR #5047:


6 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?

#47 @SergeyBiryukov
6 months ago

In 57017:

Coding Standards: Remove a redundant section in the phpcs.xml.dist ruleset.

The affected lines already have ignore annotations in the wp-includes/class-wp-block-parser-block.php file itself.

Follow-up to [56048], [56738], [56743], [56751], [56752], [56753].

Props jrf, SergeyBiryukov.
See #59161.

@SergeyBiryukov commented on PR #5047:


6 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.

#49 @SergeyBiryukov
6 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

Note: [57017] was for trunk only. Since it's not a regression in 6.4, I don't think it needs a backport.

This should be resolved now. Thanks everyone!

Note: See TracTickets for help on using tickets.