Full property visibility/staticness review & fixing of custom property merging #841

jrfnl · 2017-02-11T07:30:45Z

Primarily, this PR is a review of all sniff class properties and their handling.

I've broken up the PR into a number of isolated commits for easier review & having a clearer history for the various affected files.

tl;dr

This PR fixes/addresses a number of issues:

bullet 2 from issue The future of the WordPress_Sniff class #765 - static class properties in the WP_Sniff class
a variety of buggyness in the merging of custom lists with pre-set lists - see Fix bug in WordPress_Sniffs_VIP_DirectDatabaseQuerySniff #786, Fix custom properties whitelist in valid variable name sniff #839 and comments in the The future of the WordPress_Sniff class #765 thread
a number of issues which need to be fixed for Add wiki page with info on ruleset customizations available ? #614, i.e. some properties which are still set to public, but really should be made protected as changing these from the ruleset would break the sniff or at the very least break the intention of the sniff.
missing unit tests for custom sniff property handling

Pre-amble

Sniff properties which are public can be overruled via a custom ruleset.
Sniff properties should therefore only be public if that is the intended behaviour. Otherwise a more restrictive visibility should be used.

Changes to the visibility of properties may break backward compatibility for some users, but only if they were doing_it_wrong in the first place, so I'm not concerned about this.

Properties set via the ruleset will be handled by PHPCS as follows:

Properties with the type=array attribute - will be converted to arrays. PHPCS has the ability to handle key/value arrays if the property is set using the following syntax: key=>value,key2=>value2
All other properties will be treated as strings.
If the string value of a property is true or false, the property value will be cast to a boolean value before it's set.
Setting a property in a ruleset will overrule the previously set value. This also means that if two rulesets have conflicting values for a property, the value received from the last ruleset parsed by PHPCS will be the value used.

What this PR changes/fixes

I've reviewed every single sniff class property for visibility & static vs non-static and changed them where appropriate.
For the properties where custom lists are being merged with pre-set lists, I've added a helper function to handle the merging.
This helper function is a little bit "magical".
- It will correctly handle custom properties passed as comma delimited strings as well as arrays. Basically being forgiving to end-users who forget to set the type=array bit when adding a property in a custom ruleset.
- If anything else is passed it will disregard the value of the custom property.
- If a base array to merge the custom property with is passed, it expects the base array to be set up as value => true to allow for using isset() instead of in_array(). Only one existing array needed to be changed to allow for this $test_class_whitelist.
- When merging a custom array with a base array, the custom array will be flipped, so the values will be added as keys to be in line with the above (can be turned off by passing false as the third param).
- Any custom keys added to the base array through the merge will be given the value false. This has no impact when using isset(), but allows us to filter out anything added for a previous custom property value if the custom property would be overruled inline in a file. This in turn allows us to unit test the custom properties.
- For more detail, see the function documentation
I've implemented use of this new utility function where appropriate.
For the custom array property merging issues, most sniffs used to add the custom values to the static pre-set arrays and only added these once for efficiency, using a $added_custom_properties setting to keep track of whether the custom properties had been merged in. Some of these static pre-set properties however would be used by several sniffs and could therefore be using a "contaminated" value. Also, the fact that the custom properties were only merged in once, prevented changing these values inline using the @codingStandardsChangeSetting syntax, which prevented unit testing them.
I've fixed this by:
- Turning the $added_custom_properties property into an array to keep track of the previous merged in value(s) instead of being a true/false toggle.
- This allows us to avoid merging the same values time & time again (efficiency), while still allowing the value to be changed on the fly. This minimizes the performance impact of the change, while allowing more flexibility and greater compliance with the default capabilities of PHPCS.

Additional notes:

