Skip to content

Commit bf9d82a

Browse files
dingo-ddingo-d
authored
Merge pull request #2314 from WordPress/feature/db-preparedsqlplaceholders-review-phpcsutils-modern-php
DB/PreparedSQLPlaceholders: use PHPCSUtils, allow for modern PHP and some bug fixes
2 parents cfe143d + 9045552 commit bf9d82a

File tree

3 files changed

+370
-67
lines changed

3 files changed

+370
-67
lines changed

‎WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php

Lines changed: 94 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -10,16 +10,18 @@
1010
namespace WordPressCS\WordPress\Sniffs\DB;
1111

1212
use PHP_CodeSniffer\Util\Tokens;
13+
use PHPCSUtils\Tokens\Collections;
14+
use PHPCSUtils\Utils\Arrays;
1315
use PHPCSUtils\Utils\PassedParameters;
1416
use PHPCSUtils\Utils\TextStrings;
1517
use WordPressCS\WordPress\Helpers\MinimumWPVersionTrait;
1618
use WordPressCS\WordPress\Helpers\WPDBTrait;
1719
use WordPressCS\WordPress\Sniff;
1820

1921
/**
20-
* Check for incorrect use of the $wpdb->prepare method.
22+
* Checks for incorrect use of the $wpdb->prepare method.
2123
*
22-
* Check the following issues:
24+
* Checks the following issues:
2325
* - The only placeholders supported are: %d, %f (%F), %s, %i, and their variations.
2426
* - Literal % signs need to be properly escaped as `%%`.
2527
* - Simple placeholders (%d, %f, %F, %s, %i) should be left unquoted in the query string.
@@ -190,7 +192,11 @@ public function process_token( $stackPtr ) {
190192
return;
191193
}
192194

193-
$query = $parameters[1];
195+
$query = PassedParameters::getParameterFromStack( $parameters, 1, 'query' );
196+
if ( false === $query ) {
197+
return;
198+
}
199+
194200
$text_string_tokens_found = false;
195201
$variable_found = false;
196202
$sql_wildcard_found = false;
@@ -201,10 +207,12 @@ public function process_token( $stackPtr ) {
201207
'implode_fill' => 0,
202208
'adjustment_count' => 0,
203209
);
210+
$skip_from = null;
211+
$skip_to = null;
204212

205213
for ( $i = $query['start']; $i <= $query['end']; $i++ ) {
206214
// Skip over groups of tokens if they are part of an inline function call.
207-
if ( isset( $skip_from, $skip_to ) && $i >= $skip_from && $i < $skip_to ) {
215+
if ( isset( $skip_from, $skip_to ) && $i >= $skip_from && $i <= $skip_to ) {
208216
$i = $skip_to;
209217
continue;
210218
}
@@ -224,50 +232,80 @@ public function process_token( $stackPtr ) {
224232
$sprintf_parameters = PassedParameters::getParameters( $this->phpcsFile, $i );
225233

226234
if ( ! empty( $sprintf_parameters ) ) {
235+
/*
236+
* Check for named params. sprintf() does not support this due to its variadic nature,
237+
* and we cannot analyse the code correctly if it is used, so skip the whole sprintf()
238+
* in that case.
239+
*/
240+
$valid_sprintf = true;
241+
foreach ( $sprintf_parameters as $param ) {
242+
if ( isset( $param['name'] ) ) {
243+
$valid_sprintf = false;
244+
break;
245+
}
246+
}
247+
248+
if ( false === $valid_sprintf ) {
249+
$next = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $i + 1 ), null, true );
250+
if ( \T_OPEN_PARENTHESIS === $this->tokens[ $next ]['code']
251+
&& isset( $this->tokens[ $next ]['parenthesis_closer'] )
252+
) {
253+
$skip_from = ( $i + 1 );
254+
$skip_to = $this->tokens[ $next ]['parenthesis_closer'];
255+
}
256+
257+
continue;
258+
}
259+
260+
// We know for sure this sprintf() uses positional parameters, so this will be fine.
227261
$skip_from = ( $sprintf_parameters[1]['end'] + 1 );
228262
$last_param = end( $sprintf_parameters );
229263
$skip_to = ( $last_param['end'] + 1 );
230264

231265
$valid_in_clauses['implode_fill'] += $this->analyse_sprintf( $sprintf_parameters );
232266
$valid_in_clauses['adjustment_count'] += ( \count( $sprintf_parameters ) - 1 );
233267
}
234-
unset( $sprintf_parameters, $last_param );
268+
unset( $sprintf_parameters, $valid_sprintf, $last_param );
235269

