Someone wanted the handy return value of update_sql() but didn't want to manually escape their parameters. I've swapped it to db_query() to get it working with values containing a '.

CommentFileSizeAuthor
#2 content.install_escape_sql_0.patch1.3 KBdrewish
content.install_escape_sql.patch1.28 KBdrewish
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

drewish’s picture

drewish CreditAttribution: drewish commented

Running this I'm getting the following warning:user warning: Field 'min_word_count' doesn't have a default value query: INSERT INTO node_type

We should probably add a field to fix that too.

drewish’s picture

drewish CreditAttribution: drewish commented
FileSize
content.install_escape_sql_0.patch1.3 KB

here's one that fixes that.

yched’s picture

yched CreditAttribution: yched commented

We should probably add a text message in $ret[] saying something like "the content type %content_type has been moved to node_type table" (or just one message for all content types). With this patch it seems like the update just drops node_type_content, which might be quite scary...

BTW, isn't there some rule about 5.0 (or "4.7 to 5.0") updates having to be numbered from 1000 on, or is it just for core, or is just me mixing things up ? I have one or two commits I'm holding off until I can figure this out, to no avail so far...

RobRoy’s picture

RobRoy CreditAttribution: RobRoy commented

I think the 1000 numbers are just for core. Module updates just go sequentially.

RobRoy’s picture

RobRoy CreditAttribution: RobRoy commented

But...I could be wrong. :)

drewish’s picture

drewish CreditAttribution: drewish commented

yeah, returning a message is probably a good idea.

i didn't change the DROP. that was already there, but i agree it is scarry.

i don't think that there are any specifications for update numbering. regarding CCKs update numbering you should take a look at: http://drupal.org/node/100744

KarenS’s picture

KarenS CreditAttribution: KarenS commented

The downside of using db_query is that you don't get the db_query results displayed in the update results page, only update_sql queries show up there. So which is more beneficial, avoiding the escaping or displaying the query? I kind of like to see the query that has been run myself, but I don't feel that strongly either way, so I'll go along with whatever everyone agrees is best.

And obviously we need to update the min_word_count. I think that field was a late addition, at least I don't remember seeing when I was working with the update script originally.

RobRoy’s picture

RobRoy CreditAttribution: RobRoy commented

Hmmm...we should patch update_sql() to accept params in an optional array() so we can pass that array to db_query(). Here's an existing issue that will solve this for 6.x http://drupal.org/node/36324.

For this, I'd say run it through db_query and print out a custom message with info about the query.

KarenS’s picture

KarenS CreditAttribution: KarenS commented

I'm going to add this change to http://drupal.org/node/100744 and commit the result, so I'm marking this fixed.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented

Hm. I'd advocate continuing to use update_sql, since that is the standard for update hooks, and instead passing the arguments through db_escape_string first before they're passed to the query, since that's what db_query would normally do.

The core patch to make update_sql behave as db_query would be a good idea too.

KarenS’s picture

KarenS CreditAttribution: KarenS commented
Status: Needs review » Fixed

Yes, I'm going back to update_sql using db_escape_string, but I'm doing it in the other issue. I tried the idea of db_query plus a message, but that's actually messier than just using db_escape_string.

Anonymous’s picture

(not verified) CreditAttribution: commented
Status: Fixed » Closed (fixed)