The PHPCS 2.x minimum requirement is PHP 5.1. The new utility function however uses the array_fill_keys() function which was only introduced in PHP 5.2. This means that the minimum PHP requirement for WPCS has to go up to PHP 5.2. Considering that's an ancient version, I don't think this will cause any issues. Just to be sure, I have added this info to the requirements section of the readme.
I've corrected the default value of one property - was float, should have been string ($minimum_supported_version in WP/DeprecatedFunctions). This property is used in a version_compare(), so string is the expected value type anyway and would be the type if the property is overruled from the ruleset as well.
Reviewed the internal handling of properties.
- ControlStructureSpacing / blank_line_check and blank_line_after_check: PHPCS handles the casting to boolean already. For the compares, made sure they are done against "our" defaults, so that even if incorrectly set, the additional boolean cast is not necessary.
- ControlStructureSpacing / spaces_before_closure_open_paren: moved the cast to int up instead of doing it three times.
Implemented use of the exclude property in the WPCS rulesets, includes correcting an invalid (outdated) exclusion in the Extra ruleset.
Removed the $supportedTokenizers property in the WordPress_Sniffs_WhiteSpace_ControlStructureSpacingSniff as it was set to the default value used by PHPCS anyway.
I've moved the custom list merging to a separate function in the relevant sniffs.
Where appropriate, I've moved the merging of custom properties to a later point in the code flow to only do so if the properties are needed.
CSRF/Nonce sniff: This sniff uses the is_only_sanitized() method which calls the is_sanitized() method which uses the sanitization function lists, but there was no way to enhance them. Added the relevant custom properties to enable enhancing them for that specific sniff.
DirectDatabaseQuery sniff: This sniff did so far not extend the WP_Sniff class, but should do so within the new structure. Changed the sniff to leverage the properties of the WP_Sniff class as well.

The PR includes unit tests for all custom properties which weren't unit tested before now.

Question:

Does anyone remember whether there was a reason to make the CSRF/Nonce sniff $errorForSuperGlobals and $warnForSuperGlobals properties public ? That was the one set of properties I was in doubt about changing the visibility for.

Open issue/question:

What should be the value for the exclude property in the overall WordPress ruleset for the WordPress.PHP.DiscouragedPHPFunctions sniff ? Or should nothing be excluded ?

The Extra ruleset excludes create_function, the VIP ruleset excludes obfuscation. Based on that, I would suggest adding them both to the WordPress ruleset to maintain the same behaviour as before.

Technical details:

Previously, the excludes were done via the <exclude.. syntax, not by using the exclude property. As the WordPress ruleset includes both these ruleset, both excludes where set for the overall ruleset.
With this PR, the excludes will be done via the exclude property instead. The properties in ruleset are not cumulative, but overrule each other. That means that in effect the WordPress ruleset would now only exclude the obfuscation group as that's the value set in the last ruleset included (VIP).
Setting the exclude property to both from within the WordPress ruleset will maintain the old behaviour.
However, a case could be made to exclude nothing if the overall WordPress ruleset is used, which is why I'm making a point of putting this up for discussion.

Closes #765
Closes #839

…set with an existing array. This method is specifically intended to handle PHPCS custom properties. This function allows for resetting as follows: * If a custom property is changed during a PHPCS run, the base array will be reset to its original value before the new custom property value is merged in. * If both a `$base` as well as a `$custom` parameter are passed and the `$custom` property value is boolean (`false`/`true`), the $base property will be returned. This is especially useful during unit testing where you want to be able to add some custom functions and then reset to the original base array afterwards. This is now possible by passing `false`. * Sometimes you want to just take advantage of the handling of comma delimited lists this function provides and overwrite the original value with the value returned by this function. That's why the `$base` parameter is optional and defaults to an empty array. If it was not, you could only add to the `$base` array and never overwrite it properly.

…ome places.

…ustom list merge handling.

* Change standard WP function list properties from static to non-static and change visibility to protected. * Change `$addedCustomFunctions` property to non-static and change to array format. * Split off the list merging to a separate function * Use new `merge_custom_array()` utility function * Move the list merging to a later point in time just before the lists are used. No need to do this earlier.

* Add two more custom function properties. This sniff uses the `is_only_sanitized()` method which calls the `is_sanitized()` method which uses the sanitization function lists, but there was no way to enhance them. * Change standard WP function list properties from static to non-static and change visibility to protected. * Change `$addedCustomFunctions` property to non-static, change visibility to protected and change to array format. * Split off the list merging to a separate function * Use new `merge_custom_array()` utility function * Move the list merging to a later point in time just before the lists are used. No need to do this earlier.

* Change standard WP function list properties from static to non-static and change visibility to protected. * Change `$addedCustomFunctions` property to non-static, change visibility to protected and change to array format. * Split off the list merging to a separate function * Use new `merge_custom_array()` utility function

* Extend the WordPress_Sniff * Revert the addition of the private list properties which was part of an earlier version of this fix (unreleased). * Change standard WP function list properties from static to non-static and change visibility to protected. * Change `$addedCustomFunctions` to array format. * Change `$methods` property to non-static. * Split off the list merging to a separate function * Use new `merge_custom_array()` utility function * Move the list merging to a later point in time just before the lists are used. No need to do this earlier.

