Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.As a result of #1868972: db_query_temporary() fails to create a table when the SQL has leading whitespace, db_query_temporary() cannot be used on a non-SELECT query at all.
Before it would work (basically it would run the query it was given, although not using any kind of temporary tables at all), but after that issue there will be an error. This change was made in Drupal 8, and (perhaps slightly against my better judgment) is now in Drupal 7 as well.
Drupal 6 already documents that the function should only be used for SELECT queries, but Drupal 7 and Drupal 8 don't. We could add similar documentation to this function as db_query() already has, although the difference is with db_query() it's basically just a recommendation (for cross-database-driver compatibility) but now for db_query_temporary() it's a requirement.
| Comment | File | Size | Author |
|---|---|---|---|
| #9 | drupal-7.x-doc-db_query_temporary-2259361-9.patch | 1.1 KB | jweowu |
| #5 | drupal-8.x-doc-db_query_temporary-2259361-5.patch | 1.08 KB | jweowu |
Comments
Comment #1
David_Rothstein CreditAttribution: David_Rothstein commentedComment #2
jweowu CreditAttribution: jweowu commentedIt's true that you could pass non-SELECT queries before and have them executed, but there was no purpose to doing so. Anyone choosing to call
db_query_temporary()rather thandb_query()is doing so in order to get results into a temporary table, so any use of this function which fails to carry out that critical aspect is surely a bug.Unless we're considering code that just passes arbitrary queries to
db_query_temporary()(and then checks to see if the table exists?), but I'd be worried if anyone's writing code like that.Note that for some databases, the new code will actually be a little more flexible. With Postgres, for example, you could now pass it "A SELECT, TABLE, or VALUES command, or an EXECUTE command that runs a prepared SELECT, TABLE, or VALUES query".
MySQL will still be restricted to SELECT, so that fact is of no use to core or contrib code, but it may warrant a mention if the docstring is being edited?
Comment #3
David_Rothstein CreditAttribution: David_Rothstein commentedRight, so I tried this with
db_query_temporary("TRUNCATE {cache}")(on MySQL) and before the patch it would execute the query just like db_query() itself does (though of course not actually do anything with temporary tables). After the patch it results in a PDOException.I agree that anyone doing that is doing something wrong already (because it doesn't make much sense to use that function), but still seems like it would be useful to document the correct usage.
Comment #4
jweowu CreditAttribution: jweowu commentedYes, I agree 100% with documenting the correct usage.
Comment #5
jweowu CreditAttribution: jweowu commentedAs far as the function docstring goes, is this all we need to change?
I concluded that commenting on the abilities of the various different databases is probably an unnecessary complication (especially as getting temporary data from other queries with this function was never an option before).
People can still use
db_query()to run a hand-written CREATE TEMPORARY TABLE query if they need to do something database-specific; so although this function could accept non-SELECT query strings in some situations, it feels okay to document SELECT as a requirement.Comment #6
jhodgdonThis looks like the correct fix to me. Thanks!
Comment #7
webchickGreat, thanks!
Committed and pushed to 8.x. Moving down to 7.x.
Comment #9
jweowu CreditAttribution: jweowu commented7.x version. Documentation is identical to 8.x.
Comment #10
jhodgdonThanks! Looks like the right port for D7.
Comment #12
jhodgdonThanks again! Committed to 7.x.