From 5641bca0a64a8f5357c999f8902102240cf0e8ac Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sat, 19 Jul 2025 23:04:14 +0200 Subject: [PATCH 1/3] Constants/ConstantString: extend AbstractFunctionParameterSniff As things were, the determination of whether or not a `T_STRING` is a call to the global PHP native `define[d]()` function was severely flawed. By switching the sniff over to be based on the WordPressCS `AbstractFunctionParameterSniff` class, this flaw is mitigated. Includes tests. --- .../Sniffs/Constants/ConstantStringSniff.php | 54 ++++++++----------- .../Constants/ConstantStringUnitTest.inc | 23 ++++++++ .../Constants/ConstantStringUnitTest.php | 8 ++- 3 files changed, 52 insertions(+), 33 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Constants/ConstantStringSniff.php b/WordPressVIPMinimum/Sniffs/Constants/ConstantStringSniff.php index fa44b8f4..c50aca58 100644 --- a/WordPressVIPMinimum/Sniffs/Constants/ConstantStringSniff.php +++ b/WordPressVIPMinimum/Sniffs/Constants/ConstantStringSniff.php @@ -11,51 +11,43 @@ use PHP_CodeSniffer\Util\Tokens; use PHPCSUtils\Utils\PassedParameters; -use WordPressVIPMinimum\Sniffs\Sniff; +use WordPressCS\WordPress\AbstractFunctionParameterSniff; /** * Sniff for properly using constant name when checking whether a constant is defined. */ -class ConstantStringSniff extends Sniff { +class ConstantStringSniff extends AbstractFunctionParameterSniff { /** - * Returns an array of tokens this test wants to listen for. + * The group name for this group of functions. * - * @return array + * @var string */ - public function register() { - return [ - T_STRING, - ]; - } + protected $group_name = 'constant_functions'; + + /** + * Functions this sniff is looking for. + * + * @var array Key is the function name, value irrelevant. + */ + protected $target_functions = [ + 'define' => true, + 'defined' => true, + ]; /** - * Process this test when one of its tokens is encountered. + * Process the parameters of a matched function. * - * @param int $stackPtr The position of the current token in the stack passed in $tokens. + * @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 */ - public function process_token( $stackPtr ) { - - if ( in_array( $this->tokens[ $stackPtr ]['content'], [ 'define', 'defined' ], true ) === false ) { - return; - } - - // Find the next non-empty token. - $nextToken = $this->phpcsFile->findNext( Tokens::$emptyTokens, $stackPtr + 1, null, true, null, true ); - - if ( $this->tokens[ $nextToken ]['code'] !== T_OPEN_PARENTHESIS ) { - // Not a function call. - return; - } - - if ( isset( $this->tokens[ $nextToken ]['parenthesis_closer'] ) === false ) { - // Not a function call. - return; - } - - $param = PassedParameters::getParameter( $this->phpcsFile, $stackPtr, 1, 'constant_name' ); + public function process_parameters( $stackPtr, $group_name, $matched_content, $parameters ) { + $param = PassedParameters::getParameterFromStack( $parameters, 1, 'constant_name' ); if ( $param === false ) { // Target parameter not found. return; diff --git a/WordPressVIPMinimum/Tests/Constants/ConstantStringUnitTest.inc b/WordPressVIPMinimum/Tests/Constants/ConstantStringUnitTest.inc index ec4ab2f4..5b619b52 100644 --- a/WordPressVIPMinimum/Tests/Constants/ConstantStringUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Constants/ConstantStringUnitTest.inc @@ -31,3 +31,26 @@ if ( ! defined($generator->get()) { // OK. $defined = defined(); // OK, ignore. $defined = defined( /*comment*/ ); // OK, ignore. + +// Safeguard that calls to namespaced functions or methods are not confused with the global functions. +if ( ! Not\defined( WPCOM_VIP ) ) { // Okay. + $obj->define( WPCOM_VIP, true ); // Okay. +} + +if ( ! \Fully\Qualified\defined( WPCOM_VIP ) ) { // Okay. + namespace\define( WPCOM_VIP, true ); // Okay. +} + +// Safeguard that fully qualified function calls are handled correctly. +if ( ! \defined( 'WPCOM_VIP' ) ) { // Okay. + \define( 'WPCOM_VIP', true, ); // Okay. +} + +if ( ! \defined( WPCOM_VIP, ) ) { // Error. + \define( WPCOM_VIP, true ); // Error. +} + +// Safeguard correct handling of case-insensitivity of function names. +if ( ! DEFINED( WPCOM_VIP ) ) { // Error. + Define( WPCOM_VIP, true ); // Error. +} diff --git a/WordPressVIPMinimum/Tests/Constants/ConstantStringUnitTest.php b/WordPressVIPMinimum/Tests/Constants/ConstantStringUnitTest.php index f2cfe47d..5b6bee28 100644 --- a/WordPressVIPMinimum/Tests/Constants/ConstantStringUnitTest.php +++ b/WordPressVIPMinimum/Tests/Constants/ConstantStringUnitTest.php @@ -23,8 +23,12 @@ class ConstantStringUnitTest extends AbstractSniffUnitTest { */ public function getErrorList() { return [ - 7 => 1, - 8 => 1, + 7 => 1, + 8 => 1, + 49 => 1, + 50 => 1, + 54 => 1, + 55 => 1, ]; } From 397990fe0d8518e087ed8ac57c3a26be6a1fbe0c Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sat, 19 Jul 2025 23:19:20 +0200 Subject: [PATCH 2/3] Constants/ConstantString: safeguard and document handling of various new php syntaxes --- .../Tests/Constants/ConstantStringUnitTest.inc | 12 ++++++++++++ .../Tests/Constants/ConstantStringUnitTest.php | 1 + 2 files changed, 13 insertions(+) diff --git a/WordPressVIPMinimum/Tests/Constants/ConstantStringUnitTest.inc b/WordPressVIPMinimum/Tests/Constants/ConstantStringUnitTest.inc index 5b619b52..ffd04cd6 100644 --- a/WordPressVIPMinimum/Tests/Constants/ConstantStringUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Constants/ConstantStringUnitTest.inc @@ -54,3 +54,15 @@ if ( ! \defined( WPCOM_VIP, ) ) { // Error. if ( ! DEFINED( WPCOM_VIP ) ) { // Error. Define( WPCOM_VIP, true ); // Error. } + +// Safeguard correct handling of function calls using PHP 8.0+ named parameters. +\defined( constant_nam: WPCOM_VIP ); // Okay, well not really, typo in param name, but that's not the concern of the sniff. +define(value: true, constant_name: 'WPCOM_VIP', ); // Okay. +define(case_insensitive: true, constant_name: WPCOM_VIP ); // Error. + +// Looks like a function call, but is a PHP 8.0+ class instantiation via an attribute. +#[Define('name', 'value')] +function foo() {} + +define(...$params); // PHP 5.6 argument unpacking will be ignored. +add_action('my_action', define(...)); // PHP 8.1 first class callable will be ignored. diff --git a/WordPressVIPMinimum/Tests/Constants/ConstantStringUnitTest.php b/WordPressVIPMinimum/Tests/Constants/ConstantStringUnitTest.php index 5b6bee28..73efa934 100644 --- a/WordPressVIPMinimum/Tests/Constants/ConstantStringUnitTest.php +++ b/WordPressVIPMinimum/Tests/Constants/ConstantStringUnitTest.php @@ -29,6 +29,7 @@ public function getErrorList() { 50 => 1, 54 => 1, 55 => 1, + 61 => 1, ]; } From 68260d29b846b7fe30c9eacb1e54ac17aa2571ca Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sun, 20 Jul 2025 05:04:04 +0200 Subject: [PATCH 3/3] Constants/ConstantString: improve clarity of error message --- WordPressVIPMinimum/Sniffs/Constants/ConstantStringSniff.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/WordPressVIPMinimum/Sniffs/Constants/ConstantStringSniff.php b/WordPressVIPMinimum/Sniffs/Constants/ConstantStringSniff.php index c50aca58..c44366a4 100644 --- a/WordPressVIPMinimum/Sniffs/Constants/ConstantStringSniff.php +++ b/WordPressVIPMinimum/Sniffs/Constants/ConstantStringSniff.php @@ -64,7 +64,7 @@ public function process_parameters( $stackPtr, $group_name, $matched_content, $p $tstring_token = $this->phpcsFile->findNext( T_STRING, $param['start'], $param['end'] + 1 ); - $message = 'Constant name, as a string, should be used along with `%s()`.'; + $message = 'The `%s()` function expects to be passed the constant name as a text string.'; $data = [ $this->tokens[ $stackPtr ]['content'] ]; $this->phpcsFile->addError( $message, $tstring_token, 'NotCheckingConstantName', $data ); }