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 hook_query_alter() and hook_query_TAG_alter() mechanism only works on Select queries, and only on the outer query. Subqueries are not altered.
This should either be fixed so they can be tagged/altered, or documented. I'm not sure which.
If the answer is documentation, it should be documented in the hook doc headers (for api.drupal.org) as well as on http://drupal.org/node/310077
If the answer is to alter the queries, this would be done presumably in SelectQuery::__toString() in the section right here:
if ($table['table'] instanceof SelectQueryInterface) {
$table_string = '(' . (string)$table['table'] . ')';
}
You would need to call the alter stuff there instead of just doing (string) cast.
Presumably, that would be by taking this out of SelectQuery::preExecute() and making a separate function:
if (isset($this->alterTags)) {
drupal_alter('query', $query);
foreach ($this->alterTags as $tag => $value) {
drupal_alter("query_$tag", $query);
}
}
| Comment | File | Size | Author |
|---|---|---|---|
| #13 | 701888-reroll.patch | 3.13 KB | jhodgdon |
| #11 | 701888-onetestclass.patch | 3 KB | jhodgdon |
| #5 | 701888.patch | 2.75 KB | jhodgdon |
Comments
Comment #1
moshe weitzman CreditAttribution: moshe weitzman commentedIf a subquery has a tag, it should get altered separately IMO. So, should be fixed IMO.
Comment #2
jhodgdonI tend to agree. Patch wouldn't be difficult, but we'd also need a test...
Comment #3
Crell CreditAttribution: Crell commentedAfter thinking about it, I would agree. Let's do. jhodgdon, can you take a stab at a patch?
Comment #4
jhodgdonOK.
Comment #5
jhodgdonHere's a patch, with a test and everything. I am running all the Database group tests and they seem to be fine so far. Let's see if the test bot is any faster.
Comment #6
BerdirThere is already a Query alter test class, any reason why not to extend that one?
Comment #7
jhodgdonUmmm...
I am not sure what good extending it would do, since all it has that we could use is a setup function? There is already
This adds
I don't see why this is bad or any different?
Comment #8
BerdirI don't even know why we have two different classes already, can't be the size of them as there are much bigger test classes in core than those :)
Comment #9
jhodgdonYes, all three of them could be combined into one class. But I'm afraid of getting a "killing kittens" message if I do that in this patch to combine the first two into one class. So can we do that later on a separate issue?
Comment #10
Crell CreditAttribution: Crell commentedI discussed test structure of that sort with Jimmy in SF a bit. We really should not have a separate test class for this patch; let's incorporate the test into an existing alter test class. Let's do that and get this patch in ASAP.
There may be other classes we can combine later, but we'll do that later.
Comment #11
jhodgdonOK. Here's a patch with all of the query alter classes combined into one.
Comment #13
jhodgdonThat's strange. I am not seeing a PHP error, and the database query alter test is running fine on my machine. I'm meanwhile running all the database tests, and I'll also reroll the patch just in case. ??
Comment #14
Crell CreditAttribution: Crell commentedBot looks happy. I look happy. Everyone's happy. :-)
Comment #15
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.