Review the WordPressVIPMinimum.Classes.DeclarationCompatibility sniff

[jrfnl]

Review the WordPressVIPMinimum.Classes.DeclarationCompatibility 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 (Audit Public Sniff 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]

Noticed the following three things while looking at this sniff for something unrelated:

No parameter name check

Looks like the parameter name as declared in a child class is not checked against the parameter name in the parent class, while in the context of PHP 8.0 named parameters and these methods potentially being called via call_user_func_array(), it is actually important that the parameter names match as well.

Signature checks are incomplete

Also looks like the signature checks are incomplete.

For example: it only checks if a parameter is declared to be passed by reference when the parameter in the parent class is declared to be passed by reference, while, when the parameter in the parent is not passed by reference, we should also make sure that the parameter in the overloaded method in the child class is also not passed by reference (currently it could well be, which is wrong and will not be flagged as-is).

VIP-Coding-Standards/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php

Lines 270 to 281 in 9749b01

	if (
	(
	array_key_exists( 'default', $param ) === true &&
	array_key_exists( 'default', $signatureParams[ $i ] ) === false
	) || (
	array_key_exists( 'pass_by_reference', $param ) === true &&
	$param['pass_by_reference'] !== $signatureParams[ $i ]['pass_by_reference']
	) || (
	array_key_exists( 'variable_length', $param ) === true &&
	$param['variable_length'] !== $signatureParams[ $i ]['variable_length']
	)
	) {

Check for type declarations

IIRC, none of the parameters in the parent classes have type declarations.

We should make sure that the methods in child classes also don't have type declarations as that would violate the contravariance rules and potentially cause a fatal error.

Return type declarations are fine as those are covariant.

[jrfnl]

Also note that this class definitely needs a lot more tests.