Deprecate WordPressVIPMinimum ruleset

[rebeccahum]

With all sites migrated to Go, I think we can deprecate https://github.com/Automattic/VIP-Coding-Standards/blob/develop/WordPressVIPMinimum/ruleset.xml and use https://github.com/Automattic/VIP-Coding-Standards/blob/develop/WordPress-VIP-Go/ruleset.xml as the main source of truth.

[rebeccahum]

I guess technically, we could deprecate the Go ruleset and move everything on there to the VIPMinimum depending on which way we want to go about things...but I don't think it's necessary to have two rulesets anymore.

[GaryJones]

VIPCS contains two standards - WordPressVIPMinimum, meant for code that lives on WordPress.com VIP, and WordPress-VIP-Go, meant for code that lives on the VIP Go platform. Once customers have migrated their sites to VIP Go (which is now complete), then there's no reason for them to be using WordPressVIPMinimum ruleset directly.

The VIP Go standard doesn't have sniffs of it's own - it references sniffs that are under the WordPressVIPMinimum namespace and directory, as well as those from WPCS and built-in coding standards.

The relevant part of the VIPCS repo looks like this:

.
├── WordPress-VIP-Go/
│   ├── ruleset-test.inc
│   ├── ruleset-test.php
│   └── ruleset.xml
├── WordPressVIPMinimum/
│   ├── Sniffs/
│   │   ├── AbstractVariableRestrictionsSniff.php
│   │   ├── Classes/
│   │   ├── Compatibility/
│   │   ├── Constants/
│   │   ├── Files/
│   │   ├── Functions/
│   │   ├── Hooks/
│   │   ├── .../
│   ├── Tests/
│   │   ├── Classes/
│   │   ├── Compatibility/
│   │   ├── Constants/
│   │   ├── Files/
│   │   ├── Functions/
│   │   ├── .../
│   ├── ruleset-test.inc
│   ├── ruleset-test.php
└── └── ruleset.xml

Understanding what severity level and message applies when using the VIP Go standard means checking:

• The WordPress-VIP-Go/ruleset.xml
• The WordPressVIPMinimum/ruleset.xml,
• The WordPressVIPMinimum sniff,
• Potentially a parent (likely WPCS) sniff that the WordPressVIPMinimum sniff extends.

...as each item here overrides the one below it.

With the WordPress.com VIP Platform no longer being used, we have the option to simplify this down. This would make maintenance easier in the long run.

One of the negatives of the current naming system, is that the platform ("VIP Go") is specifically mentioned, but in the long term this should just be platform agnostic like "VIP" or "WordPressVIP". In terms of PHP namespace, this could be Automattic\VIPCodingStandards.

We could have something like:

.
├── WordPressVIP/
│   ├── Sniffs/
│   │   ├── AbstractVariableRestrictionsSniff.php
│   │   ├── Classes/
│   │   ├── Compatibility/
│   │   ├── Constants/
│   │   ├── Files/
│   │   ├── Functions/
│   │   ├── Hooks/
│   │   ├── .../
│   ├── Tests/
│   │   ├── Classes/
│   │   ├── Compatibility/
│   │   ├── Constants/
│   │   ├── Files/
│   │   ├── Functions/
│   │   ├── .../
│   ├── ruleset-test.inc
│   ├── ruleset-test.php
└── └── ruleset.xml

There seems to be three options for the changes here:

1. Just remove WordPressVIPMinimum/ruleset.xml, then update WordPress-VIP-Go/ruleset.xml to pull in sniffs via file references. Least work for us, but may lead to confusion in the long term as the sniffs still exist under the old namespace. Doesn't take advantage of the possible simplifications we can make for maintenance.
2. As per 1, then move the Sniff and Test classes into WordPressVIP directory, update namespaces and directory name to make sniffs work and remove platform name reference. Best simplification for us, but most work for clients as they need to update .phpcs.xml.dist and // phpcs:ignore ... references, etc. This should just be a search-replace task, but it means doing it at a particular timing for when the changes would get rolled out to the code analysis bot.
3. Mark WordPressVIPMinimum rules as deprecated, like WPCS did for WordPress-VIP standard. Move Sniff and Test classes into WordPressVIP directory, then create replacement sniff classes (so references are maintained) that echo the deprecation notice, then adjust WordPress-VIP-Go ruleset to use the sniffs that do not echo the deprecation. Most work - but unlike WPCS, we should have a fairly closed and contactable audience who may still be using WordPressVIPMinimum locally.

