Skip to content

Implement CS checking based on the WP_CLI_CS ruleset #5198

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.

Sign up for GitHub

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

Jump to bottom
Merged
schlessera merged 7 commits into from
Apr 24, 2019
Merged

Implement CS checking based on the WP_CLI_CS ruleset #5198

schlessera merged 7 commits into from
Apr 24, 2019

Conversation

@jrfnl
Copy link
Contributor

@jrfnl jrfnl commented Apr 24, 2019 •
edited by schlessera
Loading

This basically updates the PHPCS ruleset to use the new WP_CLI_CS standard and adds a number of whitelist comments and exclusions.

As annotated in issue #5166, there is currently only one issue still remaining to be fixed:

FILE: php\utils-wp.php
------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
------------------------------------------------------------------------------------------
 111 | WARNING | strip_tags() is discouraged. Use the more comprehensive
     |         | wp_strip_all_tags() instead.
     |         | (WordPress.WP.AlternativeFunctions.strip_tags_strip_tags)
------------------------------------------------------------------------------------------

Please see the individual commits for more detail.

N.B.: In contrast to some of my other PRs, the commits in this PR should probably not be squashed.

Fixes #5166

Relates to #5179

Commit summary

Composer: use wp-cli-tests ^2.1

... as that version contains the new WPCliCS PHPCS standard.

Includes updating the composer.lock file with composer update wp-cli/wp-cli-tests --with-all-dependencies which resulted in the following updates:

  - Removing phpcompatibility/phpcompatibility-wp (1.0.0)
  - Updating wp-coding-standards/wpcs (1.2.1 => 2.1.0)
  - Updating wp-cli/eval-command (v2.0.3 => v2.0.4)
  - Updating wp-cli/config-command (v2.0.2 => v2.0.3)
  - Updating phpcompatibility/php-compatibility (8.2.0 => 9.1.1)
  - Removing wp-cli/wp-cli-tests (v2.0.13)
  - Installing wp-cli/wp-cli-tests (v2.1.1)
  - Updating roave/security-advisories (dev-master 9c6c45a => dev-master 1b329f4)

.gitignore: ignore correct phpcs/phpunit config files

  • Remove refs which shouldn't be in this file.
    • The phpunit.xml.dist file shouldn't be ignored.
    • The two directory entries look like personal ones which shouldn't have gotten into the .gitignore file in the first place, but should have been put into someone's personal .git/info/exclude file.
  • Add files which allow devs to overwrite config files for PHPUnit and PHPCS
    • The .dist files should be committed. However, for their personal use, devs can overrule those files with versions without .dist.
      Those personal versions should never be committed.

As a side-note: for PHPCS, having a personal version while still using the original .dist file is made very easy, as you can just import the .dist file as a starting point, like so:

<?xml version="1.0"?>
<ruleset name="WP-CLI">
    <rule ref="./phpcs.xml.dist"/>
</ruleset>

PHPCS: update the ruleset/defer to WPCliCS

The file is completely documented in-line.

PHPCS: replace some old-style whitelist comments with new-style

PHPCS: replace some WPCS whitelist comments with PHPCS whitelist comments

The WPCS native whitelist comments have been deprecated in WPCS 2.0 and will be removed in 3.0, in favour of the PHPCS native selective ignore comments which were introduced in PHPCS 3.2.

PHPCS: remove some redundant old-style ignore comments

PHPCS: whitelist some code for select sniffs

Depends on #5199

@jrfnl jrfnl requested a review from a team as a code owner April 24, 2019 07:50
@jrfnl jrfnl mentioned this pull request Apr 24, 2019
Closed
3 tasks
@schlessera schlessera added this to the 2.2.0 milestone Apr 24, 2019
@schlessera
Copy link
Member

schlessera commented Apr 24, 2019

Tackling that last remaining issue now...

@schlessera schlessera mentioned this pull request Apr 24, 2019
Merged
@dep dep bot added the dependent label Apr 24, 2019
@jrfnl jrfnl force-pushed the feature/update-phpcs branch from d152ec1 to 8ff6282 Compare April 24, 2019 08:36
@jrfnl
Copy link
Contributor Author

jrfnl commented Apr 24, 2019

Rebased now #5199 has been merged. Fingers crossed the build should pass.

@schlessera
Copy link
Member

schlessera commented Apr 24, 2019

@jrfnl Looks like it is failing on a bug in PHPCS: https://travis-ci.org/wp-cli/wp-cli/jobs/523862847#L595

