From 778838e3554dfc66271907a522816c1c5603707e Mon Sep 17 00:00:00 2001 From: jrfnl Date: Mon, 30 Oct 2023 12:16:17 +0100 Subject: [PATCH 1/7] NamingConventions/ValidHookName: remove some redundant code The same (low performance impact) conditions are use directly after too, so no need to double the effort. --- Yoast/Sniffs/NamingConventions/ValidHookNameSniff.php | 4 ---- 1 file changed, 4 deletions(-) diff --git a/Yoast/Sniffs/NamingConventions/ValidHookNameSniff.php b/Yoast/Sniffs/NamingConventions/ValidHookNameSniff.php index 94e9e4a6..b8a19567 100644 --- a/Yoast/Sniffs/NamingConventions/ValidHookNameSniff.php +++ b/Yoast/Sniffs/NamingConventions/ValidHookNameSniff.php @@ -305,10 +305,6 @@ public function verify_yoast_hook_name( $stackPtr, $parameters ) { $this->phpcsFile->recordMetric( $stackPtr, 'Nr of words in hook name', $part_count ); - if ( $part_count <= $this->recommended_max_words && $part_count <= $this->max_words ) { - return; - } - if ( $part_count > $this->max_words ) { $error = 'A hook name is not allowed to consist of more than %d words after the plugin prefix. Words found: %d in %s'; $data = [ From 05987adbe60eb083a2b3f8630247f59b20c1a72b Mon Sep 17 00:00:00 2001 From: jrfnl Date: Mon, 30 Oct 2023 11:53:50 +0100 Subject: [PATCH 2/7] NamingConventions/ValidHookName: add some extra defensive coding As things are, we already know that `$first_non_empty` will always return an integer stack pointer, as the same check is done in the `process()` method and this method isn't called if the return would have been `false`. Having said that, that means these methods are (too) closely coupled and a change in the logic in the `process()` method could inadvertently cause PHP to start throwing notices in the `verify_yoast_hook_name()` method. So, I'm adding some extra defensive coding just in case, though also ignoring that code for code coverage as, as things are, the condition will always be `false`. --- Yoast/Sniffs/NamingConventions/ValidHookNameSniff.php | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Yoast/Sniffs/NamingConventions/ValidHookNameSniff.php b/Yoast/Sniffs/NamingConventions/ValidHookNameSniff.php index b8a19567..d67114a6 100644 --- a/Yoast/Sniffs/NamingConventions/ValidHookNameSniff.php +++ b/Yoast/Sniffs/NamingConventions/ValidHookNameSniff.php @@ -208,6 +208,10 @@ public function verify_yoast_hook_name( $stackPtr, $parameters ) { $param = $parameters[1]; $first_non_empty = $this->phpcsFile->findNext( Tokens::$emptyTokens, $param['start'], ( $param['end'] + 1 ), true ); + if ( $first_non_empty === false ) { + // Shouldn't be possible as we've checked this before. + return; // @codeCoverageIgnore + } /* * Check that the namespace-like prefix is used for hooks. From d8feab0a0fc3016ad55789375780ccb020eb2cdd Mon Sep 17 00:00:00 2001 From: jrfnl Date: Mon, 30 Oct 2023 12:13:38 +0100 Subject: [PATCH 3/7] NamingConventions/ValidHookName: improve message clarity of the `NonString` warning The message called to manually examine the hook name for compliance with the max word count, but did not specify the applicable max word count, making the message in-actionable. Fixed now. Related to 247 --- Yoast/Sniffs/NamingConventions/ValidHookNameSniff.php | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/Yoast/Sniffs/NamingConventions/ValidHookNameSniff.php b/Yoast/Sniffs/NamingConventions/ValidHookNameSniff.php index d67114a6..a438d91b 100644 --- a/Yoast/Sniffs/NamingConventions/ValidHookNameSniff.php +++ b/Yoast/Sniffs/NamingConventions/ValidHookNameSniff.php @@ -285,10 +285,13 @@ public function verify_yoast_hook_name( $stackPtr, $parameters ) { * be thrown when PHPCS is explicitly requested to check with a lower severity. */ $this->phpcsFile->addWarning( - 'Hook name could not reliably be examined for maximum word count. Please verify this hook name manually. Found: %s', + 'Hook name could not reliably be examined for maximum word count (max is %d words). Please verify this hook name manually. Found: %s', $first_non_empty, 'NonString', - [ $param['raw'] ], + [ + $this->max_words, + $param['raw'], + ], 3 ); From 2a92cc1c63ca4cc4b34a19acd0a299b6912443bd Mon Sep 17 00:00:00 2001 From: jrfnl Date: Mon, 30 Oct 2023 10:23:02 +0100 Subject: [PATCH 4/7] NamingConventions/ValidHookName: rename local variable The `$param` variable name is quite non-descript. This commit updates the parameter name (in two places) to `$hook_name_param` to make it more obvious what the variable signifies. --- .../Sniffs/NamingConventions/ValidHookNameSniff.php | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/Yoast/Sniffs/NamingConventions/ValidHookNameSniff.php b/Yoast/Sniffs/NamingConventions/ValidHookNameSniff.php index a438d91b..3dd840c8 100644 --- a/Yoast/Sniffs/NamingConventions/ValidHookNameSniff.php +++ b/Yoast/Sniffs/NamingConventions/ValidHookNameSniff.php @@ -113,8 +113,8 @@ public function process_parameters( $stackPtr, $group_name, $matched_content, $p * If any prefixes were passed, check if this is a hook belonging to the plugin being checked. */ if ( empty( $this->validated_prefixes ) === false ) { - $param = $parameters[1]; - $first_non_empty = $this->phpcsFile->findNext( Tokens::$emptyTokens, $param['start'], ( $param['end'] + 1 ), true ); + $hook_name_param = $parameters[1]; + $first_non_empty = $this->phpcsFile->findNext( Tokens::$emptyTokens, $hook_name_param['start'], ( $hook_name_param['end'] + 1 ), true ); $found_prefix = ''; if ( isset( Tokens::$stringTokens[ $this->tokens[ $first_non_empty ]['code'] ] ) ) { @@ -206,8 +206,8 @@ protected function transform( $text_string, $regex, $transform_type = 'full' ) { */ public function verify_yoast_hook_name( $stackPtr, $parameters ) { - $param = $parameters[1]; - $first_non_empty = $this->phpcsFile->findNext( Tokens::$emptyTokens, $param['start'], ( $param['end'] + 1 ), true ); + $hook_name_param = $parameters[1]; + $first_non_empty = $this->phpcsFile->findNext( Tokens::$emptyTokens, $hook_name_param['start'], ( $hook_name_param['end'] + 1 ), true ); if ( $first_non_empty === false ) { // Shouldn't be possible as we've checked this before. return; // @codeCoverageIgnore @@ -274,7 +274,7 @@ public function verify_yoast_hook_name( $stackPtr, $parameters ) { $allow = [ \T_CONSTANT_ENCAPSED_STRING ]; $allow += Tokens::$emptyTokens; - $has_non_string = $this->phpcsFile->findNext( $allow, $param['start'], ( $param['end'] + 1 ), true ); + $has_non_string = $this->phpcsFile->findNext( $allow, $hook_name_param['start'], ( $hook_name_param['end'] + 1 ), true ); if ( $has_non_string !== false ) { /* * Double quoted string or a hook name concatenated together, checking the word count for the @@ -290,7 +290,7 @@ public function verify_yoast_hook_name( $stackPtr, $parameters ) { 'NonString', [ $this->max_words, - $param['raw'], + $hook_name_param['raw'], ], 3 ); From da62c0d2cf1d8c78d6581509ae13f3bb1060ae07 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Mon, 30 Oct 2023 11:09:22 +0100 Subject: [PATCH 5/7] NamingConventions/ValidHookName::verify_yoast_hook_name(): change function signature Originally, the `verify_yoast_hook_name()` method would get passed the complete stack of parameters. This is not necessary as the method only examines the first - `$hook_name` - parameter and that parameter has already been retrieved from the stack before. This changes the function signature of the `verify_yoast_hook_name()` method to only expect the parameter information for the `$hook_name` parameter, instead of the complete stack of parameters. It also changes the method from `public` to `private`. This method never needed to be `public`. --- Yoast/Sniffs/NamingConventions/ValidHookNameSniff.php | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/Yoast/Sniffs/NamingConventions/ValidHookNameSniff.php b/Yoast/Sniffs/NamingConventions/ValidHookNameSniff.php index 3dd840c8..8652052d 100644 --- a/Yoast/Sniffs/NamingConventions/ValidHookNameSniff.php +++ b/Yoast/Sniffs/NamingConventions/ValidHookNameSniff.php @@ -157,7 +157,7 @@ public function process_parameters( $stackPtr, $group_name, $matched_content, $p } // Do the YoastCS specific hook name length and prefix check. - $this->verify_yoast_hook_name( $stackPtr, $parameters ); + $this->verify_yoast_hook_name( $stackPtr, $hook_name_param ); } /** @@ -199,14 +199,13 @@ protected function transform( $text_string, $regex, $transform_type = 'full' ) { /** * Additional YoastCS specific hook name checks. * - * @param int $stackPtr The position of the current token in the stack. - * @param array $parameters Array with information about the parameters. + * @param int $stackPtr The position of the current token in the stack. + * @param array $hook_name_param Array with information about the hook name parameter. * * @return void */ - public function verify_yoast_hook_name( $stackPtr, $parameters ) { + private function verify_yoast_hook_name( $stackPtr, $hook_name_param ) { - $hook_name_param = $parameters[1]; $first_non_empty = $this->phpcsFile->findNext( Tokens::$emptyTokens, $hook_name_param['start'], ( $hook_name_param['end'] + 1 ), true ); if ( $first_non_empty === false ) { // Shouldn't be possible as we've checked this before. From 395ad68485ff3e8a585953209ef648f57523b3e0 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Mon, 30 Oct 2023 11:22:29 +0100 Subject: [PATCH 6/7] NamingConventions/ValidHookName: add support for PHP 8.0+ named parameters While WP does not officially support function calls using named parameters, that doesn't mean that nobody will use them.... This adjusts the sniff to allow for calls to hook functions using named parameters. The way the correct parameter is retrieved now uses the new `WPHookHelper::get_hook_name_param()` method, which uses the PHPCSUtils `PassedParameters::getParameterFromStack()` method under the hood. The `WPHookHelper` class keeps track of which functions are considered hook functions, what parameter names are used in WP and the parameter position. Note: as parameters can be skipped and/or passed in an unconventional order, it is now possible for the parameter to not exist. While that would result in a fatal error in PHP (missing required parameter), the sniff should still handle this situation gracefully, which is why the check for the parameter has been moved up and will cause the sniff to bow out if the parameter can not be found. Includes additional unit tests. Note: function calls with named arguments are not supported for the `do_action()` or `apply_filters()` functions as those have a parameter with a spread operator (but that's irrelevant for the sniff). --- Yoast/Sniffs/NamingConventions/ValidHookNameSniff.php | 8 +++++++- Yoast/Tests/NamingConventions/ValidHookNameUnitTest.inc | 8 ++++++++ Yoast/Tests/NamingConventions/ValidHookNameUnitTest.php | 2 ++ 3 files changed, 17 insertions(+), 1 deletion(-) diff --git a/Yoast/Sniffs/NamingConventions/ValidHookNameSniff.php b/Yoast/Sniffs/NamingConventions/ValidHookNameSniff.php index 8652052d..ec5d3075 100644 --- a/Yoast/Sniffs/NamingConventions/ValidHookNameSniff.php +++ b/Yoast/Sniffs/NamingConventions/ValidHookNameSniff.php @@ -4,6 +4,7 @@ use PHP_CodeSniffer\Util\Tokens; use PHPCSUtils\Utils\TextStrings; +use WordPressCS\WordPress\Helpers\WPHookHelper; use WordPressCS\WordPress\Sniffs\NamingConventions\ValidHookNameSniff as WPCS_ValidHookNameSniff; use YoastCS\Yoast\Utils\CustomPrefixesTrait; @@ -97,6 +98,12 @@ final class ValidHookNameSniff extends WPCS_ValidHookNameSniff { * @return void */ public function process_parameters( $stackPtr, $group_name, $matched_content, $parameters ) { + $hook_name_param = WPHookHelper::get_hook_name_param( $matched_content, $parameters ); + if ( $hook_name_param === false ) { + // If we can't find the hook name parameter, there's nothing to do, so bow out. + return; + } + /* * The custom prefix should be in the first text passed to `transform()` for each * matched function call. @@ -113,7 +120,6 @@ public function process_parameters( $stackPtr, $group_name, $matched_content, $p * If any prefixes were passed, check if this is a hook belonging to the plugin being checked. */ if ( empty( $this->validated_prefixes ) === false ) { - $hook_name_param = $parameters[1]; $first_non_empty = $this->phpcsFile->findNext( Tokens::$emptyTokens, $hook_name_param['start'], ( $hook_name_param['end'] + 1 ), true ); $found_prefix = ''; diff --git a/Yoast/Tests/NamingConventions/ValidHookNameUnitTest.inc b/Yoast/Tests/NamingConventions/ValidHookNameUnitTest.inc index 80882350..6704b7dc 100644 --- a/Yoast/Tests/NamingConventions/ValidHookNameUnitTest.inc +++ b/Yoast/Tests/NamingConventions/ValidHookNameUnitTest.inc @@ -140,5 +140,13 @@ apply_filters( "Yoast\\WP\\Plugin\\hookname", $var); // OK. apply_filters( "Yoast\WP\Plugin\hookname", $var); // Warning, missing escaping x 3. apply_filters( "Yoast\WP\\Plugin\hook{$name}", $var); // Warning, missing escaping x 2 + warning at severity 3 for word count. + +// Safeguard support for PHP 8.0+ named parameters. +do_action_ref_array( hook: 'My-Hook', args: $args ); // OK. Well, not really, but using the wrong parameter name, so not our concern. +do_action_ref_array( args: $args, hook_name: 'Yoast\WP\Plugin\Test\some_hook_name', ); // OK. +do_action_ref_array( args: $args, hook_name: "yoast_plugin_some_hook_name", ); // Warning - wrong prefix. +do_action_ref_array( args: $args, hook_name: 'Yoast\WP\Plugin\some_hook', ); // OK. +do_action_ref_array( args: $args, hook_name: 'Yoast\WP\Plugin\some_hook_name_which_is_too_long', ); // Error - too long. + // Reset to default settings. // phpcs:set Yoast.NamingConventions.ValidHookName prefixes[] diff --git a/Yoast/Tests/NamingConventions/ValidHookNameUnitTest.php b/Yoast/Tests/NamingConventions/ValidHookNameUnitTest.php index 8aa47639..762b6e46 100644 --- a/Yoast/Tests/NamingConventions/ValidHookNameUnitTest.php +++ b/Yoast/Tests/NamingConventions/ValidHookNameUnitTest.php @@ -50,6 +50,7 @@ public function getErrorList(): array { 108 => 1, 111 => 1, 119 => 1, + 149 => 1, ]; } @@ -79,6 +80,7 @@ public function getWarningList(): array { 136 => 1, // Severity: 3. 140 => 1, 141 => 2, // Severity: 3 + 5. + 147 => 1, ]; } } From 43855976d0b095380a933f08348b4531b60f177f Mon Sep 17 00:00:00 2001 From: jrfnl Date: Mon, 30 Oct 2023 12:23:06 +0100 Subject: [PATCH 7/7] NamingConventions/ValidHookName: minor documentation improvements --- .../NamingConventions/ValidHookNameSniff.php | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/Yoast/Sniffs/NamingConventions/ValidHookNameSniff.php b/Yoast/Sniffs/NamingConventions/ValidHookNameSniff.php index ec5d3075..ea0274b6 100644 --- a/Yoast/Sniffs/NamingConventions/ValidHookNameSniff.php +++ b/Yoast/Sniffs/NamingConventions/ValidHookNameSniff.php @@ -23,7 +23,7 @@ * Check the number of words in hook names and the use of the correct prefix-type, * but only when plugin specific prefixes have been passed. * - * {@internal For now allows for an array of both old-style as well as new-style + * {@internal For now, allows for an array of both old-style as well as new-style * prefixes during the transition period. * Once all plugins have been transitioned over to use the new-style * namespace-like prefix for hooks, the `WrongPrefix` warning should be @@ -72,7 +72,7 @@ final class ValidHookNameSniff extends WPCS_ValidHookNameSniff { * Keep track of the content of first text string which was passed to the `transform()` * method as it may be repeatedly called for the same token. * - * @var bool + * @var string */ private $first_string = ''; @@ -90,10 +90,11 @@ final class ValidHookNameSniff extends WPCS_ValidHookNameSniff { /** * Process the parameters of a matched function. * - * @param int $stackPtr The position of the current token in the stack. - * @param string $group_name The name of the group which was matched. - * @param string $matched_content The token content (function name) which was matched. - * @param array $parameters Array with information about the parameters. + * @param int $stackPtr The position of the current token in the stack. + * @param string $group_name The name of the group which was matched. + * @param string $matched_content The token content (function name) which was + * matched in lowercase. + * @param array> $parameters Array with information about the parameters. * * @return void */ @@ -205,8 +206,8 @@ protected function transform( $text_string, $regex, $transform_type = 'full' ) { /** * Additional YoastCS specific hook name checks. * - * @param int $stackPtr The position of the current token in the stack. - * @param array $hook_name_param Array with information about the hook name parameter. + * @param int $stackPtr The position of the current token in the stack. + * @param array $hook_name_param Array with information about the hook name parameter. * * @return void */