Skip to content

Add return types to private sniff methods #901

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
GaryJones merged 3 commits into from
Oct 9, 2026
Merged

Add return types to private sniff methods #901

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8aeee35
refactor: add return types to private sniff methods
tomjn Oct 1, 2026
ad1d874
refactor: add a nullable return type to test_patterns()
tomjn Oct 2, 2026
b7d2927
refactor: return the comparison directly in two typed methods
tomjn Oct 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions WordPressVIPMinimum/Sniffs/AbstractVariableRestrictionsSniff.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,9 +218,9 @@ public function process_token( $stackPtr ) {
* Transform a wildcard pattern to a usable regex pattern.
*
* @param string $pattern Pattern.
* @return string
* @return string|null
*/
private function test_patterns( $pattern ) {
private function test_patterns( $pattern ): ?string {
$pattern = preg_quote( $pattern, '#' );
$pattern = preg_replace(
[ '#\\\\\*#', '[\'"]' ],
Expand Down
4 changes: 2 additions & 2 deletions WordPressVIPMinimum/Sniffs/Classes/DeclarationCompatibilitySniff.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -392,7 +392,7 @@ public function process( File $phpcsFile, $stackPtr ) {
*
* @return void
*/
private function addError( File $phpcsFile, $stackPtr, $currScope, $parentClassName, $methodName, $currentMethodSignature, $parentMethodSignature ) {
private function addError( File $phpcsFile, $stackPtr, $currScope, $parentClassName, $methodName, $currentMethodSignature, $parentMethodSignature ): void {
$tokens = $phpcsFile->getTokens();
$currentClassName = '[AnonymousClass]';
if ( $tokens[ $currScope ]['code'] !== T_ANON_CLASS ) {
Expand All @@ -417,7 +417,7 @@ private function addError( File $phpcsFile, $stackPtr, $currScope, $parentClassN
*
* @return array<string>
*/
private function generateParamList( $methodSignature ) {
private function generateParamList( $methodSignature ): array {
$paramList = [];
foreach ( $methodSignature as $param => $options ) {
$paramName = '$';
Expand Down
2 changes: 1 addition & 1 deletion WordPressVIPMinimum/Sniffs/Constants/RestrictedConstantsSniff.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ public function process_token( $stackPtr ) {
*
* @return void
*/
private function process_unreliable_constant( $stackPtr, $constantName ) {
private function process_unreliable_constant( $stackPtr, $constantName ): void {
if ( $this->tokens[ $stackPtr ]['code'] === T_STRING ) {
if ( ConstantsHelper::is_use_of_global_constant( $this->phpcsFile, $stackPtr ) === false ) {
// Class constant, property, function name or something else which just shares the name.
Expand Down
2 changes: 1 addition & 1 deletion WordPressVIPMinimum/Sniffs/Files/IncludingFileSniff.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ public function process_token( $stackPtr ) {
*
* @return bool True if the string partially matches a keyword in $allowedCustomKeywords, false otherwise.
*/
private function has_custom_path( $content ) {
private function has_custom_path( $content ): bool {
foreach ( $this->allowedKeywords as $keyword ) {
if ( strpos( $content, $keyword ) !== false ) {
return true;
Expand Down
6 changes: 3 additions & 3 deletions WordPressVIPMinimum/Sniffs/Functions/CheckReturnValueSniff.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ public function process_token( $stackPtr ) {
*
* @return bool
*/
private function isFunctionCall( $stackPtr ) {
private function isFunctionCall( $stackPtr ): bool {

@GaryJones GaryJones Oct 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional nit: with : bool declared, the return ! ( $this->tokens[ $previous ]['code'] === T_FUNCTION ); at the end of this method might read a little more directly as return $this->tokens[ $previous ]['code'] !== T_FUNCTION;. Entirely up to you.

@tomjn tomjn Oct 2, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both done in b7d2927


if ( $this->tokens[ $stackPtr ]['code'] !== T_STRING ) {
return false;
Expand All @@ -106,7 +106,7 @@ private function isFunctionCall( $stackPtr ) {
$previous = $this->phpcsFile->findPrevious( $search, $stackPtr - 1, null, true );

// It's a function definition, not a function call, so return false.
return ! ( $this->tokens[ $previous ]['code'] === T_FUNCTION );
return $this->tokens[ $previous ]['code'] !== T_FUNCTION;
}

/**
Expand Down Expand Up @@ -312,7 +312,7 @@ public function reduce_array( $carry, $item ) {
*
* @return void
*/
private function addNonCheckedVariableError( $stackPtr, $variableName, $callee ) {
private function addNonCheckedVariableError( $stackPtr, $variableName, $callee ): void {
$message = 'Type of `%s` must be checked before calling `%s()` using that variable.';
$data = [ $variableName, $callee ];
$this->phpcsFile->addError( $message, $stackPtr, 'NonCheckedVariable', $data );
Expand Down
4 changes: 2 additions & 2 deletions WordPressVIPMinimum/Sniffs/Functions/DynamicCallsSniff.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ public function process_token( $stackPtr ) {
*
* @return void
*/
private function collect_variables( $stackPtr ) {
private function collect_variables( $stackPtr ): void {

$current_var_name = $this->tokens[ $stackPtr ]['content'];

Expand Down Expand Up @@ -169,7 +169,7 @@ private function collect_variables( $stackPtr ) {
*
* @return void
*/
private function find_dynamic_calls( $stackPtr ) {
private function find_dynamic_calls( $stackPtr ): void {
// No variables detected; no basis for doing anything.
if ( empty( $this->variables_arr ) ) {
return;
Expand Down
2 changes: 1 addition & 1 deletion WordPressVIPMinimum/Sniffs/Functions/StripTagsSniff.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ public function process_first_class_callable( $stackPtr, $group_name, $matched_c
*
* @return void
*/
private function add_warning( $stackPtr, $error_code = 'Used' ) {
private function add_warning( $stackPtr, $error_code = 'Used' ): void {
$message = '`strip_tags()` does not strip CSS and JS in between the script and style tags. Use `wp_strip_all_tags()` to strip all tags.';
$this->phpcsFile->addWarning( $message, $stackPtr, $error_code );
}
Expand Down
14 changes: 7 additions & 7 deletions WordPressVIPMinimum/Sniffs/Hooks/AlwaysReturnInFilterSniff.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ public function process_token( $stackPtr ) {
*
* @return void
*/
private function processArray( $stackPtr ) {
private function processArray( $stackPtr ): void {

$open_close = Arrays::getOpenClose( $this->phpcsFile, $stackPtr );
if ( $open_close === false ) {
Expand Down Expand Up @@ -135,7 +135,7 @@ private function processArray( $stackPtr ) {
*
* @return void
*/
private function processString( $stackPtr, $start = 0, $end = null ) {
private function processString( $stackPtr, $start = 0, $end = null ): void {

$callbackFunctionName = TextStrings::stripQuotes( $this->tokens[ $stackPtr ]['content'] );

Expand Down Expand Up @@ -164,7 +164,7 @@ private function processString( $stackPtr, $start = 0, $end = null ) {
*
* @return void
*/
private function processFunction( $stackPtr, $start = 0, $end = null ) {
private function processFunction( $stackPtr, $start = 0, $end = null ): void {

$functionName = $this->tokens[ $stackPtr ]['content'];

Expand All @@ -187,7 +187,7 @@ private function processFunction( $stackPtr, $start = 0, $end = null ) {
*
* @return void
*/
private function processFunctionBody( $stackPtr ) {
private function processFunctionBody( $stackPtr ): void {

$filterName = $this->tokens[ $this->filterNamePtr ]['content'];

Expand Down Expand Up @@ -265,7 +265,7 @@ private function processFunctionBody( $stackPtr ) {
*
* @return bool
*/
private function isInsideIfConditonal( $stackPtr ) {
private function isInsideIfConditonal( $stackPtr ): bool {

@GaryJones GaryJones Oct 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, not from this PR, just something I noticed in passing: the two end() calls on lines 272 and 277 read conditions before the array_key_exists() / is_array() / empty() guard on line 284, so the guard doesn't actually protect them, and the reset() on line 282 doesn't affect the result. Fine to leave for a follow-up, but it could probably collapse to one guard up front plus an in_array( T_IF, $conditions, true ) check.

@tomjn tomjn Oct 2, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, I'll leave it for a separate PR so this one stays about return types


// This check helps us in situations a class or a function is wrapped
// inside a conditional as a whole. Eg.: inside `class_exists`.
Expand Down Expand Up @@ -302,7 +302,7 @@ private function isInsideIfConditonal( $stackPtr ) {
*
* @return bool
*/
private function hasTerminatingStatement( $scopeStart, $scopeEnd ) {
private function hasTerminatingStatement( $scopeStart, $scopeEnd ): bool {

$terminatingPtr = $this->phpcsFile->findNext(
[ T_EXIT, T_THROW ],
Expand All @@ -320,7 +320,7 @@ private function hasTerminatingStatement( $scopeStart, $scopeEnd ) {
*
* @return bool
**/
private function isReturningVoid( $stackPtr ) {
private function isReturningVoid( $stackPtr ): bool {

$nextToReturnTokenPtr = $this->phpcsFile->findNext(
Tokens::$emptyTokens,
Expand Down
28 changes: 12 additions & 16 deletions WordPressVIPMinimum/Sniffs/Hooks/PreGetPostsSniff.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ public function process_token( $stackPtr ) {
*
* @return void
*/
private function processArray( $stackPtr ) {
private function processArray( $stackPtr ): void {

$open_close = Arrays::getOpenClose( $this->phpcsFile, $stackPtr );
if ( $open_close === false ) {
Expand All @@ -120,7 +120,7 @@ private function processArray( $stackPtr ) {
*
* @return void
*/
private function processString( $stackPtr ) {
private function processString( $stackPtr ): void {

$callbackFunctionName = substr( $this->tokens[ $stackPtr ]['content'], 1, -1 );

Expand All @@ -147,7 +147,7 @@ private function processString( $stackPtr ) {
*
* @return void
*/
private function processFunction( $stackPtr ) {
private function processFunction( $stackPtr ): void {

$wpQueryObjectNamePtr = $this->phpcsFile->findNext(
[ T_VARIABLE ],
Expand Down Expand Up @@ -182,7 +182,7 @@ private function processFunction( $stackPtr ) {
*
* @return void
*/
private function processClosure( $stackPtr ) {
private function processClosure( $stackPtr ): void {

$wpQueryObjectNamePtr = $this->phpcsFile->findNext(
[ T_VARIABLE ],
Expand All @@ -209,7 +209,7 @@ private function processClosure( $stackPtr ) {
*
* @return void
*/
private function processFunctionBody( $stackPtr, $variableName ) {
private function processFunctionBody( $stackPtr, $variableName ): void {

$functionBodyScopeStart = $this->tokens[ $stackPtr ]['scope_opener'];
$functionBodyScopeEnd = $this->tokens[ $stackPtr ]['scope_closer'];
Expand Down Expand Up @@ -250,7 +250,7 @@ private function processFunctionBody( $stackPtr, $variableName ) {
*
* @return void
*/
private function addPreGetPostsWarning( $stackPtr ) {
private function addPreGetPostsWarning( $stackPtr ): void {
$message = 'Main WP_Query is being modified without `$query->is_main_query()` check. Needs manual inspection.';
$this->phpcsFile->addWarning( $message, $stackPtr, 'PreGetPosts' );
}
Expand All @@ -262,7 +262,7 @@ private function addPreGetPostsWarning( $stackPtr ) {
*
* @return bool
*/
private function isParentConditionalCheckingMainQuery( $stackPtr ) {
private function isParentConditionalCheckingMainQuery( $stackPtr ): bool {

if ( array_key_exists( 'conditions', $this->tokens[ $stackPtr ] ) === false
|| is_array( $this->tokens[ $stackPtr ]['conditions'] ) === false
Expand Down Expand Up @@ -312,7 +312,7 @@ private function isParentConditionalCheckingMainQuery( $stackPtr ) {
*
* @return bool
*/
private function isEarlyMainQueryCheck( $stackPtr ) {
private function isEarlyMainQueryCheck( $stackPtr ): bool {

@GaryJones GaryJones Oct 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not something this PR introduces, but I noticed it while checking the : bool here. The findNext( [ T_RETURN ], ..., 'return', true ) call further down uses $local = true, so it stops at the first semicolon in the if body. That means the early return is only recognised if it's the first statement, which gives a false positive for something like:

add_action( 'pre_get_posts', function( $query ) {
	if ( ! $query->is_main_query() ) {
		do_log( 'x' );
		return;
	}
	$query->set( 'cat', '-5' );
} );

Remove the do_log() line and the warning goes away. Happy to open an issue for it rather than widen this PR.

Tiny nit while we're here, now that it's typed: the if ( $next ) { return true; } return false; at the end could just be return $next !== false;.

@tomjn tomjn Oct 2, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can reproduce that with the snippet, but I think it needs an issue and another PR


if ( ! $this->isWPQueryMethodCall( $stackPtr, 'is_main_query' ) ) {
return false;
Expand Down Expand Up @@ -365,11 +365,7 @@ private function isEarlyMainQueryCheck( $stackPtr ) {
true
);

if ( $next ) {
return true;
}

return false;
return $next !== false;
}

/**
Expand All @@ -380,7 +376,7 @@ private function isEarlyMainQueryCheck( $stackPtr ) {
*
* @return bool
*/
private function isWPQueryMethodCall( $stackPtr, $method = null ) {
private function isWPQueryMethodCall( $stackPtr, $method = null ): bool {
$next = $this->phpcsFile->findNext(
Tokens::$emptyTokens,
$stackPtr + 1,
Expand Down Expand Up @@ -417,7 +413,7 @@ private function isWPQueryMethodCall( $stackPtr, $method = null ) {
*
* @return bool
*/
private function isPartOfIfConditional( $stackPtr ) {
private function isPartOfIfConditional( $stackPtr ): bool {

if ( array_key_exists( 'nested_parenthesis', $this->tokens[ $stackPtr ] ) === true
&& is_array( $this->tokens[ $stackPtr ]['nested_parenthesis'] ) === true
Expand Down Expand Up @@ -448,7 +444,7 @@ private function isPartOfIfConditional( $stackPtr ) {
*
* @return bool
*/
private function isInsideIfConditonal( $stackPtr ) {
private function isInsideIfConditonal( $stackPtr ): bool {

if ( array_key_exists( 'conditions', $this->tokens[ $stackPtr ] ) === true
&& is_array( $this->tokens[ $stackPtr ]['conditions'] ) === true
Expand Down
2 changes: 1 addition & 1 deletion WordPressVIPMinimum/Sniffs/Hooks/RestrictedHooksSniff.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ public function process_parameters( $stackPtr, $group_name, $matched_content, $p
*
* @return string Normalized hook name or an empty string if the hook name could not be determined.
*/
private function normalize_hook_name_from_parameter( $parameter ) {
private function normalize_hook_name_from_parameter( $parameter ): string {
$allowed_tokens = Tokens::$emptyTokens;
$allowed_tokens += [
T_STRING_CONCAT => T_STRING_CONCAT,
Expand Down
2 changes: 1 addition & 1 deletion WordPressVIPMinimum/Sniffs/JS/StringConcatSniff.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ public function process_token( $stackPtr ) {
*
* @return void
*/
private function addFoundError( $stackPtr, array $data ) {
private function addFoundError( $stackPtr, array $data ): void {
$message = 'HTML string concatenation detected, this is a security risk, use DOM node construction or a templating language instead: %s.';
$this->phpcsFile->addError( $message, $stackPtr, 'Found', $data );
}
Expand Down
2 changes: 1 addition & 1 deletion WordPressVIPMinimum/Sniffs/Performance/FetchingRemoteDataSniff.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ public function process_first_class_callable( $stackPtr, $group_name, $matched_c
*
* @return void
*/
private function add_contents_unknown_warning( $stackPtr, $data ) {
private function add_contents_unknown_warning( $stackPtr, $data ): void {
$message = '`%s()` is highly discouraged for remote requests, please use `wpcom_vip_file_get_contents()` or `vip_safe_wp_remote_get()` instead. If it\'s for a local file please use WP_Filesystem instead.';
$this->phpcsFile->addWarning( $message, $stackPtr, 'FileGetContentsUnknown', $data );
}
Expand Down
2 changes: 1 addition & 1 deletion WordPressVIPMinimum/Sniffs/Security/StaticStrreplaceSniff.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ public function process_parameters( $stackPtr, $group_name, $matched_content, $p
*
* @return bool
*/
private function is_parameter_static_text( $param_info ) {
private function is_parameter_static_text( $param_info ): bool {
// List of tokens which can be skipped over without further examination.
$static_tokens = [
T_CONSTANT_ENCAPSED_STRING => T_CONSTANT_ENCAPSED_STRING,
Expand Down
2 changes: 1 addition & 1 deletion WordPressVIPMinimum/Sniffs/UserExperience/AdminBarRemovalSniff.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -398,7 +398,7 @@ protected function process_css_style( $stackPtr ) {
*
* @return void
*/
private function addHidingDetectedError( $stackPtr ) {
private function addHidingDetectedError( $stackPtr ): void {
$message = 'Hiding of the admin bar is not allowed.';
$this->phpcsFile->addError( $message, $stackPtr, 'HidingDetected' );
}
Expand Down
Loading