I would strongly recommend that these changes are done as a major release.

As part of the updates, we would also:

• Remove reference in bin/ruleset-tests and other ancillary files.
• Update README.
• Update and improve public wpvip.com docs (not just pages about VIPCS, but references in code snippet examples on other pages too).

[rebeccahum]

I would strongly recommend that these changes are done as a major release.

I think 1) would be good short-term as the least breaking change for the time being.

And then for a bigger release afterwards, I'm a fan of eventually implementing the structure of 2), but I'm uncertain how large of an impact it would have on our customers.

[jrfnl]

I've got some thoughts on this and am writing them up. Bear with me and you'll have my input on this within the next few days.

[jrfnl]

Thanks for opening this issue to discuss this. Sorry took a little longer for me to respond (I wanted to run some tests to make sure I wasn't misrepresenting anything), but here goes.

Making sure we're all on the same page

In terms of PHP namespace, this could be Automattic\VIPCodingStandards.

The namespace for the sniffs MUST be the same as the standard name, though it can have a prefix. This is a requirement for PHPCS to recognize the sniffs and autoload them correctly.

So the prefix could be Automattic\VIPCodingStandards or Automattic, but if the namespace is changed to Automattic\VIPCodingStandards this automatically means that the Standard name will also be changed to VIPCodingStandards and changing the Standard name affects all error codes from the VIP native sniffs and would therefore be a breaking change.

Note: as I'm sure you are well aware of already: sniffs can not be moved into the WordPress-VIP-Go directory and use Automattic\WordPress-VIP-Go as a namespace as PHP does not allow dashes in namespace names.

---

1. Just remove WordPressVIPMinimum/ruleset.xml, then update WordPress-VIP-Go/ruleset.xml to pull in sniffs via file references. Least work for us, but may lead to confusion in the long term as the sniffs still exist under the old namespace. Doesn't take advantage of the possible simplifications we can make for maintenance.

The ruleset.xml file is required for PHPCS to recognize the WordPressVIPMinimum standard and to be able to refer to the sniffs by name in ruleset.
Yes, using file references will work in the WordPress-VIP-Go ruleset, but:

1. Custom rulesets will need to be updated as references to the sniff names will no longer work.
   • This will be confusing and non-intuitive for people who use custom rulesets as they will also need to use the file references, which may be quite fiddly.
   • Also note: any file references to the VIPMinimum sniffs will need to use relative path names. In the past this did not always work well cross-platform, but I believe most bugs related to that have been fixed. Using absolute path names is obviously not an option as we don't know in what path the standard will be installed.
     For custom rulesets to use file references would presume and have to enforce for all users of the custom ruleset to install everything in the same way and place relative to each other as otherwise the file references will break.
   • Also, I'm not 100% sure how relative paths are resolved when a ruleset is included by a secondary ruleset, so this would warrant a lot of testing.
2. References to sniff (error) codes in custom rulesets for overruling the type/severity etc, can still be used, but these must be after the include via the file reference as otherwise the code will not be recognized. Again, making this fiddly.

With this in mind, I'd strongly recommend against any solution which would involve removing the ruleset.xml file and using file references.

2. As per 1, then move the Sniff and Test classes into WordPressVIP directory, update namespaces and directory name to make sniffs work and remove platform name reference. Best simplification for us, but most work for clients as they need to update .phpcs.xml.dist and // phpcs:ignore ... references, etc. This should just be a search-replace task, but it means doing it at a particular timing for when the changes would get rolled out to the code analysis bot.

Same remarks as for 1) regarding the removal of the ruleset.xml file.

