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.
The pattern that is used to split the query for injecting added where, join clause returns 'null' if the main query does not include a where clause.
This results in generating a wrong SQL query. the code from version 5 works properly but this has been changed in version 6.
(line 318 database.inc)
Comment | File | Size | Author |
---|---|---|---|
#5 | nowhere.patch | 1.95 KB | chx |
Comments
Comment #1
cburschkaWould the following have the desired effect?
Comment #2
madli@radio.astral.com CreditAttribution: madli@radio.astral.com commentedNo, it doesn't help!
for testing:
- the main query:
SELECT n.*, u.name FROM core_node n INNER JOIN core_users u ON n.uid = u.uid ORDER BY n.changed DESC LIMIT 0, 50
- $join and $where are retun values of a module implemented hook_db_rewrite_sql
the original code in drupal 6.0 creates the following wrong query:
$join WHERE $where AND ( SELECT n.*, u.name FROM core_node n INNER JOIN core_users u ON n.uid = u.uid )ORDER BY n.changed DESC LIMIT 0, 50
and your suggestion
SELECT n.*, u.name FROM core_node n INNER JOIN core_users u ON n.uid = u.uid ORDER BY n.changed $join WHERE $where AND ( ) LIMIT 0, 50
Comment #3
madli@radio.astral.com CreditAttribution: madli@radio.astral.com commentedI made the following changes to fix the issue and it works (the only problem is that it wouldn't work properly if the main query includes subqueries )
Comment #4
chx CreditAttribution: chx commentedAdd
WHERE 1 = 1
. This code is fragile enough that I simply urge core developers not to touch it. Let's do something better in D7.Comment #5
chx CreditAttribution: chx commentedGoba does not like me.
Comment #6
cburschkaHeads up on a conflict with #170656: db_rewrite_sql() breaks with GROUP_CONCAT().
Comment #7
marcingy CreditAttribution: marcingy commentedThat seems to work and also works when no grants are set on a node.
This patch should be set as RTBC
Comment #8
Gábor HojtsyYes, I also run this through some manual calls on queries with and without WHERE, with and without subqueries and all run fine, so it looks fine. Committed to 6.x. RTBC for 7.x.
Comment #9
chx CreditAttribution: chx commentedJust let db_rewrite_sql die. It ran its course, let the new database layer take over.
Comment #10
cburschkaThis was already fixed in D6, however. Let's just leave it there.
(Okay, I just like it when bugs are set to Fixed instead of Won't Fix. =) )
Comment #11
Gábor HojtsyGuys, let's allow 7.x to work as 6.x until this new database layer is introduced. Do not introduce regressions knowingly IMHO.
Comment #12
vladimir.dolgopolov CreditAttribution: vladimir.dolgopolov commentedHere is the test for the issue for Simpletest module.
Patch #5 pass the test.
Comment #13
Gábor HojtsyTest looks good, thanks for doing it! (Dries might hold off waiting for PDO, so we will see whether this patch will need to be committed.). Keep this RTBC unless it becomes irrelevant.
Comment #14
Dries CreditAttribution: Dries commentedI think we might want to revisit this after the PDO / database abstraction patch landed. In fact, I'd encourage you to participate in that issue: see http://drupal.org/node/225450.
Comment #15
catchNo longer applies, almost a dup/fixed by #225450: Database Layer: The Next Generation but let's kill it once that's in.
Comment #16
Robin Monks CreditAttribution: Robin Monks commentedcatch: it's in :)
Subscribing
Robin
Comment #17
Crell CreditAttribution: Crell commentedI'm assigning this back to 6.x, as db_rewrite_sql() will be removed from D7 shortly so there's no point in trying to make it work there. Whether or not this is worth looking into further for D6 I leave up to Gabor.
Comment #18
catchIt's already done in 6, so marking fixed.
Comment #19
Damien Tournoud CreditAttribution: Damien Tournoud commentedGreat. I enjoy already that we have a plan to kill db_rewrite_sql() :)
Comment #20
catchFor those following along at home, that plan is here: #299176: Replace db_rewrite_sql() with hook_query_alter().
Comment #21
Anonymous (not verified) CreditAttribution: Anonymous commentedAutomatically closed -- issue fixed for two weeks with no activity.