Remove WordPress-Core from WordPress-VIP standard

[westonruter]

There is a lot of noise reported from the WordPress-Core standard that the VIP team doesn't care about. They don't care about whitespace and bracket usage, but rather they care about issues like security and not using restricted functions. As such, the WordPress-VIP standard should remove WordPress-Core. If the WordPress-Core are desired alongside the WordPress-VIP sniffs then all that is required is for both standards to be used when invoking PHPCS:

phpcs --standard=WordPress-Core,WordPress-VIP

Either this, or a custom project-specific ruleset.xml can be used which calls in both of these standards.

[jrfnl]

Just some thoughts on this:

I know there are a number of rules which are now in the Core ruleset which should in that case also be added to the VIP ruleset. Things like the POSIX and extract() rules to name just a few. Basically they are best practices which have been elevated to a coding standard for WP.

Most coding standards are about code style. However the WP Core coding standard encompasses both code style as well as best practices.

Removing the Core ruleset from the VIP ruleset would also remove the best practices, not just the code style rules. And correct me if I'm wrong, but I seem to remember that most of these best practices are actually also required for VIP.

So, what about maybe collecting the code style rules in a separate ruleset which can be included in Core.
In VIP, Core would then stay included to get the benefit of the best practices, but the code style ruleset could be explicitly excluded.

This would also make life easier for teams which use a different code style (Zend, Symphony), but would like to include the WP coding standards to get the benefit of the best practices.

[westonruter]

Good point, any VIP requirements that have been inherited from WordPress-Core would also need to be explicitly referenced by WordPress-VIP.

I think the simplest way to do this is to just identify the sniffs from WordPress-Core that are VIP requirements and then copy them into WordPress-VIP. I don't know for sure how much mileage we'd get by creating a new WordPress-Best-Practices intersection ruleset. VIP has published pretty comprehensive requirements for what they look for.

But, all of this to say, maybe I'm taking the completely wrong approach. The goal here is to eliminate the noise for VIP code wranglers when reviewing code submissions. So perhaps all that is really needed in this regard is to create a new custom ruleset that extends WordPress-VIP and simply turns off the severity of all sniffs they don't care about, such as bracket usage and whitespace. If we did this, then users would continue, by default, to get all of the best practices of WordPress-Core in WordPress-VIP and then only those who know what they're doing can explicitly turn off rules that aren't concerning.

[GaryJones]

One thing I'd like to highlight here, is that VIP is a commercial venture from one particular company. I realise there are VIP partners who have clients on VIP hosting who therefore need to check all the code they write (and use from 3rd parties) meets certain criteria, but ultimately, this is an Automattic-specific ruleset.

Would we allow SiteGround or WPEngine rulesets in this repo? Or would we expect them to run their own repo which has WPCS as a dependency, much like the Yoast ruleset?

I know that in many of the projects I do, which won't ever be used on VIP, the default ruleset is WordPress-Core and then excluding WordPress-VIP.

Maybe it's worth considering that, now there are more than a few trivial rules, we suggest that Automattic run their own VIP sniffs open source repo, and then they are free to include whatever sniffs from Core, Extras or Docs, as they wish?

[JDGrimes]

I second @jrfnl's suggestion of possibly splitting the core ruleset. I think that would provide the most flexibility for people who want to extend the rulesets. If someone just wants to have the whitespace or just wants to have the best-practices, they don't want to have to go through the rulesets each time WPCS releases a new versions and tease all of that out from the core ruleset. They'd much rather be able to reference a specific ruleset that just contains the type of rules that they are interested in, and then forget it.

As far as removing VIP, I usually use VIP in my custom rulesets, although I do exclude some of the rules. I think that there are a lot of useful best practices that are a part of VIP but not the extra ruleset (although maybe that has changed, I dunno). So I wouldn't really want to see VIP removed completely, unless some of that was retained for the extra ruleset.

[GaryJones]

I think that there are a lot of useful best practices that are a part of VIP but not the extra ruleset

This should definitely be changed over then IMO.

[westonruter]

So should WordPress-Core be split into something like WordPress-Core-Code-Style and WordPress-Core-Best-Practices? Where would the line be drawn between the latter and WordPress-Extra?

In terms of what would specifically be of help to the VIP code wranglers who don't care about code style but just about best practices (and specifically those which are on their blocker and caution lists), then maybe it's easiest for them to have their own custom rulesets that just exclude/suppress all of the sniffs that aren't relevant to them. These rulesets could just live in their fork at https://github.com/Automattic/WordPress-Coding-Standards

[jrfnl]

So should WordPress-Core be split into something like WordPress-Core-Code-Style and WordPress-Core-Best-Practices? Where would the line be drawn between the latter and WordPress-Extra?

I think it would be easiest to split WordPress-Core into WordPress-Code-Style and WordPress-Core where WordPress-Core would include WordPress-Code-Style as part of the standard.

Something like:

<?xml version="1.0"?>
<ruleset name="WordPress Core">
    <rule ref="WordPress-Code-Style"/>

    ...  the other rules ...
</ruleset>

The line would between extra and core will remain the same: what is in the handbook as a rule.

For the WordPress-VIP standard, the ruleset would then look something like:

<?xml version="1.0"?>
<ruleset name="WordPress VIP">
    <description>WordPress VIP Coding Standards</description>

    <rule ref="WordPress-Core">
        <exclude name="WordPress-Code-Style" />
    </rule>

    ...  the other rules ...
</ruleset>

