Deprecate Remaining VIP sniffs #1410

tomjn · 2018-07-03T10:56:41Z

To the Files category, and deprecates the old sniff with a warning.

If this PR goes well I hope to do the same to all the other sniffs mentioned in #1409

jrfnl · 2018-07-03T11:33:20Z

@tomjn Glad to see the first PR for this.

This sniff in my opinion should not be moved, only deprecated. File system writes being forbidden is a VIP specific requirement which does not easily apply to other projects in the WP ecosphere, so this sniff is a candidate for complete removal in 2.0.0.

tomjn · 2018-07-03T11:37:07Z

Ah, my goal with this PR is mostly to get to grips with the deprecation process for a sniff. This is certainly an issue for VIP, I can't comment on other hosting platforms, but this usually pops up in any situation were the uploads folder doesn't exist on the file system, which is why VIP flags it. But ignoring that, I just want to make sure I'm not making a colossal mess in follow up PRs, so consider this a practice run :)

jrfnl · 2018-07-03T12:10:11Z

I can't comment on other hosting platforms

The WPCS ruleset is not for specific hosting platforms. Other hosters can - just like Automattic - create their own PHPCS standard based on WPCS and add additional sniffs to it to facilitate their needs.

consider this a practice run

👍

tomjn · 2018-07-04T23:17:49Z

yay travis is happy 🎉

jrfnl · 2018-07-05T00:02:09Z

@tomjn Do you mean that this PR is ready for review from your point of view ?

tomjn · 2018-07-05T08:33:37Z

I think I just need to move the content back into the original sniff so that it's deprecated not migrated, at which point it'll be complete, if all is good I can apply the same process to the other sniffs. I've set aside some time later today to go through the rest

…

On Thu, 5 Jul 2018 at 01:02, Juliette ***@***.***> wrote: @tomjn <https://github.com/tomjn> Do you mean that this PR is ready for review from your point of view ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1410 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADl5_Keb3w-3JUpq_pdI50dFzixhJLcks5uDVeDgaJpZM4VAud5> .

tomjn · 2018-07-06T13:29:34Z

@jrfnl the PR's ready for review, if all is good I'll do the same to the other sniffs mentioned in #1409

jrfnl

Hi @tomjn, the PR as it is now, is definitely in line with what has been discussed previously 👍

I've left a couple of remarks in line.

On top of that, there are two more things I'd like to discuss/remark:

IMO it would be good to do the complete VIP deprecation in one go, in the same version of WPCS.
Looking at your own time schedule - will you be ready with all the PRs in the next few days/ by the middle of next week ? If so, then this can go into 1.0.0.
If not or if you are unsure, targeting 1.1.0 might make more sense (to be released as soon as all PRs regarding the deprecation of VIP are ready & merged).
In that case, the version number used in @since and @deprecated tags should be adjusted.
I'm not sure I see the value in having 7 commits for this - relatively small - change. Is there a pertinent reason not to squash them into one ?

jrfnl · 2018-07-06T15:47:29Z

WordPress/Sniffs/VIP/FileSystemWritesDisallowSniff.php

