Review the WordPressVIPMinimum.Functions.RestrictedFunctions sniff

[jrfnl]

Review the WordPressVIPMinimum.Functions.RestrictedFunctions sniff for the following in as far as relevant to that sniff:

• [ ] Code style independent sniffing / Correct handling of quirky code Typical things to add tests for and verify correct handling of:
  • [ ] Nested function/closure declarations
  • [ ] Nested class declarations
  • [ ] Comments in unexpected places
  • [ ] Variables being assigned to via list statements
  • [ ] Multiline text strings
  • [ ] Text strings provided via heredoc/nowdoc
  • [ ] Use of short open tags
  • [ ] Using PHP close tag as end of statement
  • [ ] Inline control structures (without braces)
• [ ] Code simplifications which can be made using PHPCSUtils
• [ ] Sniff stability improvements which can be made using PHPCSUtils
• [ ] Correct handling of modern PHP code Typical things to add tests for and verify correct handling of (where applicable):
  • [ ] PHP 5.0 Try/catch/finally (PHP 5.5) and exceptions
  • [ ] PHP 5.3 Namespaced code vs code in the global namespace
  • [ ] PHP 5.3 Use import statements, incl aliasing
  • [ ] PHP 5.3 Short ternaries
  • [ ] PHP 5.3 Closures, incl closure use
  • [ ] PHP 5.4 Short arrays
  • [ ] PHP 5.5 Class name resolution using ::class
  • [ ] PHP 5.5 List in foreach
  • [ ] PHP 5.5/7.0 Generators using yield and yield from
  • [ ] PHP 5.6 Constant scalar expressions
  • [ ] PHP 5.6 Importing via use function/const
  • [ ] PHP 7.0 Null coalesce
  • [ ] PHP 7.0 Anonymous classes
  • [ ] PHP 7.0 Scalar and return type declarations
  • [ ] PHP 7.0 Group use statements
  • [ ] PHP 7.1 Short lists
  • [ ] PHP 7.1 Keyed lists
  • [ ] PHP 7.1 Multi-catch
  • [ ] PHP 7.1 Nullable types
  • [ ] PHP 7.3 List reference assignments
  • [ ] PHP 7.4 arrow functions
  • [ ] PHP 7.4 numeric literals with underscores
  • [ ] PHP 7.4 null coalesce equals
  • [ ] PHP 7.4 Typed properties
  • [ ] Various versions: trailing comma's in function calls, group use, function declarations, closure use etc

Other:

• [ ] Review violation error vs warning
• [ ] Review violation severity
• [ ] Review violation message, consider adding a link
• [ ] Check open issues related to the sniff
• [ ] Review PHPDoc comments

Sniff basics, but changes need to be lined up for next major release:

• [ ] Inappropriate use of public properties (#234)
• [ ] Modular error codes (unique error code for each distinct message)

Once PHPCS/PHPCSUtils supports this:

• [ ] PHP 8.0 Constructor property promotion
• [ ] PHP 8.0 Union types
• [ ] PHP 8.0 match expressions
• [ ] PHP 8.0 Nullsafe operator
• [ ] PHP 8.0 Named arguments
• [ ] PHP 8.0 Single token namespaced names

[jrfnl]

Review notes

From https://github.com/Automattic/VIP-Coding-Standards/pull/561#issue-457419501:

The parent is_targetted_token() method has diverged quite a bit from what is in the method in this class. Rather than address this now, I propose to address this once the improved/namespace and use context aware abstracts in PHPCSUtils are available as this sniff would benefit from switching over to such abstract.

Other than that for the custom code in the is_targetted_token() method, some additional checks could be put in place to guard against both false positives as well as false negatives.

Currently the custom code checks for a particular variable name for an object. However:

• It doesn't check that variable is the WP global variable.
• It doesn't attempt to check that variable is an instance of the target class by looking at parameter type declarations, docblock type declarations or looking for the variable being set in the current scope.
• It doesn't attempt to check if a variable with a different name is used whether that variable is an instance of the target class by looking at parameter type declarations, docblock type declarations or looking for the variable being set in the current scope.

Planning / timing

For the switch over to the different parent class, I think it would be opportune to do this relatively quickly after the abstract becomes available as the sniff is looking for a large list of functions and these check would all benefit from the namespace awareness.

For adding additional checks for the object variable, IMO, this can be low priority as effectively the custom code is currently only used for one function check $wp_rewrite->flush_rules().