* Change visibility for `$whitelisted_mixed_case_member_var_names` property to `protected`. * Copy in `merge_custom_array()` as this class extends an upstream sniff and therefore can't use the version include in `WordPress_Sniff`. * Use new `merge_custom_array()` utility function * Move the list merging to a later point in time just before the lists are used. No need to do this earlier.

…non-static.

Sniff properties which are `public` can be overruled via a custom ruleset. Sniff properties should therefore only be `public` if that is the intended behaviour. Otherwise a more restrictive visibility should be used. Changes to the visibility of properties *may* break backward compatibility for some users, but only if they were **_doing_it_wrong_** in the first place, so I'm not concerned about this. Notes: * Changed the visibility of some properties which shouldn't be changeable through the ruleset from `public` to `protected`. * Corrected the default value of one property - was float, should have been string. This property is used in a `version_compare()`, so string is the expected value type anyway. * Reviewed the internal handling of properties. - ControlStructureSpacing / `blank_line_check` and `blank_line_after_check`: PHPCS handles the casting to boolean already. For the compares, made sure they are done against "our" defaults, so that even if incorrectly set, the additional boolean cast is not necessary. - ControlStructureSpacing / `spaces_before_closure_open_paren`: moved the cast to int up instead of doing it three times. * Implemented use of the `exclude` property in the WPCS rulesets, includes correcting an invalid (outdated) exclusion in the `Extra` ruleset. * Removed the `$supportedTokenizers` property in the `WordPress_Sniffs_WhiteSpace_ControlStructureSpacingSniff` as it was set to the default value used by PHPCS anyway.

grappler · 2017-02-11T13:51:41Z

Why do we need to change from <exclude.. syntax to the exclude property?

JDGrimes

Looks good.

Yes, I did a performance check. The merge_custom_array() method might add a slight time hit. Potentially also a < 1 MB memory increase (2%) from these changes. I'm not worried by it. 😄

JDGrimes · 2017-02-11T13:25:30Z

WordPress/Tests/XSS/EscapeOutputUnitTest.inc