+	);
+
+	/**
+	 * Don't use.


I think this needs a better description. The sniff is still active, just deprecated.

Agreed, this is mostly due to copy paste from another deprecated VIP sniff

jrfnl · 2018-07-06T15:51:42Z

WordPress/Sniffs/VIP/FileSystemWritesDisallowSniff.php

+	/**
+	 * Don't use.
+	 *
+	 * @deprecated 1.0.0


I would expect a @since 1.0.0 Added to allow for throwing the deprecation notices. tag here instead.

The method itself is not deprecated, the sniff is.

In that respect, I'm missing a @deprecated tag in the class docblock.

+1 will change

jrfnl · 2018-07-06T15:57:19Z

WordPress/Sniffs/VIP/FileSystemWritesDisallowSniff.php

+	 *
+	 * @return void|int
+	 */
+	public function process_token( $stackPtr ) {


For consistency with other sniffs which have getGroups() and process_token() function declarations, the getGroups() method normally goes first, the process_token() method comes after it.
This is in line with the order in which the functions are called - getGroups() is called from within the register() method -, as well as the order used in the abstract sniff which this sniff extends.

jrfnl · 2018-07-06T16:11:11Z

WordPress-VIP/ruleset.xml

+	</rule>
+
+	<!-- Prevent duplicate messages from deprecated sniff. -->
+	<rule ref="WordPress.VIP.FileSystemWritesDisallow">


This would disable the sniff completely in contrast to deprecating it. I think this block should be removed.

jrfnl · 2018-07-06T19:42:19Z

@tomjn Just scanned through the latest update related to the FileSystemWrites sniff and - ~~except for missing a @deprecated tag in the class docblock~~ [Edit: appears my file view was out of date, it's showing now 👍 ] -, all seems good now.

jrfnl · 2018-07-06T19:44:29Z

WordPress/Sniffs/VIP/FileSystemWritesDisallowSniff.php

+	/**
+	 * This sniff is active, but it's deprecated, handle the deprecation notices
+	 *
+	 * @deprecated 1.0.0 Added to allow for throwing the deprecation notices.


@deprecated => @since

jrfnl · 2018-07-06T19:48:04Z

@tomjn I can see you are updating the PR with further changes. Could you please ping me when you are ready for the next review ?

Requested changes were made. Not approving yet as more commits are still being added to this PR.

tomjn · 2018-07-07T19:48:03Z

@jrfnl So I believe I'm ready for review, my main concern though is I'm unsure why the one travis check is failing :/ Any help appreciated

GaryJones · 2018-07-07T22:42:01Z

my main concern though is I'm unsure why the one travis check is failing :/ Any help appreciated

The report says:

ERROR: Referenced sniff "WordPress.VIP.AdminBarRemovalSniff.DeprecatedSniff" does not exist

jrfnl · 2018-07-07T23:13:36Z

WordPress.VIP.AdminBarRemovalSniff.DeprecatedSniff

Should be: WordPress.VIP.AdminBarRemoval.DeprecatedSniff

👉 Once you fix this one, you'll start getting the same error for most of the others. When referencing sniffs in a ruleset, you need to leave out the Sniff at the end of the sniff name.

tomjn · 2018-07-08T00:12:46Z

Gah such an obvious mistake, fixed

jrfnl · 2018-07-08T03:23:28Z

@tomjn The build still isn't passing. I suspect this is because of trailing whitespace within the touched XML file.

jrfnl

@tomjn Ok, I've reviewed the changes and left most feedback inline.

As the AbstractVariableRestrictionsSniff is now no longer used by any non-deprecated sniffs, I would like to suggest to deprecated the abstract class as well.

No warnings need to be thrown for that, it only needs an annotated @deprecated tag in the class docblock.
Looks like the Sniff::has_html_open_tag() method from the Sniff class is only used by the now deprecated AdminBarRemoval sniff, so it should probably be deprecated as well.
I'm presuming you meant to amend commit c1736c2 with commit e6b90a2 ? If so, something went wrong there. You may want to have a look at that.
As it looks like this PR will be ready in time for the WPCS 1.0.0 release, please incorporate the changes from Add note regarding WordPress-VIP ruleset #1403 + the additional changes suggested in the comments in that PR.
I've confirmed that there is trailing whitespace in the VIP XML ruleset. Please tidy it up.

N.B. The AbstractArrayAssignmentRestrictionsSniff can not be deprecated as it is still in use by the DB.SlowDBQuery and the WP.PostsPerPage sniffs which will not be deprecated.

jrfnl · 2018-07-08T03:25:01Z

WordPress/Sniffs/VIP/AdminBarRemovalSniff.php

+	 *
+	 * @var bool
+	 */
+	public $error = true;


I'm presuming this is a copy/paste error as I don't understand why this property would be added.
Please remove it here and everywhere else where it was inadvertently copied into.

jrfnl · 2018-07-08T03:25:46Z

WordPress/Sniffs/VIP/AdminBarRemovalSniff.php

@@ -71,15 +94,15 @@ class AdminBarRemovalSniff extends AbstractFunctionParameterSniff {
 	 */
 	protected $target_css_properties = array(
 		'visibility' => array(
-			'type'  => '!=',
+			'type' => '!=',


Please undo this formatting change, here and in the lines below it.

jrfnl · 2018-07-08T03:30:17Z

WordPress/Sniffs/VIP/AdminBarRemovalSniff.php

+			);
+		}
+
+		if ( false === $this->thrown['FoundPropertyForDeprecatedSniff'] ) {


The FoundPropertyForDeprecatedSniff error message is only needed when a sniff has public properties which could be set/changed from within a custom ruleset.

Before throwing the warning, you need to actually check whether the properties have been set from the ruleset, i.e. whether the values for public properties this class uses have been changed.

For this sniff, the only relevant property is the $remove_only property. The warning should only be thrown when the value for that property is something other than true.

See: https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/wiki/Customizable-sniff-properties#admin-bar-visibility-manipulations

jrfnl · 2018-07-08T03:35:27Z

WordPress/Tests/VIP/AdminBarRemovalUnitTest.php

 	 * @return array <int line number> => <int number of warnings>
 	 */
-	public function getWarningList() {
-		return array();
+	public function getWarningList( $testFile = '' ) {


This function needs some work.

Regarding the switch: one of the two cases is wrong - both currently refer to the AdminBarRemovalUnitTest.css file.

Also, all cases contain a return, so the return statement underneath the switch will never be reached.

Lastly, the alignment of the double arrows in the arrays in the case statements is off.

Either way, the function needs to be changed.

jrfnl · 2018-07-08T03:50:12Z

WordPress/Sniffs/VIP/FileSystemWritesDisallowSniff.php

+			);
+		}
+
+		if ( ! empty( $this->exclude )


This sniff also has an error property which can be changed from within a custom ruleset, so the condition needs to check the value of that property too.

See: https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/wiki/Customizable-sniff-properties#error-type-for-file-system-writes

Unit tests need to be added to the unit test case file with the non-default property values to make sure the deprecated property warning is thrown correctly.

jrfnl · 2018-07-08T04:23:06Z

WordPress/Sniffs/VIP/SessionFunctionsUsageSniff.php

+			&& false === $this->thrown['FoundPropertyForDeprecatedSniff']
+		) {
+			$this->thrown['FoundPropertyForDeprecatedSniff'] = $this->phpcsFile->addWarning(
+				'The "WordPress.VIP.SessionFunctionsUsageSniff" sniff has been deprecated. Please update your custom ruleset.',


👉 See previous remark - remove Sniff

jrfnl · 2018-07-08T04:23:33Z

WordPress/Sniffs/VIP/SessionVariableUsageSniff.php

@@ -46,6 +69,24 @@ public function register() {
 	 * @return void
 	 */
 	public function process_token( $stackPtr ) {
+		if ( false === $this->thrown['DeprecatedSniff'] ) {
+			$this->thrown['DeprecatedSniff'] = $this->phpcsFile->addWarning(
+				'The "WordPress.VIP.SessionVariableUsageSniff" sniff has been deprecated. Please update your custom ruleset.',


👉 See previous remark - remove Sniff

jrfnl · 2018-07-08T04:24:22Z

WordPress/Sniffs/VIP/SuperGlobalInputUsageSniff.php

+	 */
+	private $thrown = array(
+		'DeprecatedSniff'                 => false,
+		'FoundPropertyForDeprecatedSniff' => false,


This sniff does not have any public properties which can be changed, so the FoundPropertyForDeprecatedSniff entry can be removed.

jrfnl · 2018-07-08T04:24:38Z

WordPress/Sniffs/VIP/SuperGlobalInputUsageSniff.php

+			);
+		}
+
+		if ( ! empty( $this->exclude )


This sniff does not have any public properties which can be changed, so this complete code block for the FoundPropertyForDeprecatedSniff warning can be removed.

jrfnl · 2018-07-08T04:25:35Z

WordPress/Sniffs/VIP/SuperGlobalInputUsageSniff.php

@@ -43,6 +66,24 @@ public function register() {
 	 * @return void
 	 */
 	public function process_token( $stackPtr ) {
+		if ( false === $this->thrown['DeprecatedSniff'] ) {
+			$this->thrown['DeprecatedSniff'] = $this->phpcsFile->addWarning(
+				'The "WordPress.VIP.SessionVariableUsageSniff" sniff has been deprecated. Please update your custom ruleset.',


References the wrong sniff: WordPress.VIP.SessionVariableUsageSniff should be WordPress.VIP.SuperGlobalInputUsage.

tomjn · 2018-07-09T18:20:08Z

I've addressed a lot of the feedback, integrated the changes suggested in #1403, todo items:

check each sniff for parameters and add appropriate FoundPropertyForDeprecatedSniff notices
update unit tests with new checks, not entirely sure how to do that though for sniff parameters
Figure out squashing c1736c2 and e6b90a2 into eachother

…ons in 2 sniffs

tomjn · 2018-07-10T12:28:22Z

So an updated TODO list:

adjust unit tests
add checks for parameters on sniffs
amend the VIP deprecation paragraph

jrfnl · 2018-07-12T20:43:42Z

@tomjn Just checking in... will you be able to finish the work on this PR tomorrow so we can include it in the WPCS 1.0.0 release ? 🙏

tomjn · 2018-07-13T11:47:18Z

Yesterday was my birthday so I took some time AFK, it's on my todo list though for later today

jrfnl · 2018-07-13T13:46:43Z

@tomjn 👍 and happy birthday and many happy returns! 🎉

tomjn · 2018-07-13T16:47:49Z

As far as I can tell, all the parameters for the VIP sniffs are already handled, moving on to unit tests

tomjn · 2018-07-13T17:10:30Z

For reference I'm using the VIP Slow DB Query tests as a basis since the majority of what I need to do work wise is near identical

<?php

// @codingStandardsChangeSetting WordPress.VIP.SlowDBQuery exclude something
$query = 'foo=bar&meta_key=foo';

// @codingStandardsChangeSetting WordPress.VIP.SlowDBQuery exclude false

…it tests

tomjn · 2018-07-14T00:36:15Z

The travis builds seem a bit.. off, hmm, only thing to do is the final wording of the VIP deprecation paragraph

jrfnl · 2018-07-14T01:34:29Z

The travis builds seem a bit.. off

I restarted the one build which errored out and Travis is ok now.

jrfnl · 2018-07-15T04:02:23Z

@tomjn How can we help you to finish the PR ?

tomjn · 2018-07-15T22:57:03Z

@jrfnl It's just the paragraph for the readme that needs doing, but my mind is blank. I'm also pretty tired out from the last week and have a busy week ahead of me. Chances are I won't be able to do much more on this for a few days, and that all that's actually needed is less than 20 characters of prose.

jrfnl · 2018-07-15T23:10:32Z

@tomjn Hmm... maybe @GaryJones can come up with some good phrasing ?
The 1.0.0 release is waiting on this PR and is already two weeks overdue now, so it would really help if we can finish this off now.

GaryJones · 2018-07-16T07:11:12Z

I'll come up with something today.

GaryJones · 2018-07-16T07:21:47Z

@jrfnl If the rest of the PR is good, then let's merge it, and I'll re-do my other PR for the readme - that way, we won't be waiting on Tom to merge readme changes into his branch before merging his branch into develop.

GaryJones · 2018-07-16T08:15:37Z

This WPCS package contains the sniffs for another ruleset, WordPress-VIP. This ruleset was originally intended to aid with the WordPress.com VIP coding requirements, but this is no longer used or recommended by the WordPress.com VIP team or their clients, since they prefer to use their official VIP coding standards ruleset instead.

Before WPCS 1.0.0, the WordPress-VIP ruleset was included as part of the complete WordPress ruleset. As of 1.0.0 the WordPress-VIP ruleset is not part of the WordPress ruleset, and it is deprecated. The remaining WordPress-VIP sniffs may still be referenced in custom rulesets, so to maintain some backwards compatibility, they will remain in WPCS until 2.0.0. See #1309 for more information.

tomjn · 2018-07-16T11:00:20Z

@jrfnl I've updated the PR with the version @GaryJones wrote, thanks for the wordsmithing :)

jrfnl

Let's merge this baby!

jrfnl previously requested changes Jul 6, 2018

View reviewed changes

tomjn force-pushed the vip-migrate-filesystem-write-disallow-sniff branch 2 times, most recently from cc96a0d to 5fb6bb5 Compare July 6, 2018 19:37

jrfnl reviewed Jul 6, 2018

View reviewed changes

tomjn force-pushed the vip-migrate-filesystem-write-disallow-sniff branch from 5fb6bb5 to 79170a3 Compare July 6, 2018 21:02

tomjn changed the title ~~Migrates the VIP Filesystem writes disallow sniff~~ Deprecate Remaining VIP sniffs Jul 7, 2018

tomjn force-pushed the vip-migrate-filesystem-write-disallow-sniff branch from f5d83d8 to e62e6cc Compare July 8, 2018 00:11

jrfnl requested changes Jul 8, 2018

View reviewed changes

jrfnl added this to the 1.0.0 milestone Jul 8, 2018

jrfnl added Type: Cleanup Status: Review ready Status: Awaiting feedback labels Jul 8, 2018

tomjn force-pushed the vip-migrate-filesystem-write-disallow-sniff branch from bea9d9f to f1bbf24 Compare July 9, 2018 18:02

tomjn force-pushed the vip-migrate-filesystem-write-disallow-sniff branch from 8ace487 to e62bf21 Compare July 9, 2018 19:01

tomjn added 4 commits July 10, 2018 13:08

unit test fixes

ad1c014

Fixes stray whitespace characters in the WP ruleset, and yoda conditi…

9c1e44f

…ons in 2 sniffs

Apply spelling and grammar corrections to VIP deprecations

e3206b7

merges the xml comment in the VIP ruleset into the description

20494c4

tomjn force-pushed the vip-migrate-filesystem-write-disallow-sniff branch from a3b7b84 to 20494c4 Compare July 10, 2018 12:08

jrfnl mentioned this pull request Jul 10, 2018

Release 1.0.0 #1388

Closed

10 tasks

tomjn force-pushed the vip-migrate-filesystem-write-disallow-sniff branch from 14e6919 to 4573545 Compare July 13, 2018 17:50

test the exclude parameter deprecation in the deprecated VIP sniff un…

50c3471

…it tests

tomjn force-pushed the vip-migrate-filesystem-write-disallow-sniff branch from 4573545 to 50c3471 Compare July 13, 2018 23:21

GaryJones mentioned this pull request Jul 16, 2018

Add note regarding WordPress-VIP ruleset #1403

Closed

Updated the VIP deprecation notice in the readme to read better

50c6d77

jrfnl approved these changes Jul 16, 2018

View reviewed changes

jrfnl merged commit 58b4c23 into WordPress:develop Jul 16, 2018

GaryJones removed Status: Review ready Status: Awaiting feedback labels Aug 21, 2018

Deprecate Remaining VIP sniffs #1410

Deprecate Remaining VIP sniffs #1410

Conversation

tomjn commented Jul 3, 2018

jrfnl commented Jul 3, 2018

tomjn commented Jul 3, 2018

jrfnl commented Jul 3, 2018

tomjn commented Jul 4, 2018

jrfnl commented Jul 5, 2018

tomjn commented Jul 5, 2018 via email

tomjn commented Jul 6, 2018

jrfnl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrfnl commented Jul 6, 2018 • edited Loading

Choose a reason for hiding this comment

jrfnl commented Jul 6, 2018

tomjn commented Jul 7, 2018 • edited Loading

GaryJones commented Jul 7, 2018

jrfnl commented Jul 7, 2018 • edited Loading

tomjn commented Jul 8, 2018

jrfnl commented Jul 8, 2018 • edited Loading

jrfnl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomjn commented Jul 9, 2018

tomjn commented Jul 10, 2018

jrfnl commented Jul 12, 2018

tomjn commented Jul 13, 2018

jrfnl commented Jul 13, 2018

tomjn commented Jul 13, 2018

tomjn commented Jul 13, 2018

tomjn commented Jul 14, 2018

jrfnl commented Jul 14, 2018

jrfnl commented Jul 15, 2018

tomjn commented Jul 15, 2018 • edited Loading

jrfnl commented Jul 15, 2018

GaryJones commented Jul 16, 2018

GaryJones commented Jul 16, 2018

GaryJones commented Jul 16, 2018 • edited Loading

tomjn commented Jul 16, 2018

jrfnl left a comment

Choose a reason for hiding this comment

jrfnl commented Jul 6, 2018 •

edited

Loading

tomjn commented Jul 7, 2018 •

edited

Loading

jrfnl commented Jul 7, 2018 •

edited

Loading

jrfnl commented Jul 8, 2018 •

edited

Loading

tomjn commented Jul 15, 2018 •

edited

Loading

GaryJones commented Jul 16, 2018 •

edited

Loading