236270
} elseif ( 'implode' === strtolower( $this->tokens[ $i ]['content'] ) ) {
237271
$prev = $this->phpcsFile->findPrevious(
238-
Tokens::$textStringTokens,
272+
Tokens::$emptyTokens + array( \T_STRING_CONCAT => \T_STRING_CONCAT ),
239273
( $i - 1 ),
240-
$query['start']
274+
$query['start'],
275+
true
241276
);
242277

243-
$prev_content = TextStrings::stripQuotes( $this->tokens[ $prev ]['content'] );
244-
$regex_quote = $this->get_regex_quote_snippet( $prev_content, $this->tokens[ $prev ]['content'] );
278+
if ( isset( Tokens::$textStringTokens[ $this->tokens[ $prev ]['code'] ] ) ) {
279+
$prev_content = TextStrings::stripQuotes( $this->tokens[ $prev ]['content'] );
280+
$regex_quote = $this->get_regex_quote_snippet( $prev_content, $this->tokens[ $prev ]['content'] );
245281

246-
// Only examine the implode if preceded by an ` IN (`.
247-
if ( preg_match( '`\s+IN\s*\(\s*(' . $regex_quote . ')?$`i', $prev_content, $match ) > 0 ) {
248-
249-
if ( isset( $match[1] ) && $regex_quote !== $this->regex_quote ) {
250-
$this->phpcsFile->addError(
251-
'Dynamic placeholder generation should not have surrounding quotes.',
252-
$i,
253-
'QuotedDynamicPlaceholderGeneration'
254-
);
255-
}
282+
// Only examine the implode if preceded by an ` IN (`.
283+
if ( preg_match( '`\s+IN\s*\(\s*(' . $regex_quote . ')?$`i', $prev_content, $match ) > 0 ) {
256284

257-
if ( $this->analyse_implode( $i ) === true ) {
258-
++$valid_in_clauses['uses_in'];
259-
++$valid_in_clauses['implode_fill'];
285+
if ( isset( $match[1] ) && $regex_quote !== $this->regex_quote ) {
286+
$this->phpcsFile->addError(
287+
'Dynamic placeholder generation should not have surrounding quotes.',
288+
$prev,
289+
'QuotedDynamicPlaceholderGeneration'
290+
);
291+
}
260292

261-
$next = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $i + 1 ), null, true );
262-
if ( \T_OPEN_PARENTHESIS === $this->tokens[ $next ]['code']
263-
&& isset( $this->tokens[ $next ]['parenthesis_closer'] )
264-
) {
265-
$skip_from = ( $i + 1 );
266-
$skip_to = ( $this->tokens[ $next ]['parenthesis_closer'] + 1 );
293+
if ( $this->analyse_implode( $i ) === true ) {
294+
++$valid_in_clauses['uses_in'];
295+
++$valid_in_clauses['implode_fill'];
296+
297+
$next = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $i + 1 ), null, true );
298+
if ( \T_OPEN_PARENTHESIS === $this->tokens[ $next ]['code']
299+
&& isset( $this->tokens[ $next ]['parenthesis_closer'] )
300+
) {
301+
$skip_from = ( $i + 1 );
302+
$skip_to = $this->tokens[ $next ]['parenthesis_closer'];
303+
}
267304
}
268305
}
306+
unset( $next, $prev_content, $regex_quote, $match );
269307
}
270-
unset( $prev, $next, $prev_content, $regex_quote, $match );
308+
unset( $prev );
271309
}
272310
}
273311

@@ -537,20 +575,22 @@ function ( $symbol ) {
537575
}
538576

539577
$replacements = $parameters;
540-
array_shift( $replacements ); // Remove the query.
578+
unset( $replacements['query'], $replacements[1] ); // Remove the query param, whether passed positionally or named.
541579

