From 72be374fe9cc46210505671d509bfffac3367df9 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 10 Aug 2023 03:56:32 +0200 Subject: [PATCH 1/3] Core: properly check formatting of function declaration statements As things were, the `WordPress.WhiteSpace.ControlStructureSpacing` sniff was being _abused_ to also check the formatting of function declaration statements, while it really isn't suitable for this. There have been numerous issues regarding this and various "plaster on the wound" fixes have been made previously. This PR is intended to fix the problem once and for all in a more proper fashion. The checks which were previously done by the `WordPress.WhiteSpace.ControlStructureSpacing` sniff consisted of the following: * Exactly one space after the `function` keyword (configurable for closures). * No space between the function _name_ and the open parenthesis. * Exactly one space after the open parenthesis. * Exactly one space before the close parenthesis. * Exactly one space before the open brace/after the close parenthesis. The above didn't take return type declarations into account properly, would run into problems with function declarations which didn't have an open brace, such as `abstract` functions or interface functions (also see 2204), and handling multi-line function declarations was undefined territory. It also led to: * Duplicate messages - the `ControlStructureSpacing` sniff reporting things also reported by the `Squiz.Functions.FunctionDeclarationArgumentSpacing` sniff or by the `Generic.WhiteSpace.LanguageConstructSpacing` sniff. * Unclear messages [1] - messages were often thrown on the `function` keyword, while the problem was further down the line. * Unclear messages [2] - messages would refer to control structures instead of functions and would otherwise not always be clear. This commit: * Removes all code related to checking function declarations from the `WordPress.WhiteSpace.ControlStructureSpacing` sniff. Includes removing the tests which were specific to function declarations. This effectively reverts the function related changes in PRs 417, 432, 440, 463, 1319, 1837 in so far as those touched the `ControlStructureSpacing` sniff. * Adds the `Squiz.Functions.MultiLineFunctionDeclaration` sniff to the `Core` ruleset to do those checks instead. The change as currently proposed has been extensively researched and tested and should provide a smooth switch over. Notes about the ruleset changes: * The `Squiz.Functions.FunctionDeclarationArgumentSpacing.SpacingBeforeClose` code was previously silenced to prevent duplicate messages, but can now be re-enabled as we'll defer to that sniff for the spacing on the inside of the parentheses. * The `Squiz.Functions.MultiLineFunctionDeclaration` sniff, by default, expects the opening brace for named functions on the line _after_ the declaration and for closures, it expects the opening brace on the _same_ line as the closure declaration. WPCS expects the opening brace on the _same_ line as the function declaration in both cases and enforces this via the `Generic.Functions.OpeningFunctionBraceKernighanRitchie` sniff (which is also used by the `Squiz.Functions.MultiLineFunctionDeclaration` sniff for the closure check). The ruleset now contains three error code exclusions to bypass the "brace on new line for named functions" checks. * As the opening brace for closures is enforced to be on the same line, we can let the new Squiz sniff handle this. To that end the `checkClosures` property for the `Generic.Functions.OpeningFunctionBraceKernighanRitchie` sniff is set to `false` to prevent duplicate messages for the same. * To prevent duplicate messages about code after the open brace for named functions, the `Generic.Functions.OpeningFunctionBraceKernighanRitchie.ContentAfterBrace` code has to be silenced. We cannot silence the same error code for the Squiz sniff as that would also silence the notice for closures, so silencing it for the `OpeningFunctionBraceKernighanRitchie` sniff, which now only handles named functions, is the only option, but should prevent the duplicate messages. * Additionally, the `Squiz.Functions.MultiLineFunctionDeclaration.SpaceAfterUse` error code is excluded as it overlaps with the `Generic.WhiteSpace.LanguageConstructSpacing` sniff, which also checks this. Notes about changes in behaviour before/after this change: * If the open & close parentheses are not on the same line, the space on the inside of the parentheses is no longer checked (as it is seen as a multi-line function declaration for which different rules may apply). * While not commonly used in WP, for multi-line function declarations, the sniff will now enforce one parameter per line with the first parameter on the line _after_ the open parenthesis and the close parenthesis on the line after the last parameter and indented the same as the start of the function declaration statement. A function is considered multi-line when the open and close parentheses are not on the same line, which means that for closures with `use` statements, these rules will apply to both the function parameter list as well as the use parameter list. * It is no longer possible to choose the spacing between the `function` keyword for closures and the open parenthesis. This will no always be enforced to be a single space, which is in line with PSR12. A check of WP Core showed that there was no consistency at all regarding this spacing, with approximately 50/50 of closures having no space vs 1 space, so defering to PSR12 and enforcing consistency in this, seems like a reasonable choice. Other notes: * The `WordPress.WhiteSpace.ControlStructureSpacing` sniff did not support formatting checks for PHP 7.4 arrow functions. At this time, the `Squiz.Functions.MultiLineFunctionDeclaration` sniff does not do so either. There is a PR open upstream squizlabs/php_codesniffer 3661 to add support for arrow functions, though that PR still needs an update for the (very inconsistent) rule chosen by PSR-PER to [enforce _no_ space between the `fn` keyword and the open parenthesis for arrow functions](https://github.com/php-fig/per-coding-style/pull/53). * There is also a PR open upstream - squizlabs/php_codesniffer 3739 - to fix two fixer issues. At this time, these do not pose a problem for WP Core as (aside from the closure keyword spacing), WP Core is "clean", but would be nice if that PR got merged to prevent future issues. * While doing the research for this PR, I discovered an issue with the `Generic.Functions.OpeningFunctionBraceKernighanRitchie` sniff. It doesn't check the spacing before the open brace for empty functions. PR squizlabs/php_codesniffer 3869 has been opened to fix that. As empty functions are the exception, not the rule, I'm hoping it will get merged before any issues about this are reported. Fixes 1101 Note: check 8 identified in issue 1101 is still open, but there is a separate issue open to discuss that in more depth - 1474 - and that issue remains open (for now). --- WordPress-Core/ruleset.xml | 26 ++- .../ControlStructureSpacingSniff.php | 155 ++---------------- .../ControlStructureSpacingUnitTest.1.inc | 125 -------------- ...ontrolStructureSpacingUnitTest.1.inc.fixed | 123 -------------- .../ControlStructureSpacingUnitTest.php | 40 ++--- 5 files changed, 47 insertions(+), 422 deletions(-) diff --git a/WordPress-Core/ruleset.xml b/WordPress-Core/ruleset.xml index e871818cba..c38349e5c4 100644 --- a/WordPress-Core/ruleset.xml +++ b/WordPress-Core/ruleset.xml @@ -199,9 +199,30 @@ - + + + + 0 + + + + + + 0 + + + 0 + + + 0 + + + + 0 + + @@ -209,9 +230,6 @@ - - 0 - 0 diff --git a/WordPress/Sniffs/WhiteSpace/ControlStructureSpacingSniff.php b/WordPress/Sniffs/WhiteSpace/ControlStructureSpacingSniff.php index c95dc859fc..24cf2f5331 100644 --- a/WordPress/Sniffs/WhiteSpace/ControlStructureSpacingSniff.php +++ b/WordPress/Sniffs/WhiteSpace/ControlStructureSpacingSniff.php @@ -11,7 +11,6 @@ use WordPressCS\WordPress\Sniff; use PHP_CodeSniffer\Util\Tokens; -use PHPCSUtils\Utils\UseStatements; /** * Enforces spacing around logical operators and assignments, based upon Squiz code. @@ -23,6 +22,7 @@ * @since 0.3.0 This sniff now has the ability to fix most errors it flags. * @since 0.7.0 This class now extends the WordPressCS native `Sniff` class. * @since 0.13.0 Class name changed: this class is now namespaced. + * @since 3.0.0 Checks related to function declarations have been removed from this sniff. * * Last synced with base class 2017-01-15 at commit b024ad84656c37ef5733c6998ebc1e60957b2277. * Note: This class has diverged quite far from the original. All the same, checking occasionally @@ -52,21 +52,9 @@ final class ControlStructureSpacingSniff extends Sniff { */ public $space_before_colon = 'required'; - /** - * How many spaces should be between a T_CLOSURE and T_OPEN_PARENTHESIS. - * - * `function[*]() {...}` - * - * @since 0.7.0 - * - * @var int - */ - public $spaces_before_closure_open_paren = -1; - /** * Tokens for which to ignore extra space on the inside of parenthesis. * - * For functions, this is already checked by the Squiz.Functions.FunctionDeclarationArgumentSpacing sniff. * For do / else / try, there are no parenthesis, so skip it. * * @since 0.11.0 @@ -74,11 +62,9 @@ final class ControlStructureSpacingSniff extends Sniff { * @var array */ private $ignore_extra_space_after_open_paren = array( - \T_FUNCTION => true, - \T_CLOSURE => true, - \T_DO => true, - \T_ELSE => true, - \T_TRY => true, + \T_DO => true, + \T_ELSE => true, + \T_TRY => true, ); /** @@ -96,9 +82,6 @@ public function register() { \T_DO, \T_ELSE, \T_ELSEIF, - \T_FUNCTION, - \T_CLOSURE, - \T_USE, \T_TRY, \T_CATCH, ); @@ -112,18 +95,8 @@ public function register() { * @return void */ public function process_token( $stackPtr ) { - $this->spaces_before_closure_open_paren = (int) $this->spaces_before_closure_open_paren; - - if ( \T_USE === $this->tokens[ $stackPtr ]['code'] - && UseStatements::isClosureUse( $this->phpcsFile, $stackPtr ) === false - ) { - return; - } - if ( isset( $this->tokens[ ( $stackPtr + 1 ) ] ) && \T_WHITESPACE !== $this->tokens[ ( $stackPtr + 1 ) ]['code'] && ! ( \T_ELSE === $this->tokens[ $stackPtr ]['code'] && \T_COLON === $this->tokens[ ( $stackPtr + 1 ) ]['code'] ) - && ! ( \T_CLOSURE === $this->tokens[ $stackPtr ]['code'] - && 0 >= $this->spaces_before_closure_open_paren ) ) { $error = 'Space after opening control structure is required'; $fix = $this->phpcsFile->addFixableError( $error, $stackPtr, 'NoSpaceAfterStructureOpen' ); @@ -133,12 +106,8 @@ public function process_token( $stackPtr ) { } } - if ( ! isset( $this->tokens[ $stackPtr ]['scope_closer'] ) ) { - - if ( \T_USE === $this->tokens[ $stackPtr ]['code'] ) { - $scopeOpener = $this->phpcsFile->findNext( \T_OPEN_CURLY_BRACKET, ( $stackPtr + 1 ) ); - $scopeCloser = $this->tokens[ $scopeOpener ]['scope_closer']; - } elseif ( \T_WHILE !== $this->tokens[ $stackPtr ]['code'] ) { + if ( ! isset( $this->tokens[ $stackPtr ]['scope_opener'], $this->tokens[ $stackPtr ]['scope_closer'] ) ) { + if ( \T_WHILE !== $this->tokens[ $stackPtr ]['code'] ) { return; } } else { @@ -174,102 +143,15 @@ public function process_token( $stackPtr ) { $parenthesisOpener = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $stackPtr + 1 ), null, true ); - // If this is a function declaration. - if ( \T_FUNCTION === $this->tokens[ $stackPtr ]['code'] ) { - - if ( \T_STRING === $this->tokens[ $parenthesisOpener ]['code'] ) { - - $function_name_ptr = $parenthesisOpener; - - } elseif ( \T_BITWISE_AND === $this->tokens[ $parenthesisOpener ]['code'] ) { - - // This function returns by reference, i.e. `function &function_name() {}`. - $parenthesisOpener = $this->phpcsFile->findNext( - Tokens::$emptyTokens, - ( $parenthesisOpener + 1 ), - null, - true - ); - $function_name_ptr = $parenthesisOpener; - } - - if ( isset( $function_name_ptr ) ) { - $parenthesisOpener = $this->phpcsFile->findNext( - Tokens::$emptyTokens, - ( $parenthesisOpener + 1 ), - null, - true - ); - - // Checking space between name and open parentheses, i.e. `function my_function[*](...) {}`. - if ( ( $function_name_ptr + 1 ) !== $parenthesisOpener ) { - - $error = 'Space between function name and opening parenthesis is prohibited.'; - $fix = $this->phpcsFile->addFixableError( - $error, - $stackPtr, - 'SpaceBeforeFunctionOpenParenthesis', - array( $this->tokens[ ( $function_name_ptr + 1 ) ]['content'] ) - ); - - if ( true === $fix ) { - $this->phpcsFile->fixer->replaceToken( ( $function_name_ptr + 1 ), '' ); - } - } - } - } elseif ( \T_CLOSURE === $this->tokens[ $stackPtr ]['code'] ) { - - // Check if there is a use () statement. - if ( isset( $this->tokens[ $parenthesisOpener ]['parenthesis_closer'] ) ) { - - $usePtr = $this->phpcsFile->findNext( - Tokens::$emptyTokens, - ( $this->tokens[ $parenthesisOpener ]['parenthesis_closer'] + 1 ), - null, - true, - null, - true - ); - - // If it is, we set that as the "scope opener". - if ( \T_USE === $this->tokens[ $usePtr ]['code'] ) { - $scopeOpener = $usePtr; - } - } - } - if ( \T_COLON !== $this->tokens[ $parenthesisOpener ]['code'] - && \T_FUNCTION !== $this->tokens[ $stackPtr ]['code'] + && ( $stackPtr + 1 ) === $parenthesisOpener ) { + // Checking space between keyword and open parenthesis, i.e. `if[*](...) {}`. + $error = 'No space before opening parenthesis is prohibited'; + $fix = $this->phpcsFile->addFixableError( $error, $stackPtr, 'NoSpaceBeforeOpenParenthesis' ); - if ( \T_CLOSURE === $this->tokens[ $stackPtr ]['code'] - && 0 === $this->spaces_before_closure_open_paren - ) { - - if ( ( $stackPtr + 1 ) !== $parenthesisOpener ) { - // Checking space between keyword and open parenthesis, i.e. `function[*](...) {}`. - $error = 'Space before closure opening parenthesis is prohibited'; - $fix = $this->phpcsFile->addFixableError( $error, $stackPtr, 'SpaceBeforeClosureOpenParenthesis' ); - - if ( true === $fix ) { - $this->phpcsFile->fixer->replaceToken( ( $stackPtr + 1 ), '' ); - } - } - } elseif ( - ( - \T_CLOSURE !== $this->tokens[ $stackPtr ]['code'] - || 1 === $this->spaces_before_closure_open_paren - ) - && ( $stackPtr + 1 ) === $parenthesisOpener - ) { - - // Checking space between keyword and open parenthesis, i.e. `if[*](...) {}`. - $error = 'No space before opening parenthesis is prohibited'; - $fix = $this->phpcsFile->addFixableError( $error, $stackPtr, 'NoSpaceBeforeOpenParenthesis' ); - - if ( true === $fix ) { - $this->phpcsFile->fixer->addContent( $stackPtr, ' ' ); - } + if ( true === $fix ) { + $this->phpcsFile->fixer->addContent( $stackPtr, ' ' ); } } @@ -292,7 +174,7 @@ public function process_token( $stackPtr ) { if ( \T_CLOSE_PARENTHESIS !== $this->tokens[ ( $parenthesisOpener + 1 ) ]['code'] ) { if ( \T_WHITESPACE !== $this->tokens[ ( $parenthesisOpener + 1 ) ]['code'] ) { - // Checking space directly after the open parenthesis, i.e. `$value = my_function([*]...)`. + // Checking space directly after the open parenthesis, i.e. `if ([*]...) {}`. $error = 'No space after opening parenthesis is prohibited'; $fix = $this->phpcsFile->addFixableError( $error, $stackPtr, 'NoSpaceAfterOpenParenthesis' ); @@ -351,11 +233,6 @@ public function process_token( $stackPtr ) { } if ( \T_WHITESPACE !== $this->tokens[ ( $parenthesisCloser + 1 ) ]['code'] - && ! ( // Do NOT flag : immediately following ) for return types declarations. - \T_COLON === $this->tokens[ ( $parenthesisCloser + 1 ) ]['code'] - && ( isset( $this->tokens[ $parenthesisCloser ]['parenthesis_owner'] ) === false - || in_array( $this->tokens[ $this->tokens[ $parenthesisCloser ]['parenthesis_owner'] ]['code'], array( \T_FUNCTION, \T_CLOSURE ), true ) ) - ) && ( isset( $scopeOpener ) && \T_COLON !== $this->tokens[ $scopeOpener ]['code'] ) ) { $error = 'Space between opening control structure and closing parenthesis is required'; @@ -367,10 +244,7 @@ public function process_token( $stackPtr ) { } } - // Ignore this for function declarations. Handled by the OpeningFunctionBraceKernighanRitchie sniff. - if ( \T_FUNCTION !== $this->tokens[ $stackPtr ]['code'] - && \T_CLOSURE !== $this->tokens[ $stackPtr ]['code'] - && isset( $this->tokens[ $parenthesisOpener ]['parenthesis_owner'] ) + if ( isset( $this->tokens[ $parenthesisOpener ]['parenthesis_owner'] ) && ( isset( $scopeOpener ) && $this->tokens[ $parenthesisCloser ]['line'] !== $this->tokens[ $scopeOpener ]['line'] ) ) { @@ -392,7 +266,6 @@ public function process_token( $stackPtr ) { } elseif ( \T_WHITESPACE === $this->tokens[ ( $parenthesisCloser + 1 ) ]['code'] && ' ' !== $this->tokens[ ( $parenthesisCloser + 1 ) ]['content'] ) { - // Checking space between the close parenthesis and the open brace, i.e. `if (...) [*]{}`. $error = 'Expected exactly one space between closing parenthesis and opening control structure; "%s" found.'; $fix = $this->phpcsFile->addFixableError( diff --git a/WordPress/Tests/WhiteSpace/ControlStructureSpacingUnitTest.1.inc b/WordPress/Tests/WhiteSpace/ControlStructureSpacingUnitTest.1.inc index 6e848342a1..1f1b47abf1 100644 --- a/WordPress/Tests/WhiteSpace/ControlStructureSpacingUnitTest.1.inc +++ b/WordPress/Tests/WhiteSpace/ControlStructureSpacingUnitTest.1.inc @@ -45,91 +45,6 @@ endif; if ( false ) : else : endif; -// phpcs:set WordPress.WhiteSpace.ControlStructureSpacing spaces_before_closure_open_paren 1 -$a = function($arg){}; // Bad. -$a = function ( $arg ) { - // Ok. -}; - -$a = function () { - // Ok. -}; - -function something($arg){} // Bad. -function foo( $arg ) { - // Ok. -} - -function no_params() { - // Ok. -} - -function another () {} // Bad, space before open parenthesis prohibited. -function and_another() {} // Bad, space before function name prohibited. -function -bar() {} // Bad. -function baz() {} // Bad. -function testA() -{} // Bad. - -function &return_by_ref() {} // Ok. - -// phpcs:set WordPress.WhiteSpace.ControlStructureSpacing spaces_before_closure_open_paren 0 - -$a = function() {}; // Ok. -$a = function( $arg ) {}; // Ok. -$a = function($arg){}; // Bad. -$a = function () {}; // Bad. - -$closureWithArgsAndVars = function( $arg1, $arg2 ) use ( $var1, $var2 ) {}; // Ok. -$closureWithArgsAndVars = function ( $arg1, $arg2 ) use ( $var1, $var2 ) {}; // Bad. - -// phpcs:set WordPress.WhiteSpace.ControlStructureSpacing spaces_before_closure_open_paren 1 - -$closureWithArgsAndVars = function ( $arg1, $arg2 ) use ( $var1, $var2 ) {}; // Ok. - -$closureWithArgsAndVars = function ( $arg1, $arg2 ) use( $var1, $var2 ) {}; // Bad, no space before open parenthesis prohibited. -$closureWithArgsAndVars = function ( $arg1, $arg2 ) use ( $var1, $var2 ) {}; // Bad, expected exactly one space before opening parenthesis. - -$closureWithArgsAndVars = function ( $arg1, $arg2 ) use ( $var1, $var2 ){}; // Bad, space between closing parenthesis and control structure required. -$closureWithArgsAndVars = function ( $arg1, $arg2 ) use ( $var1, $var2 ) {}; // Bad, expected exactly one space between closing parenthesis and control structure. - -$closureWithArgsAndVars = function ( $arg1, $arg2 )use ( $var1, $var2 ) {}; // Bad, expected exactly one space between closing parenthesis and control structure. -$closureWithArgsAndVars = function ( $arg1, $arg2 ) use ( $var1, $var2 ) {}; // Bad, expected exactly one space between closing parenthesis and control structure. - -// Namespaces. -use Foo\Admin; - -// phpcs:set WordPress.WhiteSpace.ControlStructureSpacing spaces_before_closure_open_paren -1 - -$a = function( $arg ) {}; // Ok. -$a = function ( $arg ) {}; // Ok. - -$closureWithArgsAndVars = function( $arg1, $arg2 ) use ( $var1, $var2 ) {}; // Ok. -$closureWithArgsAndVars = function ( $arg1, $arg2 ) use ( $var1, $var2 ) {}; // Ok. - -/* - * Test for bug where this sniff was triggering a "Blank line found after control structure" error - * if there is a blank line after the last method in a class. - * - * Bug did not trigger when a comment was found after the closing brace of the method. - * - * Neither of the below examples should trigger the error. - */ -class Bar_Foo { - - function foo() { - } // Now you won't see the bug. - -} - -class Foo_Bar { - - // Now you will. - function bar() { - } - -} // Handle try/catch statements as well. try{ // Bad. @@ -176,9 +91,6 @@ if ( $foo ) { // phpcs:set WordPress.WhiteSpace.ControlStructureSpacing blank_line_check false // Check for too many spaces as long as the next non-blank token is on the same line. -function test( $blah ) {} // Bad. -$a = function( $bar ) {}; // Bad. - if ( 'abc' === $test ) { // Bad. echo 'hi'; } elseif ( false === $foo ) { // Bad. @@ -268,40 +180,3 @@ if ( $foo ) { } - -// Issue 1202. -function hi(): array -{ - return []; -} - -// Issue 1420. -function returntype1(): string {} // Ok. -function returntype2( $input ): string {} // Ok. -function returntype3( $input ) : string {} // Ok. -function returntype4( string $input ): string {} // Ok. -function returntype5( string $input ) : string {} // Ok. -function returntype6( string $input, array $inputs ): string {} // Ok. -function returntype7( string $input, array $inputs ) : string {} // Ok. -$returntype = function (): string {}; // Ok. -$returntype = function ( $input ): string {}; // Ok. -$returntype = function ( $input ) : string {}; // Ok. -$returntype = function ( string $input ): string {}; // Ok. -$returntype = function ( string $input ) : string {}; // Ok. -$returntype = function ( string $input, array $inputs ): string {}; // Ok. -$returntype = function ( string $input, array $inputs ) : string {}; // Ok. - -// Issue 1792. -$matching_options = array_filter( - $imported_options, - function ( $option ) use ( $option_id ): bool { - return $option['id'] === $option_id; - } -); - -$matching_options = array_filter( - $imported_options, - function ( $option ) use ( $option_id ) : bool { - return $option['id'] === $option_id; - } -); diff --git a/WordPress/Tests/WhiteSpace/ControlStructureSpacingUnitTest.1.inc.fixed b/WordPress/Tests/WhiteSpace/ControlStructureSpacingUnitTest.1.inc.fixed index 32c01f655d..c57462793c 100644 --- a/WordPress/Tests/WhiteSpace/ControlStructureSpacingUnitTest.1.inc.fixed +++ b/WordPress/Tests/WhiteSpace/ControlStructureSpacingUnitTest.1.inc.fixed @@ -43,89 +43,6 @@ endif; if ( false ) : else : endif; -// phpcs:set WordPress.WhiteSpace.ControlStructureSpacing spaces_before_closure_open_paren 1 -$a = function ( $arg ) {}; // Bad. -$a = function ( $arg ) { - // Ok. -}; - -$a = function () { - // Ok. -}; - -function something( $arg ) {} // Bad. -function foo( $arg ) { - // Ok. -} - -function no_params() { - // Ok. -} - -function another() {} // Bad, space before open parenthesis prohibited. -function and_another() {} // Bad, space before function name prohibited. -function bar() {} // Bad. -function baz() {} // Bad. -function testA() {} // Bad. - -function &return_by_ref() {} // Ok. - -// phpcs:set WordPress.WhiteSpace.ControlStructureSpacing spaces_before_closure_open_paren 0 - -$a = function() {}; // Ok. -$a = function( $arg ) {}; // Ok. -$a = function( $arg ) {}; // Bad. -$a = function() {}; // Bad. - -$closureWithArgsAndVars = function( $arg1, $arg2 ) use ( $var1, $var2 ) {}; // Ok. -$closureWithArgsAndVars = function( $arg1, $arg2 ) use ( $var1, $var2 ) {}; // Bad. - -// phpcs:set WordPress.WhiteSpace.ControlStructureSpacing spaces_before_closure_open_paren 1 - -$closureWithArgsAndVars = function ( $arg1, $arg2 ) use ( $var1, $var2 ) {}; // Ok. - -$closureWithArgsAndVars = function ( $arg1, $arg2 ) use ( $var1, $var2 ) {}; // Bad, no space before open parenthesis prohibited. -$closureWithArgsAndVars = function ( $arg1, $arg2 ) use ( $var1, $var2 ) {}; // Bad, expected exactly one space before opening parenthesis. - -$closureWithArgsAndVars = function ( $arg1, $arg2 ) use ( $var1, $var2 ) {}; // Bad, space between closing parenthesis and control structure required. -$closureWithArgsAndVars = function ( $arg1, $arg2 ) use ( $var1, $var2 ) {}; // Bad, expected exactly one space between closing parenthesis and control structure. - -$closureWithArgsAndVars = function ( $arg1, $arg2 ) use ( $var1, $var2 ) {}; // Bad, expected exactly one space between closing parenthesis and control structure. -$closureWithArgsAndVars = function ( $arg1, $arg2 ) use ( $var1, $var2 ) {}; // Bad, expected exactly one space between closing parenthesis and control structure. - -// Namespaces. -use Foo\Admin; - -// phpcs:set WordPress.WhiteSpace.ControlStructureSpacing spaces_before_closure_open_paren -1 - -$a = function( $arg ) {}; // Ok. -$a = function ( $arg ) {}; // Ok. - -$closureWithArgsAndVars = function( $arg1, $arg2 ) use ( $var1, $var2 ) {}; // Ok. -$closureWithArgsAndVars = function ( $arg1, $arg2 ) use ( $var1, $var2 ) {}; // Ok. - -/* - * Test for bug where this sniff was triggering a "Blank line found after control structure" error - * if there is a blank line after the last method in a class. - * - * Bug did not trigger when a comment was found after the closing brace of the method. - * - * Neither of the below examples should trigger the error. - */ -class Bar_Foo { - - function foo() { - } // Now you won't see the bug. - -} - -class Foo_Bar { - - // Now you will. - function bar() { - } - -} // Handle try/catch statements as well. try { // Bad. @@ -171,9 +88,6 @@ if ( $foo ) { // phpcs:set WordPress.WhiteSpace.ControlStructureSpacing blank_line_check false // Check for too many spaces as long as the next non-blank token is on the same line. -function test( $blah ) {} // Bad. -$a = function( $bar ) {}; // Bad. - if ( 'abc' === $test ) { // Bad. echo 'hi'; } elseif ( false === $foo ) { // Bad. @@ -257,40 +171,3 @@ if ( $foo ) { // Something } // End try/catch <- Bad: "blank line after". } - -// Issue 1202. -function hi(): array -{ - return []; -} - -// Issue 1420. -function returntype1(): string {} // Ok. -function returntype2( $input ): string {} // Ok. -function returntype3( $input ) : string {} // Ok. -function returntype4( string $input ): string {} // Ok. -function returntype5( string $input ) : string {} // Ok. -function returntype6( string $input, array $inputs ): string {} // Ok. -function returntype7( string $input, array $inputs ) : string {} // Ok. -$returntype = function (): string {}; // Ok. -$returntype = function ( $input ): string {}; // Ok. -$returntype = function ( $input ) : string {}; // Ok. -$returntype = function ( string $input ): string {}; // Ok. -$returntype = function ( string $input ) : string {}; // Ok. -$returntype = function ( string $input, array $inputs ): string {}; // Ok. -$returntype = function ( string $input, array $inputs ) : string {}; // Ok. - -// Issue 1792. -$matching_options = array_filter( - $imported_options, - function ( $option ) use ( $option_id ): bool { - return $option['id'] === $option_id; - } -); - -$matching_options = array_filter( - $imported_options, - function ( $option ) use ( $option_id ) : bool { - return $option['id'] === $option_id; - } -); diff --git a/WordPress/Tests/WhiteSpace/ControlStructureSpacingUnitTest.php b/WordPress/Tests/WhiteSpace/ControlStructureSpacingUnitTest.php index 58e7cf7265..2ceeff7cc4 100644 --- a/WordPress/Tests/WhiteSpace/ControlStructureSpacingUnitTest.php +++ b/WordPress/Tests/WhiteSpace/ControlStructureSpacingUnitTest.php @@ -41,37 +41,19 @@ public function getErrorList( $testFile = '' ) { 37 => 1, 41 => 1, 42 => 1, - 49 => 5, - 58 => 3, - 67 => 1, - 68 => 1, - 69 => 1, - 71 => 1, - 72 => 1, - 81 => 3, - 82 => 1, - 85 => 1, - 91 => 2, - 92 => 1, + 50 => 2, + 52 => 5, + 59 => 1, + 67 => 2, 94 => 1, - 95 => 1, - 97 => 1, - 98 => 1, - 135 => 2, - 137 => 5, - 144 => 1, - 152 => 2, + 96 => 1, + 102 => 1, + 104 => 1, + 108 => 2, + 112 => 2, + 159 => 1, + 169 => 1, 179 => 1, - 180 => 1, - 182 => 1, - 184 => 1, - 190 => 1, - 192 => 1, - 196 => 2, - 200 => 2, - 247 => 1, - 257 => 1, - 267 => 1, ); return $ret; From dfc72e4d12a6dc830e961ef4945c49291b6e374b Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 10 Aug 2023 04:11:30 +0200 Subject: [PATCH 2/3] CS: fix up closure spacing to comply with new rules --- WordPress/Sniffs/Security/ValidatedSanitizedInputSniff.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/WordPress/Sniffs/Security/ValidatedSanitizedInputSniff.php b/WordPress/Sniffs/Security/ValidatedSanitizedInputSniff.php index 4551973d96..3ec36a706e 100644 --- a/WordPress/Sniffs/Security/ValidatedSanitizedInputSniff.php +++ b/WordPress/Sniffs/Security/ValidatedSanitizedInputSniff.php @@ -71,7 +71,7 @@ public function process_token( $stackPtr ) { ) { // Retrieve all embeds, but use only the initial variable name part. $interpolated_variables = array_map( - function( $embed ) { + function ( $embed ) { return '$' . preg_replace( '`^(\{?\$\{?\(?)([a-zA-Z_\x80-\xff][a-zA-Z0-9_\x80-\xff]*)(.*)$`', '$2', $embed ); }, TextStrings::getEmbeds( $this->tokens[ $stackPtr ]['content'] ) From 602d9ef96f1b56cc788d062c5a93e8c9b01a169e Mon Sep 17 00:00:00 2001 From: jrfnl Date: Fri, 11 Aug 2023 04:28:49 +0200 Subject: [PATCH 3/3] GlobalVariablesOverride: move parse error test to separate file ... as it would cause a fixer conflict with the new rules. As the fixer conflict is directly related to the parse error, I'm not concerned about it and I don't believe a fix is needed for the sniff. Excluding this particular parse error file from the fixer conflict check should be an acceptable solution. --- .github/workflows/basic-qa.yml | 2 +- .../WP/GlobalVariablesOverrideUnitTest.1.inc | 21 +++++++++---------- .../WP/GlobalVariablesOverrideUnitTest.7.inc | 20 ++++++++++++++++++ .../WP/GlobalVariablesOverrideUnitTest.php | 6 +++++- 4 files changed, 36 insertions(+), 13 deletions(-) create mode 100644 WordPress/Tests/WP/GlobalVariablesOverrideUnitTest.7.inc diff --git a/.github/workflows/basic-qa.yml b/.github/workflows/basic-qa.yml index 15bcfc518b..8e0b604763 100644 --- a/.github/workflows/basic-qa.yml +++ b/.github/workflows/basic-qa.yml @@ -152,7 +152,7 @@ jobs: continue-on-error: true run: | set +e - $(pwd)/vendor/bin/phpcbf -pq ./WordPress/Tests/ --standard=WordPress --extensions=inc --exclude=Generic.PHP.Syntax --report=summary --ignore=/WordPress/Tests/NamingConventions/ValidVariableNameUnitTest.inc + $(pwd)/vendor/bin/phpcbf -pq ./WordPress/Tests/ --standard=WordPress --extensions=inc --exclude=Generic.PHP.Syntax --report=summary --ignore=/WordPress/Tests/NamingConventions/ValidVariableNameUnitTest.inc,/WordPress/Tests/WP/GlobalVariablesOverrideUnitTest.7.inc exitcode="$?" echo "EXITCODE=$exitcode" >> $GITHUB_OUTPUT exit "$exitcode" diff --git a/WordPress/Tests/WP/GlobalVariablesOverrideUnitTest.1.inc b/WordPress/Tests/WP/GlobalVariablesOverrideUnitTest.1.inc index da9d002194..80603e7e8b 100644 --- a/WordPress/Tests/WP/GlobalVariablesOverrideUnitTest.1.inc +++ b/WordPress/Tests/WP/GlobalVariablesOverrideUnitTest.1.inc @@ -183,20 +183,20 @@ function global_vars() { return $closure( $pagenow ); // OK, not an assignment. } -// Verify skipping over rest of the function when live coding/parse error in nested scope structure. -function global_vars() { - global $pagenow; - $closure = function ( $pagenow ) { - global $feeds; - $nested_closure_with_parse_error = function ( $feeds ) - $feeds = 'something'; // Bad, but ignored because of the parse error in the closure. - }; - $pagenow = 'something'; // Bad, should be picked up. Tests that skipping on parse error doesn't skip too far. -} + + + + + + + + + + $GLOBALS[] = 'something'; $GLOBALS[103] = 'something'; @@ -314,4 +314,3 @@ list( // Live coding/parse error. // This has to be the last test in the file! list( $tab, $tabs - diff --git a/WordPress/Tests/WP/GlobalVariablesOverrideUnitTest.7.inc b/WordPress/Tests/WP/GlobalVariablesOverrideUnitTest.7.inc new file mode 100644 index 0000000000..ca21a420fd --- /dev/null +++ b/WordPress/Tests/WP/GlobalVariablesOverrideUnitTest.7.inc @@ -0,0 +1,20 @@ + 1, 146 => 1, 181 => 1, - 198 => 1, 212 => 4, 230 => 2, 231 => 2, @@ -91,6 +90,11 @@ public function getErrorList( $testFile = '' ) { 29 => 1, ); + case 'GlobalVariablesOverrideUnitTest.7.inc': + return array( + 19 => 1, + ); + default: return array(); }