I couldn't find an issue reporting this bug, so here it is :
We notice that we are unable to cache a view in Commerce Kickstart without loosing some informations. (see #1691142: Make it possible to cache collection views).
This view is in charge of displaying nodes attached to the term passed as parameter and group them according to terms from another vocabulary. When cache is enabled we loose grouping informations.
Query without cache
SELECT DISTINCT taxonomy_term_data_node.tid AS taxonomy_term_data_node_tid, taxonomy_term_data_node.name AS taxonomy_term_data_node_name, taxonomy_term_data_node.vid AS taxonomy_term_data_node_vid, taxonomy_term_data_node__taxonomy_vocabulary.machine_name AS taxonomy_term_data_node__taxonomy_vocabulary_machine_name, node.nid AS nid
FROM
{node} node
LEFT JOIN (SELECT td.*, tn.nid AS nid
FROM
{taxonomy_term_data} td
LEFT JOIN {taxonomy_vocabulary} tv ON td.vid = tv.vid
LEFT JOIN {taxonomy_index} tn ON tn.tid = td.tid
WHERE (tv.machine_name IN (:db_placeholder_0)) ) taxonomy_term_data_node ON node.nid = taxonomy_term_data_node.nid
LEFT JOIN {taxonomy_vocabulary} taxonomy_term_data_node__taxonomy_vocabulary ON taxonomy_term_data_node.vid = taxonomy_term_data_node__taxonomy_vocabulary.vid
WHERE (( (node.status = 1 OR (node.uid = 1 AND 1 <> 0 AND 1 = 1) OR 1 = 1) AND (node.nid IN (SELECT tn.nid AS nid
FROM
{taxonomy_index} tn
WHERE ( (tn.tid = :db_placeholder_1) ))) ))
ORDER BY taxonomy_term_data_node_name ASC
Query with cache
SELECT DISTINCT taxonomy_term_data_node.tid AS taxonomy_term_data_node_tid, taxonomy_term_data_node.name AS taxonomy_term_data_node_name, taxonomy_term_data_node.vid AS taxonomy_term_data_node_vid, taxonomy_term_data_node__taxonomy_vocabulary.machine_name AS taxonomy_term_data_node__taxonomy_vocabulary_machine_name, node.nid AS nid
FROM
{node} node
LEFT JOIN (SELECT td.*, tn.nid AS nid
FROM
{taxonomy_term_data} td
LEFT JOIN {taxonomy_vocabulary} tv ON td.vid = tv.vid
LEFT JOIN {taxonomy_index} tn ON tn.tid = td.tid
WHERE (tv.machine_name IN (:db_placeholder_0_0)) ) taxonomy_term_data_node ON node.nid = taxonomy_term_data_node.nid
LEFT JOIN {taxonomy_vocabulary} taxonomy_term_data_node__taxonomy_vocabulary ON taxonomy_term_data_node.vid = taxonomy_term_data_node__taxonomy_vocabulary.vid
WHERE (( (node.status = 1 OR (node.uid = 1 AND 1 <> 0 AND 1 = 1) OR 1 = 1) AND (node.nid IN (SELECT tn.nid AS nid
FROM
{taxonomy_index} tn
WHERE ( (tn.tid =:db_placeholder_0_0) ))) ))
ORDER BY taxonomy_term_data_node_name ASC
After digging into views cache system I found that it's probably the fault of views_plugin_cache::get_results_key() function which enforce the view query to be compiled. Then when the query is about to be executed it is recompiled but as part of it has already been compiled, some placeholder goes wrong. And so part of the query failed, that's why we are missing some informations.
Here is the faulty line
views_plugin_cache.inc line 270 :
$build_info[$index] = (string) $query;
Comment | File | Size | Author |
---|---|---|---|
#21 | 1792380-Condition-clone-value-d7-20-test.patch | 1.64 KB | theo_ |
#21 | 1792380-Condition-clone-value-d7-20.patch | 2.5 KB | theo_ |
#18 | 1792380-Condition-clone-value-18.patch | 2.55 KB | theo_ |
#15 | 1792380-Condition-clone-value-test-15.patch | 1.68 KB | theo_ |
#15 | 1792380-Condition-clone-value-15.patch | 2.6 KB | theo_ |
Comments
Comment #1
dawehnerGood spot of the actual problem!
Mh we had this issue before before views got released but i was sure this once worked.
There have been even a core issue: #1112854: Subqueries use wrong arguments.
I tried to reproduce the problem using some custom code but failed, maybe it is helpful for you.
Comment #2
theo_ CreditAttribution: theo_ commentedFinally i found that it's a core issue related to the __clone method of the DatabaseCondition class.
When you clone a DatabaseCondition object containing a value which is a SelectQuery, the value object is not cloned and it should be.
For example views cache system does the following :
clone the query
compile the cloned query (the value object is compiled on the original query too)
execute the original query -> fail cause we are unable to re-compile query cause of value which is already compiled
So here is a patch to take care about cloning the value object.
Comment #3
theo_ CreditAttribution: theo_ commentedComment #5
dawehnerThanks for working on this issue!
Things like that have to be fixed first in drupal8 and then get backported
Comment #6
theo_ CreditAttribution: theo_ commentedHere is the patch for drupal8
Comment #7
dawehnerThere should be for sure a test which proves that the current behavior is actually broken.
Comment #8
theo_ CreditAttribution: theo_ commentedHere is the way to reproduce it in both d7 & d8.
Comment #9
theo_ CreditAttribution: theo_ commented#2: 1792380-DatabaseCondition-clone.patch queued for re-testing.
Comment #10
theo_ CreditAttribution: theo_ commentedanyone ?
Comment #11
dawehnerWhat about document that this might be a subquery? In general i would say this looks fine but i'm not an expert in the database system.
Comment #12
chx CreditAttribution: chx commentedIt does look fine; we need a standard for clone, I really dislike clone() it's wrong though it works , clone is not a function; it needs tests.
Comment #13
theo_ CreditAttribution: theo_ commentedHere is first the failing test, then the test + patch
Comment #14
theo_ CreditAttribution: theo_ commentedI found that previous tests are not relevant since this issue #1808624: SelectQuery __clone method does not recompute the Query uniqueIdentifier while other QueryPlaceholderInterface does fix it.
So here is a new test which expose the issue in a more obvious way.
Comment #15
theo_ CreditAttribution: theo_ commentedImprove test using countQuery
Comment #16
dawehner#15: 1792380-Condition-clone-value-15.patch queued for re-testing.
Comment #17
dawehnerThe fix and especially the tests are looking great so far, so remove the tags.
Maybe damien is a good person to finally review this change.
Just a small nitpick: The current standard in http://drupal.org/node/1354 says "Contains \Drupal...."
Comment #18
theo_ CreditAttribution: theo_ commentedUsing correct standard
Comment #19
Crell CreditAttribution: Crell commentedI think this seems reasonable.
Comment #20
catchCommitted/pushed to 8.x, thanks!
Moving to 7.x for backport.
Comment #21
theo_ CreditAttribution: theo_ commentedBackported to D7
Comment #22
Crell CreditAttribution: Crell commentedComment #23
David_Rothstein CreditAttribution: David_Rothstein commentedCommitted to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/ad244e2
Pretty sure this makes the PHPDoc on that __clone() method out of date, though?
Comment #24
theo_ CreditAttribution: theo_ commentedYes, you are right.
We might have something like :
"Copy conditional statements and subqueries." instead of the actual "Only copies fields that implement Drupal\Core\Database\Query\ConditionInterface."
Comment #28
daffie CreditAttribution: daffie commentedThis issue is fixed.