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;

Files: 
CommentFileSizeAuthor
#21 1792380-Condition-clone-value-d7-20-test.patch1.64 KBtheo_
FAILED: [[SimpleTest]]: [MySQL] 39,690 pass(es), 1 fail(s), and 0 exception(s). View
#21 1792380-Condition-clone-value-d7-20.patch2.5 KBtheo_
PASSED: [[SimpleTest]]: [MySQL] 39,732 pass(es). View
#18 1792380-Condition-clone-value-18.patch2.55 KBtheo_
PASSED: [[SimpleTest]]: [MySQL] 49,635 pass(es). View
#15 1792380-Condition-clone-value-test-15.patch1.68 KBtheo_
FAILED: [[SimpleTest]]: [MySQL] 42,804 pass(es), 1 fail(s), and 0 exception(s). View
#15 1792380-Condition-clone-value-15.patch2.6 KBtheo_
PASSED: [[SimpleTest]]: [MySQL] 49,412 pass(es). View
#14 1792380-Condition-clone-value-test-14.patch1.42 KBtheo_
FAILED: [[SimpleTest]]: [MySQL] 42,250 pass(es), 1 fail(s), and 0 exception(s). View
#14 1792380-Condition-clone-value-14.patch2.33 KBtheo_
PASSED: [[SimpleTest]]: [MySQL] 42,258 pass(es). View
#13 1792380-Condition-clone-value-test.patch1.66 KBtheo_
FAILED: [[SimpleTest]]: [MySQL] 42,082 pass(es), 1 fail(s), and 0 exception(s). View
#13 1792380-Condition-clone-value-13.patch2.58 KBtheo_
PASSED: [[SimpleTest]]: [MySQL] 42,071 pass(es). View
#6 1792380-Condition-clone-value.patch967 bytestheo_
PASSED: [[SimpleTest]]: [MySQL] 41,896 pass(es). View
#2 1792380-DatabaseCondition-clone.patch909 bytestheo_
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1792380-DatabaseCondition-clone.patch. Unable to apply patch. See the log in the details link for more information. View

Comments

dawehner’s picture

Good 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.

$subquery = db_select('users')
->fields('users', array('uid'));

$or = db_or()
->condition('status', 1)
->condition('status', 0);

$subquery->condition($or);

dsm((string) $subquery);

$select = db_select('node')
->condition('type', 'story')
->condition('uid', $subquery, 'IN');

$clone = clone $select;
dsm((string) $clone);
$clone->preExecute();
dsm((string) $select);
theo_’s picture

Project: Views » Drupal core
Version: 7.x-3.5 » 7.x-dev
Component: Code » database system
Status: Active » Needs review
FileSize
909 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1792380-DatabaseCondition-clone.patch. Unable to apply patch. See the log in the details link for more information. View

Finally 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.

theo_’s picture

Title: Views plugin cache issue » DatabaseCondition not cloning SelectQuery value object

Status: Needs review » Needs work

The last submitted patch, 1792380-DatabaseCondition-clone.patch, failed testing.

dawehner’s picture

Version: 7.x-dev » 8.x-dev

Thanks for working on this issue!

Things like that have to be fixed first in drupal8 and then get backported

theo_’s picture

Status: Needs work » Needs review
FileSize
967 bytes
PASSED: [[SimpleTest]]: [MySQL] 41,896 pass(es). View

Here is the patch for drupal8

dawehner’s picture

Issue tags: +Needs tests

There should be for sure a test which proves that the current behavior is actually broken.

theo_’s picture

Here is the way to reproduce it in both d7 & d8.

$user = db_select('users')
  ->fields('users', array('uid'));
$or = db_or()
  ->condition('status', 1)
  ->condition('status', 0);
$user->condition($or);

$type = db_select('node_type')
  ->fields('node_type', array('type'));
$or = db_or()
  ->condition('module', 'node')
  ->condition('locked', 0);
$type->condition($or);

$select = db_select('node');
$select->condition('uid', $user, 'IN');

$clone = clone $select;
$clone->preExecute();
dsm((string) $clone);

$select->condition('type', $type, 'IN');
dsm((string) $select);
theo_’s picture

theo_’s picture

anyone ?

dawehner’s picture

What 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.

chx’s picture

It 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.

theo_’s picture

FileSize
2.58 KB
PASSED: [[SimpleTest]]: [MySQL] 42,071 pass(es). View
1.66 KB
FAILED: [[SimpleTest]]: [MySQL] 42,082 pass(es), 1 fail(s), and 0 exception(s). View

Here is first the failing test, then the test + patch

theo_’s picture

FileSize
2.33 KB
PASSED: [[SimpleTest]]: [MySQL] 42,258 pass(es). View
1.42 KB
FAILED: [[SimpleTest]]: [MySQL] 42,250 pass(es), 1 fail(s), and 0 exception(s). View

I 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.

theo_’s picture

FileSize
2.6 KB
PASSED: [[SimpleTest]]: [MySQL] 49,412 pass(es). View
1.68 KB
FAILED: [[SimpleTest]]: [MySQL] 42,804 pass(es), 1 fail(s), and 0 exception(s). View

Improve test using countQuery

dawehner’s picture

dawehner’s picture

Issue tags: -Needs tests

The 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.

+++ b/core/modules/system/lib/Drupal/system/Tests/Database/SelectCloneTest.phpundefined
@@ -0,0 +1,47 @@
+ * Definition of Drupal\system\Tests\Database\SelectCloneTest.

Just a small nitpick: The current standard in http://drupal.org/node/1354 says "Contains \Drupal...."

theo_’s picture

FileSize
2.55 KB
PASSED: [[SimpleTest]]: [MySQL] 49,635 pass(es). View

Using correct standard

Crell’s picture

Status: Needs review » Reviewed & tested by the community

I think this seems reasonable.

catch’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)
Issue tags: +needs backport to D7

Committed/pushed to 8.x, thanks!

Moving to 7.x for backport.

theo_’s picture

Status: Patch (to be ported) » Needs review
FileSize
2.5 KB
PASSED: [[SimpleTest]]: [MySQL] 39,732 pass(es). View
1.64 KB
FAILED: [[SimpleTest]]: [MySQL] 39,690 pass(es), 1 fail(s), and 0 exception(s). View

Backported to D7

Crell’s picture

Status: Needs review » Reviewed & tested by the community
David_Rothstein’s picture

Title: DatabaseCondition not cloning SelectQuery value object » DatabaseCondition not cloning SelectQuery value object (documentation followup)
Version: 7.x-dev » 8.x-dev
Status: Reviewed & tested by the community » Active

Committed 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?

theo_’s picture

Yes, 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."

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

  • catch committed 80e1ff2 on 8.3.x
    Issue #1792380 by theo_: Fixed DatabaseCondition not cloning SelectQuery...

  • catch committed 80e1ff2 on 8.3.x
    Issue #1792380 by theo_: Fixed DatabaseCondition not cloning SelectQuery...