@@ -182,3 +182,27 @@ _deprecated_hook( 'some_filter', '1.3.0', esc_html__( 'The $arg is deprecated.'
 _deprecated_hook( "filter_{$context}", '1.3.0', __( 'The $arg is deprecated.' ), sprintf( __( 'Some parsed message %s', $variable ) ) ); // Bad.

 echo add_filter( get_the_excerpt( get_the_ID() ) ; // Bad, but ignored as code contains a parse error.
+
+


Extra blank line here.

JDGrimes · 2017-02-11T13:35:36Z

WordPress/Sniffs/CSRF/NonceVerificationSniff.php

 	 *
 	 * @var array
 	 */
-	public $errorForSuperGlobals = array( '$_POST', '$_FILE' );
+	protected $errorForSuperGlobals = array( '$_POST', '$_FILE' );


I originally made these public so that they could be modified in custom rulesets. I don't know if anybody is using that feature, but it was intentional. The reason for letting them be completely overridden rather than just merging custom settings with the default lists is so that values could be removed and not just added. I haven't actually used that feature though, and I doubt anybody else has, so it is fine with me if we change these to protected.

Got it. Thanks for explaining the history. I'm open to leaving these public and adding them to the upcoming wiki page if you prefer.

jrfnl · 2017-02-11T15:58:15Z

Why do we need to change from <exclude.. syntax to the exclude property?

I should probably have changed this already in #814. The errorcodes for these specific messages have changed, so the "old" codes won't work anymore.
I could have changed it over to the "new" codes, but especially for obfuscation that would mean adding a lot more codes.
I also figured that leading by example with using the property would be a good thing.

grappler · 2017-02-11T16:18:36Z

WordPress/ruleset.xml

@@ -6,4 +6,10 @@
 	<rule ref="WordPress-Docs"/>
 	<rule ref="WordPress-Extra"/>
 	<rule ref="WordPress-VIP"/>
+
+	<rule ref="WordPress.PHP.DiscouragedPHPFunctions">


I would add a comment here explaining why we are adding an exception or referencing to that they have been excluded from their respective rulesets.

Good point. Done.

jrfnl · 2017-02-25T09:24:38Z

@GaryJones Have you got an opinion on this PR or should I remove you as a reviewer ?

GaryJones · 2017-02-25T09:45:35Z

Have you got an opinion on this PR or should I remove you as a reviewer ?

Reviewed and approved.

jrfnl · 2017-02-25T09:54:51Z

@GaryJones Great! Thanks.

jrfnl added 14 commits February 10, 2017 15:38

Implement use of the new merge_custom_array() utility function in s…

1c5ac93

…ome places.

Fix existing unit tests for custom properties to deal with improved c…

3efac5e

…ustom list merge handling.

Add unit tests testing the use of the custom properties

8e76313

DirectDatabaseQuery: Leverage that the class now extends WordPress_Sniff

ef79249

Change other sniff specific static properties to non-static.

69fcf87

WP Sniff: Change the remaining public static properties to protected …

a32886a

…non-static.

Add note about minimum PHP version to the readme

c09c0b8

jrfnl added Status: Review ready Type: Bug Type: Cleanup labels Feb 11, 2017

jrfnl added this to the 0.11.0 milestone Feb 11, 2017

jrfnl requested review from GaryJones and JDGrimes February 11, 2017 07:30

This was referenced Feb 11, 2017

Fix custom properties whitelist in valid variable name sniff #839

Closed

Add process() method to WordPress_Sniff … #833

Merged

JDGrimes approved these changes Feb 11, 2017

View reviewed changes

grappler reviewed Feb 11, 2017

View reviewed changes

Document overall ruleset & remove extraneous blank line.

433b410

grappler mentioned this pull request Feb 17, 2017

Prevent erroneous dropping of customPropertiesWhitelist array on ValidVariableName sniff #846

Closed

GaryJones approved these changes Feb 25, 2017

View reviewed changes

JDGrimes merged commit 543bd68 into develop Feb 25, 2017

JDGrimes deleted the feature/better-custom-property-handling branch February 25, 2017 13:51

JDGrimes mentioned this pull request Feb 25, 2017

Only deprecated customVariablesWhitelist seems to work in WordPress.NamingConventions.ValidVariableName #852

Closed

jrfnl mentioned this pull request Mar 6, 2017

Changelog for version 0.11.0 #866

Merged

GaryJones removed the Status: Review ready label Aug 21, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Full property visibility/staticness review & fixing of custom property merging #841

Full property visibility/staticness review & fixing of custom property merging #841

jrfnl commented Feb 11, 2017 •

edited

grappler commented Feb 11, 2017 •

edited

JDGrimes left a comment

JDGrimes Feb 11, 2017

jrfnl Feb 12, 2017

JDGrimes Feb 11, 2017

jrfnl Feb 11, 2017

jrfnl commented Feb 11, 2017 •

edited

grappler Feb 11, 2017

jrfnl Feb 12, 2017

jrfnl commented Feb 25, 2017

GaryJones commented Feb 25, 2017

jrfnl commented Feb 25, 2017

		@@ -182,3 +182,27 @@ _deprecated_hook( 'some_filter', '1.3.0', esc_html__( 'The $arg is deprecated.'
		_deprecated_hook( "filter_{$context}", '1.3.0', __( 'The $arg is deprecated.' ), sprintf( __( 'Some parsed message %s', $variable ) ) ); // Bad.

		echo add_filter( get_the_excerpt( get_the_ID() ) ; // Bad, but ignored as code contains a parse error.

Full property visibility/staticness review & fixing of custom property merging #841

Full property visibility/staticness review & fixing of custom property merging #841

Conversation

jrfnl commented Feb 11, 2017 • edited

tl;dr

Pre-amble

What this PR changes/fixes

Additional notes:

Question:

Open issue/question:

Technical details:

grappler commented Feb 11, 2017 • edited

JDGrimes left a comment

Choose a reason for hiding this comment

JDGrimes Feb 11, 2017

Choose a reason for hiding this comment

jrfnl Feb 12, 2017

Choose a reason for hiding this comment

JDGrimes Feb 11, 2017

Choose a reason for hiding this comment

jrfnl Feb 11, 2017

Choose a reason for hiding this comment

jrfnl commented Feb 11, 2017 • edited

grappler Feb 11, 2017

Choose a reason for hiding this comment

jrfnl Feb 12, 2017

Choose a reason for hiding this comment

jrfnl commented Feb 25, 2017

GaryJones commented Feb 25, 2017

jrfnl commented Feb 25, 2017

jrfnl commented Feb 11, 2017 •

edited

grappler commented Feb 11, 2017 •

edited

jrfnl commented Feb 11, 2017 •

edited