Skip to content

Commit 60d8d6e

Browse files
rodrigoprimorodrigoprimo
committed
WIP DB/PreparedSQLPlaceholders: fix a potential bug and add more tests
I first need to discuss if this is indeed a potential bug
1 parent a5b0519 commit 60d8d6e

File tree

3 files changed

+191
-19
lines changed

3 files changed

+191
-19
lines changed

‎WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php‎

Lines changed: 51 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,50 @@ 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+
* This method verifies that:
768+
* - The token is a T_STRING.
769+
* - The token content matches the function name (case-insensitive).
770+
* - It's followed by an opening parenthesis.
771+
* - It's not a method call (no object operator before it).
772+
* - It's not a namespaced function call.
773+
*
774+
* Note: This is not a comprehensive global function call check. It was designed specifically
775+
* for this sniff's needs and may not handle all edge cases such as first-class callables or
776+
* `use function` imports.
777+
*
778+
* @since 3.3.0
779+
*
780+
* @param int $functionPtr The position of the token to check.
781+
* @param string $function_name The function name to check for (case-insensitive).
782+
*
783+
* @return bool True if it's a call to the global function, false otherwise.
784+
*/
785+
protected function is_global_function_call( $functionPtr, $function_name ) {
786+
if ( \T_STRING !== $this->tokens[ $functionPtr ]['code'] ) {
787+
return false;
788+
}
789+
790+
if ( strtolower( $this->tokens[ $functionPtr ]['content'] ) !== $function_name ) {
791+
return false;
792+
}
793+
794+
$next = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $functionPtr + 1 ), null, true );
795+
if ( false === $next || \T_OPEN_PARENTHESIS !== $this->tokens[ $next ]['code'] ) {
796+
return false;
797+
}
798+
799+
if ( ContextHelper::has_object_operator_before( $this->phpcsFile, $functionPtr ) === true ) {
800+
return false;
801+
}
802+
803+
if ( ContextHelper::is_token_namespaced( $this->phpcsFile, $functionPtr ) === true ) {
804+
return false;
805+
}
806+
807+
return true;
808+
}
766809
}

‎WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.inc‎