What do you think ?

[westonruter]

I haven't worked with the project rulesets to this degree. If PHPCS can handle indirect exclusions like this that would be awesome. So let's say the WordPress-Core ruleset looks like:

<?xml version="1.0"?>
<ruleset name="WordPress Core">
    <rule ref="WordPress-Code-Style" />
    <!-- ...  the sniffs for best practices ... -->
</ruleset>

Assuming then WordPress-VIP ruleset is modified to look like:

<?xml version="1.0"?>
<ruleset name="WordPress VIP">
    <rule ref="WordPress-Core" />
    <!-- ...  the other VIP rules for caution and blocker issues ... -->
</ruleset>

For the sake of the VIP code wranglers (who don't want strict, i.e. code style), wouldn't it instead be:

<?xml version="1.0"?>
<ruleset name="WordPress VIP Critical">
    <description>WordPress VIP Coding Standards for blockers and cautions</description>
    <rule ref="WordPress-VIP">
        <exclude name="WordPress-Code-Style" />
    </rule>
</ruleset>

Does this work? Namely, <exclude name="WordPress-Code-Style" />? I've seen that these exclude elements are normally referencing sniffs not standards (note name for sniff instead of ref for standard, apparently).

[jrfnl]

Does this work? Namely, ? I've seen that these exclude elements are normally referencing sniffs not standards (note name for sniff instead of ref for standard, apparently).

It should - I checked the PHPCS wiki before I suggested it.

Quote taken from https://github.com/squizlabs/PHP_CodeSniffer/wiki/Annotated-ruleset.xml:

<!--
    You can even exclude a whole standard. This example includes
    all sniffs from the Squiz standard, but excludes any that come
    from the Generic standard.
 -->
 <rule ref="Squiz">
  <exclude name="Generic"/>
 </rule>

[westonruter]

Nice! I didn't see that. It's not reflected on the PEAR documentation page, which is what I had seen: https://pear.php.net/manual/en/package.php.php-codesniffer.annotated-ruleset.php

[jrfnl]

A proposal following up on this can be found in #740

[GaryJones]

The approach of splitting WordPress Core up into WordPress Core + WordPress Coding Style, is that it will break BC - not as many checks will be done on code as it was previously if someone has specified WordPress-Core in their phpcs.xml.

I'm not convinced of the value in splitting up WP Core into yet another subgroup. If a rule is in the handbook, then it's in the handbook, regardless of whether it's for security, whitespace or general formatting. The Core standard is already somewhat broken up, by the directories the Sniffs are stored in, and excludes can be done on that basis.

My understanding of the VIP rules here, is that they are for developers who are writing code that could be used on the VIP platform.

What the Automattic employees (a different group of people) want is their own ruleset which includes our VIP rules, but excludes certain others.

Again, both the VIP, and the "VIP Review" rulesets are very company-specific, and I'm uncomfortable with that. I'd rather our efforts were spent in identifying, agreeing and moving the best practice parts of VIP over to Extras (I agree, some are useful), and encouraging Automattic to create their own "VIP Review" ruleset (it's their internal tool after all), and ideally taking ownership of the VIP ruleset as well.

For me, we should be maintaining and improving just the following standards:

• WordPress-Core (WordPress-Handbook / WordPress) - Code standard from the Handbook
• WordPress-Docs - Inline Docs standard from the Handbook
• WordPress-Extras - Recommended best practices, not sufficiently covered in the Handbook.

There is a lot of noise reported from the WordPress-Core standard that the VIP team doesn't care about.

In short, I don't think this is our problem to solve, so long as there are sufficient ways for them (or anyone) to exclude whatever individual rules they don't want reported, and no separate ruleset is needed to be created by us for that to happen.

[jrfnl]

The approach of splitting WordPress Core up into WordPress Core + WordPress Coding Style, is that it will break BC - not as many checks will be done on code as it was previously if someone has specified WordPress-Core in their phpcs.xml.

The current proposal as I've outlined in #740 will not break backward compatibility. The Core ruleset would be split into two completely different rulesets which both would then be included in the Core again, breaking nothing.

I'm not convinced of the value in splitting up WP Core into yet another subgroup.

While the original issue was opened by @westonruter with an eye on VIP, I think the Theme Review Automation project might find this splitting off useful as well as they could then include the Core best practice rules without the code style rules (which themes don't have to comply with).
Of course, they could include any relevant rules in their ruleset and not include core, but from a DRY perspective, I can see merit for them too. /cc @grappler

I'd rather our efforts were spent in identifying, agreeing and moving the best practice parts of VIP over to Extras (...), and encouraging Automattic to create their own "VIP Review" ruleset (...), and ideally taking ownership of the VIP ruleset as well.

One doesn't exclude the other, but yes, I agree that "our" focus should not be on VIP.

[grappler]

Again, both the VIP, and the "VIP Review" rulesets are very company-specific, and I'm uncomfortable with that. I'd rather our efforts were spent in identifying, agreeing and moving the best practice parts of VIP over to Extras (...), and encouraging Automattic to create their own "VIP Review" ruleset (...), and ideally taking ownership of the VIP ruleset as well.

+1 to this. The highlighting my own emphasis. PR #633 is a start at the separation.

I think the Theme Review Automation project might find this splitting off useful as well as they could then include the Core best practice rules without the code style rules

I think I would prefer to keep all of the control in the WordPress-Theme ruleset then be dependant on another ruleset. This would require the the TRT to keep uptodate with all of the changes being made.