WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 5 days ago

#30153 closed enhancement (wontfix)

Integrate PHP_CodeSniffer into build/test tools to enforce coding standards on new patches

Reported by: westonruter Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords:
Focuses: coding-standards Cc:

Description

There is a community project to develop PHP_CodeSniffer standard(s) for the WordPress Coding Standards: https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards

Currently it only (mainly) applies to PHP files, though PHP_CodeSniffer can also tokenize and apply sniffs to JS and CSS.

It would be helpful if PHP_CodeSniffer could automatically check new patches being committed, to incorporate it into a pre-commit hook. A key here is to restrict PHP_CodeSniffer to only report errors and warnings on the lines that are part of the patch being committed. There is an initial version of such a “phpcs-patch” script which needs to be tested and integrated into the build/test tools: https://github.com/xwpco/PHP_CodeSniffer/pull/1

Change History (10)

This ticket was mentioned in Slack in #core by westonruter. View the logs.


4 years ago

This ticket was mentioned in Slack in #core by jorbin. View the logs.


4 years ago

#5 @GaryJ
7 months ago

  • Focuses coding-standards added

#6 @netweb
7 months ago

Related: #41057

#7 @pento
2 weeks ago

  • Milestone changed from Awaiting Review to 5.0

@westonruter: Did you ever get phpcs-patch merged upstream?

A reasonable first version of this could just run phpcbf on any changed files (after another full phpcbf run is done on trunk, of course).

#8 @westonruter
2 weeks ago

@pento No, it was not merged. But there is an implementation of it in wp-dev-lib in its check-diff.sh and accompanying diff tools.

However, @jrf has wisely pointed out that it is unreliable to filter out errors that aren't related to the lines in a diff. That is, errors can slip through. For example, when applying to approach to JSHint, if a variable is removed but then its var remains then an error will slip through regarding an unused variable.

#9 @pento
2 weeks ago

That's a good point.

Okay, let's go with running phpcbf as a first version, it can be changed to phpcs if we ever manage to fix all of coding standards issues that require manual intervention. 🙂

#10 @pento
5 days ago

  • Keywords needs-patch removed
  • Milestone 5.0 deleted
  • Resolution set to wontfix
  • Status changed from new to closed

Given that phpcs-patch doesn't appear to be a viable option, I'm closing this in favour of adding the phpcbf check in #44600.

Note: See TracTickets for help on using tickets.