Make WordPress Core

Opened 11 years ago

Closed 12 months ago

#25559 closed enhancement (duplicate)

Enhance prepare method to better support SQL IN operator

Reported by: cannona's profile cannona Owned by:
Milestone: Priority: normal
Severity: normal Version: 3.8
Component: Database Keywords: dev-feedback needs-refresh
Focuses: Cc:

Description

When querying the database for rows that have fields that match any of several values, you typically use the IN operator. For example:

$wpdb->prepare("select a from b where c in (%d, %d, %d, %d)", $values);

I propose that the sprintf-like formatting allowed by the prepare method be extended to additionally accept markers such as "%#3s", which would be expanded to "%s, %s, %s". So, the previous example could be rewritten as:

$wpdb->prepare("select a from b where c in (%#4d)", $values);

To be more clear, the syntax I'm proposing is a "%", followed by a "#", followed by a 1 or more digit number specifying how many times to repeat the marker, followed by the type of the marker (d, s, or f). Other markers would continue to work as usual.

The other part of this proposal is that the requirements for the 2nd and all following parameters would be relaxed, so that the user could pass single values, arrays of values, or some combination of the above, and the arrays would be flattened into a single array of values. For example, the following calls to prepare would be equivalent:

$wpdb->prepare(
  $query,
  1,
  2,
  3,
  4,
  5
);

$wpdb->prepare(
  $query,
  array(
    1,
    2,
    3,
    4,
    5,
  )
);

$wpdb->prepare(
  $query,
  1,
  array(
    2,
    3,
  ),
  4,
  array(5,)
);

// or even
$wpdb->prepare(
  $query,
  array(
    1,
    array(
      2,
    ),
    3,
  ),
  4,
  5
);

The reason I am making this proposal is that, while I do try to avoid raw SQL whenever practical, I often find myself forced to resort to it, and when I do, it is not uncommon for me to have to use the IN operator. So far, I've had to build my format string via a loop, which usually adds a placeholder for each item in an array. It's not a very complex bit of code, but because, at least in my experience, this situation is not super uncommon, it would be nice if the prepare method could make this a bit easier.

I have created a draft version of a patch which will add the above functionality.

Comments and questions are appreciated.

Attachments (1)

wp-db_mods.patch (4.9 KB) - added by cannona 11 years ago.
A first stab at a patch.

Download all attachments as: .zip

Change History (10)

@cannona
11 years ago

A first stab at a patch.

#1 @cannona
11 years ago

  • Summary changed from Enhance prepare method to better support SQL IN syntax to Enhance prepare method to better support SQL IN operator

#2 @cannona
11 years ago

  • Keywords has-patch added

#3 @johnbillion
11 years ago

  • Keywords 2nd-opinion dev-feedback added
  • Type changed from feature request to enhancement

I like the idea of a new conversion specification that converts an array to a comma-separated list that can be used in the IN and NOT IN operators.

That said, the approach taken in wp-db_mods.patch over-complicates matters and doesn't solve the underlying problem. Here's some feedback on that patch:

  • The %#ns format (where n is the number of times to repeat the value) falls down as soon as the number of values is variable and unknown, which defeats the whole point. If you're hardcoding the number of values into the conversion specification then how can it handle a variable number of values?
  • Accepting and flattening a variety of array and non-array arguments in wpdb::prepare() is an unnecessary complication, and is asking for trouble. What's the use case for accepting a mixture of multiple arguments, arrays, and multidimensional arrays, and flattening them all into one operator? If you're passing values like this then you're doing something wrong. What happens when you have arguments that follow your arguments for your IN/NOT IN clause? It becomes unclear which arguments are which. This also doesn't allow for a variable number of values (see first point). The argument should accept one single-dimensional array only.

One other thing to bear in mind is that wpdb::prepare() will also accept its arguments as a single array instead of separate arguments (think vsprintf() vs sprintf()). wp-db_mods.patch handles this by flattening all the arguments, but doesn't allow for a variable number of values in an argument.

My suggestion would be to simplify the conversion specification to %#s, where # is the element which specifies this is an argument of multiple values and varying length, which is to be converted to a comma-separated list. Of course, s can be s, d, or f as currently.

One other thought: We're introducing a new conversion specification that deviates from those in PHP's string formatting functions. Is this a good idea?

#4 @cannona
11 years ago

Thanks johnbillion for the well-thought-out response.

Here is some of the reasoning behind the decisions I made in my patch:

  • Like the printf family of functions, you can pass additional arguments to prepare, and any extra arguments not accounted for in the format string will be ignored. So, this is why I required the number be specified in the placeholder; so the parser knows when to stop.
  • I'll grant that there is not a compelling use case for allowing arrays inside arrays, but it is more of a side-effect, and also gives the developer the flexibility to pass parameters in whatever way is most convenient.
  • You are correct that the patch doesn't allow for multiple variables in a single parameter, and everything is flattened, but again this is not a problem, because we know how many variables to process because of the counts.

That being said, I think your suggestion could work just as well, if not better. Here's how I understand that might work:

  • If the first placeholder in the format string is %#d %#f or %#s, and if the second parameter is an array, whose first element was an array, we would assume that the parameters are passed inside an array.
  • If the first placeholder is not %#d, %#f or %#s, and the second parameter was an array, we would again assume that the parameters are passed inside an array.
  • Otherwise, we assume that parameters were passed as parameters.
  • Either way, anywhere the placeholder starts with %#, we would expect to see an array at that position in the parameters list, whether that list of parameters was passed as an array, or as regular function parameters.
  • Empty arrays would cause their placeholders to simply be replaced with nothing, as I believe it is valid to write something like "SELECT a FROM b WHERE i IN ()".
  • In the docs, we'll want to discourage users from manually adding additional placeholders, like "WHERE c IN (%#s, %s)" unless they are certain that the array behind "%#s" will never be empty.

Thoughts? Am I missing something?

#5 @johnbillion
11 years ago

I'm coming around to the idea of flattening all the arguments, because it means arrays and argument lists would both be supported. I can see that being a nice-to-have, but I'm still not sure if it has a real-world use.

With wp-db_mods.patch in place, you'd need to count the number of items in your array and then insert the count into the conversion specification like so:

$count = count( $array );
$sql = $wpdb->prepare( "
	SELECT *
	FROM table
	WHERE field IN ( %#{$count}s )
", $array );

It works, but it muddies the format quite a bit. You're inserting a variable into a conversion specification in order to convert a second variable into the format you need. Better:

$sql = $wpdb->prepare( "
	SELECT *
	FROM table
	WHERE field IN ( %#s )
", $array );

I'm going to give this some more thought.

#6 @cannona
11 years ago

I agree that having the number in the placeholder is awkward. However, the only alternative that occurs to me is to place the length in the parameters list, before the items to insert. For example:

prepare(
  "SELECT a FROM b WHERE c in (%#s)",
  count($d),
  $d
);

It seems a little cleaner, but still feels a bit off to me.

#7 @jdgrimes
11 years ago

  • Cc jdg@… added

#8 @chriscct7
9 years ago

  • Keywords needs-refresh added; has-patch 2nd-opinion removed

#9 @iandunn
12 months ago

  • Resolution set to duplicate
  • Status changed from new to closed

This looks like a "duplicate" of #54052. This one is older, but the other one is active(ish) and has more traction, so I think it's better to close this one.

Note: See TracTickets for help on using tickets.