3. Mark WordPressVIPMinimum rules as deprecated, like WPCS did for WordPress-VIP standard. Move Sniff and Test classes into WordPressVIP directory, then create replacement sniff classes (so references are maintained) that echo the deprecation notice, then adjust WordPress-VIP-Go ruleset to use the sniffs that do not echo the deprecation. Most work - but unlike WPCS, we should have a fairly closed and contactable audience who may still be using WordPressVIPMinimum locally.

What is not mentioned here, is that this includes the same breaking change as per 2) where all the non-deprecated sniffs, i.e. all the sniffs which will still be used by WordPress-VIP-Go, will be renamed and all references to these sniffs in custom ruleset or in inline ignore annotations will need to be updated.

---

Observations about the rulesets/messages/severity

• The WordPressVIPMinimum ruleset currently includes a lot of references to external sniffs, which, as the WordPress-VIP-Go ruleset includes the WordPressVIPMinimum ruleset are inherited by the WordPress-VIP-Go ruleset.
• The WordPressVIPMinimum ruleset also contains message content and type changes for a number of these external sniffs.
• The WordPress-VIP-Go ruleset contains message content, type and severity changes for a number of the inherited external sniffs, as well as for numerous VIP native sniffs.

When consolidating, the message content and type changes for the VIP native sniffs should be moved to the actual sniffs and removed from the WordPress-VIP-Go ruleset.

As for the severity changes where the severity would be set to below 5, I think it might actually make sense to keep those in the WordPress-VIP-Go ruleset as this is lesser known functionality and if an external ruleset includes an individual sniff from VIP and no messages show up because the severity is below 5, this would confuse most configurators.
For those sniffs were the severity is set to anything above 5, moving those changes to the actual sniffs, again, makes sense.

About standard names

If a (base) standard rename would take place, I would recommend using a short, simple, generic name, not linked to a specific platform variant - like Go - for the following reasons:

1. If a next iteration on the platform would use another name and would require another ruleset, the generic name will not need to be changed.
2. When using the sniff names on the command line, a short, simple name will save a number of keystrokes and diminish the risk of spelling mistakes.

So, in case a standard rename would take place, I'd suggest something like VIPCS or WPVIP as a standard name for the standard containing the actual sniffs, though WordPressVIP is workable if needs be.

About consolidating the rulesets to one

If the WordPressVIPMinimum and WordPress-VIP-Go rulesets and standards would effectively be merged into one WordPressVIP standard, it has the following side-effects:

1. If a future iteration of the platform would require a new ruleset, we'd get into the same naming/message content/type/severity confusion as we are currently in as the new ruleset would need to overrule everything which is different from "Go".
2. As is currently the case, there would be no easy way to use the VIP native sniffs in combination with the WPCS sniffs without having the VIP ruleset overrule/silence a significant number of errors coming from WPCS.
3. It removes some of the flexibility.

In other words, I'd recommend sticking with two standards, split between 1) a standard with sniffs and no further customizations in the ruleset and 2) a platform specific ruleset, which would include external sniffs as well and possibly minor customizations for the notice severity of some messages.

Users of the coding standards

unlike WPCS, we should have a fairly closed and contactable audience who may still be using WordPressVIPMinimum locally.

While your primary audience is the VIP platform customers and decisions should be primarily based on the impact on them, these coding standards do have a secondary audience as well.

• A variety of plugin builders use (select sniffs from) the standard to make sure their plugins would be allowed on the VIP platform.
• A variety of agencies use (select sniffs from) the standard.
  Probably originally because some of their customers would roll out to the VIP platform, but some may have taken a liking to some of the VIP sniffs anyway.
• Miscellaneous other users.

In some cases, this means the standards are included in custom phpcs.xml[.dist] rulesets, but in a number of cases, (select sniffs from) the standard are included in a third-party standard. In both cases, these users will be impacted by any changes as well.

If we're talking numbers:

• The GitHub dependency graph currently shows 343 repositories and 47 packages which depend on the automattic/vipwpcs package.
• The Packagist dependents page shows 52 dependents.
• Code searches on GitHub show 208 references to automattic/vipwpcs in composer.json files and 112 references to WordPressVIPMinimum in xml[.dist] files.

Note: these numbers are based on public repos only. Private repos are not shown in any of these searches.

Also: as a number of external standards use the VIP sniffs, the actual number of external user repos will likely be higher as I have not searched for references to/stats for those external standards.

Possible strategies

Taking all the above into account, here are some outlines for possible strategies.

Goals/constraints

1. Be the least disruptive for existing customers (and external users) which have custom rulesets in place referencing the VIP sniffs.
2. Be the least disruptive for code containing inline annotations referencing the VIP sniffs.
3. Improve maintainability for the VIPCS repo for the future.

Strategy: Break as little as possible

With the "Break as little as possible" strategy:

• The ruleset/Standard names would stay the same.
• Redundant sniffs will be removed from WordPressVIPMinimum in 3.0.0, possibly with deprecation in a previous minor/patch release.
• The error messages/types/severity changes for the VIP native sniffs now contained in the WordPress-VIP-Go ruleset, will be moved to the actual sniffs (in 3.0.0).
• Recommendation: move the references to the external sniffs from the WordPressVIPMinimum standard to the WordPress-VIP-Go standard (in 3.0.0).

Advantages:

• Seemingly the least amount of custom ruleset/ignore annotation breakage for customers, as well as other external users, as sniff names/error codes won't change (other than some being removed).
• If the external sniff references would be moved, the WordPressVIPMinimum ruleset can be used (by the "other" users) in combination with WPCS without overruling/silencing WPCS issues.

Disadvantages:

• While the ruleset names don't change, their behaviour will change, which may be very confusing for people.
• While sniff names/error codes will not change, the error message content, type and severity will change for a significant number of them, which may be confusing for people.
• Little advance notice. Once the 3.0.0 version is released, the changes are active and customers will have to deal with the changed behaviour of rulesets there and then.
• The WordPressVIPMinimum standard name would remain, even though it is outdated.
• Will probably cause quite some customer queries around the roll-out of the 3.0.0 version.

Strategy: Break and be done with it

With the "Break and be done with it" strategy:

• The WordPress-VIP-Go standard name would remain the same.
• The WordPressVIPMinimum standard name would be changed to WordPressVIP (or similar).
• Redundant sniffs will be removed in 3.0.0, possibly with deprecation in a previous minor/patch release.
• All remaining VIP native sniffs will be moved from WordPressVIPMinimum to WordPressVIP, including namespace changes.
• The error messages/types/severity for the VIP native sniffs now contained in the WordPress-VIP-Go ruleset, will be moved to the actual sniffs in the WordPressVIP standard.
• Recommendation: move the references to the external sniffs from the WordPressVIPMinimum/WordPressVIP standard to the WordPress-VIP-Go standard.

Advantages:

• The WordPressVIP name no longer refers to the outdated concept of "VIP Minimum".
• If the external sniff references would be moved, the WordPressVIP ruleset can be used in combination with WPCS without overruling/silencing WPCS issues.

Disadvantages:

• All references to individual sniff names/error codes in custom rulesets and ignore annotations will need to be updated.
• While the standard name part of the sniff names will change, the error message content, type and severity will also change for a significant number of them, which may be very confusing for people.
• Little advance notice. Once the 3.0.0 version is released, the changes are active and customers will have to deal with the changed behaviour of rulesets there and then.
• Will most definitely cause quite some customer queries around the roll-out of the 3.0.0 version.

Strategy: Gradual change-over, clean break

With the "Gradual change-over, clean break" strategy:

