search

Automattic / VIP-Coding-Standards

PHP_CodeSniffer ruleset to enforce WordPress VIP coding standards.
https://wpvip.com/documentation/how-to-install-php-code-sniffer-for-wordpress-com-vip/
Other
236 stars 40 forks source link

Review all sniffs for compatibility with PHPCS >= 4.0 #552

Open jrfnl opened 4 years ago

jrfnl commented 4 years ago

Development on PHPCS 4.x has started a while back.

While there is no timeline known for when it will be ready/released, some (breaking) changes which PHPCS 4.x will contain are already known.

Notable changes as known at the time of writing this:

I expect most of this won't have much effect on the sniffs in VIPCS. If/when more changes become known, I will update the above list.

However, a decision is needed about the future of the sniffs which look at JS/CSS code. There is no urgency (yet), but I'm opening this issue now to raise awareness and open the discussion about this.

GaryJones commented 4 years ago

The minimum supported PHP version is slated to become PHP 7.2.

VIP sites run on PHP 7.3, so client developers may/should already have at least PHP 7.2. I'd be fine with having a "squizlabs/phpc_codesniffer": "^3.55 | ^4" constraint once we support it, so that if the client does have PHP 7.2+ available, then they may be able to use PHPCS. We should be able to run for a few months supporting both PHPCS 3.x and PHPCS 4, though our Review Bot will likely only be running one or the other, so once it's running PHPCS 4, we'll be encouraging (via communication and constraints) that clients also use PHPCS 4.

Support for the PEAR installation method will be removed.

Doesn't affect VIPCS.

Support for JS and CSS sniffs will be removed.

This is the biggest change for VIPCS. We have a few sniffs that use these tokenisers, including ones related to security. Our proposal so far has been to see if eslint could be added to the Review Bot code, along with packages that could flag the same function calls as what we currently do in our sniffs.

Support for the "old-style" ignore annotations, like @codingStandardsIgnoreLine will be removed. These were deprecated in PHPCS 3.2.0.

Shouldn't be any of those in VIPCS, but we still occasionally see clients submit code with these in, and I even have a Saved Reply in GH with saying not to use them (and // WPCS: XSS ok-style comments)!

Support for the "old-style" array properties in rulesets, where array items would be passed via a comma separated value instead of as individual items. This was deprecated in PHPCS 3.3.0.

Old style array properties are already eliminated from VIPCS, and the Review Bot doesn't yet run with full custom XML config files.

Deprecated tokens will be removed, most notably T_ARRAY_HINT and T_RETURN_TYPE which were both deprecated in PHPCS 3.3.0.

Those particular two tokens are not used in VIPCS.

jrfnl commented 4 years ago

Shouldn't be any of those in VIPCS, but we still occasionally see clients submit code with these in, and I even have a Saved Reply in GH with saying not to use them (and // WPCS: XSS ok-style comments)!

For the PHPCS native old-style annotation, I'm working on a sniff to detect these and warn about them (and auto-fix). Once that sniff is ready, we could consider including it.

For the WPCS old-style comments: the relevant WPCS sniffs have been throwing warnings about the use of the old-style comments for quite some time now.

GaryJones commented 4 years ago

the relevant WPCS sniffs have been throwing warnings about the use of the old-style comments for quite some time now.

I don't think we include those in VIPCS - the client just sees the violation instead.

jrfnl commented 4 years ago

@GaryJones So the question then is: should those be included ? Mind: they will be (have been) removed in WPCS 3.0.0.

GaryJones commented 4 years ago

Let's not add them to only remove them again relatively shortly. The worst case is that someone sees a violation when they think they've ignored it (so, user error).