update_sql to support printf-style arguments and escape strings.

I have posted a patch for this bug to http://drupal.org/node/22215

moving to feature requests...

Moving to 6.x and referencing a related CCK issue http://drupal.org/node/100742.

subscribing

Actually... correct me if I'm wrong, but can't we do this without an API change with just the following patch? If so, we could fix this in 5.x.

I'd actually consider this a bug, since it leaves sanitizing up to the module developer before parameters are passed into update_sql, and is thus inconsistent and potentially insecure. (See CCK issue for reference)

Sorry; I think I meant this.

Hm. No. It'll be a bit more than that, because $sql is getting printed, and as-is would end up like "UPDATE soandso SET blah = %d" and stuff.

Maybe this?

Hmmm...I think you meant this. LOL. :D

I added some docs and stuff too.

If we're going to do this, we need to support the full db_query syntax, including passing in all arguments as a single array.

I don't see why update_sql() can't just be a pure wrapper around db_query(), with a simple print added to it. I don't see a problem with printing %s and %d instead of the actual values. In fact, this could make the update output more readable...

Right now, we often use db_query() instead of update_sql() when data is involved. This has the benefit of now showing e.g. 100 result queries when updating a table of 100. However, showing nothing is not good either. If we keep queries in the original form, we could group queries that are the same except for their data.

Ha, yes I certainly did. ;) Thanks for the PHPDoc!! I wasn't sure whether to handle that in a separate patch but makes sense here. :)

Oops.

Yeah, I was thinking that but being lazy. Not sure why as it's not really that much more work.

How's this? Okay, I'm off to work.

Hm. Well then correct me if I'm wrong, but isn't what Steven's talking about just this? (note: PHPDoc has changed)

Ahem. :P

Ahem!!

Right, I saw Steven's comment about '%s' and should have put my response in there. I think we *should* show the actual data being used in the update page. So I still side with my patch, no offense. I think admin's would like to see the exact queries that are being executed. If everyone disagrees then webchick's patch looks good.

I also like the patch in #15 better. It's handy to get a query back that you can copy/paste to see what went wrong. UPDATE {table} SET string = '%s' WHERE num = %d doesn't cut it. ;P

However, what I don't like about #15 is we've now copy/pasted all the code from db_query. So I wonder if there's a way satisfy both requirements.

#15 looks good

Patch is bad. We need to preserve the method of passing the arguments.

Right now, if you call update_sql('query', 1, 2, 3), we end up calling db_query('query', array(1, 2, 3)). This is not a problem, as it still works.

However, if you call update_sql('query', array(1, 2, 3)), we end up calling db_query('query', array(array(1, 2, 3))) which does not work.

We should use call_user_func_array() with func_get_args() to preserve the method of passing variables around.

Good call. This better?

I still think that printing out all the data is a bad idea. What if you start updating node bodies and such? You don't want all that crap out into the update log. At the very least, we should truncate string arguments to 64-128 characters or so.

Marked http://drupal.org/node/119760 a dupe of this.

Yes, truncation makes sense here ... subscribe.

Ayyone up for refreshing this? Just got bitten by this in devel.install()

Rerolled RobRoy's patch -- untested.

Subscribing.

Steven has a point. Passing large data into update_sql doesn't happen often (only if the data has to be read form the database, changed in PHP and re-inserted, rather than changing it inside MySQL), but when it does, cluttering the page isn't nice.

This patch, building on #28, limits the length of displayed values to 32 characters plus "..." if it gets truncated, which should be enough for debug output. Not yet tested.

Note: Technically, this change also fixes a bug besides the clutter - the existing patch passes $args to _db_query_callback unchecked, which assumes that $args is no longer in "all arguments in one array" syntax. That will break output of queries with such syntax, though the queries are executed successfully.

I ran into the same problem as everybody else who tried to pass non trivial arguments to the update_sql function. But my issue was with the update_sql passing everything (including the values as they are embedded in the query up front) through db_prefix_tables. As the db_prefix_tables doesn't allow any escaping of {}:s to avoid prefixing this made it impossible to insert serialized php, json or any other data containing {}:s. This became a problem when I was migrating some cck-fields to another field type: http://gist.github.com/52780

This was my first stab at the problem: http://gist.github.com/52779

After reading your discussion here I think that I've made a patch that should satisfy everyone. My main issue with #30 was that it returns the sql with prefixed table names. The original update_sql preserves the {}:s, mainly because it doesn't really do anything, but this behaviour is shared by the other update-functions, so I think we should do the same post-patch.

This would have been great to have sooner in 6.x, but is this a little too late to get into Drupal 6 considering it is changing API?

No, it doesn't change the api. It just adds support for sending parameters with the sql queries. All old-style calls will still work just fine.

I'm liking the approach in #31.

My only change was to change the dot-dot-dot (...) into a real ellipsis (…) to satisfy those of us concerned about typography. I also padded it with spaces since "normally an ellipsis should be spaced fore-and-aft to separate it from the text"
http://en.wikipedia.org/wiki/Ellipsis

Strange, half of my comment was cut off.

My tendency is that this is a D6 issue since there are possible security implications: For those less experienced developers in our community, on seeing that they can't simply add additional arguments like they would with db_query, might just throw their arguments in-line with the query. Lets make it easy for them to do the safe thing.

I've known ellispsis characters to cause encoding problems, and we should definitely avoid them in code or comments. We don't use typographical quote characters or apostrophes either. However, I see you didn't introduce an ellipsis character after all (@param has three dots still), so the patch is okay.

By the way, your comment text is locked as soon as you upload a file attachment. Any further changes have to be made by editing after submission. This is a bug introduced on d.o with the D6 upgrade. Until they fix it, just make sure to finish writing your post before attaching the file.

I just got hurt by this one too. Did not realize it doesn't take parameters...
If this goes in, #394268: DIE update_sql() DIE!, the issue will be moot

API change is not possible for D6 and we have killed update_sql in D7.