Classes/DeclarationCompatibility: various sniff improvements

[jrfnl]

Pre-amble: while I have modernized and fixed this sniff, I also have my doubts about the value of this sniff nowadays.

While signature mismatches would yield a PHP Warning on PHP 7, which could easily be missed - hence the sniff; on PHP 8, they result in a fatal error.

As far as I know all of VIP has moved to PHP 8 by now and I would expect PHP linting against the applicable server PHP version to be run as part of the push protection automation, in which case, (nearly) everything this sniff checks for would be caught via PHP linting anyway.

If the sniff will remain, I'd recommend for the error message to be made more informative and to use modular error code as of VIPCS 4.0.
The sniff currently throws only one error, similar to PHP itself, which leaves it up to the user to figure out what the "signature mismatch" which was detected actually is, while the sniff already has that information ("extra param not optional", "expected reference param" etc) and could provide it, pointing the user in the right direction.

I have not implemented this at this time as changing this would be a BC-break which needs a new major.

Future Scope (if the sniff does not get removed):

1. Add check for correct method visibility
2. Add check that methods do not return by reference
3. Add check for matching parameter names (context: PHP 8.0 named parameter use, exception: methods with variadic as named params is not an option)
4. Add check for no param types (or type mixed - compatible see https://3v4l.org/BWol3#veol )
5. Add check for either no return type or return type which is co-variant
6. Add checks that deprecated methods + methods which are marked to NOT overload, are not declared
   • WP_Widget::WP_Widget() (deprecated PHP 4 constructor)
   • WP_Widget::display_callback() (Do NOT override)
   • WP_Widget::update_callback() (Do NOT override)

Note: errors for the checks listed as [3], [5] and [6] would not be caught by PHP linting, so those would add value to the sniff.

---

Classes/DeclarationCompatibility: bug fix - incorrect method name

The sniff would try to find a register_one() method declaration and check that, while the method in the parent class is called _register_one().

This would lead to both false positives (if a register_one() method would be declared in the child class) and false negatives (if an incompatible _register_one() method would be declared in the child class).

Fixed now.

Includes updating the tests.

Classes/DeclarationCompatibility: improve the tests

Completely rewrite and greatly expand the tests to ensure a significantly larger portion of what the sniff checks is actually tested.

Classes/DeclarationCompatibility: don't listen outside classes

The method signature of the AbstractScopeSniff::__construct() method is as follows:

public function __construct(array $scopeTokens, array $tokens, $listenOutside=false) {}

... with the documentation for the $listenOutside parameter being as follows:

$listenOutside If true this test will also alert the extending class when a token is found outside
the scope, by calling the processTokenOutsideScope method.

Except the DeclarationCompatibility sniff is not interested in tokens "outside scope" and has no implementation for the processTokenOutsideScope() method, so it makes no sense to have the $listenOutside parameter set to true.

Classes/DeclarationCompatibility: normalize method signature

It is more common and more predictable to have the $phpcsFile and $stackPtr parameters at the start of the parameter list, rather than at the end.

Classes/DeclarationCompatibility: remove redundant property

Remove a property which keeps track of the current "state". These type of properties are error prone and in this case not necessary anyway as the value of the property is only used in one place and could just have easily been retrieved where it is actually needed.

Include minor reformatting of a few long function calls for readability.

Classes/DeclarationCompatibility: code simplification [1] / deprecate public $checkClassesGroups property

Okay, so the class contained a public $checkClassesGroups property with as its value a multi-level array.

Sniff properties which are public can be set/overruled from a custom PHPCS ruleset to allow for customizing sniff behaviour, but multi-level property declarations are not supported for setting a property from a custom PHPCS ruleset, at least not without adding extensive special handling for these within the sniff code.

Independently of that, looking at the $checkClassesGroups property and the intentions for the sniff, it does not appear that this property was ever intended to be overrulable.

Now, I don't expect anyone to have overruled the property, but removing a public property should be considered a BC-break even so.
Even more so, as since PHP 8.2 deprecated dynamic properties, PHPCS will show a warning to users if their ruleset attempts to set a property which wasn't explicitly declared.

So, what to do ?

Well, with this commit, I propose the following:

• Deprecate the $checkClassesGroups property and render it unused by the sniff.
  The deprecation should be added to the action list in ticket 849 of deprecated code to remove in VIPCS 4.0.0.
• Introduce a new private $extendedClassToSignatures property with a simplified array format for the information the original property contained.
• Start using the new property in the sniff code, which allows for some code simplification.

Related to #234

Classes/DeclarationCompatibility: code simplification [2] / deprecate public $checkClasses property

Similar to the previous commit, the class contained a public $checkClasses property with as its value a complex multi-level array.

Sniff properties which are public can be set/overruled from a custom PHPCS ruleset to allow for customizing sniff behaviour, but multi-level property declarations are not supported for setting a property from a custom PHPCS ruleset, at least not without adding extensive special handling for these within the sniff code.

Independently of that, looking at the $checkClasses property and the intentions for the sniff, it does not appear that this property was ever intended to be overrulable.

Now, I don't expect anyone to have overruled the property, but removing a public property should be considered a BC-break even so.
Even more so, as since PHP 8.2 deprecated dynamic properties, PHPCS will show a warning to users if their ruleset attempts to set a property which wasn't explicitly declared.

Aside from all this, the array format of the property was also inconsistent - sometimes a parameter name was an array key with an array value, sometimes, the parameter name was the array value.

Having to take both situations into account makes the code more complicated than it needs to be and significantly raises the cognitive load for anyone working on this sniff.

So, what to do ?

Well, with this commit, I propose the following:

• Deprecate the $checkClasses property and render it unused by the sniff.
  The deprecation should be added to the action list in ticket 849 of deprecated code to remove in VIPCS 4.0.0.
• Introduce a new private $methodSignatures property with a more consistent array format for the information the original property contained.
  Parameter names are now always array keys and the value will always be an array, albeit sometimes an empty array.
• Start using the new property in the sniff code, which allows for some code simplification.

Related to #234

Classes/DeclarationCompatibility: rename some parameters

.. to improve code readability.

Classes/DeclarationCompatibility: remove duplicate calls to count()

Classes/DeclarationCompatibility: bug fix - reference mismatch not reported

If the parent class specifies a parameter by reference and the child class did not, the sniff would report an incompatible signature.

However, if the child class specified a parameter by reference, but the parent class did not, the sniff would fail to report the incompatible signature, while this is just as much a signature mismatch in PHP. See: https://3v4l.org/hLfbR#veol

Fixed now.

Includes tests.

Classes/DeclarationCompatibility: bug fix - false positives for valid extra parameters

It's perfectly valid for a child class to overload a parent method and to add additional optional parameters to the method.

The sniff did include code to handle this, but the logic of that code block was incorrect.

The old condition basically checked if the 'default' array index existed and the value of the default parameter was not true. Not sure where that came from, but it makes no sense.

Fixed now.

Includes adding support for additional optional parameters being declared as variadic (PHP 5.6+), which makes them optional by nature.
Includes minor efficiency fix - no need to continue examining extra parameters to see if they are optional if we already saw one which was not.

Includes tests.

Classes/DeclarationCompatibility: bug fix - properly handle PHP 5.6+ variadic parameters

As things were, the sniff would verify that if a parent method would expect a parameter to be variadic, the child method would also declare the parameter as variadic.

This check is insufficient and leads to both false positives and false negatives.

Based on testing this extensively, I've boiled the rules down to this:

1. Variadic parameters must always be the last parameter in a signature.
2. If a parent method has the last parameter as optional by declaring a default value, a child method which makes that last parameter variadic and removes the default value, still complies with the signature requirements as variadic parameters are optional by nature.
3. If a parent method has the last parameter as variadic, the child method must also have the last parameter as variadic, but the child method may insert additional optional parameters before the variadic parameter.

This commit adjusts the sniff code to apply all three of these rules properly.

Includes tests.
Tests for the third rule are, in part, included in the tests added in the previous commit in the *OkayExtraOptionalParams classes.

Classes/DeclarationCompatibility: minor code consistency tweak

Classes/DeclarationCompatibility: bug fix - false negatives for errors icw extra optional parameters

Now the checks for optional parameters and variadic parameters have been fixed, it becomes clear that IF the child method declares extra optional parameters AND the child method has a signature mismatch in the parameters inherited from the parent method, the sniff would stay silent as the sniff bows out on the if ( $all_extra_params_have_default === true ) condition, resulting in false negatives.

Fixed now.

Includes tests.

Classes/DeclarationCompatibility: bug fix - class and function names are not case-sensitive

As things were, the comparisons/checks whether a function should be examined and whether a function was included in a class extending a class which should be examined, were all done case-sensitively, while PHP treats (ascii-based) class and function names case-insensitively.

Fixed now.

Includes code to ensure that the class and method names used in the error messages are in "proper case", i.e. the expected/official case for each class/method.

Tested via updating some pre-existing tests.

Classes/DeclarationCompatibility: move some variable declarations

... to where they are actually needed/used. No need to have this information available before this point.

Classes/DeclarationCompatibility: bug fix - recognize extended class if fully qualified

It is perfectly valid in namespaced code to use fully qualified class names for classes not imported via an import use statement.

The sniff did not handle this correctly, leading to false negatives.

Fixed now.

Includes some updated tests to safeguard this.

Classes/DeclarationCompatibility: bug fix - false positives for nested functions / refactor

As things were, the DeclarationCompatibilitySniff class extended the PHPCS native AbstractScopeSniff.

The AbstractScopeSniff has a fundamental problem, in that it will call the child sniff for every target token in every target scope.

To illustrate, let's have a look at the tests I've added:

class WidgetOkayNestedFunctionIsNotMethod extends WP_Widget {
	public function hasNested() {
		// Functions can be declared nested within other functions. That doesn't make them methods.
		function is_preview($extra_param) {}
	}
}

As both the hasNested() function as well as the is_preview() function are nested somewhere in a T_CLASS scope, the DeclarationCompatibilitySniff will be called for both T_FUNCTION tokens and give both the T_CLASS token for the WidgetOkayNestedFunctionIsNotMethod class as the "current scope".

Along the same lines, for the second code sample:

class WidgetOkayNestedClassDoesNotExtend extends WP_Widget {
	function hasNestedClass() {
		return new class {
			public function display_callback() {}
		};
	}
}

The DeclarationCompatibilitySniff::processTokenWithinScope() method will be called for both the hasNestedClass() as well as the display_callback() functions and both will have the T_CLASS token for the WidgetOkayNestedClassDoesNotExtend class as the "current scope", no matter that the display_callback() method is in actual fact a method on the anonymous class and not on the WidgetOkayNestedClassDoesNotExtend class.

As can be expected, this leads to false positives as both the (global) is_preview() function from the first code sample as well as the display_callback() method in the anonymous class from the second code sample would be analyzed as if they were methods on the top-level class.

Now, this can be fixed by adding a check at the start of the processTokenWithinScope() function to make sure that the "current scope" is the "deepest" valid scope, however, there is a better solution, which should also improve the performance of the sniff.

As of PHPCSUtils 1.1.0, PHPCSUtils offers a highly optimized ObjectDeclarations::getDeclaredMethods() method, which will return an array with the names of all found methods and the stack pointer to the T_FUNCTION keyword for each.

So now, instead of the AbstractScopeSniff being called for every single function and needing to do processing before deciding whether to bow out or to pass off to child classes, we can sniff for the - much rarer - T_CLASS token instead, then check if the class extends one of the classes we are looking for and if so, retrieve the list of methods on that class via the new PHPCSUtils method and just loop through them.

As the results for the PHPCSUtils method are cached, this also means that if multiple sniffs would use the ObjectDeclarations::getDeclared*() methods, the result for subsequent calls will be returned nearly instantaneous, giving a PHPCS run an even bigger performance boost.

Classes/DeclarationCompatibility: add support for PHP 7.0+ anonymous classes

Now support for nested structures has been fixed, we can add support for anonymous classes.

Anonymous classes can also extend the WP native classes, so should be checked too.

Includes updating some of the existing tests to safeguard this and adding some extra tests with nested structures.

Classes/DeclarationCompatibility: minor code stability tweaks

• Prefer isset() over array_key_exists() as it's faster and improves code readability.
• Add additional conditions to support the possibility of the pass_by_reference and variable_length keys in the parameter sub-arrays in the $methodSignatures property being set to false.

Classes/DeclarationCompatibility: minor documentation improvements

Related to #507 (but considering the future scope, doesn't close it)

[GaryJones]

Amazing.