• The WordPressVIPMinimum standard would be deprecated in 3.0.0 and removed in 4.0.0.
• The WordPress-VIP-Go standard would be deprecated in 3.0.0 and removed in 4.0.0.
• Redundant sniffs will be removed in 3.0.0 (or 4.0.0), with deprecation in a previous minor/patch release (or in 3.0.0).
• VIP native sniffs which remain, will be moved from WordPressVIPMinimum to WordPressVIP in 3.0.0 with a placeholder sniff remaining in the WordPressVIPMinimum standard.
  The placeholder sniff would extend the WordPressVIP sniff for the actual checks and throw a deprecation notice when included directly.
• The error messages/types/severity for the VIP native sniffs, currently contained in the WordPress-VIP-Go ruleset, will be moved to the actual sniffs in the WordPressVIP standard.
  Any such changes will be undone via the rulesets for the deprecated WordPressVIPMinimum standard and where applicable for the deprecated WordPress-VIP-Go standard, so the "old" behaviour is maintained for the "old"/deprecated standards during the 3.x cycle.
• A new VIPGo ruleset will contain the references to external sniffs and include the new WordPressVIP standard.

Advantages:

• The WordPressVIP name no longer refers to the outdated concept of "VIP Minimum".
• With the external sniff references moved, the WordPressVIP ruleset can be used in combination with WPCS without overruling/silencing WPCS issues.
• By having both the "old" rulesets with their original behaviour as well as the "new" rulesets with the new behaviour available during the full 3.x release cycle, customers have some flexibility to choose when they want to switch over from the old rulesets to the new.
• The different ruleset and sniff names clearly signal to customers (and external users) that they can expect changed behaviour and will need to spend a little time testing and making adjustments before switching over.
• While there will be queries as soon as 3.0.0 is released due to the deprecation notice(s), the actual support burden to help customers switch over can be spread out over a longer period and nothing will really break until the release of 4.0.0.

Disadvantages:

• The biggest break, the ruleset/sniff name/error code references for all customers will need to change.
• Most work for the maintainers of VIPCS.
• Limited period (the full 3.x release cycle) of having to very carefully evaluate the impact of every change on multiple rulesets (VIPCS maintenance).
  A checkbox item in a PR template could help make sure that each PR is reviewed for this.

Other considerations

It could be considered to move the references to the external sniffs to a separate ruleset VIPExternal.
While not strictly necessary for any of the above strategies, for strategy 3 (Gradual change-over, clean break), it could make maintainance slightly easier during the 3.x cycle as both the WordPressVIPMinimum as well as the VIPGo standard could include that ruleset, lessening duplication.

It would also be beneficial if at some point in the future a new, different VIP platform based ruleset would be needed which would have a significant overlap with the Go ruleset for the externally included sniffs, as it would prevent having to maintain the same or very similar groups of inclusions in two or more places.

Then again, in a way, we'd be then setting things up for a potential event in the future, which may never happen, so it could just as easily be argued that having a VIPExternal ruleset is unnecessary.

---

Depending on preferred strategy I could work out a detailed roadmap of all steps which would need to be taken to implement that strategy.

What do you think ?

[rebeccahum]

Thank you for the well thought-out response, @jrfnl!

So, in case a standard rename would take place, I'd suggest something like VIPCS or WPVIP as a standard name for the standard containing the actual sniffs, though WordPressVIP is workable if needs be.

I'm a fan of using "WPVIP" as the standard name, as it is in line with our current domain (wpvip.com).

If a future iteration of the platform would require a new ruleset, we'd get into the same naming/message content/type/severity confusion as we are currently in as the new ruleset would need to overrule everything which is different from "Go".

I don't think we should worry about this since that the likeliness of that is yet to be determined.

In other words, I'd recommend sticking with two standards, split between 1) a standard with sniffs and no further customizations in the ruleset and 2) a platform specific ruleset, which would include external sniffs as well and possibly minor customizations for the notice severity of some messages.

Would 2) have to be platform specific if we want to move towards platform agnostic in our naming conventions? Could 1)'s purpose be there as a parent for inheritance?

The gradual change-over, clean break approach seems the best to me since it gives users time to update their references, while accounting for our need to reflect the status quo .