I updated to latest cvs, and ran update.php.
for 2005-10-23, there are a series of updates that failed:

INSERT INTO {menu} (mid, pid, path, title, description, weight, type)
 VALUES (150, 147, 'blog', 'Blogs', 'Community member's blogs', 0, 118)
FAILED

Which in my case is due to the fact that the description field includes a single quote ' (Community member's blogs).

I have updated my table manually, but this quote should have been escaped by the update/db_query() api.

Comments

Richard Archer’s picture

Status: Active » Fixed

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

beginner’s picture

Thanks Richard.

Could you tell me if this is the intended behaviour?:
http://drupal.org/node/36335

thank you.

beginner’s picture

Title: update.php for 2005-10-23 query broken. » update_sql to support printf-style arguments and escape strings.
Priority: Critical » Normal
Status: Fixed » Active

http://drupal.org/node/22215#comment-52874 :

Why is it that update_sql doesn't support printf-style arguments?
Someone should post a feature request about that :)

there you are!

killes@www.drop.org’s picture

Category: bug » feature

moving to feature requests...

RobRoy’s picture

Version: x.y.z » 6.x-dev

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

drewish’s picture

subscribing

webchick’s picture

Version: 6.x-dev » 5.x-dev
Category: feature » bug
FileSize
522 bytes

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)

webchick’s picture

Status: Active » Needs review
webchick’s picture

FileSize
516 bytes

Sorry; I think I meant this.

webchick’s picture

FileSize
628 bytes

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?

RobRoy’s picture

FileSize
1.6 KB

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

I added some docs and stuff too.

Steven’s picture

Status: Needs review » Needs work

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.

webchick’s picture

Status: Needs work » Needs review

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. :)

webchick’s picture

Status: Needs review » Needs work

Oops.

RobRoy’s picture

Status: Needs work » Needs review
FileSize
1.95 KB

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.

webchick’s picture

FileSize
1.25 KB

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

webchick’s picture

FileSize
1.27 KB

Ahem. :P

webchick’s picture

FileSize
1.27 KB

Ahem!!

RobRoy’s picture

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.

webchick’s picture

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.

drewish’s picture

#15 looks good

Steven’s picture

Status: Needs review » Needs work

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.

RobRoy’s picture

Status: Needs work » Needs review
FileSize
1.79 KB

Good call. This better?

Steven’s picture

Status: Needs review » Needs work

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.

RobRoy’s picture

Version: 5.x-dev » 6.x-dev
Component: base system » update system

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

moshe weitzman’s picture

Yes, truncation makes sense here ... subscribe.

moshe weitzman’s picture

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

sun’s picture

Rerolled RobRoy's patch -- untested.

colan’s picture

Subscribing.

cburschka’s picture

Status: Needs work » Needs review
FileSize
2.39 KB

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.

Hugo Wetterberg’s picture

Priority: Normal » Critical
FileSize
2.38 KB

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.

Dave Reid’s picture

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?

Hugo Wetterberg’s picture

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.

dalin’s picture

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

dalin’s picture

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.

cburschka’s picture

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.

Jody Lynn’s picture

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

Dave Reid’s picture

Status: Needs review » Closed (won't fix)

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