A follow-on to: http://drupal.org/node/167284
the lack of a simple placeholder for multi-int lists in DB queries has led to insecure queries like "WHERE nid IN (%s)"
Attached script can be run from the command line to compare the speed of various methods of handling this (note edit line ~75 to try arg arrays of different sizes). What's interesting is that creating a set of %d placeholders is a loser from a speed standpoint, and probably also in terms of convenience (speed test results follow).
| Comment | File | Size | Author |
|---|---|---|---|
| #5 | test-placeholders2.txt | 3.14 KB | pwolanin |
| #4 | list_ph_int_str_1.patch | 1.59 KB | pwolanin |
| #2 | list_ph_1.patch | 1.24 KB | pwolanin |
| #1 | results.txt | 1.83 KB | pwolanin |
| test-placeholders.txt | 3.13 KB | pwolanin |
Comments
Comment #1
pwolanin commentedresults of speed comparisons attached. The bottom line - using
implode(',', array_fill(0, count($args), '%d'))is not very good compared to having a placeholder that can handle multi-int lists. Interestingly, we could usestr_tok()to give added flexibility (if desired) with similar or better performance than just usingexplode().Comment #2
pwolanin commentedsince '%dd' won't work (the 1st 2 chars are matched first), how about '%D' ?
Comment #3
pwolanin commentedThis technique could, potentially, be extended to strings, but it's harder to get right since we need to be sure each string end up wrapped in single quotes.
Comment #4
pwolanin commentedhere's a patch that does the same thing for Strings - Moshe says this would be useful for Views, OG, etc.
Note usage is like:
note the wrapping single quotes are required as with %s - I think this makes sense for consistency.
(note: it would be better in terms of security to make %s and this add the outer quotes automatically, but that would require changing a lot of core code - should it be proposed?).
Comment #5
pwolanin commentedHeres a test script - again there results are similar to above. For a single argument, it's better to use '%s', but for ~3 or more, the str_tok() or explode() is faster.
For example, in a typical run with PHP 4.4.7 I got:
Comment #6
dries commentedThe db_query() syntax is likely to completely change in D7.
Comment #7
pwolanin commented@Dries - yes, I expect so. But does that mean this patch is going to cause problems then? Or that it would just be a short-lived changed to the API, or that we'd be better off just implementing a placeholder helper function?
Comment #8
moshe weitzman commentedsubscribe. lets do s simple function, no?
Comment #9
dave reidSince this is not likely to land in D6 anymore since it has been out so long, I am marking this won't fix.