Lines changed: 120 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -519,15 +519,124 @@ $where = $wpdb->prepare(
519519
$callback = $wpdb->prepare(...); // OK.
520520

521521
/*
522-
* Safeguard correct handling of namespaced function calls.
522+
* Safeguard correct handling of all types of namespaced calls to the WPDB::prepare() method.
523+
*
524+
* Note that calling wpdb::prepare() statically will result in an error. Still, the tests are included here since the
525+
* sniff handles those calls.
523526
*/
524-
//$sql = MyNamespace\WPDB::prepare( "SELECT * FROM $wpdb->users WHERE id = %d AND user_login = %s" );
525-
//$sql = \MyNamespace\WPDB::prepare( "SELECT * FROM $wpdb->users WHERE id = %d AND user_login = %s" );
526-
//$sql = namespace\WPDB::prepare( "SELECT * FROM $wpdb->users WHERE id = %d AND user_login = %s" ); // The sniff should start flagging this once it can resolve relative namespaces.
527-
//$where = $wpdb->prepare(
528-
// MyNamespace\sprintf(
529-
// "{$wpdb->posts}.post_type IN (%s)",
530-
// MyNamespace\implode( ',', MyNamespace\array_fill( 0, count($post_types), '%s' ) )
531-
// ),
532-
// $post_types
533-
//);
527+
$sql = \WPDB::prepare( "SELECT * FROM $wpdb->users WHERE id = %d AND user_login = %s" );
528+
$sql = MyNamespace\WPDB::prepare( "SELECT * FROM $wpdb->users WHERE id = %d AND user_login = %s" );
529+
$sql = \MyNamespace\WPDB::prepare( "SELECT * FROM $wpdb->users WHERE id = %d AND user_login = %s" );
530+
$sql = namespace\WPDB::prepare( "SELECT * FROM $wpdb->users WHERE id = %d AND user_login = %s" ); // The sniff should start flagging this once it can resolve relative namespaces.
531+
532+
/*
533+
* Safeguard correct handling of all types of namespaced calls to sprintf() except fully qualified calls to the
534+
* global sprintf() function, which is already handled above.
535+
*/
536+
$where = $wpdb->prepare(
537+
MyNamespace\sprintf(
538+
"{$wpdb->posts}.post_type IN (%s)",
539+
implode( ',', array_fill( 0, count($post_types), '%s' ) )
540+
),
541+
$post_types
542+
);
543+
$where = $wpdb->prepare(
544+
\MyNamespace\sprintf(
545+
"{$wpdb->posts}.post_type IN (%s)",
546+
implode( ',', array_fill( 0, count($post_types), '%s' ) )
547+
),
548+
$post_types
549+
);
550+
$where = $wpdb->prepare(
551+
namespace\sprintf( // This should be flagged in the future once the sniff is able to resolve relative namespaces.
552+
"{$wpdb->posts}.post_type IN (%s)",
553+
implode( ',', array_fill( 0, count($post_types), '%s' ) )
554+
),
555+
$post_types
556+
);
557+
558+
/*
559+
* Safeguard correct handling of all types of namespaced calls to implode() except fully qualified calls to the
560+
* global implode() function, which is already handled above.
561+
*/
562+
$where = $wpdb->prepare(
563+
sprintf(
564+
"{$wpdb->posts}.post_type IN (%s)",
565+
MyNamespace\implode( ',', array_fill( 0, count($post_types), '%s' ) )
566+
),
567+
$post_types
568+
);
569+
$where = $wpdb->prepare(
570+
sprintf(
571+
"{$wpdb->posts}.post_type IN (%s)",
572+
\MyNamespace\implode( ',', array_fill( 0, count($post_types), '%s' ) )
573+
),
574+
$post_types
575+
);
576+
$where = $wpdb->prepare(
577+
sprintf(
578+
"{$wpdb->posts}.post_type IN (%s)",
579+
namespace\implode( ',', array_fill( 0, count($post_types), '%s' ) ) // This should be flagged in the future once the sniff is able to resolve relative namespaces.
580+
),
581+
$post_types
582+
);
583+
584+
/*
585+
* Safeguard correct handling of all types of namespaced calls to array_fill() except fully qualified calls to the
586+
* global array_fill() function, which is already handled above.
587+
*/
588+
$where = $wpdb->prepare(
589+
sprintf(
590+
"{$wpdb->posts}.post_type IN (%s)",
591+
implode( ',', MyNamespace\array_fill( 0, count($post_types), '%s' ) )
592+
),
593+
$post_types
594+
);
595+
$where = $wpdb->prepare(
596+
sprintf(
597+
"{$wpdb->posts}.post_type IN (%s)",
598+
implode( ',', \MyNamespace\array_fill( 0, count($post_types), '%s' ) )
599+
),
600+
$post_types
601+
);
602+
$where = $wpdb->prepare(
603+
sprintf(
604+
"{$wpdb->posts}.post_type IN (%s)",
605+
implode( ',', namespace\array_fill( 0, count($post_types), '%s' ) ) // This should be flagged in the future once the sniff is able to resolve relative namespaces.
606+
),
607+
$post_types
608+
);
609+
610+
/*
611+
* Safeguard correct handling of namespaced implode() calls detected in the main loop (preceded by IN ()).
612+
* These test the implode detection at line 269 in the sniff.
613+
*/
614+
$where = $wpdb->prepare(
615+
"SELECT * FROM {$wpdb->posts} WHERE post_status = %s AND post_type IN (" . MyNamespace\implode( ',', array_fill( 0, count($post_types), '%s' ) ) . ")",
616+
'publish'
617+
);
618+
$where = $wpdb->prepare(
619+
"SELECT * FROM {$wpdb->posts} WHERE post_status = %s AND post_type IN (" . \MyNamespace\implode( ',', array_fill( 0, count($post_types), '%s' ) ) . ")",
620+
'publish'
621+
);
622+
$where = $wpdb->prepare(
623+
"SELECT * FROM {$wpdb->posts} WHERE post_status = %s AND post_type IN (" . namespace\implode( ',', array_fill( 0, count($post_types), '%s' ) ) . ")",
624+
'publish'
625+
);
626+
627+
/*
628+
* Safeguard correct handling of namespaced array_fill() calls detected via main loop implode detection.
629+
* These test the array_fill detection at line 739 called from main loop's implode at line 269.
630+
*/
631+
$where = $wpdb->prepare(
632+
"SELECT * FROM {$wpdb->posts} WHERE post_status = %s AND post_type IN (" . implode( ',', MyNamespace\array_fill( 0, count($post_types), '%s' ) ) . ")",
633+
'publish'
634+
);
635+
$where = $wpdb->prepare(
636+
"SELECT * FROM {$wpdb->posts} WHERE post_status = %s AND post_type IN (" . implode( ',', \MyNamespace\array_fill( 0, count($post_types), '%s' ) ) . ")",
637+
'publish'
638+
);
639+
$where = $wpdb->prepare(
640+
"SELECT * FROM {$wpdb->posts} WHERE post_status = %s AND post_type IN (" . implode( ',', namespace\array_fill( 0, count($post_types), '%s' ) ) . ")",
641+
'publish'
642+
);

‎WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.php‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -107,6 +107,9 @@ public function getErrorList() {
107107

108108
// Named parameter support.
109109
418 => 1,
110+
111+
// Fully qualified \WPDB::prepare() call.
112+
527 => 1,
110113
);
111114
}
112115

@@ -162,6 +165,23 @@ public function getWarningList() {
162165
482 => 1,
163166
490 => 1,
164167
498 => 1,
168+
169+
// Namespaced sprintf/implode/array_fill calls.
170+
536 => 1, // MyNamespace\sprintf in sprintf/implode pattern.
171+
543 => 1, // \MyNamespace\sprintf in sprintf/implode pattern.
172+
550 => 1, // namespace\sprintf in sprintf/implode pattern.
173+
562 => 1, // MyNamespace\implode inside sprintf parameter.
174+
569 => 1, // \MyNamespace\implode inside sprintf parameter.
175+
576 => 1, // namespace\implode inside sprintf parameter.
176+
588 => 1, // MyNamespace\array_fill inside sprintf->implode.
177+
595 => 1, // \MyNamespace\array_fill inside sprintf->implode.
178+
602 => 1, // namespace\array_fill inside sprintf->implode.
179+
614 => 1, // MyNamespace\implode in main loop (preceded by IN).
180+
618 => 1, // \MyNamespace\implode in main loop (preceded by IN).
181+
622 => 1, // namespace\implode in main loop (preceded by IN).
182+
631 => 1, // MyNamespace\array_fill via main loop implode.
183+
635 => 1, // \MyNamespace\array_fill via main loop implode.
184+
639 => 1, // namespace\array_fill via main loop implode.
165185
);
166186
}
167187
}

0 commit comments

Comments
 (0)