WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 17 months ago

#11102 closed enhancement (wontfix)

$wpdb->prepare should merge all arguments to pass to vsprintf

Reported by: westi Owned by: westi
Milestone: Priority: normal
Severity: normal Version: 2.9
Component: Database Keywords: has-patch close
Focuses: Cc:

Description

Currently you can't do something like this:

$wpdb->prepare("SELECT * FROM $table WHERE `x` = %s AND type IN (%d,%d,%d)", $where, $in_array);

You can do:

$wpdb->prepare("SELECT * FROM $table WHERE type IN (%d,%d,%d)", $in_array);

It would be really good if prepare took all the replacement arguments and built up a single array for you so you had the freedom to pass in arrays and strings for different replacement types.

Inspired by #11100

Attachments (2)

11102.diff (1021 bytes) - added by dd32 4 years ago.
11102.2.diff (1.1 KB) - added by dd32 4 years ago.
with bail on unmatched args/%'s

Download all attachments as: .zip

Change History (13)

comment:1 westi4 years ago

Inspiration for this was a discussion with Viper007Bond and DD32 in #wordpress-dev

dd324 years ago

comment:2 dd324 years ago

  • Keywords has-patch added; needs-patch removed

attachment 11102.diff added

Nothing really to say about the patch.. Pretty simple really.

As long as you pass the correct number of args then it works as expected.

If for some reason, an Array is passed in when a string was expected, There's the downside that things overflow into the next fields.. ie:

$a = 'A';
$thought_to_be_string = array('B1', 'B2');
$c = 'C';
var_dump( $wpdb->prepare('SELECT * FROM $TABLE WHERE a = %s AND b = %s AND c = %s', $a, $thought_to_be_string, $c) );
// string(60) "SELECT * FROM $TABLE WHERE a = 'A' AND b = 'B1' AND c = 'B2'"

One way around it, could be:

		if ( count($args) != substr_count($query, '%') )
			return false;

which prevents mistaken overflows.. vsprintf() returns false if too few args are passed in as it is.

But that solution doesn't take into account %% being present in the string.

If those gotcha's are thought to be OK for the added benefit, then so be it :)

dd324 years ago

with bail on unmatched args/%'s

comment:3 follow-up: azaozz4 years ago

Not sure if that really improves things. Passing an array seems to make sense only in "IN (5,10,20, ...)" where the length of the array is not fixed.

MySQL seems to accept both "IN (5,10,20, ...)" and "IN ('5','10','20', ...)" (it casts string to int when comparing with a numeric column) so we can join( "','", $array ) and pass it as a string. This is already used in about 50 places in core including #11100.

In this case a query would look like:

$wpdb->prepare("SELECT * FROM $table WHERE `x` = %s AND type IN (%s)", $where, $in_array);

that would expand to:

SELECT * FROM $table WHERE `x` = 'bar' AND type IN ('5','10','20');

or

SELECT * FROM $table WHERE `x` = 'bar' AND type IN ('bar1','bar2','bar3');

comment:4 in reply to: ↑ 3 westi4 years ago

Replying to azaozz:

Not sure if that really improves things. Passing an array seems to make sense only in "IN (5,10,20, ...)" where the length of the array is not fixed.

MySQL seems to accept both "IN (5,10,20, ...)" and "IN ('5','10','20', ...)" (it casts string to int when comparing with a numeric column) so we can join( "','", $array ) and pass it as a string. This is already used in about 50 places in core including #11100.

The point is to make it easy to use the correct data type specifier in the prepare statement. If we are preparing in what we believe/expect are integers we should use %d.

Replying to azaozz:

In this case a query would look like:

$wpdb->prepare("SELECT * FROM $table WHERE `x` = %s AND type IN (%s)", $where, $in_array);

that would expand to:

SELECT * FROM $table WHERE `x` = 'bar' AND type IN ('5','10','20');

or

SELECT * FROM $table WHERE `x` = 'bar' AND type IN ('bar1','bar2','bar3');

With the current code this would expand to

SELECT * FROM $table WHERE `x` = 'bar' AND type IN (''bar1','bar2','bar3'');

Assuming $in_array was a string containing 'bar1','bar2','bar3' as we force %s to be quoted.

comment:5 Denis-de-Bernardy4 years ago

what I'd find interesting, personally, is:

prepare("SELECT * FROM table WHERE col IN (%s)", $array);

anything else doesn't make much sense. depending on whether we have %d or %s, $array should then expand to:

implode(", ", array_map('intval', $array))

or:

"'" . implode("', '", $wpdb->escape($array)) . "'"

before it does, though, it should check if the array is empty, and fallback to NULL. Queries with an IN () clause are invalid SQL, and they're the kind of crippling bugs that are difficult to spot when scanning a query log. If the same query contains a big fat NULL in the SQL, it immediately becomes more obvious what the problem is.

comment:6 sirzooro4 years ago

  • Cc sirzooro added

comment:7 hakre4 years ago

Related: #11608

comment:8 nacin4 years ago

  • Milestone changed from 3.0 to Future Release

comment:9 pento21 months ago

Passing an array to fill an IN() clause is cute, but I'd be inclined not to do this. Neither mysqli nor PDO support this behaviour, which would just make things confusing.

comment:10 nacin21 months ago

  • Keywords close added; early removed

The reasoning in the ticket description is a bit tough to follow — I'm inclined to think that this is far too convoluted to be added to prepare().

comment:11 nacin17 months ago

  • Milestone Future Release deleted
  • Resolution set to wontfix
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.