db_placeholders uses wasteful implode/array_fill functions

This patch uses a regular if-statement and has an added line of documentation (hopefully it is in line with what you had in mind). Also, I reversed the order of the str_repeat so that it is clear that the first one is assumed and any additional ones are appended. This mindset seems to be more straightforward than "repeat all but the last one, then append the last one".

Please let me know if the documentation I added is inadequate, the formatting is incorrect, or you notice any other issue. Thank you!

Alright. Here is the same patch, instead against the Drupal 6 tag.

[edit] How do I get in touch with Gabor?

@Crell: Thanks to #474072: Use taxonomy_load functions in blogapi.module, db_placeholders() is expunged from core. Should I open an issue with a patch which simply removes it from core?

Technically, it is a bug that a situation exists where an empty array is passed in which causes array_fill to throw a warning.

The solution depends on viewpoint. The calling function could verify that it has some work to perform (i.e. verify that the array is not empty) before passing in the values. Or the calling function should expect the parameters to be validated by the function being called (which has the benefit of collapsing the logic in to the function itself, but does not prevent the *useless* query from being executed which is likely worse for performance).

Of course, the patch on this issue returns empty string for the 0 case, which - although it avoids the array_fill and implode warnings - has the disadvantage of still resulting in an SQL syntax error. This might not be a problem. Rather, it could be viewed as "here is a place which doesn't adequately determine whether it should even execute a query before it does so". I believe this is the best viewpoint, because avoiding wasteful queries is a big +1. If, however, the goal is to silence such syntax errors, the zero case for this function should return the string containing the empty string as follows:

  return "''";

I believe this is better for the following reasons:

• Average code execution in my testing is up to twice as fast
• Warnings are not wastefully triggered by the zero case (4300x as fast)
• The code is easier to read

I have attached a modified version which saves time in the zero case and is much more competitive (similar to HEAD) in the one case.

Rerolled.

#14: db_placeholders_performance-485618-14.patch queued for re-testing.

Rerolling again. I don't understand why this keeps failing.

I can't get the test bot to be happy with the patch, Drupal 7 is out and this probably doesn't matter in the larger scheme of things.

It might still be good to update http://drupal.org/writing-secure-code

Git reroll.