Skip to content

Commit e9af795

Browse files
rodrigoprimorodrigoprimo
committed
DB/PreparedSQLPlaceholders: fix false negatives for namespaced and method calls to sprintf()
The sniff was incorrectly analyzing namespaced function calls and method calls to `sprintf()` as if they were calls to the global `sprintf()` function causing false negatives. This commit introduces the `is_global_function_call()` helper method that properly distinguishes global function calls from method calls and namespaced function calls. This issue only affected `sprintf()`. The detection of `implode()` and array_fill() was already correctly filtering out namespaced and method calls by checking the preceding token (excluding empty tokens and T_NS_SEPARATOR). That being said, the helper method is applied to all sprintf(), implode(), and array_fill() checks for consistency and future-proofing.
1 parent 8637a99 commit e9af795

File tree

3 files changed

+211
-8
lines changed

3 files changed

+211
-8
lines changed

‎WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php‎

Lines changed: 43 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
use PHPCSUtils\Utils\Arrays;
1515
use PHPCSUtils\Utils\PassedParameters;
1616
use PHPCSUtils\Utils\TextStrings;
17+
use WordPressCS\WordPress\Helpers\ContextHelper;
1718
use WordPressCS\WordPress\Helpers\MinimumWPVersionTrait;
1819
use WordPressCS\WordPress\Helpers\WPDBTrait;
1920
use WordPressCS\WordPress\Sniff;
@@ -226,7 +227,7 @@ public function process_token( $stackPtr ) {
226227
// Detect a specific pattern for variable replacements in combination with `IN`.
227228
if ( \T_STRING === $this->tokens[ $i ]['code'] ) {
228229

229-
if ( 'sprintf' === strtolower( $this->tokens[ $i ]['content'] ) ) {
230+
if ( $this->is_global_function_call( $i, 'sprintf' ) ) {
230231
$sprintf_parameters = PassedParameters::getParameters( $this->phpcsFile, $i );
231232

232233
if ( ! empty( $sprintf_parameters ) ) {
@@ -265,7 +266,7 @@ public function process_token( $stackPtr ) {
265266
}
266267
unset( $sprintf_parameters, $valid_sprintf, $last_param );
267268

268-
} elseif ( 'implode' === strtolower( $this->tokens[ $i ]['content'] ) ) {
269+
} elseif ( $this->is_global_function_call( $i, 'implode' ) ) {
269270
$ignore_tokens = Tokens::$emptyTokens + array(
270271
\T_STRING_CONCAT => \T_STRING_CONCAT,
271272
\T_NS_SEPARATOR => \T_NS_SEPARATOR,
@@ -679,9 +680,7 @@ protected function analyse_sprintf( $sprintf_params ) {
679680
$sprintf_param['end'],
680681
true
681682
);
682-
if ( \T_STRING === $this->tokens[ $implode ]['code']
683-
&& 'implode' === strtolower( $this->tokens[ $implode ]['content'] )
684-
) {
683+
if ( $this->is_global_function_call( $implode, 'implode' ) ) {
685684
if ( $this->analyse_implode( $implode ) === true ) {
686685
++$found;
687686
}
@@ -737,9 +736,7 @@ protected function analyse_implode( $implode_token ) {
737736
true
738737
);
739738

740-
if ( \T_STRING !== $this->tokens[ $array_fill ]['code']
741-
|| 'array_fill' !== strtolower( $this->tokens[ $array_fill ]['content'] )
742-
) {
739+
if ( ! $this->is_global_function_call( $array_fill, 'array_fill' ) ) {
743740
return false;
744741
}
745742

@@ -763,4 +760,42 @@ protected function analyse_implode( $implode_token ) {
763760

764761
return (bool) preg_match( '`^(["\'])%[dfFs]\1$`', $array_fill_value_param['clean'] );
765762
}
763+
764+
/**
765+
* Check if a token represents a call to a global function with the specified name.
766+
*
767+
* Note: This is not a comprehensive global function call check. It was designed specifically
768+
* for this sniff's needs and may not handle all edge cases.
769+
*
770+
* @since 3.3.0
771+
*
772+
* @param int $function_ptr The position of the token to check.
773+
* @param string $function_name The function name to check for (case-insensitive).
774+
*
775+
* @return bool True if it's a call to the global function, false otherwise.
776+
*/
777+
protected function is_global_function_call( $function_ptr, $function_name ) {
778+
if ( \T_STRING !== $this->tokens[ $function_ptr ]['code'] ) {
779+
return false;
780+
}
781+
782+
if ( strtolower( $this->tokens[ $function_ptr ]['content'] ) !== $function_name ) {
783+
return false;
784+
}
785+
786+
$next = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $function_ptr + 1 ), null, true );
787+
if ( false === $next || \T_OPEN_PARENTHESIS !== $this->tokens[ $next ]['code'] ) {
788+
return false;
789+
}
790+
791+
if ( ContextHelper::has_object_operator_before( $this->phpcsFile, $function_ptr ) === true ) {
792+
return false;
793+
}
794+
795+
if ( ContextHelper::is_token_namespaced( $this->phpcsFile, $function_ptr ) === true ) {
796+
return false;
797+
}
798+
799+
return true;
800+
}
766801
}

‎WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.inc‎

Lines changed: 147 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -653,3 +653,150 @@ $where = $wpdb->prepare(
653653
. ')',
654654
'publish'
655655
);
656+
657+
/*
658+
* Safeguard correct handling of all types of namespaced calls to sprintf() except fully qualified calls to the
659+
* global sprintf() function, which is already handled above.
660+
*/
661+
$where = $wpdb->prepare(
662+
MyNamespace\sprintf(
663+
"{$wpdb->posts}.post_type IN (%s)",
664+
implode( ',', array_fill( 0, count($post_types), '%s' ) )
665+
),
666+
$post_types
667+
);
668+
$where = $wpdb->prepare(
669+
\MyNamespace\sprintf(
670+
"{$wpdb->posts}.post_type IN (%s)",
671+
implode( ',', array_fill( 0, count($post_types), '%s' ) )
672+
),
673+
$post_types
674+
);
675+
$where = $wpdb->prepare(
676+
namespace\sprintf( // This should NOT be flagged in the future once the sniff is able to resolve relative namespaces.
677+
"{$wpdb->posts}.post_type IN (%s)",
678+
implode( ',', array_fill( 0, count($post_types), '%s' ) )
679+
),
680+
$post_types
681+
);
682+
$where = $wpdb->prepare(
683+
namespace\Sub\sprintf(
684+
"{$wpdb->posts}.post_type IN (%s)",
685+
implode( ',', array_fill( 0, count($post_types), '%s' ) )
686+
),
687+
$post_types
688+
);
689+
690+
/*
691+
* Safeguard correct handling of method calls to methods named sprintf(), implode(), or array_fill().
692+
* These should NOT be analyzed as global function calls.
693+
*/
694+
695+
// sprintf() method calls.
696+
$where = $wpdb->prepare(
697+
$obj->sprintf(
698+
"{$wpdb->posts}.post_type IN (%s)",
699+
implode( ',', array_fill( 0, count($post_types), '%s' ) )
700+
),
701+
$post_types
702+
);
703+
$where = $wpdb->prepare(
704+
$obj?->sprintf(
705+
"{$wpdb->posts}.post_type IN (%s)",
706+
implode( ',', array_fill( 0, count($post_types), '%s' ) )
707+
),
708+
$post_types
709+
);
710+
$where = $wpdb->prepare(
711+
MyClass::sprintf(
712+
"{$wpdb->posts}.post_type IN (%s)",
713+
implode( ',', array_fill( 0, count($post_types), '%s' ) )
714+
),
715+
$post_types
716+
);
717+
718+
// implode() method calls nested inside sprintf().
719+
$where = $wpdb->prepare(
720+
sprintf(
721+
"{$wpdb->posts}.post_type IN (%s)",
722+
$obj->implode( ',', array_fill( 0, count($post_types), '%s' ) )
723+
),
724+
$post_types
725+
);
726+
$where = $wpdb->prepare(
727+
sprintf(
728+
"{$wpdb->posts}.post_type IN (%s)",
729+
$obj?->implode( ',', array_fill( 0, count($post_types), '%s' ) )
730+
),
731+
$post_types
732+
);
733+
$where = $wpdb->prepare(
734+
sprintf(
735+
"{$wpdb->posts}.post_type IN (%s)",
736+
MyClass::implode( ',', array_fill( 0, count($post_types), '%s' ) )
737+
),
738+
$post_types
739+
);
740+
741+
// implode() method calls in direct string concatenation.
742+
$where = $wpdb->prepare(
743+
"post_status = %s AND post_type IN ("
744+
. $obj->implode( ',', array_fill( 0, count($post_types), '%s' ) )
745+
. ')',
746+
'publish'
747+
);
748+
$where = $wpdb->prepare(
749+
"post_status = %s AND post_type IN ("
750+
. $obj?->implode( ',', array_fill( 0, count($post_types), '%s' ) )
751+
. ')',
752+
'publish'
753+
);
754+
$where = $wpdb->prepare(
755+
"post_status = %s AND post_type IN ("
756+
. MyClass::implode( ',', array_fill( 0, count($post_types), '%s' ) )
757+
. ')',
758+
'publish'
759+
);
760+
761+
// array_fill() method calls nested inside implode() which is nested inside sprintf().
762+
$where = $wpdb->prepare(
763+
sprintf(
764+
"{$wpdb->posts}.post_type IN (%s)",
765+
implode( ',', $obj->array_fill( 0, count($post_types), '%s' ) )
766+
),
767+
$post_types
768+
);
769+
$where = $wpdb->prepare(
770+
sprintf(
771+
"{$wpdb->posts}.post_type IN (%s)",
772+
implode( ',', $obj?->array_fill( 0, count($post_types), '%s' ) )
773+
),
774+
$post_types
775+
);
776+
$where = $wpdb->prepare(
777+
sprintf(
778+
"{$wpdb->posts}.post_type IN (%s)",
779+
implode( ',', MyClass::array_fill( 0, count($post_types), '%s' ) )
780+
),
781+
$post_types
782+
);
783+
784+
// array_fill() method calls nested inside implode() in direct string concatenation.
785+
$where = $wpdb->prepare(
786+
"post_status = %s AND post_type IN ("
787+
. implode( ',', $obj->array_fill( 0, count($post_types), '%s' ) )
788+
. ')',
789+
'publish'
790+
);
791+
$where = $wpdb->prepare(
792+
"post_status = %s AND post_type IN ("
793+
. implode( ',', $obj?->array_fill( 0, count($post_types), '%s' ) )
794+
. ')',
795+
'publish'
796+
);
797+
$where = $wpdb->prepare(
798+
"post_status = %s AND post_type IN ("
799+
. implode( ',', MyClass::array_fill( 0, count($post_types), '%s' ) )
800+
. ')',
801+
'publish'
802+
);

‎WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.php‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -184,6 +184,27 @@ public function getWarningList() {
184184
639 => 1,
185185
644 => 1,
186186
650 => 1,
187+
661 => 1,
188+
668 => 1,
189+
675 => 1,
190+
682 => 1,
191+
192+
// Method sprintf/implode/array_fill calls.
193+
696 => 1,
194+
703 => 1,
195+
710 => 1,
196+
719 => 1,
197+
726 => 1,
198+
733 => 1,
199+
742 => 1,
200+
748 => 1,
201+
754 => 1,
202+
762 => 1,
203+
769 => 1,
204+
776 => 1,
205+
785 => 1,
206+
791 => 1,
207+
797 => 1,
187208
);
188209
}
189210
}

0 commit comments

Comments
 (0)