Skip to content

Commit

Permalink
Merge pull request #2328 from WordPress/feature/core-check-function-d…
Browse files Browse the repository at this point in the history 
…eclarations
  • Loading branch information
@GaryJones
GaryJones authored Aug 11, 2023
2 parents 9cf9f24 + 602d9ef commit 306542c
Show file tree
Hide file tree
Showing 10 changed files with 84 additions and 436 deletions.
2 changes: 1 addition & 1 deletion .github/workflows/basic-qa.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
26 changes: 22 additions & 4 deletions WordPress-Core/ruleset.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -199,19 +199,37 @@
<!-- Covers rule: Define a function like so: function my_function( $param1 = 'foo', $param2 = 'bar' ) { -->
<rule ref="Generic.Functions.OpeningFunctionBraceKernighanRitchie">
<properties>
<property name="checkClosures" value="true"/>
<property name="checkClosures" value="false"/>
</properties>
</rule>
<!-- Prevent duplicate message. This is now checked by the Squiz.Functions.MultiLineFunctionDeclaration sniff. -->
<rule ref="Generic.Functions.OpeningFunctionBraceKernighanRitchie.ContentAfterBrace">
<severity>0</severity>
</rule>

<rule ref="Squiz.Functions.MultiLineFunctionDeclaration"/>
<!-- WP demands brace on same line, not on the next line. Silence errors related to "brace on same line". -->
<rule ref="Squiz.Functions.MultiLineFunctionDeclaration.BraceOnSameLine">
<severity>0</severity>
</rule>
<rule ref="Squiz.Functions.MultiLineFunctionDeclaration.BraceSpacing">
<severity>0</severity>
</rule>
<rule ref="Squiz.Functions.MultiLineFunctionDeclaration.BraceIndent">
<severity>0</severity>
</rule>
<!-- Prevent duplicate message. This is already checked by the Generic.WhiteSpace.LanguageConstructSpacing sniff. -->
<rule ref="Squiz.Functions.MultiLineFunctionDeclaration.SpaceAfterUse">
<severity>0</severity>
</rule>

<rule ref="Squiz.Functions.FunctionDeclarationArgumentSpacing">
<properties>
<property name="equalsSpacing" value="1"/>
<property name="requiredSpacesAfterOpen" value="1"/>
<property name="requiredSpacesBeforeClose" value="1"/>
</properties>
</rule>
<rule ref="Squiz.Functions.FunctionDeclarationArgumentSpacing.SpacingBeforeClose">
<severity>0</severity>
</rule>
<rule ref="Squiz.Functions.FunctionDeclarationArgumentSpacing.SpacingAfterVariadic">
<severity>0</severity>
</rule>
Expand Down
2 changes: 1 addition & 1 deletion WordPress/Sniffs/Security/ValidatedSanitizedInputSniff.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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'] )
Expand Down
155 changes: 14 additions & 141 deletions WordPress/Sniffs/WhiteSpace/ControlStructureSpacingSniff.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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
Expand Down Expand Up @@ -52,33 +52,19 @@ 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
*
* @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,
);

/**
Expand All @@ -96,9 +82,6 @@ public function register() {
\T_DO,
\T_ELSE,
\T_ELSEIF,
\T_FUNCTION,
\T_CLOSURE,
\T_USE,
\T_TRY,
\T_CATCH,
);
Expand All @@ -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' );
Expand All @@ -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 {
Expand Down Expand Up @@ -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, ' ' );
}
}

Expand All @@ -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' );

Expand Down Expand Up @@ -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';
Expand All @@ -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'] )
) {
Expand All @@ -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(
Expand Down
21 changes: 10 additions & 11 deletions WordPress/Tests/WP/GlobalVariablesOverrideUnitTest.1.inc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -314,4 +314,3 @@ list(
// Live coding/parse error.
// This has to be the last test in the file!
list( $tab, $tabs

20 changes: 20 additions & 0 deletions WordPress/Tests/WP/GlobalVariablesOverrideUnitTest.7.inc
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
<?php

/*
* Separate test file to isolate the parse error test.
*/

// 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.
}
6 changes: 5 additions & 1 deletion WordPress/Tests/WP/GlobalVariablesOverrideUnitTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,6 @@ public function getErrorList( $testFile = '' ) {
143 => 1,
146 => 1,
181 => 1,
198 => 1,
212 => 4,
230 => 2,
231 => 2,
Expand Down Expand Up @@ -91,6 +90,11 @@ public function getErrorList( $testFile = '' ) {
29 => 1,
);

case 'GlobalVariablesOverrideUnitTest.7.inc':
return array(
19 => 1,
);

default:
return array();
}
Expand Down
Loading

0 comments on commit 306542c

Please sign in to comment.