Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Each QueryPlaceholderInterface implementation are re-computing a unique id on clone events. SelectQuery is not.
Current __clone code is:
public function __clone() {
// On cloning, also clone the dependent objects. However, we do not
// want to clone the database connection object as that would duplicate the
// connection itself.
$this->where = clone($this->where);
$this->having = clone($this->having);
foreach ($this->union as $key => $aggregate) {
$this->union[$key]['query'] = clone($aggregate['query']);
}
}
It lacks the parent::__clone();
or $this->uniqueIdentifier = uniqid('', TRUE);
.
Comment | File | Size | Author |
---|---|---|---|
#4 | 1808624-drupal_core-selectquery_clone_missinguniqueid_recomputing-4.patch | 543 bytes | Aron Novak |
#1 | drupal_core-selectquery_clone_missinguniqueid_recomputing-1808624-2.patch | 508 bytes | regilero |
Comments
Comment #1
regilero CreditAttribution: regilero commentedHere is the simple patch (very very simple).
Comment #2
Aron NovakI faced with an issue in Views. That module tries to compute a cache ID from the content of the query, it clones the query but the cloning is far from perfect, somehow the args were modified by __toString, even called on clone, resulting in broken views. After applying this patch, the issue disappeared. The patch seems to be RTBC for me then, it solves a real problem.
Comment #3
David_Rothstein CreditAttribution: David_Rothstein commentedThis looks like it needs to go in Drupal 8 first.
Comment #4
Aron NovakRerolled to 8.x
Comment #5
regilero CreditAttribution: regilero commentedCommon guys. This is a real core bug in D7, and the patch does fix it. And then One year later we are waiting for it to be applied on D8 which is a completly different code base?
Comment #6
dawehnerWell, we have a backport policy, see https://www.drupal.org/node/767608
Changing this behaviour is not part for this issue, though you can have "fun" starting that discussion again.
Can we at least have tests to proove that the fix actually really works?
Comment #7
pounardBackport policy is good for features, but really not for bugfixes that cause real life problems on production sites, the policy should be the other way arround for bugs.
I'll try to write the test when I'll have some spare time.
Comment #8
Damien Tournoud CreditAttribution: Damien Tournoud commentedThere are some other obvious things missing in there:
$this->prepared
flag should be nuked;$this->tables
that are implementations ofSelectInterface
need to be cloned tooBut more importantly, this cannot really work because alter hooks are allowed to modify the query object ad-lib. That makes preparation non reentrant, so we should either:
->execute()
(probably with a non-negligible performance impact);This whole concept of "compiled" vs "prepared" for Select queries needs to be rethought.
Comment #9
Damien Tournoud CreditAttribution: Damien Tournoud commentedAnyway, needs work for tests.
Comment #10
pounardYour comments make sense, and we can easily forbid objects to be cloned once prepared, which eradicates the need of nuking
$this->prepared
. We don't really need to rethink the prepared concept as long as we forbid cloning when prepared.Nice catch for the tables objects, and it's also true for many other objects such as conditions (and maybe others) which can be subqueries.
Comment #11
Damien Tournoud CreditAttribution: Damien Tournoud commentedThose should already be cloned by the condition object they belong to.
Comment #22
quietone CreditAttribution: quietone at PreviousNext commentedA little digging and found that this was fixed in 8.x in #2142107: Complex cloned query dependent on __toString() call
The D7 issue is #3000191: Complex cloned query dependent on __toString() call
Therefore, closing as a duplicate and moving credit there. If that is wrong, reopen the issue, by setting the status to 'Active', and add a comment.
Thanks!