542-
// The parameters may have been passed as an array in parameter 2.
543-
if ( isset( $parameters[2] ) && 2 === $total_parameters ) {
580+
// The parameters may have been passed as an array in the variadic $args parameter.
581+
$args_param = PassedParameters::getParameterFromStack( $parameters, 2, 'args' );
582+
if ( false !== $args_param && 2 === $total_parameters ) {
544583
$next = $this->phpcsFile->findNext(
545584
Tokens::$emptyTokens,
546-
$parameters[2]['start'],
547-
( $parameters[2]['end'] + 1 ),
585+
$args_param['start'],
586+
( $args_param['end'] + 1 ),
548587
true
549588
);
550589

551590
if ( false !== $next
552591
&& ( \T_ARRAY === $this->tokens[ $next ]['code']
553-
|| \T_OPEN_SHORT_ARRAY === $this->tokens[ $next ]['code'] )
592+
|| ( isset( Collections::shortArrayListOpenTokensBC()[ $this->tokens[ $next ]['code'] ] )
593+
&& Arrays::isShortArray( $this->phpcsFile, $next ) === true ) )
554594
) {
555595
$replacements = PassedParameters::getParameters( $this->phpcsFile, $next );
556596
}
@@ -627,15 +667,11 @@ protected function get_regex_quote_snippet( $stripped_content, $original_content
627667
protected function analyse_sprintf( $sprintf_params ) {
628668
$found = 0;
629669

630-
unset( $sprintf_params[1] );
670+
unset( $sprintf_params[1] ); // Remove the positionally passed $format param.
631671

632672
foreach ( $sprintf_params as $sprintf_param ) {
633-
if ( strpos( strtolower( $sprintf_param['raw'] ), 'implode' ) === false ) {
634-
continue;
635-
}
636-
637673
$implode = $this->phpcsFile->findNext(
638-
Tokens::$emptyTokens,
674+
Tokens::$emptyTokens + array( \T_NS_SEPARATOR => \T_NS_SEPARATOR ),
639675
$sprintf_param['start'],
640676
$sprintf_param['end'],
641677
true
@@ -675,23 +711,26 @@ protected function analyse_sprintf( $sprintf_params ) {
675711
*/
676712
protected function analyse_implode( $implode_token ) {
677713
$implode_params = PassedParameters::getParameters( $this->phpcsFile, $implode_token );
678-
679714
if ( empty( $implode_params ) || \count( $implode_params ) !== 2 ) {
680715
return false;
681716
}
682717

683-
if ( preg_match( '`^(["\']), ?\1$`', $implode_params[1]['raw'] ) !== 1 ) {
718+
$implode_separator_param = PassedParameters::getParameterFromStack( $implode_params, 1, 'separator' );
719+
if ( false === $implode_separator_param
720+
|| preg_match( '`^(["\']), ?\1$`', $implode_separator_param['clean'] ) !== 1
721+
) {
684722
return false;
685723
}
686724

687-
if ( strpos( strtolower( $implode_params[2]['raw'] ), 'array_fill' ) === false ) {
725+
$implode_array_param = PassedParameters::getParameterFromStack( $implode_params, 2, 'array' );
726+
if ( false === $implode_array_param ) {
688727
return false;
689728
}
690729

691730
$array_fill = $this->phpcsFile->findNext(
692-
Tokens::$emptyTokens,
693-
$implode_params[2]['start'],
694-
$implode_params[2]['end'],
731+
Tokens::$emptyTokens + array( \T_NS_SEPARATOR => \T_NS_SEPARATOR ),
732+
$implode_array_param['start'],
733+
$implode_array_param['end'],
695734
true
696735
);
697736

@@ -701,23 +740,24 @@ protected function analyse_implode( $implode_token ) {
701740
return false;
702741
}
703742

704-
$array_fill_params = PassedParameters::getParameters( $this->phpcsFile, $array_fill );
705-
706-
if ( empty( $array_fill_params ) || \count( $array_fill_params ) !== 3 ) {
743+
$array_fill_value_param = PassedParameters::getParameter( $this->phpcsFile, $array_fill, 3, 'value' );
744+
if ( false === $array_fill_value_param ) {
707745
return false;
708746
}
709747

710-
if ( "'%i'" === $array_fill_params[3]['raw']
711-
|| '"%i"' === $array_fill_params[3]['raw']
748+
if ( "'%i'" === $array_fill_value_param['clean']
749+
|| '"%i"' === $array_fill_value_param['clean']
712750
) {
751+
$firstNonEmpty = $this->phpcsFile->findNext( Tokens::$emptyTokens, $array_fill_value_param['start'], $array_fill_value_param['end'], true );
752+
713753
$this->phpcsFile->addError(
714754
'The %i placeholder cannot be used within SQL `IN()` clauses.',
715-
$implode_token,
755+
$firstNonEmpty,
716756
'IdentifierWithinIN'
717757
);
718758
return false;
719759
}
720760

721-
return (bool) preg_match( '`^(["\'])%[dfFs]\1$`', $array_fill_params[3]['raw'] );
761+
return (bool) preg_match( '`^(["\'])%[dfFs]\1$`', $array_fill_value_param['clean'] );
722762
}
723763
}

0 commit comments

Comments
 (0)