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
237 stars 40 forks source link

Deprecate WordPressVIPMinimum ruleset #600

Open rebeccahum opened 3 years ago

rebeccahum commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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:

...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:

rebeccahum commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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._

  1. 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.

  1. 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

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.

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:

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:

Advantages:

Disadvantages:

Strategy: Break and be done with it

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

Advantages:

Disadvantages:

Strategy: Gradual change-over, clean break

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

Advantages:

Disadvantages:

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 commented 3 years ago

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 .