Deprecate VIP ruleset, and remove from WordPress ruleset

[tomjn]

At the moment there's a VIP ruleset in the WPCS, but this is confusing, especially when VIPs and VIP itself make use of a ruleset located here named WordPressVIPMinimum. It's confusing for our clients and misleading for WPCS users.

Instead, VIP would like to move the ruleset to our own repo, or integrate it so that it's no longer necessary. At that point talking about the two should become a lot easier, and the possibility of migrating rules back to WPCS should be more feasible. We also have plans to change severity and messages specifically for the VIP ruleset so the extra flexibility of having all VIP stuff in a VIP repo would be good

Are there any particular notes implementation wise or suggestions to keep in mind?

[jrfnl]

@tomjn Could you give us a status update please ?

[tomjn]

When it got to the point were I would have to deprecate rather than remove things, the scope of what needed doing far outstripped the available time, so nothing further was done on that front, and I focused elsewhere. I haven't had the time to circle back to this, and I don't know when I will.

Having said that, feel free to burn the VIP ruleset with the fire of a thousand suns, with extreme prejudice. It's dead weight

[aristath]

While I agree with this one, the VIP ruleset does have some useful stuff in it...
Particularly the validation & sanitization rulesets

[tomjn]

I agree and they should be migrated into the other rules, but right now:

• Nobody at VIP uses the ruleset
• Our clients are told not to use it
• Anybody who does use these rules gets a lot of existing deprecation warnings about sniffs
• It's a source of confusion for all involved

We have the funeral party ready at hand, we just need someone to do the deed and put it out of its misery

[GaryJones]

@tomjn Are folks at VIP using https://github.com/Automattic/VIP-Coding-Standards/ and/or https://github.com/Automattic/VIP-Go-PHPCS-Blockers consistently then?

[tomjn]

We use https://github.com/Automattic/VIP-Coding-Standards/, which is ran on all code before it gets reviewed, all commits that arrive in SVN, and via CI for client git repos. If the clients don't run it which we encourage them to do, then it gets ran at our end

https://github.com/Automattic/VIP-Go-PHPCS-Blockers can be ignored and needs archiving, it's creation is more of an internal miscommunication, which became the WordPress-VIP-Go ruleset in the other repo

[jrfnl]

Ok, so, let's summarize and formulate an action-plan.

• PRs Issue 1157/Proposal 2: rename sniffs #1242 and PostsPerPage: split sniff #1260 already moved everything which is more generically useful out of the VIP sniff category (including the sanitization/validation sniff) and deprecated the old sniffs.
• While some of these sniffs are still only explicitly included in the VIP ruleset, they are automatically included in the WordPress ruleset (is as was).
• The only sniffs which are still left active in the VIP sniff category are very VIP specific sniffs and are not useful to the wider WP community.

I propose to make the following changes to be included in the 1.0.0 release:

• Change the WordPress ruleset to remove the included VIP ruleset from it and to exclude the VIP category from it.
• Add a deprecation notice to the remaining sniffs in the VIP category.
• Double-check that no VIP sniffs are used in the Core and Extra ruleset. (pretty sure they aren't).
• See about adding a deprecation notice prior to showing the run results when people explicitly use the VIP ruleset.
  Ref: https://github.com/squizlabs/PHP_CodeSniffer/wiki/Advanced-Usage#using-a-bootstrap-file
• Have these changes very explicitly mentioned in the changelog for the 1.0.0 release.
• Remove both the VIP ruleset as well as the VIP sniff category in version 2.0.0.

How does that sound? And if agreed, who will send in the PR?

[tomjn]

This sounds good but the steps needed to actually implement this are hazy, and the resources available VIP side limited

Change the WordPress ruleset to remove the included VIP ruleset from it and to exclude the VIP category from it.

Sounds straight forward, this is just removing bits of XML and updating the changelog correct?

Add a deprecation notice to the remaining sniffs in the VIP category.

I'm unclear on the mechanism for deprecation, for some of the sniffs it's clear that they were replaced, and so the replacements are included, then the originals are changed to something like this:

	<!-- Prevent deprecation notice when the sniff is not explicitely included. -->
	<rule ref="WordPress.VIP.TimezoneChange.DeprecatedSniff">
		<severity>0</severity>
	</rule>

But if a sniff has not been replaced, I'm unclear on the solution, or how the .DeprecatedSniff results in deprecation

Double-check that no VIP sniffs are used in the Core and Extra ruleset. (pretty sure they aren't).

Sounds straight forward

See about adding a deprecation notice prior to showing the run results when people explicitly use the VIP ruleset.
Ref: https://github.com/squizlabs/PHP_CodeSniffer/wiki/Advanced-Usage#using-a-bootstrap-file

hmmm how would one do this via the xml?

Have these changes very explicitly mentioned in the change log for the 1.0.0 release.
Remove both the VIP ruleset as well as the VIP sniff category in version 2.0.0.

What's the ETA for v2? Are we talking weeks, months, years? My personal opinion is that we invent a time machine and remove the ruleset in the past. I honestly believe this ruleset is damaging, and the longer it remains the higher the chances of new people finding it and using it.

It's already confusing just to talk about VIP rulesets without getting confused between this one and the others. If a deprecation notice is required it should be an ultra high priority to prevent new adoption of the ruleset, and bundled in the next release, be it a point release or a major release.

[jrfnl]

What's the ETA for v2? Are we talking weeks, months, years?

The next release is v 1.0.0 and will be released as soon as the last issues for it have been sorted, either next week or soon after - see #1388.
Version 2.0.0 is targeted for the end of the year, see #1389.

... the resources available VIP side limited
... it should be an ultra high priority to prevent new adoption of the ruleset, and bundled in the next release

In that case, I suggest VIP should prioritize freeing up some resources to action this issue so it can still be included in the 1.0.0 release.

[tomjn]

Version 2.0.0 is targeted for the end of the year,

:(

In that case, I suggest VIP should prioritize freeing up some resources to action this issue so it can still be included in the 1.0.0 release.

That's easier said than done, this ruleset poses a problem for all, not just VIP. I know you would prefer that VIP themselves removed the ruleset, but right now the knowledge to do so simply isn't there. If it was simply removal then it would have been done by now, but deprecation raises unanswered questions, and adds additional time and resourcing cost for unknowns and research.

[tomjn]

I've some suggestions:

• This issue is labelled as an enhancement, it should instead be considered a bug
• The VIP blocker label issues should all be closed as wontfix, referencing impending deprecation and this ticket, directing them to the actual VIP coding standards instead
• I would reconsider the final removal in 2.0. The actual removal of the VIP ruleset happening in 2019 or the more likely 2020, is an excessively long wait, especially for something that ideally would have been removed in 2016. A 2-4 month deprecation period followed by a 1.1 release would be more appropriate
• I've seen references to valuable sniffs, even after deletion those sniffs still exist in version control and have alternatives in the official VIP coding standard
• A July 2nd deadline at the moment is unreasonable, it's too soon if VIP are expected to do this, and problematic since July 4th celebrations mean any spare capacity we might have had is spent covering AFK time
• There aren't many VIP engineers with the knowledge to maintain PHPCS coding standards, and none to my knowledge capable of doing the things asked here at this time.

[jrfnl]

That's easier said than done, this ruleset poses a problem for all, not just VIP.

Sorry, but no. This is a problem for the commercial company Automattic.
For the open source community project WPCS, it is not a problem as what's there is not in our way.

Adding the advise as per #1403 and possibly removing the ruleset from the WordPress ruleset is all that's needed from a WPCS point of view. If people then still elect to use the VIP ruleset or individual sniffs from it, it's their choice.

I know for a fact that the Theme Review Team actually intend to use some of the VIP sniffs in the new Theme Checker, such as the ones regarding touching sessions and the admin bar. /cc @dingo-d @grappler
(If this ticket gets actioned, they will need to copy those sniffs to their fork).

I know you would prefer that VIP themselves removed the ruleset, but right now the knowledge to do so simply isn't there. If it was simply removal then it would have been done by now, but deprecation raises unanswered questions, and adds additional time and resourcing cost for unknowns and research.

Well, resources can take several forms, people or money. You cannot expect/demand that a volunteer from this project spends a few days to solve the problem of a commercial company for free.

This issue is labelled as an enhancement, it should instead be considered a bug

See my comment about point of view above, This is not a bug for WPCS.

The VIP blocker label issues should all be closed as wontfix, referencing impending deprecation and this ticket, directing them to the actual VIP coding standards instead

No, they should not be blindly closed as wont-fix.
These issues need reviewing and either closing or relabeling. Issue #461, for instance, could be considered a good idea for the Extra ruleset, so closing that without further discussion would IMHO be a bad idea.

N.B.: There is also the VIP Caution label. Issues labeled with that will also need reviewing.

• https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/labels/Component%3A%20VIP%20Blocker
• https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/labels/Component%3A%20VIP%20Caution

I would reconsider the final removal in 2.0. The actual removal of the VIP ruleset happening in 2019 or the more likely 2020, is an excessively long wait, especially for something that ideally would have been removed in 2016. A 2-4 month deprecation period followed by a 1.1 release would be more appropriate

No. WPCS uses SemVer and has said it would since it has started keeping a changelog in 2014.
Removing sniffs and/or rulesets is a breaking change and will need to be done in a major release.

The timing of the 2.0.0 release is being discussed in #1389. However, the interests of a commercial company should not be leading in that discussion. The interests of our users who may encounter breaking builds should be.

I've seen references to valuable sniffs, even after deletion those sniffs still exist in version control and have alternatives in the official VIP coding standard

Not sure what you intend to say with this. We all know that what's removed, will still be available in the project history, but that's not the point.

A July 2nd deadline at the moment is unreasonable, it's too soon if VIP are expected to do this, and problematic since July 4th celebrations mean any spare capacity we might have had is spent covering AFK time
There aren't many VIP engineers with the knowledge to maintain PHPCS coding standards, and none to my knowledge capable of doing the things asked here at this time.

We may or may not make that deadline and if a PR for this is expected within a week or two, that deadline can be moved, but we will release version 1.0.0 soon(ish).

If knowledge or people are the issue, see my remark above.

[jrfnl]

As a side-note: there is an open issue in PHPCS to make deprecating sniffs easier, but that won't be implemented until PHPCS 4.0 (if at all) and based on the above discussion, I can't imagine you'd want to wait until that time.
See: squizlabs/PHP_CodeSniffer#2061

[tomjn]

Sorry, but no. This is a problem for the commercial company Automattic.
For the open source community project WPCS, it is not a problem as what's there is not in our way.

Yes it's a problem for VIP, but I find it disingenuous to say it is only a problem for VIP. It was only yesterday I was handling a question on stack exchange regarding the VIP ruleset. It's in everybody's interests to Jettison the VIP ruleset, not just VIPs

Adding the advise as per #1403 and possibly removing the ruleset from the WordPress ruleset is all that's needed from a WPCS point of view. If people then still elect to use the VIP ruleset or individual sniffs from it, it's their choice.

That was not my understanding, as I understood it, PHP changes and bootstrap files and warnings when ran were needed, not just a Readme change

Well, resources can take several forms, people or money. You cannot expect/demand that a volunteer from this project spends a few days to solve the problem of a commercial company for free.

It's not just a commercial company's problem, it's your problem too, it's a UX problem. Telling us to go acquire them is easily said, but a 48 hour deadline plus a major national holiday is unreasonable, as is a potential years wait.

As for contracting or hiring somebody to make those changes, I've already made recommendations to do so, but that decision isn't mine to make, and is dependent on things I'm unaware of and out of my control.

However, the interests of a commercial company should not be leading in that discussion. The interests of our users who may encounter breaking builds should be.

My understanding is that without special measures the VIP ruleset already breaks builds with deprecation notices

No, they should not be blindly closed as wont-fix.
These issues need reviewing and either closing or relabeling. Issue #461, for instance, could be considered a good idea for the Extra ruleset, so closing that without further discussion would IMHO be a bad idea.

I agree that they need reviewing, but question that they should remain labelled as VIP. Perhaps the Component: Extras would be more appropriate for the issue cited. Eitherway expectations should be set

[dingo-d]

The theme ruleset uses 10 of VIP rules (sanitizations, admin bar removal, plugin menu slug, session usage and timezones).

If we would remove the VIP ruleset, we should either move some of the rules from the VIP to the Extras ruleset and see what could be moved to the planned WordPress-Theme ruleset (which we will 'defork' probably after 1.0.0. release).

[jrfnl]

That was not my understanding, as I understood it, PHP changes and bootstrap files and warnings when ran were needed, not just a Readme change

The clue is in my "all that's needed from a WPCS point of view".

The difference is between "discouraging usage of" (WPCS) or "removing altogether" (Automattic).
To remove it, more is needed as outlined previously.

Telling us to go acquire them is easily said, but a 48 hour deadline plus a major national holiday is unreasonable

This issue has been open since February. You've had four months which I think is quite reasonable.

My understanding is that without special measures the VIP ruleset already breaks builds with deprecation notices

That's incorrect and has already been solved.

[jrfnl]

The theme ruleset uses 10 of VIP rules

@dingo-d That's no longer true as a number of those sniffs have been moved out of VIP. Once TRTCS starts using WPCS 1.0.0 instead of 0.14.1, the number will go down significantly.

we should either move some of the rules from the VIP to the Extras ruleset

That's already been done. See #1261

[GaryJones]

Throwing an idea out:

• A 1.0.0 release, which removes the VIP ruleset and any associated sniffs that don't need to be kept, renames sniff categories and has all of the bug fixes currently in develop.
• A 0.15.0 release at the same time as 1.0.0, which keeps the VIP ruleset and sniffs intact and doesn't rename sniff categories. For those that want a few of the more recent bug fixes (unpicking all bug fixes from breaking changes may not be possible), but don't want to deal with BC-breaks requiring updating their phpcs.xml.dist files.

WPCS uses SemVer and has said it would since it has started keeping a changelog in 2014.
Removing sniffs and/or rulesets is a breaking change and will need to be done in a major release.

Since the project follows SemVer, 1.0.0 would be the next major release, and since we've been 0.x.y until now, we're covered by SemVer that "Anything may change at any time". In short, we don't need a period of deprecation for the VIP ruleset / sniffs, which I think is the difference of viewpoints here.

If the VIP team aren't using the ruleset of unique sniffs, I see no point in keeping them around for 1.0.0. I know Juliette is keen that we're good citizens, and that's what the 0.15.0 release would handle.

[jrfnl]

@GaryJones' suggestion could work. I would just like to point out though, that this would involve nearly as much work (if not more) in creating a separate 0.15.0 branch and release, as doing the VIP deprecation would.

So while it would solve the timing issue, it would still not solve the resources issue.

[tomjn]

I'm in favour of Garys proposal

[jrfnl]

@tomjn I'm glad to hear it. In that case, I look forward to seeing two PRs to that effect - one for the 0.15.0 release, one for the changes needed to the 1.0.0 release -, in time before we are ready to release 1.0.0.

I guess presuming you will make someone available part-time to handle all the support requests about "phpcs for WP does not work anymore" once the releases are out, would be a bit much ?

[tomjn]

I'm glad to hear it. In that case, I look forward to seeing two PRs to that effect - one for the 0.15.0 release, one for the changes needed to the 1.0.0 release -, in time before we are ready to release 1.0.0.

I will give it a best effort attempt, but chances are I'm going to be pulled away to help cover colleagues AFK for July 4th celebrations. The timescale is looking to be towards the end of next week assuming I can figure out all the parts on my own. I'll update here as I go

I guess presuming you will make someone available part-time to handle all the support requests about "phpcs for WP does not work anymore" once the releases are out, would be a bit much ?

Unlikely, but I expect all of these would involve a pre-written response with a link to the relevant release posts, this issue, and documentation. I can spend some time and do a sweep every few days to catch items, and I'll see if I can set up some sort of notification

With that in mind, what's you preferred approach to documenting this? I expect users of VIP sniffs would simply be told that they can use the 0.15 release, or rename the sniff used in their ruleset to instead reference the migrated version of the sniff should they want to use 1.0? In which case a table of deprecated sniffs and their replacements would be best placed somewhere

[jrfnl]

@tomjn The changelog for the 1.0.0 version will contain detailed information on all the changes in the 1.0.0 milestone, including information about all the breaking changes, and will be copy & pasted into the release notes for the 1.0.0 release,

As annotated in issue #1388, I've already prepared this changelog for all currently merged PRs in the milestone and will pull it once the last PRs for the milestone have either been merged or re-tagged - see action list in the same issue.

The 0.15.0 release will need its own changelog which I expect to be included with the 0.15.0 PR and the 1.0.0 changelog will need to be adjusted depending on what goes into 0.15.0.

[GaryJones]

The 1.0.0 milestone has a lot of tickets completed and a few outstanding, but I don't have a problem with moving the release date (which was arbitrarily picked) to late in July, or even early August. That would give us time to address this VIP ruleset issue correctly, and with calm supportive mindsets, rather than trying to rush it through and overlooking something.

A release date as late as August would still give sufficient timespan before a 2.0.0 release at the start of the year.

@jrfnl In terms of workload, what's your gut feeling about the percentage of 1.0.0 changes that would need un-doing to allow 0.15.0?

💡️ Having future BC-breaking commits messages start with [BC] would enable us to recognise these more easily in the future.

[jrfnl]

@GaryJones While I appreciate how you're trying to be the voice of reason in this discussion, I don't like the idea of moving the release date more than is strictly necessary.

1. All prep for the release has been done. To keep things up to date now, places an additional burden on me until the release which I only want to shoulder for as short a time as possible.
2. As the prep is done, delaying the release will bring more support questions from confused users as all sniff references to renamed sniffs have been adjusted everywhere, but won't be in effect yet if they use the last stable release.
3. The release is long overdue already.
4. If this is done via deprecation, there is no need to wait for version 1.0.0. Deprecation can be done in a minor release, the removal would just have to wait until the next major.
5. We've already waited four months since this was first discussed and the steps needed were outlined in PR Wpcs vip migration #1310 at that time. I'm not confident that delaying the release now will make a difference, other than delaying version 1.0.0 indefinitely.

As an alternative, I'd be OK with bringing the 2.0.0 release forward by a month or two as long as we release 1.0.0 as soon as possible. There should be at least four months between those two releases to give people time to adjust their rulesets and standards based on WPCS, but we can keep that to the minimum.

@jrfnl In terms of workload, what's your gut feeling about the percentage of 1.0.0 changes that would need un-doing to allow 0.15.0?

As I indicated before, my gut feeling is that having a separate 0.15.0 release will be just as much work, if not more, than doing the deprecations and such.

Having future BC-breaking commits messages start with [BC] would enable us to recognise these more easily in the future.

Future BC-breaks will be milestoned with a different major ;-)

[tomjn]

In that case, for 1.0.0, does anything need doing in addition to the deprecation notice in the readme?

[jrfnl]

In that case, for 1.0.0, does anything need doing in addition to the deprecation notice in the readme?

I think it would be good to remove the VIP ruleset and the VIP sniff category from the WordPress ruleset as I suggested here: #1403 (review).

Other than that, I think - for this issue - we should be good to go for 1.0.0.

It would be great if we could release a 1.1.0 within the next month or so which includes all the VIP deprecations.

[jrfnl]

@tomjn Is suggest closing this issue now all the sniffs and the ruleset have been deprecated. The removal will be done in 2.0.0.

[tomjn]

Done