jrfnl added 2 commits April 24, 2019 12:03
@jrfnl
Composer: use wp-cli-tests ^2.1
82658f4 
... as that version contains the new WP_CLI_CS PHPCS standard.

Includes updating the `composer.lock` file with `composer update wp-cli/wp-cli-tests --with-all-dependencies` which resulted in the following updates:
```
  - Removing phpcompatibility/phpcompatibility-wp (1.0.0)
  - Updating wp-coding-standards/wpcs (1.2.1 => 2.1.0): Loading from cache
  - Updating wp-cli/eval-command (v2.0.3 => v2.0.4): Loading from cache
  - Updating wp-cli/config-command (v2.0.2 => v2.0.3): Loading from cache
  - Updating phpcompatibility/php-compatibility (8.2.0 => 9.1.1): Loading from cache
  - Removing wp-cli/wp-cli-tests (v2.0.13)
  - Installing wp-cli/wp-cli-tests (v2.1.1): Loading from cache
  - Updating roave/security-advisories (dev-master 9c6c45a => dev-master 1b329f4)
```
@jrfnl
.gitignore: ignore correct phpcs/phpunit config files
3a027ee 
* Remove refs which shouldn't be in this file.
    - The `phpunit.xml.dist` file shouldn't be ignored.
    - The two directory entries look like personal ones which shouldn't have gotten into the `.gitignore` file in the first place, but should have been put into someone's personal `.git/info/exclude` file.
* Add files which allow devs to overwrite config files for PHPUnit and PHPCS
    - The `.dist` files should be committed. However, for their personal use, devs can overrule those files with versions without `.dist`.
        Those personal versions should never be committed.

As a side-note: for PHPCS, having a personal version while still using the original `.dist` file is made very easy, as you can just import the `.dist` file as a starting point, like so:
```xml
<?xml version="1.0"?>
<ruleset name="WP-CLI">
    <rule ref="./phpcs.xml.dist"/>
</ruleset>
```
@jrfnl jrfnl force-pushed the feature/update-phpcs branch from 8ff6282 to 3bda68a Compare April 24, 2019 10:03
@jrfnl
Copy link
Contributor Author

jrfnl commented Apr 24, 2019

Looks like it is failing on a bug in PHPCS

My bad. I whitelisted the wrong file: https://github.com/wp-cli/wp-cli/pull/5198/files#diff-6758ef1f416bd888bd1a5cad8f97cc2bR119

Should be fixed now.

jrfnl added 5 commits April 24, 2019 12:13
@jrfnl
PHPCS: update the ruleset/defer to WP_CLI_CS
9be2c82 
The file is completely documented in-line.
@jrfnl
PHPCS: replace some old-style whitelist comments with new-style
bd3fc73
@jrfnl
PHPCS: replace some WPCS whitelist comments with PHPCS whitelist comm…
4872602 
…ents

The WPCS native whitelist comments have been deprecated in WPCS 2.0 and will be removed in 3.0, in favour of the PHPCS native selective ignore comments which were introduced in PHPCS 3.2.
@jrfnl
PHPCS: remove some redundant old-style ignore comments
a7c73a4
@jrfnl
PHPCS: whitelist some code for select sniffs
7e12cae
@jrfnl jrfnl force-pushed the feature/update-phpcs branch from 3bda68a to 7e12cae Compare April 24, 2019 10:14
@jrfnl
Copy link
Contributor Author

jrfnl commented Apr 24, 2019

LOL Ok, so I hadn't whitelisted the wrong file, except since I last scanned, the same issue is now being reported for a second file....

As this relates to a bug in PHPCS itself, I've now just chosen to exclude that complete error code for the time being with a note that that exclusion should be removed once PHPCS 3.5.0 has been released and starts to be used.

@jrfnl
Copy link
Contributor Author

jrfnl commented Apr 24, 2019

Builds are ✔️ 🎉

@schlessera schlessera merged commit 779bdd1 into wp-cli:master Apr 24, 2019
@jrfnl jrfnl deleted the feature/update-phpcs branch April 24, 2019 10:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@schlessera schlessera schlessera approved these changes

Assignees

No one assigned

Labels

dependent scope:framework

Projects

None yet

Milestone

2.2.0

Development

Successfully merging this pull request may close these issues.

Adopt and enforce new WP_CLI_CS standard

2 participants

@jrfnl @schlessera