From 5ed8cb3a4374abbc2bd0adade8c9d5d153d8072b Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 24 Jul 2025 02:14:49 +0200 Subject: [PATCH 01/21] 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. --- .../Sniffs/Classes/DeclarationCompatibilitySniff.php | 2 +- .../Tests/Classes/DeclarationCompatibilityUnitTest.inc | 8 ++++++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php b/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php index 9a930319..c5614cd8 100644 --- a/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php +++ b/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php @@ -70,7 +70,7 @@ class DeclarationCompatibilitySniff extends AbstractScopeSniff { 'default' => '1', ], ], - 'register_one' => [ + '_register_one' => [ 'number' => [ 'default' => '-1', ], diff --git a/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.inc b/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.inc index 740a7883..237c1426 100644 --- a/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.inc @@ -46,7 +46,7 @@ class MyWidget extends WP_Widget { function form_callback() { } // Bad, missing $widget_args param. - function register_one() { + function _register_one() { } // Bad, missing $number param. function save_settings() { @@ -103,7 +103,7 @@ class MyWidget2 extends WP_Widget { function form_callback( $widget_args = 2 ) { } // Ok, despite having different default value. - function register_one( $number ) { + function _register_one( $number ) { } // Bad, the param should have a default value. Should be $number. function save_settings( $setting ) { @@ -143,3 +143,7 @@ class MyWalker extends Walker { function end_el( &$output, $data_object, $depth = 0, $args = array() ) { } // Ok. } + +class MyWidget3 extends WP_Widget { + function register_one( $number ) {} // Okay. Not one of the methods we're looking for. +} From e9ac45fbad75f610d97679d1c1006858d57ca46a Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 24 Jul 2025 23:03:25 +0200 Subject: [PATCH 02/21] 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. --- .../DeclarationCompatibilityUnitTest.1.inc | 101 ++++++++++++ .../DeclarationCompatibilityUnitTest.2.inc | 94 +++++++++++ .../DeclarationCompatibilityUnitTest.3.inc | 12 ++ .../DeclarationCompatibilityUnitTest.inc | 149 ------------------ .../DeclarationCompatibilityUnitTest.php | 115 ++++++++++---- 5 files changed, 294 insertions(+), 177 deletions(-) create mode 100644 WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.1.inc create mode 100644 WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.2.inc create mode 100644 WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.3.inc delete mode 100644 WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.inc diff --git a/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.1.inc b/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.1.inc new file mode 100644 index 00000000..1ee50816 --- /dev/null +++ b/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.1.inc @@ -0,0 +1,101 @@ + Key is the line number, value is the number of expected errors. */ - public function getErrorList() { - return [ - 4 => 1, - 7 => 1, - 10 => 1, - 13 => 1, - 16 => 1, - 19 => 1, - 25 => 1, - 40 => 1, - 43 => 1, - 46 => 1, - 49 => 1, - 52 => 1, - 61 => 1, - 67 => 1, - 70 => 1, - 76 => 1, - 79 => 1, - 88 => 1, - 106 => 1, - 112 => 1, - 119 => 1, - 128 => 1, - 134 => 1, - 137 => 1, - 140 => 1, - ]; + public function getErrorList( $testFile = '' ) { + switch ( $testFile ) { + case 'DeclarationCompatibilityUnitTest.1.inc': + return [ + 53 => 1, + 54 => 1, + 55 => 1, + 56 => 1, + 57 => 1, + 58 => 1, + 59 => 1, + 60 => 1, + 61 => 1, + 62 => 1, + 63 => 1, + 64 => 1, + 68 => 1, + 69 => 1, + 70 => 1, + 71 => 1, + 75 => 1, + 76 => 1, + 77 => 1, + 78 => 1, + 79 => 1, + 80 => 1, + 81 => 1, + 82 => 1, + 83 => 1, + 84 => 1, + 85 => 1, + 86 => 1, + 87 => 1, + 88 => 1, + 89 => 1, + 90 => 1, + 91 => 1, + 92 => 1, + 96 => 1, + 97 => 1, + 98 => 1, + 99 => 1, + 100 => 1, + ]; + + case 'DeclarationCompatibilityUnitTest.2.inc': + return [ + 43 => 1, + 44 => 1, + 45 => 1, + 46 => 1, + 47 => 1, + 48 => 1, + 49 => 1, + 50 => 1, + 51 => 1, + 55 => 1, + 56 => 1, + 57 => 1, + 58 => 1, + 59 => 1, + 60 => 1, + 61 => 1, + 62 => 1, + 66 => 1, + 67 => 1, + 68 => 1, + 72 => 1, + 73 => 1, + 74 => 1, + 75 => 1, + 79 => 1, + 80 => 1, + 81 => 1, + 82 => 1, + 83 => 1, + 84 => 1, + 88 => 1, + 92 => 1, + 93 => 1, + ]; + + default: + return []; + } } /** From d029cf33e19beae86d3bdcd8a6eb53aeecc4512c Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 24 Jul 2025 04:20:05 +0200 Subject: [PATCH 03/21] Classes/DeclarationCompatibility: don't listen outside classes The method signature of the `AbstractScopeSniff::__construct()` method is as follows: ```php 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`. --- .../Sniffs/Classes/DeclarationCompatibilitySniff.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php b/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php index c5614cd8..e0ec1713 100644 --- a/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php +++ b/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php @@ -189,7 +189,7 @@ class DeclarationCompatibilitySniff extends AbstractScopeSniff { * Constructs the test with the tokens it wishes to listen for. */ public function __construct() { - parent::__construct( [ T_CLASS ], [ T_FUNCTION ], true ); + parent::__construct( [ T_CLASS ], [ T_FUNCTION ], false ); } /** From 8e6ac380e15ce0516ec3c3d119e3f9ceb73b8a95 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 24 Jul 2025 22:59:00 +0200 Subject: [PATCH 04/21] 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. --- .../Sniffs/Classes/DeclarationCompatibilitySniff.php | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php b/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php index e0ec1713..81cec678 100644 --- a/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php +++ b/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php @@ -262,7 +262,7 @@ protected function processTokenWithinScope( File $phpcsFile, $stackPtr, $currSco } if ( count( $signatureParams ) !== count( $parentSignature ) ) { - $this->addError( $originalParentClassName, $methodName, $signatureParams, $parentSignature, $phpcsFile, $stackPtr ); + $this->addError( $phpcsFile, $stackPtr, $originalParentClassName, $methodName, $signatureParams, $parentSignature ); return; } @@ -281,7 +281,7 @@ protected function processTokenWithinScope( File $phpcsFile, $stackPtr, $currSco $param['variable_length'] !== $signatureParams[ $i ]['variable_length'] ) ) { - $this->addError( $originalParentClassName, $methodName, $signatureParams, $parentSignature, $phpcsFile, $stackPtr ); + $this->addError( $phpcsFile, $stackPtr, $originalParentClassName, $methodName, $signatureParams, $parentSignature ); return; } } @@ -292,16 +292,16 @@ protected function processTokenWithinScope( File $phpcsFile, $stackPtr, $currSco /** * Generates an error with nice current and parent class method notations * + * @param File $phpcsFile The PHP_CodeSniffer file where the token was found. + * @param int $stackPtr The position of the current token in the stack. * @param string $parentClassName The name of the extended (parent) class. * @param string $methodName The name of the method currently being examined. * @param array $currentMethodSignature The list of params and their options of the method which is being examined. * @param array $parentMethodSignature The list of params and their options of the parent class method. - * @param File $phpcsFile The PHP_CodeSniffer file where the token was found. - * @param int $stackPtr The position of the current token in the stack. * * @return void */ - private function addError( $parentClassName, $methodName, $currentMethodSignature, $parentMethodSignature, $phpcsFile, $stackPtr ) { + private function addError( File $phpcsFile, $stackPtr, $parentClassName, $methodName, $currentMethodSignature, $parentMethodSignature ) { $currentSignature = sprintf( '%s::%s(%s)', $this->currentClass, $methodName, implode( ', ', $this->generateParamList( $currentMethodSignature ) ) ); From 5e2b887c2310c6a88187ec32e2e0f4d2cc343329 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 24 Jul 2025 23:12:43 +0200 Subject: [PATCH 05/21] 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/DeclarationCompatibilitySniff.php | 27 +++++++------------ 1 file changed, 9 insertions(+), 18 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php b/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php index 81cec678..2603f540 100644 --- a/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php +++ b/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php @@ -19,13 +19,6 @@ */ class DeclarationCompatibilitySniff extends AbstractScopeSniff { - /** - * The name of the class we are currently checking. - * - * @var string - */ - private $currentClass = ''; - /** * A list of classes and methods to check. * @@ -203,12 +196,6 @@ public function __construct() { */ protected function processTokenWithinScope( File $phpcsFile, $stackPtr, $currScope ) { - $className = ObjectDeclarations::getName( $phpcsFile, $currScope ); - - if ( $className !== $this->currentClass ) { - $this->currentClass = $className; - } - $methodName = FunctionDeclarations::getName( $phpcsFile, $stackPtr ); $parentClassName = ObjectDeclarations::findExtendedClassName( $phpcsFile, $currScope ); @@ -262,7 +249,7 @@ protected function processTokenWithinScope( File $phpcsFile, $stackPtr, $currSco } if ( count( $signatureParams ) !== count( $parentSignature ) ) { - $this->addError( $phpcsFile, $stackPtr, $originalParentClassName, $methodName, $signatureParams, $parentSignature ); + $this->addError( $phpcsFile, $stackPtr, $currScope, $originalParentClassName, $methodName, $signatureParams, $parentSignature ); return; } @@ -281,7 +268,7 @@ protected function processTokenWithinScope( File $phpcsFile, $stackPtr, $currSco $param['variable_length'] !== $signatureParams[ $i ]['variable_length'] ) ) { - $this->addError( $phpcsFile, $stackPtr, $originalParentClassName, $methodName, $signatureParams, $parentSignature ); + $this->addError( $phpcsFile, $stackPtr, $currScope, $originalParentClassName, $methodName, $signatureParams, $parentSignature ); return; } } @@ -294,6 +281,7 @@ protected function processTokenWithinScope( File $phpcsFile, $stackPtr, $currSco * * @param File $phpcsFile The PHP_CodeSniffer file where the token was found. * @param int $stackPtr The position of the current token in the stack. + * @param int $currScope A pointer to the start of the scope. * @param string $parentClassName The name of the extended (parent) class. * @param string $methodName The name of the method currently being examined. * @param array $currentMethodSignature The list of params and their options of the method which is being examined. @@ -301,11 +289,14 @@ protected function processTokenWithinScope( File $phpcsFile, $stackPtr, $currSco * * @return void */ - private function addError( File $phpcsFile, $stackPtr, $parentClassName, $methodName, $currentMethodSignature, $parentMethodSignature ) { + private function addError( File $phpcsFile, $stackPtr, $currScope, $parentClassName, $methodName, $currentMethodSignature, $parentMethodSignature ) { + $currentClassName = ObjectDeclarations::getName( $phpcsFile, $currScope ); - $currentSignature = sprintf( '%s::%s(%s)', $this->currentClass, $methodName, implode( ', ', $this->generateParamList( $currentMethodSignature ) ) ); + $currentSignature = implode( ', ', $this->generateParamList( $currentMethodSignature ) ); + $currentSignature = sprintf( '%s::%s(%s)', $currentClassName, $methodName, $currentSignature ); - $parentSignature = sprintf( '%s::%s(%s)', $parentClassName, $methodName, implode( ', ', $this->generateParamList( $parentMethodSignature ) ) ); + $parentSignature = implode( ', ', $this->generateParamList( $parentMethodSignature ) ); + $parentSignature = sprintf( '%s::%s(%s)', $parentClassName, $methodName, $parentSignature ); $message = 'Declaration of `%s` should be compatible with `%s`.'; $data = [ $currentSignature, $parentSignature ]; From f4daea0194944fe000c8c0552047028a2f1f0096 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 24 Jul 2025 06:14:26 +0200 Subject: [PATCH 06/21] 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/DeclarationCompatibilitySniff.php | 51 +++++++++---------- 1 file changed, 25 insertions(+), 26 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php b/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php index 2603f540..6559c718 100644 --- a/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php +++ b/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php @@ -164,18 +164,28 @@ class DeclarationCompatibilitySniff extends AbstractScopeSniff { /** * List of grouped classes with same methods (as they extend the same parent class) * + * @deprecated 3.1.0 This should never have been a public property and is now unused. + * * @var array */ - public $checkClassesGroups = [ - 'Walker' => [ - 'Walker_Category_Checklist', - 'Walker_Category', - 'Walker_CategoryDropdown', - 'Walker_PageDropdown', - 'Walker_Nav_Menu', - 'Walker_Page', - 'Walker_Comment', - ], + public $checkClassesGroups = []; + + /** + * Classes this sniff checks for being extended. + * + * @var array Key is the name of a potentially extended class, + * value the canonical name for the method signatures definition. + */ + private $extendedClassToSignatures = [ + 'WP_Widget' => 'WP_Widget', + 'Walker' => 'Walker', + 'Walker_Category_Checklist' => 'Walker', + 'Walker_Category' => 'Walker', + 'Walker_CategoryDropdown' => 'Walker', + 'Walker_PageDropdown' => 'Walker', + 'Walker_Nav_Menu' => 'Walker', + 'Walker_Page' => 'Walker', + 'Walker_Comment' => 'Walker', ]; /** @@ -204,26 +214,15 @@ protected function processTokenWithinScope( File $phpcsFile, $stackPtr, $currSco return; } - // Need to define the originalParentClassName since we might override the parentClassName due to signature notations grouping. + // Store the originalParentClassName since we might override the parentClassName due to signature notations grouping. $originalParentClassName = $parentClassName; - - if ( array_key_exists( $parentClassName, $this->checkClasses ) === false ) { + if ( isset( $this->extendedClassToSignatures[ $parentClassName ] ) === false ) { // This class does not extend a class we are interested in. - foreach ( $this->checkClassesGroups as $parent => $children ) { - // But it might be one of the grouped classes. - foreach ( $children as $child ) { - if ( $child === $parentClassName ) { - $parentClassName = $parent; - break 2; - } - } - } - if ( array_key_exists( $parentClassName, $this->checkClasses ) === false ) { - // This class really does not extend a class we are interested in. - return; - } + return; } + $parentClassName = $this->extendedClassToSignatures[ $parentClassName ]; + if ( array_key_exists( $methodName, $this->checkClasses[ $parentClassName ] ) === false && in_array( $methodName, $this->checkClasses[ $parentClassName ], true ) === false ) { From b6863bca30cd26beb6ed6010cfae0bd6d0d55e9c Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 24 Jul 2025 06:42:51 +0200 Subject: [PATCH 07/21] 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/DeclarationCompatibilitySniff.php | 147 ++++++++++-------- 1 file changed, 84 insertions(+), 63 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php b/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php index 6559c718..91122cad 100644 --- a/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php +++ b/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php @@ -22,16 +22,42 @@ class DeclarationCompatibilitySniff extends AbstractScopeSniff { /** * A list of classes and methods to check. * + * @deprecated 3.1.0 This should never have been a public property. + * * @var array>>> */ - public $checkClasses = [ + public $checkClasses = []; + + /** + * List of grouped classes with same methods (as they extend the same parent class) + * + * @deprecated 3.1.0 This should never have been a public property. + * + * @var array + */ + public $checkClassesGroups = []; + + /** + * A list of classes and information on the methods to check for those classes. + * + * @var array>>> + */ + private $methodSignatures = [ 'WP_Widget' => [ - 'widget' => [ 'args', 'instance' ], - 'update' => [ 'new_instance', 'old_instance' ], - 'form' => [ 'instance' ], + 'widget' => [ + 'args' => [], + 'instance' => [], + ], + 'update' => [ + 'new_instance' => [], + 'old_instance' => [], + ], + 'form' => [ + 'instance' => [], + ], 'WP_Widget' => [ - 'id_base', - 'name', + 'id_base' => [], + 'name' => [], 'widget_options' => [ 'default' => 'array()', ], @@ -39,16 +65,22 @@ class DeclarationCompatibilitySniff extends AbstractScopeSniff { 'default' => 'array()', ], ], - 'get_field_name' => [ 'field_name' ], - 'get_field_id' => [ 'field_name' ], + 'get_field_name' => [ + 'field_name' => [], + ], + 'get_field_id' => [ + 'field_name' => [], + ], '_register' => [], - '_set' => [ 'number' ], + '_set' => [ + 'number' => [], + ], '_get_display_callback' => [], '_get_update_callback' => [], '_get_form_callback' => [], 'is_preview' => [], 'display_callback' => [ - 'args', + 'args' => [], 'widget_args' => [ 'default' => '1', ], @@ -68,9 +100,12 @@ class DeclarationCompatibilitySniff extends AbstractScopeSniff { 'default' => '-1', ], ], - 'save_settings' => [ 'settings' ], + 'save_settings' => [ + 'settings' => [], + ], 'get_settings' => [], ], + 'Walker' => [ 'start_lvl' => [ 'output' => [ @@ -98,7 +133,7 @@ class DeclarationCompatibilitySniff extends AbstractScopeSniff { 'output' => [ 'pass_by_reference' => true, ], - 'data_object', + 'data_object' => [], 'depth' => [ 'default' => '0', ], @@ -110,50 +145,50 @@ class DeclarationCompatibilitySniff extends AbstractScopeSniff { ], ], 'end_el' => [ - 'output' => [ + 'output' => [ 'pass_by_reference' => true, ], - 'data_object', - 'depth' => [ + 'data_object' => [], + 'depth' => [ 'default' => '0', ], - 'args' => [ + 'args' => [ 'default' => 'array()', ], ], 'display_element' => [ - 'element', + 'element' => [], 'children_elements' => [ 'pass_by_reference' => true, ], - 'max_depth', - 'depth', - 'args', + 'max_depth' => [], + 'depth' => [], + 'args' => [], 'output' => [ 'pass_by_reference' => true, ], ], 'walk' => [ - 'elements', - 'max_depth', - 'args' => [ + 'elements' => [], + 'max_depth' => [], + 'args' => [ 'variable_length' => true, ], ], 'paged_walk' => [ - 'elements', - 'max_depth', - 'page_num', - 'per_page', - 'args' => [ + 'elements' => [], + 'max_depth' => [], + 'page_num' => [], + 'per_page' => [], + 'args' => [ 'variable_length' => true, ], ], 'get_number_of_root_elements' => [ - 'elements', + 'elements' => [], ], 'unset_children' => [ - 'element', + 'element' => [], 'children_elements' => [ 'pass_by_reference' => true, ], @@ -161,15 +196,6 @@ class DeclarationCompatibilitySniff extends AbstractScopeSniff { ], ]; - /** - * List of grouped classes with same methods (as they extend the same parent class) - * - * @deprecated 3.1.0 This should never have been a public property and is now unused. - * - * @var array - */ - public $checkClassesGroups = []; - /** * Classes this sniff checks for being extended. * @@ -222,17 +248,14 @@ protected function processTokenWithinScope( File $phpcsFile, $stackPtr, $currSco } $parentClassName = $this->extendedClassToSignatures[ $parentClassName ]; - - if ( array_key_exists( $methodName, $this->checkClasses[ $parentClassName ] ) === false && - in_array( $methodName, $this->checkClasses[ $parentClassName ], true ) === false - ) { - // This method is not a one we are interested in. + if ( isset( $this->methodSignatures[ $parentClassName ][ $methodName ] ) === false ) { + // This method is not one we are interested in. return; } $signatureParams = FunctionDeclarations::getParameters( $phpcsFile, $stackPtr ); - $parentSignature = $this->checkClasses[ $parentClassName ][ $methodName ]; + $parentSignature = $this->methodSignatures[ $parentClassName ][ $methodName ]; if ( count( $signatureParams ) > count( $parentSignature ) ) { $extra_params = array_slice( $signatureParams, count( $parentSignature ) - count( $signatureParams ) ); @@ -254,22 +277,20 @@ protected function processTokenWithinScope( File $phpcsFile, $stackPtr, $currSco $i = 0; foreach ( $parentSignature as $key => $param ) { - if ( is_array( $param ) === true ) { - 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'] - ) - ) { - $this->addError( $phpcsFile, $stackPtr, $currScope, $originalParentClassName, $methodName, $signatureParams, $parentSignature ); - return; - } + 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'] + ) + ) { + $this->addError( $phpcsFile, $stackPtr, $currScope, $originalParentClassName, $methodName, $signatureParams, $parentSignature ); + return; } ++$i; } @@ -313,8 +334,8 @@ private function generateParamList( $methodSignature ) { $paramList = []; foreach ( $methodSignature as $param => $options ) { $paramName = '$'; - if ( is_array( $options ) === false ) { - $paramList[] = '$' . $options; + if ( empty( $options ) === true ) { + $paramList[] = '$' . $param; continue; } From a4a1b8abc45dfafe9bbe17077bf6ac4baf35d86a Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 24 Jul 2025 18:31:52 +0200 Subject: [PATCH 08/21] Classes/DeclarationCompatibility: rename some parameters .. to improve code readability. --- .../Classes/DeclarationCompatibilitySniff.php | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php b/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php index 91122cad..3255c1fd 100644 --- a/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php +++ b/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php @@ -253,12 +253,12 @@ protected function processTokenWithinScope( File $phpcsFile, $stackPtr, $currSco return; } - $signatureParams = FunctionDeclarations::getParameters( $phpcsFile, $stackPtr ); + $childParams = FunctionDeclarations::getParameters( $phpcsFile, $stackPtr ); - $parentSignature = $this->methodSignatures[ $parentClassName ][ $methodName ]; + $parentParams = $this->methodSignatures[ $parentClassName ][ $methodName ]; - if ( count( $signatureParams ) > count( $parentSignature ) ) { - $extra_params = array_slice( $signatureParams, count( $parentSignature ) - count( $signatureParams ) ); + if ( count( $childParams ) > count( $parentParams ) ) { + $extra_params = array_slice( $childParams, count( $parentParams ) - count( $childParams ) ); $all_extra_params_have_default = true; foreach ( $extra_params as $extra_param ) { if ( array_key_exists( 'default', $extra_param ) === false || $extra_param['default'] !== 'true' ) { @@ -270,26 +270,26 @@ protected function processTokenWithinScope( File $phpcsFile, $stackPtr, $currSco } } - if ( count( $signatureParams ) !== count( $parentSignature ) ) { - $this->addError( $phpcsFile, $stackPtr, $currScope, $originalParentClassName, $methodName, $signatureParams, $parentSignature ); + if ( count( $childParams ) !== count( $parentParams ) ) { + $this->addError( $phpcsFile, $stackPtr, $currScope, $originalParentClassName, $methodName, $childParams, $parentParams ); return; } $i = 0; - foreach ( $parentSignature as $key => $param ) { + foreach ( $parentParams as $key => $param ) { if ( ( array_key_exists( 'default', $param ) === true && - array_key_exists( 'default', $signatureParams[ $i ] ) === false + array_key_exists( 'default', $childParams[ $i ] ) === false ) || ( array_key_exists( 'pass_by_reference', $param ) === true && - $param['pass_by_reference'] !== $signatureParams[ $i ]['pass_by_reference'] + $param['pass_by_reference'] !== $childParams[ $i ]['pass_by_reference'] ) || ( array_key_exists( 'variable_length', $param ) === true && - $param['variable_length'] !== $signatureParams[ $i ]['variable_length'] + $param['variable_length'] !== $childParams[ $i ]['variable_length'] ) ) { - $this->addError( $phpcsFile, $stackPtr, $currScope, $originalParentClassName, $methodName, $signatureParams, $parentSignature ); + $this->addError( $phpcsFile, $stackPtr, $currScope, $originalParentClassName, $methodName, $childParams, $parentParams ); return; } ++$i; From 5fc17c6af7076481ac68b668519bef762df55c31 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 24 Jul 2025 18:33:28 +0200 Subject: [PATCH 09/21] Classes/DeclarationCompatibility: remove duplicate calls to `count()` --- .../Sniffs/Classes/DeclarationCompatibilitySniff.php | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php b/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php index 3255c1fd..fa5b2fb4 100644 --- a/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php +++ b/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php @@ -253,12 +253,14 @@ protected function processTokenWithinScope( File $phpcsFile, $stackPtr, $currSco return; } - $childParams = FunctionDeclarations::getParameters( $phpcsFile, $stackPtr ); + $childParams = FunctionDeclarations::getParameters( $phpcsFile, $stackPtr ); + $childParamCount = count( $childParams ); - $parentParams = $this->methodSignatures[ $parentClassName ][ $methodName ]; + $parentParams = $this->methodSignatures[ $parentClassName ][ $methodName ]; + $parentParamCount = count( $parentParams ); - if ( count( $childParams ) > count( $parentParams ) ) { - $extra_params = array_slice( $childParams, count( $parentParams ) - count( $childParams ) ); + if ( $childParamCount > $parentParamCount ) { + $extra_params = array_slice( $childParams, $parentParamCount - $childParamCount ); $all_extra_params_have_default = true; foreach ( $extra_params as $extra_param ) { if ( array_key_exists( 'default', $extra_param ) === false || $extra_param['default'] !== 'true' ) { @@ -270,7 +272,7 @@ protected function processTokenWithinScope( File $phpcsFile, $stackPtr, $currSco } } - if ( count( $childParams ) !== count( $parentParams ) ) { + if ( $childParamCount !== $parentParamCount ) { $this->addError( $phpcsFile, $stackPtr, $currScope, $originalParentClassName, $methodName, $childParams, $parentParams ); return; } From 957c44288f25f2f7ee0838bd9a4573f5c77ca719 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 24 Jul 2025 07:14:13 +0200 Subject: [PATCH 10/21] 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/DeclarationCompatibilitySniff.php | 5 ++ .../DeclarationCompatibilityUnitTest.1.inc | 17 +++- .../DeclarationCompatibilityUnitTest.2.inc | 14 ++- .../DeclarationCompatibilityUnitTest.php | 87 ++++++++++++------- 4 files changed, 88 insertions(+), 35 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php b/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php index fa5b2fb4..7d4a2cbe 100644 --- a/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php +++ b/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php @@ -284,8 +284,13 @@ protected function processTokenWithinScope( File $phpcsFile, $stackPtr, $currSco array_key_exists( 'default', $param ) === true && array_key_exists( 'default', $childParams[ $i ] ) === false ) || ( + // Parameter in parent class has reference, child does not. array_key_exists( 'pass_by_reference', $param ) === true && $param['pass_by_reference'] !== $childParams[ $i ]['pass_by_reference'] + ) || ( + // Parameter in parent class does *not* have reference, child does. + array_key_exists( 'pass_by_reference', $param ) === false && + $childParams[ $i ]['pass_by_reference'] === true ) || ( array_key_exists( 'variable_length', $param ) === true && $param['variable_length'] !== $childParams[ $i ]['variable_length'] diff --git a/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.1.inc b/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.1.inc index 1ee50816..6e98e147 100644 --- a/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.1.inc +++ b/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.1.inc @@ -4,7 +4,7 @@ class WidgetOkay extends WP_Widget { // Not a method we're interested in. Should be ignored. - public function register_one($whatever, $you, $want = null) {} + public function register_one($whatever, &$you, $want = null) {} // Methods which are being checked. public function widget( $args, $instance ) {} @@ -99,3 +99,18 @@ class WidgetBadOptionalParamChangedToRequiredMissingDefaultValue extends WP_Widg public function form_callback( $widget_args ) {} public function _register_one( $number ) {} } + +class WidgetBadChildHasParamReferenceParentDoesNot extends WP_Widget { + public function widget( $args, &$instance ) {} + public function update( &$new_instance, $old_instance ) {} + public function form( &$instance ) {} + public function WP_Widget( $id_base, &$name, $widget_options = array(), &$control_options = array() ) {} + public function get_field_name( &$field_name ) {} + public function get_field_id( &$field_name ) {} + public function _set( &$number ) {} + public function display_callback( &$args, $widget_args = 1 ) {} + public function update_callback( &$deprecated = 1 ) {} + public function form_callback( &$widget_args = 1 ) {} + public function _register_one( &$number = -1 ) {} + public function save_settings( &$settings ) {} +} diff --git a/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.2.inc b/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.2.inc index f0233898..547bc448 100644 --- a/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.2.inc +++ b/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.2.inc @@ -4,7 +4,7 @@ class WalkerOkay extends Walker { // Not a method we're interested in. Should be ignored. - public function custom_method($whatever, $you, $want = null) {} + public function custom_method($whatever, &$you, $want = null) {} // Methods which are being checked. public function start_lvl( &$output, $depth = 0, $args = array() ) {} @@ -92,3 +92,15 @@ class WalkerBadVariadicArgumentNotDeclaredAsVariadic extends Walker { public function walk( $elements, $max_depth, $args ) {} public function paged_walk( $elements, $max_depth, $page_num, $per_page, $args ) {} } + +class WalkerBadChildHasParamReferenceParentDoesNot extends Walker_Category { + public function start_lvl( &$output, &$depth = 0, $args = array() ) {} + public function end_lvl( &$output, $depth = 0, &$args = array() ) {} + public function start_el( &$output, $data_object, $depth = 0, $args = array(), &$current_object_id = 0 ) {} + public function end_el( &$output, $data_object, &$depth = 0, $args = array(), ) {} + public function display_element( &$element, &$children_elements, $max_depth, $depth, $args, &$output ) {} + public function walk( $elements, &$max_depth, ...$args ) {} + public function paged_walk( $elements, $max_depth, &$page_num, $per_page, ...$args ) {} + public function get_number_of_root_elements( &$elements, ) {} + public function unset_children( &$element, &$children_elements ) {} +} diff --git a/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.php b/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.php index ff651fe8..678cc860 100644 --- a/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.php +++ b/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.php @@ -66,43 +66,64 @@ public function getErrorList( $testFile = '' ) { 98 => 1, 99 => 1, 100 => 1, + 104 => 1, + 105 => 1, + 106 => 1, + 107 => 1, + 108 => 1, + 109 => 1, + 110 => 1, + 111 => 1, + 112 => 1, + 113 => 1, + 114 => 1, + 115 => 1, ]; case 'DeclarationCompatibilityUnitTest.2.inc': return [ - 43 => 1, - 44 => 1, - 45 => 1, - 46 => 1, - 47 => 1, - 48 => 1, - 49 => 1, - 50 => 1, - 51 => 1, - 55 => 1, - 56 => 1, - 57 => 1, - 58 => 1, - 59 => 1, - 60 => 1, - 61 => 1, - 62 => 1, - 66 => 1, - 67 => 1, - 68 => 1, - 72 => 1, - 73 => 1, - 74 => 1, - 75 => 1, - 79 => 1, - 80 => 1, - 81 => 1, - 82 => 1, - 83 => 1, - 84 => 1, - 88 => 1, - 92 => 1, - 93 => 1, + 43 => 1, + 44 => 1, + 45 => 1, + 46 => 1, + 47 => 1, + 48 => 1, + 49 => 1, + 50 => 1, + 51 => 1, + 55 => 1, + 56 => 1, + 57 => 1, + 58 => 1, + 59 => 1, + 60 => 1, + 61 => 1, + 62 => 1, + 66 => 1, + 67 => 1, + 68 => 1, + 72 => 1, + 73 => 1, + 74 => 1, + 75 => 1, + 79 => 1, + 80 => 1, + 81 => 1, + 82 => 1, + 83 => 1, + 84 => 1, + 88 => 1, + 92 => 1, + 93 => 1, + 97 => 1, + 98 => 1, + 99 => 1, + 100 => 1, + 101 => 1, + 102 => 1, + 103 => 1, + 104 => 1, + 105 => 1, ]; default: From 735558f4b2eecc3de34af273587cf83ffab03a70 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 24 Jul 2025 04:50:17 +0200 Subject: [PATCH 11/21] 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/DeclarationCompatibilitySniff.php | 5 ++++- .../DeclarationCompatibilityUnitTest.1.inc | 22 +++++++++++++++++++ .../DeclarationCompatibilityUnitTest.2.inc | 13 +++++++++++ 3 files changed, 39 insertions(+), 1 deletion(-) diff --git a/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php b/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php index 7d4a2cbe..6ef0e928 100644 --- a/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php +++ b/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php @@ -263,8 +263,11 @@ protected function processTokenWithinScope( File $phpcsFile, $stackPtr, $currSco $extra_params = array_slice( $childParams, $parentParamCount - $childParamCount ); $all_extra_params_have_default = true; foreach ( $extra_params as $extra_param ) { - if ( array_key_exists( 'default', $extra_param ) === false || $extra_param['default'] !== 'true' ) { + if ( isset( $extra_param['default'] ) === false + && $extra_param['variable_length'] === false + ) { $all_extra_params_have_default = false; + break; } } if ( $all_extra_params_have_default === true ) { diff --git a/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.1.inc b/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.1.inc index 6e98e147..e569d873 100644 --- a/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.1.inc +++ b/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.1.inc @@ -114,3 +114,25 @@ class WidgetBadChildHasParamReferenceParentDoesNot extends WP_Widget { public function _register_one( &$number = -1 ) {} public function save_settings( &$settings ) {} } + +// It's perfectly valid for child classes to declare extra optional parameters. +class WidgetOkayExtraOptionalParams extends WP_Widget { + public function widget( $args, $instance, $extra_param = array() ) {} + public function update( $new_instance, $old_instance, ...$extra_param ) {} + public function form( $instance, $extra_param = true ) {} + public function WP_Widget( $id_base, $name, $widget_options = array(), $control_options = array(), $extra_param = array() ) {} + public function get_field_name( $field_name, $extra_param = 'default') {} + public function get_field_id( $field_name, $extra_param = 'id') {} + public function _register( $extra_param = false ) {} + public function _set( $number, $extra_param = 5 ) {} + public function _get_display_callback($paramA = 'A', $paramB = 'B' ) {} + public function _get_update_callback( $my_param = true, $my_param2 = 10) {} + public function _get_form_callback(...$extra_param) {} + public function is_preview( $is_preview = true ) {} + public function display_callback( $args, $widget_args = 1, $extra_param = [] ) {} + public function update_callback( $deprecated = 1, ...$extra_param ) {} + public function form_callback( $widget_args = 1, ...$extra_param ) {} + public function _register_one( $number = -1, $extra_param = []) {} + public function save_settings( $settings, $extra_param = 10.5) {} + public function get_settings( $settings = 'foo', $setting = self::BAR, ...$my_param ) {} +} diff --git a/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.2.inc b/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.2.inc index 547bc448..d65e771b 100644 --- a/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.2.inc +++ b/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.2.inc @@ -104,3 +104,16 @@ class WalkerBadChildHasParamReferenceParentDoesNot extends Walker_Category { public function get_number_of_root_elements( &$elements, ) {} public function unset_children( &$element, &$children_elements ) {} } + +// It's perfectly valid for child classes to declare extra optional parameters. +class WalkerOkayExtraOptionalParams extends Walker_CategoryDropdown { + public function start_lvl( &$output, $depth = 0, $args = array(), $extra_param = true ) {} + public function end_lvl( &$output, $depth = 0, $args = array(), ...$extra_param ) {} + public function start_el( &$output, $data_object, $depth = 0, $args = array(), $current_object_id = 0, $extra_param = array() ) {} + public function end_el( &$output, $data_object, $depth = 0, $args = array(), $extra_param = [], ) {} + public function display_element( $element, &$children_elements, $max_depth, $depth, $args, &$output, $extra_param = 'default' ) {} + public function walk( $elements, $max_depth, $extra_param = null, ...$args ) {} + public function paged_walk( $elements, $max_depth, $page_num, $per_page, $extra_param = null, ...$args ) {} + public function get_number_of_root_elements( $elements, $extra_param = 10, ) {} + public function unset_children( $element, &$children_elements, $extra_param = '', ) {} +} From d960ea43b64d1a497f09aea0bfa17e0387035317 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 24 Jul 2025 19:42:49 +0200 Subject: [PATCH 12/21] 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/DeclarationCompatibilitySniff.php | 38 ++++++++++++++++--- .../DeclarationCompatibilityUnitTest.1.inc | 29 ++++++++++++++ .../DeclarationCompatibilityUnitTest.2.inc | 20 ++++++++++ .../DeclarationCompatibilityUnitTest.php | 18 +++++++++ 4 files changed, 100 insertions(+), 5 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php b/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php index 6ef0e928..519b0051 100644 --- a/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php +++ b/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php @@ -259,6 +259,36 @@ protected function processTokenWithinScope( File $phpcsFile, $stackPtr, $currSco $parentParams = $this->methodSignatures[ $parentClassName ][ $methodName ]; $parentParamCount = count( $parentParams ); + /* + * If there are parameters, verify if the last parameter of both the parent and the child are variadic. + * Only the last parameter can be variadic and if the parent has this, the child must also, + * independently of potential extra optional parameters having been inserted before that last parameter. + * + * Also note that a child can make the last parameter variadic, even if the parent parameter was not. + * This will no longer trigger a warning since PHP 8.0. + */ + if ( $childParamCount > 0 && $parentParamCount > 0 ) { + $childLastParam = $childParams[ $childParamCount - 1 ]; + $parentLastParam = $parentParams[ array_keys( $parentParams )[ $parentParamCount - 1 ] ]; + + if ( ( isset( $parentLastParam['variable_length'] ) === true && $parentLastParam['variable_length'] === true ) + && $childLastParam['variable_length'] !== true + ) { + $this->addError( $phpcsFile, $stackPtr, $currScope, $originalParentClassName, $methodName, $childParams, $parentParams ); + return; + } + } + + if ( $childParamCount > 0 ) { + // Check that no other parameters in the child signature are declared as variadic. + for ( $i = 0; $i < ( $childParamCount - 1 ); $i++ ) { + if ( $childParams[ $i ]['variable_length'] === true ) { + $this->addError( $phpcsFile, $stackPtr, $currScope, $originalParentClassName, $methodName, $childParams, $parentParams ); + return; + } + } + } + if ( $childParamCount > $parentParamCount ) { $extra_params = array_slice( $childParams, $parentParamCount - $childParamCount ); $all_extra_params_have_default = true; @@ -284,8 +314,9 @@ protected function processTokenWithinScope( File $phpcsFile, $stackPtr, $currSco foreach ( $parentParams as $key => $param ) { if ( ( - array_key_exists( 'default', $param ) === true && - array_key_exists( 'default', $childParams[ $i ] ) === false + array_key_exists( 'default', $param ) === true + && array_key_exists( 'default', $childParams[ $i ] ) === false + && $childParams[ $i ]['variable_length'] === false ) || ( // Parameter in parent class has reference, child does not. array_key_exists( 'pass_by_reference', $param ) === true && @@ -294,9 +325,6 @@ protected function processTokenWithinScope( File $phpcsFile, $stackPtr, $currSco // Parameter in parent class does *not* have reference, child does. array_key_exists( 'pass_by_reference', $param ) === false && $childParams[ $i ]['pass_by_reference'] === true - ) || ( - array_key_exists( 'variable_length', $param ) === true && - $param['variable_length'] !== $childParams[ $i ]['variable_length'] ) ) { $this->addError( $phpcsFile, $stackPtr, $currScope, $originalParentClassName, $methodName, $childParams, $parentParams ); diff --git a/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.1.inc b/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.1.inc index e569d873..71fba1ba 100644 --- a/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.1.inc +++ b/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.1.inc @@ -136,3 +136,32 @@ class WidgetOkayExtraOptionalParams extends WP_Widget { public function save_settings( $settings, $extra_param = 10.5) {} public function get_settings( $settings = 'foo', $setting = self::BAR, ...$my_param ) {} } + +class WidgetOkayChildCanMakeLastParamVariadic extends WP_Widget { + public function widget( $args, ...$instance ) {} + public function update( $new_instance, ...$old_instance ) {} + public function form( ...$instance ) {} + public function WP_Widget( $id_base, $name, $widget_options = array(), ...$control_options ) {} + public function get_field_name( ...$field_name ) {} + public function get_field_id( ...$field_name ) {} + public function _set( ...$number ) {} + public function display_callback( $args, ...$widget_args ) {} + public function update_callback( ...$deprecated ) {} + public function form_callback( ...$widget_args ) {} + public function _register_one( ...$number ) {} + public function save_settings( ...$settings ) {} +} + +class WidgetBadVariadicParamNotLast extends WP_Widget { + public function widget( ...$args, $instance ) {} + public function update( ...$new_instance, $old_instance ) {} + public function WP_Widget( $id_base, $name, ...$widget_options = array(), $control_options = array() ) {} + public function get_field_name( ...$field_name, $extra_param = null ) {} + public function get_field_id( ...$field_name, $extra_param = null ) {} + public function _set( ...$number, $extra_param = null ) {} + public function display_callback( ...$args, $widget_args = 1 ) {} + public function update_callback( ...$deprecated, $extra_param = null ) {} + public function form_callback( ...$widget_args, $extra_param = null ) {} + public function _register_one( ...$number, $extra_param = null ) {} + public function save_settings( ...$settings, $extra_param = null ) {} +} diff --git a/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.2.inc b/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.2.inc index d65e771b..e9495d11 100644 --- a/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.2.inc +++ b/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.2.inc @@ -117,3 +117,23 @@ class WalkerOkayExtraOptionalParams extends Walker_CategoryDropdown { public function get_number_of_root_elements( $elements, $extra_param = 10, ) {} public function unset_children( $element, &$children_elements, $extra_param = '', ) {} } + +class WalkerOkayChildCanMakeLastParamVariadic extends Walker_Page { + public function start_lvl( &$output, $depth = 0, ...$args ) {} + public function end_lvl( &$output, $depth = 0, ...$args ) {} + public function start_el( &$output, $data_object, $depth = 0, $args = array(), ...$current_object_id ) {} + public function end_el( &$output, $data_object, $depth = 0, ...$args, ) {} + public function display_element( $element, &$children_elements, $max_depth, $depth, $args, &...$output ) {} + public function get_number_of_root_elements( ...$elements, ) {} + public function unset_children( $element, &...$children_elements ) {} +} + +class WalkerBadVariadicParamNotLast extends Walker_Nav_Menu { + public function start_lvl( &$output, ...$depth, $args = array() ) {} + public function end_lvl( &...$output, $depth = 0, $args = array() ) {} + public function start_el( &$output, ...$data_object, $depth = 0, $args = array(), $current_object_id = 0 ) {} + public function end_el( &$output, $data_object, ...$depth, $args = array(), ) {} + public function display_element( $element, &$children_elements, ...$max_depth, $depth, $args, &$output ) {} + public function get_number_of_root_elements( ...$elements, $extra_param = 10, ) {} + public function unset_children( ...$element, &$children_elements ) {} +} diff --git a/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.php b/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.php index 678cc860..0f97bcfb 100644 --- a/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.php +++ b/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.php @@ -78,6 +78,17 @@ public function getErrorList( $testFile = '' ) { 113 => 1, 114 => 1, 115 => 1, + 156 => 1, + 157 => 1, + 158 => 1, + 159 => 1, + 160 => 1, + 161 => 1, + 162 => 1, + 163 => 1, + 164 => 1, + 165 => 1, + 166 => 1, ]; case 'DeclarationCompatibilityUnitTest.2.inc': @@ -124,6 +135,13 @@ public function getErrorList( $testFile = '' ) { 103 => 1, 104 => 1, 105 => 1, + 132 => 1, + 133 => 1, + 134 => 1, + 135 => 1, + 136 => 1, + 137 => 1, + 138 => 1, ]; default: From 08a7195dac111319585ccb03466c833c4bcf271b Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 24 Jul 2025 19:46:18 +0200 Subject: [PATCH 13/21] Classes/DeclarationCompatibility: minor code consistency tweak --- .../Sniffs/Classes/DeclarationCompatibilitySniff.php | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php b/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php index 519b0051..fbee2d25 100644 --- a/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php +++ b/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php @@ -319,12 +319,12 @@ protected function processTokenWithinScope( File $phpcsFile, $stackPtr, $currSco && $childParams[ $i ]['variable_length'] === false ) || ( // Parameter in parent class has reference, child does not. - array_key_exists( 'pass_by_reference', $param ) === true && - $param['pass_by_reference'] !== $childParams[ $i ]['pass_by_reference'] + array_key_exists( 'pass_by_reference', $param ) === true + && $param['pass_by_reference'] !== $childParams[ $i ]['pass_by_reference'] ) || ( // Parameter in parent class does *not* have reference, child does. - array_key_exists( 'pass_by_reference', $param ) === false && - $childParams[ $i ]['pass_by_reference'] === true + array_key_exists( 'pass_by_reference', $param ) === false + && $childParams[ $i ]['pass_by_reference'] === true ) ) { $this->addError( $phpcsFile, $stackPtr, $currScope, $originalParentClassName, $methodName, $childParams, $parentParams ); From 54a1580e1a3c0ec488cef4f8e061fba973eb9453 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 24 Jul 2025 20:00:50 +0200 Subject: [PATCH 14/21] 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. --- .../Sniffs/Classes/DeclarationCompatibilitySniff.php | 10 +++++----- .../Classes/DeclarationCompatibilityUnitTest.1.inc | 6 ++++++ .../Classes/DeclarationCompatibilityUnitTest.2.inc | 6 ++++++ .../Tests/Classes/DeclarationCompatibilityUnitTest.php | 4 ++++ 4 files changed, 21 insertions(+), 5 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php b/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php index fbee2d25..a2df322f 100644 --- a/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php +++ b/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php @@ -300,12 +300,12 @@ protected function processTokenWithinScope( File $phpcsFile, $stackPtr, $currSco break; } } - if ( $all_extra_params_have_default === true ) { - return; // We're good. - } - } - if ( $childParamCount !== $parentParamCount ) { + if ( $all_extra_params_have_default === false ) { + $this->addError( $phpcsFile, $stackPtr, $currScope, $originalParentClassName, $methodName, $childParams, $parentParams ); + return; + } + } elseif ( $childParamCount !== $parentParamCount ) { $this->addError( $phpcsFile, $stackPtr, $currScope, $originalParentClassName, $methodName, $childParams, $parentParams ); return; } diff --git a/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.1.inc b/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.1.inc index 71fba1ba..527406f3 100644 --- a/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.1.inc +++ b/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.1.inc @@ -165,3 +165,9 @@ class WidgetBadVariadicParamNotLast extends WP_Widget { public function _register_one( ...$number, $extra_param = null ) {} public function save_settings( ...$settings, $extra_param = null ) {} } + +// The fact that all extra parameters are optional, should not hide other errors. +class WidgetBadExtraOptionalParamsANDOtherError extends WP_Widget { + public function update_callback( $deprecated, $extra_param = [] ) {} // Missing default value for $deprecated. + public function form_callback( &$widget_args, ...$extra_param ) {} // Invalid reference for $widget_args. +} diff --git a/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.2.inc b/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.2.inc index e9495d11..74fcda6f 100644 --- a/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.2.inc +++ b/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.2.inc @@ -137,3 +137,9 @@ class WalkerBadVariadicParamNotLast extends Walker_Nav_Menu { public function get_number_of_root_elements( ...$elements, $extra_param = 10, ) {} public function unset_children( ...$element, &$children_elements ) {} } + +// The fact that all extra parameters are optional, should not hide other errors. +class WalkerBadExtraOptionalParamsANDOtherError extends Walker_Category_Checklist { + public function start_lvl( $output, $depth = 0, $args = array(), $extra_param = true ) {} // Missing reference for $output. + public function end_lvl( &$output, $depth, $args = array(), ...$extra_param ) {} // Missing default value for $depth. +} diff --git a/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.php b/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.php index 0f97bcfb..c4381f86 100644 --- a/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.php +++ b/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.php @@ -89,6 +89,8 @@ public function getErrorList( $testFile = '' ) { 164 => 1, 165 => 1, 166 => 1, + 171 => 1, + 172 => 1, ]; case 'DeclarationCompatibilityUnitTest.2.inc': @@ -142,6 +144,8 @@ public function getErrorList( $testFile = '' ) { 136 => 1, 137 => 1, 138 => 1, + 143 => 1, + 144 => 1, ]; default: From 931ec2081b366626731ecbca592563e5c73eb823 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 24 Jul 2025 22:51:19 +0200 Subject: [PATCH 15/21] 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/DeclarationCompatibilitySniff.php | 58 +++++++++++++++---- .../DeclarationCompatibilityUnitTest.1.inc | 44 +++++++------- .../DeclarationCompatibilityUnitTest.2.inc | 24 ++++---- 3 files changed, 80 insertions(+), 46 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php b/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php index a2df322f..a97ca548 100644 --- a/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php +++ b/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php @@ -214,10 +214,39 @@ class DeclarationCompatibilitySniff extends AbstractScopeSniff { 'Walker_Comment' => 'Walker', ]; + /** + * Translate from case-insensitive names to proper case method names. + * + * @var array> Primary key is the class name in proper case. + * Value is an array with method names in lowercase as keys + * and these same method names in proper case as values. + */ + private $methodToProperCase = []; + + /** + * Translate from case-insensitive names to proper case class names. + * + * @var array Key is the lowercase name of a class, value the proper case. + */ + private $classToProperCase = []; + /** * Constructs the test with the tokens it wishes to listen for. */ public function __construct() { + // Lowercase all names to allow for correct comparisons, as PHP treats class/function names case-insensitively. + // But also store translation tables to be able to get the proper case. + foreach ( $this->methodSignatures as $key => $value ) { + $methodNames = array_keys( $value ); + $this->methodToProperCase[ $key ] = array_change_key_case( array_combine( $methodNames, $methodNames ), CASE_LOWER ); + + $this->methodSignatures[ $key ] = array_change_key_case( $value, CASE_LOWER ); + } + + $classNames = array_keys( $this->extendedClassToSignatures ); + $this->classToProperCase = array_change_key_case( array_combine( $classNames, $classNames ), CASE_LOWER ); + $this->extendedClassToSignatures = array_change_key_case( $this->extendedClassToSignatures, CASE_LOWER ); + parent::__construct( [ T_CLASS ], [ T_FUNCTION ], false ); } @@ -232,7 +261,8 @@ public function __construct() { */ protected function processTokenWithinScope( File $phpcsFile, $stackPtr, $currScope ) { - $methodName = FunctionDeclarations::getName( $phpcsFile, $stackPtr ); + $methodName = FunctionDeclarations::getName( $phpcsFile, $stackPtr ); + $methodNameLC = strtolower( $methodName ); $parentClassName = ObjectDeclarations::findExtendedClassName( $phpcsFile, $currScope ); if ( $parentClassName === false ) { @@ -240,23 +270,27 @@ protected function processTokenWithinScope( File $phpcsFile, $stackPtr, $currSco return; } - // Store the originalParentClassName since we might override the parentClassName due to signature notations grouping. - $originalParentClassName = $parentClassName; - if ( isset( $this->extendedClassToSignatures[ $parentClassName ] ) === false ) { + $parentClassNameLC = strtolower( $parentClassName ); + if ( isset( $this->extendedClassToSignatures[ $parentClassNameLC ] ) === false ) { // This class does not extend a class we are interested in. return; } - $parentClassName = $this->extendedClassToSignatures[ $parentClassName ]; - if ( isset( $this->methodSignatures[ $parentClassName ][ $methodName ] ) === false ) { + // Store the originalParentClassName since we might override the parentClassName due to signature notations grouping. + $originalParentClassNamePC = $this->classToProperCase[ $parentClassNameLC ]; + + $parentClassName = $this->extendedClassToSignatures[ $parentClassNameLC ]; + if ( isset( $this->methodSignatures[ $parentClassName ][ $methodNameLC ] ) === false ) { // This method is not one we are interested in. return; } + $methodNamePC = $this->methodToProperCase[ $parentClassName ][ $methodNameLC ]; + $childParams = FunctionDeclarations::getParameters( $phpcsFile, $stackPtr ); $childParamCount = count( $childParams ); - $parentParams = $this->methodSignatures[ $parentClassName ][ $methodName ]; + $parentParams = $this->methodSignatures[ $parentClassName ][ $methodNameLC ]; $parentParamCount = count( $parentParams ); /* @@ -274,7 +308,7 @@ protected function processTokenWithinScope( File $phpcsFile, $stackPtr, $currSco if ( ( isset( $parentLastParam['variable_length'] ) === true && $parentLastParam['variable_length'] === true ) && $childLastParam['variable_length'] !== true ) { - $this->addError( $phpcsFile, $stackPtr, $currScope, $originalParentClassName, $methodName, $childParams, $parentParams ); + $this->addError( $phpcsFile, $stackPtr, $currScope, $originalParentClassNamePC, $methodNamePC, $childParams, $parentParams ); return; } } @@ -283,7 +317,7 @@ protected function processTokenWithinScope( File $phpcsFile, $stackPtr, $currSco // Check that no other parameters in the child signature are declared as variadic. for ( $i = 0; $i < ( $childParamCount - 1 ); $i++ ) { if ( $childParams[ $i ]['variable_length'] === true ) { - $this->addError( $phpcsFile, $stackPtr, $currScope, $originalParentClassName, $methodName, $childParams, $parentParams ); + $this->addError( $phpcsFile, $stackPtr, $currScope, $originalParentClassNamePC, $methodNamePC, $childParams, $parentParams ); return; } } @@ -302,11 +336,11 @@ protected function processTokenWithinScope( File $phpcsFile, $stackPtr, $currSco } if ( $all_extra_params_have_default === false ) { - $this->addError( $phpcsFile, $stackPtr, $currScope, $originalParentClassName, $methodName, $childParams, $parentParams ); + $this->addError( $phpcsFile, $stackPtr, $currScope, $originalParentClassNamePC, $methodNamePC, $childParams, $parentParams ); return; } } elseif ( $childParamCount !== $parentParamCount ) { - $this->addError( $phpcsFile, $stackPtr, $currScope, $originalParentClassName, $methodName, $childParams, $parentParams ); + $this->addError( $phpcsFile, $stackPtr, $currScope, $originalParentClassNamePC, $methodNamePC, $childParams, $parentParams ); return; } @@ -327,7 +361,7 @@ protected function processTokenWithinScope( File $phpcsFile, $stackPtr, $currSco && $childParams[ $i ]['pass_by_reference'] === true ) ) { - $this->addError( $phpcsFile, $stackPtr, $currScope, $originalParentClassName, $methodName, $childParams, $parentParams ); + $this->addError( $phpcsFile, $stackPtr, $currScope, $originalParentClassNamePC, $methodNamePC, $childParams, $parentParams ); return; } ++$i; diff --git a/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.1.inc b/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.1.inc index 527406f3..34216176 100644 --- a/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.1.inc +++ b/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.1.inc @@ -64,35 +64,35 @@ class WidgetBadTooFewParamsNoParam extends WP_Widget { public function save_settings() {} // Bad, missing $settings param. } -final class WidgetBadTooFewParamsNotAllParams extends WP_Widget { +final class WidgetBadTooFewParamsNotAllParams extends WP_WIDGET { public function widget( $args ) {} // Bad, missing $instance param. public function update( $new_instance ) {} // Bad, missing $old_instance param. public function WP_Widget( $id_base, $name, ) {} // Bad, missing $widget_options and $control_options params. public function display_callback( $args ) {} // Bad, missing $widget_args param. } -class WidgetBadExtraRequiredParams extends WP_Widget { - public function widget( $args, $instance, $extra_param ) {} - public function update( $new_instance, $old_instance, $extra_param ) {} - public function form( $instance, $extra_param ) {} - public function WP_Widget( $id_base, $name, $widget_options = array(), $control_options = array(), $extra_param) {} - public function get_field_name( $field_name, $extra_param ) {} - public function get_field_id( $field_name, $extra_param ) {} - public function _register( $extra_param ) {} - public function _set( $number, $extra_param ) {} - public function _get_display_callback($extra_paramA, $extra_paramB) {} - public function _get_update_callback( $extra_paramA, $extra_paramB ) {} - public function _get_form_callback($extra_param) {} - public function is_preview($extra_param) {} - public function display_callback( $args, $widget_args = 1, $extra_param ) {} - public function update_callback( $deprecated = 1, $extra_param, ) {} - public function form_callback( $widget_args = 1, $extra_param ) {} - public function _register_one( $number = -1, $extra_param ) {} - public function save_settings( $settings, $extra_param) {} - public function get_settings( $extra_paramA, $extra_paramB, $extra_paramC ) {} +class WidgetBadExtraRequiredParamsAndMethodNamesAreCaseInsensitive extends WP_Widget { + public function Widget( $args, $instance, $extra_param ) {} + public function Update( $new_instance, $old_instance, $extra_param ) {} + public function Form( $intance, $extra_param ) {} + public function wp_widget( $id_base, $name, $widget_options = array(), $control_options = array(), $extra_param) {} + public function Get_Field_Name( $field_name, $extra_param ) {} + public function Get_Field_ID( $field_name, $extra_param ) {} + public function _Register( $extra_param ) {} + public function _Set( $number, $extra_param ) {} + public function _Get_Display_Callback($extra_paramA, $extra_paramB) {} + public function _Get_Update_Callback( $extra_paramA, $extra_paramB ) {} + public function _Get_Form_Callback($extra_param) {} + public function is_Preview($extra_param) {} + public function display_Callback( $args, $widget_args = 1, $extra_param ) {} + public function update_Callback( $deprecated = 1, $extra_param, ) {} + public function form_Callback( $widget_args = 1, $extra_param ) {} + public function _register_ONE( $number = -1, $extra_param ) {} + public function SAVE_SETTINGS( $settings, $extra_param) {} + public function get_Settings( $extra_paramA, $extra_paramB, $extra_paramC ) {} }; -class WidgetBadOptionalParamChangedToRequiredMissingDefaultValue extends WP_Widget { +class WidgetBadOptionalParamChangedToRequiredMissingDefaultValue extends wp_widget { public function WP_Widget( $id_base, $name, $widget_options, $control_options = array() ) {} // Bad, missing default value for $widget_options. public function display_callback( $args, $widget_args ) {} // Bad, missing default value for $widget_args. public function update_callback( $deprecated ) {} @@ -152,7 +152,7 @@ class WidgetOkayChildCanMakeLastParamVariadic extends WP_Widget { public function save_settings( ...$settings ) {} } -class WidgetBadVariadicParamNotLast extends WP_Widget { +class WidgetBadVariadicParamNotLast extends Wp_widget { public function widget( ...$args, $instance ) {} public function update( ...$new_instance, $old_instance ) {} public function WP_Widget( $id_base, $name, ...$widget_options = array(), $control_options = array() ) {} diff --git a/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.2.inc b/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.2.inc index 74fcda6f..adfb8a39 100644 --- a/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.2.inc +++ b/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.2.inc @@ -39,19 +39,19 @@ class WalkerOkayDifferentDefaultValue extends Walker_Category_Checklist { public function end_el( &$output, $data_object, $depth = 10, $args = '' ) {} }; -class WalkerBadTooFewParamsNoParam extends Walker_Category { - public function start_lvl() {} - public function end_lvl() {} - public function start_el() {} - public function end_el() {} - public function display_element() {} - public function walk() {} - public function paged_walk() {} - public function get_number_of_root_elements() {} - public function unset_children() {} +class WalkerBadTooFewParamsNoParamAndMethodNamesAreCaseInsensitive extends Walker_Category { + public function start_LVL() {} + public function End_Lvl() {} + public function START_EL() {} + public function end_EL() {} + public function Display_Element() {} + public function WalK() {} + public function paged_WALK() {} + public function get_Number_Of_Root_Elements() {} + public function unset_Children() {} } -readonly class WalkerBadTooFewParamsNotAllParams extends Walker_CategoryDropdown { +readonly class WalkerBadTooFewParamsNotAllParams extends walker_categorydropdown { public function start_lvl( &$output, $depth = 0 ) {} // Bad, missing $args param. public function end_lvl( &$output ) {} // Bad, missing $depth and $args params. public function start_el( &$output, $data_object, $depth = 0, ) {} // Bad, missing $args and $current_object_id params. @@ -128,7 +128,7 @@ class WalkerOkayChildCanMakeLastParamVariadic extends Walker_Page { public function unset_children( $element, &...$children_elements ) {} } -class WalkerBadVariadicParamNotLast extends Walker_Nav_Menu { +class WalkerBadVariadicParamNotLast extends WALKER_NAV_MENU { public function start_lvl( &$output, ...$depth, $args = array() ) {} public function end_lvl( &...$output, $depth = 0, $args = array() ) {} public function start_el( &$output, ...$data_object, $depth = 0, $args = array(), $current_object_id = 0 ) {} From 724d67c55fd9b2adf8bf9ca35324cf8957920738 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 24 Jul 2025 23:24:13 +0200 Subject: [PATCH 16/21] Classes/DeclarationCompatibility: move some variable declarations ... to where they are actually needed/used. No need to have this information available before this point. --- .../Sniffs/Classes/DeclarationCompatibilitySniff.php | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php b/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php index a97ca548..ff9c3728 100644 --- a/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php +++ b/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php @@ -261,9 +261,6 @@ public function __construct() { */ protected function processTokenWithinScope( File $phpcsFile, $stackPtr, $currScope ) { - $methodName = FunctionDeclarations::getName( $phpcsFile, $stackPtr ); - $methodNameLC = strtolower( $methodName ); - $parentClassName = ObjectDeclarations::findExtendedClassName( $phpcsFile, $currScope ); if ( $parentClassName === false ) { // This class does not extend any other class. @@ -280,6 +277,8 @@ protected function processTokenWithinScope( File $phpcsFile, $stackPtr, $currSco $originalParentClassNamePC = $this->classToProperCase[ $parentClassNameLC ]; $parentClassName = $this->extendedClassToSignatures[ $parentClassNameLC ]; + $methodName = FunctionDeclarations::getName( $phpcsFile, $stackPtr ); + $methodNameLC = strtolower( $methodName ); if ( isset( $this->methodSignatures[ $parentClassName ][ $methodNameLC ] ) === false ) { // This method is not one we are interested in. return; From 5b168fe36e2eb1a923be3d6142e266dc51dbdc52 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 24 Jul 2025 23:48:30 +0200 Subject: [PATCH 17/21] 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. --- .../Sniffs/Classes/DeclarationCompatibilitySniff.php | 2 +- .../Tests/Classes/DeclarationCompatibilityUnitTest.1.inc | 4 ++-- .../Tests/Classes/DeclarationCompatibilityUnitTest.2.inc | 4 ++-- .../Tests/Classes/DeclarationCompatibilityUnitTest.3.inc | 8 ++++++++ 4 files changed, 13 insertions(+), 5 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php b/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php index ff9c3728..7997f723 100644 --- a/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php +++ b/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php @@ -267,7 +267,7 @@ protected function processTokenWithinScope( File $phpcsFile, $stackPtr, $currSco return; } - $parentClassNameLC = strtolower( $parentClassName ); + $parentClassNameLC = ltrim( strtolower( $parentClassName ), '\\' ); // Trim off potential FQN indicator. if ( isset( $this->extendedClassToSignatures[ $parentClassNameLC ] ) === false ) { // This class does not extend a class we are interested in. return; diff --git a/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.1.inc b/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.1.inc index 34216176..4bc4f04a 100644 --- a/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.1.inc +++ b/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.1.inc @@ -41,7 +41,7 @@ readonly class WidgetOkayDefaultValueWhereParentHasNone extends WP_Widget { public function save_settings( $settings = array() ) {} } -class WidgetOkayDifferentDefaultValue extends WP_Widget { +class WidgetOkayDifferentDefaultValue extends \WP_Widget { public function WP_Widget( $id_base, $name, $widget_options = null, $control_options = null ) {} public function display_callback( $args, $widget_args = [] ) {} public function update_callback( $deprecated = true, ) {} @@ -100,7 +100,7 @@ class WidgetBadOptionalParamChangedToRequiredMissingDefaultValue extends wp_widg public function _register_one( $number ) {} } -class WidgetBadChildHasParamReferenceParentDoesNot extends WP_Widget { +class WidgetBadChildHasParamReferenceParentDoesNot extends \WP_Widget { public function widget( $args, &$instance ) {} public function update( &$new_instance, $old_instance ) {} public function form( &$instance ) {} diff --git a/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.2.inc b/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.2.inc index adfb8a39..f39d2524 100644 --- a/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.2.inc +++ b/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.2.inc @@ -20,7 +20,7 @@ class WalkerOkay extends Walker { // These methods all change required parameters from the parent class into optional ones by adding a default value. // This will not cause a signature mismatch in PHP since PHP 5.3. -final class WalkerOkayDefaultValueWhereParentHasNone extends Walker_Page { +final class WalkerOkayDefaultValueWhereParentHasNone extends \Walker_Page { public function start_lvl( &$output = '', $depth = 0, $args = array() ) {} public function end_lvl( &$output = '', $depth = 0, $args = array() ) {} public function start_el( &$output = '', $data_object = new stdClass, $depth = 0, $args = array(), $current_object_id = 0 ) {} @@ -62,7 +62,7 @@ readonly class WalkerBadTooFewParamsNotAllParams extends walker_categorydropdown public function unset_children( $element ) {} // Bad, missing $children_elements param. } -class WalkerBadExtraRequiredParams extends Walker_PageDropdown { +class WalkerBadExtraRequiredParams extends \Walker_PageDropdown { public function display_element( $element, &$children_elements, $max_depth, $depth, $args, &$output, $extra_param ) {} public function get_number_of_root_elements( $elements, $extra_param, ) {} public function unset_children( $element, &$children_elements, $extra_paramA, $extra_paramB ) {} diff --git a/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.3.inc b/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.3.inc index 69ec3568..94d8fcaa 100644 --- a/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.3.inc +++ b/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.3.inc @@ -10,3 +10,11 @@ class DoesNotExtend { class ExtendsDifferentClass extends FooBar { function foo() {} } + +class ExtendsFullyQualifiedClass extends \Different\NS\WP_Widget { + function save_settings() {} +} + +class ExtendsPartiallyQualifiedClass extends Also\Different\NS\Walker_Page { + function save_settings() {} +} From e65437975bb74dc30425adceea4a561059f6719a Mon Sep 17 00:00:00 2001 From: jrfnl Date: Fri, 25 Jul 2025 03:33:05 +0200 Subject: [PATCH 18/21] 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: ```php 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: ```php 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/DeclarationCompatibilitySniff.php | 191 +++++++++--------- .../DeclarationCompatibilityUnitTest.3.inc | 17 ++ 2 files changed, 112 insertions(+), 96 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php b/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php index 7997f723..98a2c2b3 100644 --- a/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php +++ b/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php @@ -10,14 +10,14 @@ namespace WordPressVIPMinimum\Sniffs\Classes; use PHP_CodeSniffer\Files\File; -use PHP_CodeSniffer\Sniffs\AbstractScopeSniff; +use PHP_CodeSniffer\Sniffs\Sniff; use PHPCSUtils\Utils\FunctionDeclarations; use PHPCSUtils\Utils\ObjectDeclarations; /** * Class WordPressVIPMinimum_Sniffs_Classes_DeclarationCompatibilitySniff */ -class DeclarationCompatibilitySniff extends AbstractScopeSniff { +class DeclarationCompatibilitySniff implements Sniff { /** * A list of classes and methods to check. @@ -231,9 +231,11 @@ class DeclarationCompatibilitySniff extends AbstractScopeSniff { private $classToProperCase = []; /** - * Constructs the test with the tokens it wishes to listen for. + * Returns the token types that this sniff is interested in. + * + * @return array */ - public function __construct() { + public function register() { // Lowercase all names to allow for correct comparisons, as PHP treats class/function names case-insensitively. // But also store translation tables to be able to get the proper case. foreach ( $this->methodSignatures as $key => $value ) { @@ -247,21 +249,20 @@ public function __construct() { $this->classToProperCase = array_change_key_case( array_combine( $classNames, $classNames ), CASE_LOWER ); $this->extendedClassToSignatures = array_change_key_case( $this->extendedClassToSignatures, CASE_LOWER ); - parent::__construct( [ T_CLASS ], [ T_FUNCTION ], false ); + return [ T_CLASS ]; } /** - * Processes this test when one of its tokens is encountered. + * Processes the tokens that this sniff is interested in. * - * @param File $phpcsFile The PHP_CodeSniffer file where the token was found. - * @param int $stackPtr The position of the current token in the stack passed in $tokens. - * @param int $currScope A pointer to the start of the scope. + * @param \PHP_CodeSniffer\Files\File $phpcsFile The file being scanned. + * @param int $stackPtr The position of the current token + * in the stack passed in $tokens. * * @return void */ - protected function processTokenWithinScope( File $phpcsFile, $stackPtr, $currScope ) { - - $parentClassName = ObjectDeclarations::findExtendedClassName( $phpcsFile, $currScope ); + public function process( File $phpcsFile, $stackPtr ) { + $parentClassName = ObjectDeclarations::findExtendedClassName( $phpcsFile, $stackPtr ); if ( $parentClassName === false ) { // This class does not extend any other class. return; @@ -275,95 +276,101 @@ protected function processTokenWithinScope( File $phpcsFile, $stackPtr, $currSco // Store the originalParentClassName since we might override the parentClassName due to signature notations grouping. $originalParentClassNamePC = $this->classToProperCase[ $parentClassNameLC ]; + $parentClassName = $this->extendedClassToSignatures[ $parentClassNameLC ]; - $parentClassName = $this->extendedClassToSignatures[ $parentClassNameLC ]; - $methodName = FunctionDeclarations::getName( $phpcsFile, $stackPtr ); - $methodNameLC = strtolower( $methodName ); - if ( isset( $this->methodSignatures[ $parentClassName ][ $methodNameLC ] ) === false ) { - // This method is not one we are interested in. + $methods = ObjectDeclarations::getDeclaredMethods( $phpcsFile, $stackPtr ); + if ( empty( $methods ) ) { return; } - $methodNamePC = $this->methodToProperCase[ $parentClassName ][ $methodNameLC ]; - - $childParams = FunctionDeclarations::getParameters( $phpcsFile, $stackPtr ); - $childParamCount = count( $childParams ); - - $parentParams = $this->methodSignatures[ $parentClassName ][ $methodNameLC ]; - $parentParamCount = count( $parentParams ); - - /* - * If there are parameters, verify if the last parameter of both the parent and the child are variadic. - * Only the last parameter can be variadic and if the parent has this, the child must also, - * independently of potential extra optional parameters having been inserted before that last parameter. - * - * Also note that a child can make the last parameter variadic, even if the parent parameter was not. - * This will no longer trigger a warning since PHP 8.0. - */ - if ( $childParamCount > 0 && $parentParamCount > 0 ) { - $childLastParam = $childParams[ $childParamCount - 1 ]; - $parentLastParam = $parentParams[ array_keys( $parentParams )[ $parentParamCount - 1 ] ]; - - if ( ( isset( $parentLastParam['variable_length'] ) === true && $parentLastParam['variable_length'] === true ) - && $childLastParam['variable_length'] !== true - ) { - $this->addError( $phpcsFile, $stackPtr, $currScope, $originalParentClassNamePC, $methodNamePC, $childParams, $parentParams ); - return; + foreach ( $methods as $methodName => $functionPtr ) { + $methodNameLC = strtolower( $methodName ); + if ( isset( $this->methodSignatures[ $parentClassName ][ $methodNameLC ] ) === false ) { + // This method is not one we are interested in. + continue; } - } - if ( $childParamCount > 0 ) { - // Check that no other parameters in the child signature are declared as variadic. - for ( $i = 0; $i < ( $childParamCount - 1 ); $i++ ) { - if ( $childParams[ $i ]['variable_length'] === true ) { - $this->addError( $phpcsFile, $stackPtr, $currScope, $originalParentClassNamePC, $methodNamePC, $childParams, $parentParams ); - return; + $methodNamePC = $this->methodToProperCase[ $parentClassName ][ $methodNameLC ]; + + $childParams = FunctionDeclarations::getParameters( $phpcsFile, $functionPtr ); + $childParamCount = count( $childParams ); + + $parentParams = $this->methodSignatures[ $parentClassName ][ $methodNameLC ]; + $parentParamCount = count( $parentParams ); + + /* + * If there are parameters, verify if the last parameter of both the parent and the child are variadic. + * Only the last parameter can be variadic and if the parent has this, the child must also, + * independently of potential extra optional parameters having been inserted before that last parameter. + * + * Also note that a child can make the last parameter variadic, even if the parent parameter was not. + * This will no longer trigger a warning since PHP 8.0. + */ + if ( $childParamCount > 0 && $parentParamCount > 0 ) { + $childLastParam = $childParams[ $childParamCount - 1 ]; + $parentLastParam = $parentParams[ array_keys( $parentParams )[ $parentParamCount - 1 ] ]; + + if ( ( isset( $parentLastParam['variable_length'] ) === true && $parentLastParam['variable_length'] === true ) + && $childLastParam['variable_length'] !== true + ) { + $this->addError( $phpcsFile, $functionPtr, $stackPtr, $originalParentClassNamePC, $methodNamePC, $childParams, $parentParams ); + continue; } } - } - if ( $childParamCount > $parentParamCount ) { - $extra_params = array_slice( $childParams, $parentParamCount - $childParamCount ); - $all_extra_params_have_default = true; - foreach ( $extra_params as $extra_param ) { - if ( isset( $extra_param['default'] ) === false - && $extra_param['variable_length'] === false - ) { - $all_extra_params_have_default = false; - break; + if ( $childParamCount > 0 ) { + // Check that no other parameters in the child signature are declared as variadic. + for ( $i = 0; $i < ( $childParamCount - 1 ); $i++ ) { + if ( $childParams[ $i ]['variable_length'] === true ) { + $this->addError( $phpcsFile, $functionPtr, $stackPtr, $originalParentClassNamePC, $methodNamePC, $childParams, $parentParams ); + continue 2; + } } } - if ( $all_extra_params_have_default === false ) { - $this->addError( $phpcsFile, $stackPtr, $currScope, $originalParentClassNamePC, $methodNamePC, $childParams, $parentParams ); - return; + if ( $childParamCount > $parentParamCount ) { + $extra_params = array_slice( $childParams, $parentParamCount - $childParamCount ); + $all_extra_params_have_default = true; + foreach ( $extra_params as $extra_param ) { + if ( isset( $extra_param['default'] ) === false + && $extra_param['variable_length'] === false + ) { + $all_extra_params_have_default = false; + break; + } + } + + if ( $all_extra_params_have_default === false ) { + $this->addError( $phpcsFile, $functionPtr, $stackPtr, $originalParentClassNamePC, $methodNamePC, $childParams, $parentParams ); + continue; + } + } elseif ( $childParamCount !== $parentParamCount ) { + $this->addError( $phpcsFile, $functionPtr, $stackPtr, $originalParentClassNamePC, $methodNamePC, $childParams, $parentParams ); + continue; } - } elseif ( $childParamCount !== $parentParamCount ) { - $this->addError( $phpcsFile, $stackPtr, $currScope, $originalParentClassNamePC, $methodNamePC, $childParams, $parentParams ); - return; - } - $i = 0; - foreach ( $parentParams as $key => $param ) { - if ( - ( - array_key_exists( 'default', $param ) === true - && array_key_exists( 'default', $childParams[ $i ] ) === false - && $childParams[ $i ]['variable_length'] === false - ) || ( - // Parameter in parent class has reference, child does not. - array_key_exists( 'pass_by_reference', $param ) === true - && $param['pass_by_reference'] !== $childParams[ $i ]['pass_by_reference'] - ) || ( - // Parameter in parent class does *not* have reference, child does. - array_key_exists( 'pass_by_reference', $param ) === false - && $childParams[ $i ]['pass_by_reference'] === true - ) - ) { - $this->addError( $phpcsFile, $stackPtr, $currScope, $originalParentClassNamePC, $methodNamePC, $childParams, $parentParams ); - return; + $i = 0; + foreach ( $parentParams as $key => $param ) { + if ( + ( + array_key_exists( 'default', $param ) === true + && array_key_exists( 'default', $childParams[ $i ] ) === false + && $childParams[ $i ]['variable_length'] === false + ) || ( + // Parameter in parent class has reference, child does not. + array_key_exists( 'pass_by_reference', $param ) === true + && $param['pass_by_reference'] !== $childParams[ $i ]['pass_by_reference'] + ) || ( + // Parameter in parent class does *not* have reference, child does. + array_key_exists( 'pass_by_reference', $param ) === false + && $childParams[ $i ]['pass_by_reference'] === true + ) + ) { + $this->addError( $phpcsFile, $functionPtr, $stackPtr, $originalParentClassNamePC, $methodNamePC, $childParams, $parentParams ); + continue 2; + } + ++$i; } - ++$i; } } @@ -371,8 +378,8 @@ protected function processTokenWithinScope( File $phpcsFile, $stackPtr, $currSco * Generates an error with nice current and parent class method notations * * @param File $phpcsFile The PHP_CodeSniffer file where the token was found. - * @param int $stackPtr The position of the current token in the stack. - * @param int $currScope A pointer to the start of the scope. + * @param int $stackPtr The position of the current T_FUNCTION token in the stack. + * @param int $currScope A pointer to the start of the OO scope. * @param string $parentClassName The name of the extended (parent) class. * @param string $methodName The name of the method currently being examined. * @param array $currentMethodSignature The list of params and their options of the method which is being examined. @@ -433,12 +440,4 @@ private function generateParamList( $methodSignature ) { return $paramList; } - - /** - * Do nothing outside the scope. Has to be implemented accordingly to parent abstract class. - * - * @param File $phpcsFile PHPCS File. - * @param int $stackPtr Stack position. - */ - public function processTokenOutsideScope( File $phpcsFile, $stackPtr ) {} } diff --git a/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.3.inc b/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.3.inc index 94d8fcaa..b82b03c0 100644 --- a/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.3.inc +++ b/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.3.inc @@ -18,3 +18,20 @@ class ExtendsFullyQualifiedClass extends \Different\NS\WP_Widget { class ExtendsPartiallyQualifiedClass extends Also\Different\NS\Walker_Page { function save_settings() {} } + +class NoMethods extends WP_Widget {} + +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) {} + } +} + +class WidgetOkayNestedClassDoesNotExtend extends WP_Widget { + function hasNestedClass() { + return new class { + public function display_callback() {} + }; + } +} From d9e13cc4259e82a84db2ad4eed183db055973dce Mon Sep 17 00:00:00 2001 From: jrfnl Date: Fri, 25 Jul 2025 03:38:32 +0200 Subject: [PATCH 19/21] 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/DeclarationCompatibilitySniff.php | 11 ++++++++-- .../DeclarationCompatibilityUnitTest.1.inc | 20 +++++++++++++++++-- .../DeclarationCompatibilityUnitTest.2.inc | 4 ++-- .../DeclarationCompatibilityUnitTest.php | 2 ++ 4 files changed, 31 insertions(+), 6 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php b/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php index 98a2c2b3..d603aa22 100644 --- a/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php +++ b/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php @@ -249,7 +249,10 @@ public function register() { $this->classToProperCase = array_change_key_case( array_combine( $classNames, $classNames ), CASE_LOWER ); $this->extendedClassToSignatures = array_change_key_case( $this->extendedClassToSignatures, CASE_LOWER ); - return [ T_CLASS ]; + return [ + T_CLASS, + T_ANON_CLASS, + ]; } /** @@ -388,7 +391,11 @@ public function process( File $phpcsFile, $stackPtr ) { * @return void */ private function addError( File $phpcsFile, $stackPtr, $currScope, $parentClassName, $methodName, $currentMethodSignature, $parentMethodSignature ) { - $currentClassName = ObjectDeclarations::getName( $phpcsFile, $currScope ); + $tokens = $phpcsFile->getTokens(); + $currentClassName = '[AnonymousClass]'; + if ( $tokens[ $currScope ]['code'] !== T_ANON_CLASS ) { + $currentClassName = ObjectDeclarations::getName( $phpcsFile, $currScope ); + } $currentSignature = implode( ', ', $this->generateParamList( $currentMethodSignature ) ); $currentSignature = sprintf( '%s::%s(%s)', $currentClassName, $methodName, $currentSignature ); diff --git a/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.1.inc b/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.1.inc index 4bc4f04a..8ae13ced 100644 --- a/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.1.inc +++ b/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.1.inc @@ -41,7 +41,7 @@ readonly class WidgetOkayDefaultValueWhereParentHasNone extends WP_Widget { public function save_settings( $settings = array() ) {} } -class WidgetOkayDifferentDefaultValue extends \WP_Widget { +$WidgetOkayDifferentDefaultValue = new #[MyAttribute] readonly class extends \WP_Widget { public function WP_Widget( $id_base, $name, $widget_options = null, $control_options = null ) {} public function display_callback( $args, $widget_args = [] ) {} public function update_callback( $deprecated = true, ) {} @@ -71,7 +71,7 @@ final class WidgetBadTooFewParamsNotAllParams extends WP_WIDGET { public function display_callback( $args ) {} // Bad, missing $widget_args param. } -class WidgetBadExtraRequiredParamsAndMethodNamesAreCaseInsensitive extends WP_Widget { +$WidgetBadExtraRequiredParamsAndMethodNamesAreCaseInsensitive = new class() extends WP_Widget { public function Widget( $args, $instance, $extra_param ) {} public function Update( $new_instance, $old_instance, $extra_param ) {} public function Form( $intance, $extra_param ) {} @@ -171,3 +171,19 @@ class WidgetBadExtraOptionalParamsANDOtherError extends WP_Widget { public function update_callback( $deprecated, $extra_param = [] ) {} // Missing default value for $deprecated. public function form_callback( &$widget_args, ...$extra_param ) {} // Invalid reference for $widget_args. } + +class WidgetBadNestedClassExtends { + function nested() { + return new class extends WP_Widget { + public function display_callback() {} + }; + } +} + +class WidgetBadNestedClassAlsoExtends extends WP_Widget { + function nested() { + return new class extends WP_Widget { + public function display_callback() {} + }; + } +} diff --git a/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.2.inc b/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.2.inc index f39d2524..8317e84d 100644 --- a/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.2.inc +++ b/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.2.inc @@ -32,7 +32,7 @@ final class WalkerOkayDefaultValueWhereParentHasNone extends \Walker_Page { public function unset_children( $element = new stdClass, &$children_elements = array(), ) {} } -class WalkerOkayDifferentDefaultValue extends Walker_Category_Checklist { +$WalkerOkayDifferentDefaultValue = new class() extends Walker_Category_Checklist { public function start_lvl( &$output, $depth = 2, $args = null ) {} public function end_lvl( &$output, $depth = 1, $args = new stdClass ) {} public function start_el( &$output, $data_object, $depth = 5, $args = null, $current_object_id = 10 ) {} @@ -62,7 +62,7 @@ readonly class WalkerBadTooFewParamsNotAllParams extends walker_categorydropdown public function unset_children( $element ) {} // Bad, missing $children_elements param. } -class WalkerBadExtraRequiredParams extends \Walker_PageDropdown { +$WalkerBadExtraRequiredParams = new #[MyAttribute] readonly class extends \Walker_PageDropdown { public function display_element( $element, &$children_elements, $max_depth, $depth, $args, &$output, $extra_param ) {} public function get_number_of_root_elements( $elements, $extra_param, ) {} public function unset_children( $element, &$children_elements, $extra_paramA, $extra_paramB ) {} diff --git a/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.php b/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.php index c4381f86..7f38c00c 100644 --- a/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.php +++ b/WordPressVIPMinimum/Tests/Classes/DeclarationCompatibilityUnitTest.php @@ -91,6 +91,8 @@ public function getErrorList( $testFile = '' ) { 166 => 1, 171 => 1, 172 => 1, + 178 => 1, + 186 => 1, ]; case 'DeclarationCompatibilityUnitTest.2.inc': From a900e2fd375d874dcc7661b1235c12d631b879c4 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Fri, 25 Jul 2025 03:52:12 +0200 Subject: [PATCH 20/21] 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/DeclarationCompatibilitySniff.php | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php b/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php index d603aa22..af3ae353 100644 --- a/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php +++ b/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php @@ -353,19 +353,20 @@ public function process( File $phpcsFile, $stackPtr ) { } $i = 0; - foreach ( $parentParams as $key => $param ) { + foreach ( $parentParams as $param ) { if ( ( - array_key_exists( 'default', $param ) === true - && array_key_exists( 'default', $childParams[ $i ] ) === false + isset( $param['default'] ) === true + && isset( $childParams[ $i ]['default'] ) === false && $childParams[ $i ]['variable_length'] === false ) || ( // Parameter in parent class has reference, child does not. - array_key_exists( 'pass_by_reference', $param ) === true + isset( $param['pass_by_reference'] ) === true && $param['pass_by_reference'] !== $childParams[ $i ]['pass_by_reference'] ) || ( // Parameter in parent class does *not* have reference, child does. - array_key_exists( 'pass_by_reference', $param ) === false + ( isset( $param['pass_by_reference'] ) === false + || $param['pass_by_reference'] === false ) && $childParams[ $i ]['pass_by_reference'] === true ) ) { @@ -424,21 +425,21 @@ private function generateParamList( $methodSignature ) { continue; } - if ( array_key_exists( 'name', $options ) === true ) { + if ( isset( $options['name'] ) === true ) { $paramName = $options['name']; } else { $paramName .= $param; } - if ( array_key_exists( 'variable_length', $options ) === true && $options['variable_length'] === true ) { + if ( isset( $options['variable_length'] ) === true && $options['variable_length'] === true ) { $paramName = '...' . $paramName; } - if ( array_key_exists( 'pass_by_reference', $options ) === true && $options['pass_by_reference'] === true ) { + if ( isset( $options['pass_by_reference'] ) === true && $options['pass_by_reference'] === true ) { $paramName = '&' . $paramName; } - if ( array_key_exists( 'default', $options ) === true && empty( $options['default'] ) === false ) { + if ( isset( $options['default'] ) === true && empty( $options['default'] ) === false ) { $paramName .= ' = ' . trim( $options['default'] ); } From 632d810292e4950fc698e5814dfb26d6e0684f9c Mon Sep 17 00:00:00 2001 From: jrfnl Date: Fri, 25 Jul 2025 04:16:37 +0200 Subject: [PATCH 21/21] Classes/DeclarationCompatibility: minor documentation improvements --- .../Classes/DeclarationCompatibilitySniff.php | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php b/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php index af3ae353..10381feb 100644 --- a/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php +++ b/WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php @@ -29,7 +29,7 @@ class DeclarationCompatibilitySniff implements Sniff { public $checkClasses = []; /** - * List of grouped classes with same methods (as they extend the same parent class) + * List of grouped classes with same methods (as they extend the same parent class). * * @deprecated 3.1.0 This should never have been a public property. * @@ -379,15 +379,16 @@ public function process( File $phpcsFile, $stackPtr ) { } /** - * Generates an error with nice current and parent class method notations + * Generates an error with nice current and parent class method notations. * - * @param File $phpcsFile The PHP_CodeSniffer file where the token was found. - * @param int $stackPtr The position of the current T_FUNCTION token in the stack. - * @param int $currScope A pointer to the start of the OO scope. - * @param string $parentClassName The name of the extended (parent) class. - * @param string $methodName The name of the method currently being examined. - * @param array $currentMethodSignature The list of params and their options of the method which is being examined. - * @param array $parentMethodSignature The list of params and their options of the parent class method. + * @param File $phpcsFile The PHP_CodeSniffer file where the token was found. + * @param int $stackPtr The position of the current T_FUNCTION token in the stack. + * @param int $currScope A pointer to the start of the OO scope. + * @param string $parentClassName The name of the extended (parent) class. + * @param string $methodName The name of the method currently being examined. + * @param array> $currentMethodSignature The list of params and their options of the method + * which is being examined. + * @param array> $parentMethodSignature The list of params and their options of the parent class method. * * @return void */