Subqueries use wrong arguments

Same issue here, also from Views (so this might be a Views issue... I'll post something on the Views issue list and refer to this issue).

In my case, a sample query shows that :db_condition_placeholder_1 and :db_condition_placeholder_2 are used twice in the query, which I believe is not supposed to happen:

SELECT node.title AS node_title, node.nid AS nid, 'node' AS field_data_field_activites_node_entity_type, 'node' AS field_data_field_regions_node_entity_type 
FROM node node 
INNER JOIN taxonomy_index taxonomy_index ON node.nid = taxonomy_index.nid 
WHERE (( (node.status = :db_condition_placeholder_0) AND 
(node.nid IN (SELECT tn.nid AS nid FROM taxonomy_index tn 
LEFT OUTER JOIN taxonomy_term_hierarchy th ON th.tid = tn.tid 
LEFT OUTER JOIN taxonomy_term_hierarchy th1 ON th.parent = th1.tid 
LEFT OUTER JOIN taxonomy_term_hierarchy th2 ON th1.parent = th2.tid 
LEFT OUTER JOIN taxonomy_term_hierarchy th3 ON th2.parent = th3.tid 
LEFT OUTER JOIN taxonomy_term_hierarchy th4 ON th3.parent = th4.tid 
WHERE ( (tn.tid = :db_condition_placeholder_1) OR (th1.tid = :db_condition_placeholder_2) OR (th2.tid = :db_condition_placeholder_3) OR (th3.tid = :db_condition_placeholder_4) OR (th4.tid = :db_condition_placeholder_5) ))) AND (node.type IN (:db_condition_placeholder_1)) AND (taxonomy_index.tid = :db_condition_placeholder_2) )) 
ORDER BY node_title ASC LIMIT 10 OFFSET 0

The query ends up having "tn.tid = 'destination'" which doesn't make sense ('destination' is actually the node type).

I am slowing learning the art of Drupal debugging, but I think I traced the issue to the following comment in includes/database/select.inc, function __toString, around line 1500:

      // The following line will not generate placeholders correctly if there
      // is a subquery. Fortunately, it is also called from getArguments() first
      // so it's not a problem in practice... unless you try to call __toString()
      // before calling getArguments().  That is a problem that we will have to
      // fix in Drupal 8, because it requires more refactoring than we are
      // able to do in Drupal 7.
      // @todo Move away from __toString() For SelectQuery compilation at least.

So, maybe __toString() *is* called somewhere before getArguments().

Hope this helps.

It's really an issue in Views.

Views reuses the same subquery twice, once for the count query and once for the query. This is unsupported.

The issue can be easily demonstrated by applying this:

diff --git a/plugins/views_plugin_query_default.inc b/plugins/views_plugin_query_default.inc
index b108366..31f1589 100644
--- a/plugins/views_plugin_query_default.inc
+++ b/plugins/views_plugin_query_default.inc
@@ -1041,7 +1041,7 @@ class views_plugin_query_default extends views_plugin_query {
           }
           else {
             $has_condition = TRUE;
-            $sub_group->condition($clause['field'], $clause['value'], $clause['operator']);
+            $sub_group->condition($clause['field'], is_object($clause['value']) ? clone $clause['value'] : $clause['value'], $clause['operator']);
           }
         }
         $main_group->condition($sub_group);

... which is obviously not a real fix. I leave fixing Views as an exercice for the reader :)

I confirm that the above change (#2) by Damien fixes my issue.

Unfortunately, I don't have the knowledge to offer a proper patch, so I'll leave that to the professionals ;)

Merci Damien.

Yes, I can confirm that the patch in #4 fixes my issue also.

Thanks dereine.

Patch looks good to me. We already do that with UNION's for example, it's just wrong that we don't do that for other SelectQuery instances.

Now, I guess the only thing that's missing here is a test :)

This solves the issue in Views, but only by chance. It does for only one reason: Views uses the same subquery object to build two queries, but it runs one of them through SelectQuery::countQuery() which clone the second query object.

After the patch, you still cannot reuse the same subquery object to build two queries when you don't clone one of them. This is very very ugly :) We should rather make sure we force the query and all its conditions to rebuild itself when the source object is changed. One way of doing that would be to store the SPL Hash of the $queryPlaceholder and make sure we rebuild if ->compile() is called with a different object.

subscribe

subscribe

subscribe

subscribe

Because this has been tagged D8 I have opened a issue in Views for a workaround until this is fixed: #1201908: Taxonomy filter with depth generating incorrect results because of core subquery bug

In my case it affects the taxonomy (with depth filter) as above. I have attached dereine's first patch there.

subscribing

I just saw a very similar issue crop up in a Views Taxonomy with depth argument (or whatever they're called now in D7), along with a node access module. The query that Views ended up running had the taxonomy term ID mixed up with the node access gid, due to (I believe) duplicate use of placeholder arguments.

Probably the next thing to do would be to write a test...

subscribe

This issue over here was marked as a duplicate #972934: Contextual filter "Content: User posted or commented" argument does not work

another duplicate here #1223320: Filter "Has Taxonomy Term (with Depth)" and contextual filters
PS: solved with the patch above

Just a note that after applying patch in #4 to Views and #6 to Core, Views is working OK. Today I was seeing the problem when I used a Taxonomy filter along with a Language filter - the taxonomy term ID was getting set to 'en' instead of the number it was supposed to be.

This issue may also be the cause of #1202416: Search is not working with node access turned on.

See #22. You need to patch Views as well as Drupal Core. At least, that worked for me.

I just applied the patch in #4 to Views 7.x-3.x-dev from 2011-06-26 and the patch in #6 to Drupal 7.4

The views argument "Content: User posted or commented" is still broken. Just wanted to test it so I could give feedback.

Here is a screenshot of the arguments setup http://awesomescreenshot.com/0b7id7id0

Javier, as one of the people who could fix this, I am back in Hungary to help my brother with a 2.5yrs old and a newborn. Meanwhile I need to work to earn a living. Think again about "not hard".

I am going to raise the status of this issue to Major.

I have worked on a total of two Drupal 7 site build projects so far. Both involved Views. Both ran into this issue when making slightly complex queries. For me, this is a "blocker" -- it makes me think Drupal 7 is not ready for real time use, because the nearly-core-everyone-needs module Views that we all rely on cannot be used effectively without hacking core and/or Views.

Completely agree with #30. Makes Drupal 6 look more attractive for many situations.

I can break views by adding a relationship: content: taxonomy terms on node
Adding a contextual filter: Taxonomy term: name - allow multiple filter values to work together

Sorry, I didn't realize you wanted a views export. Don't be rude.

Add a couple of arguments to the preview, and voila!

@dereine, the post #972934: Contextual filter "Content: User posted or commented" argument does not work was marked as a duplicate of this one. Here is a views export using that argument.

http://drupal.org/files/issues/48584310_1.txt

dereine has seen this, but for the benefit of others, here is my working sample illustrating the bug, along with SQL queries and my views export:

http://drupal.org/node/1228280

In the name of readability I have deleted and attached those Views exports. Please be considerate and do not copypaste into a complicated bug report such long things.

Please be considerate and ask me to change my post next time, I'm obviously as active as I can be in this conversation. I do agree with your concern of readability, but ascetics is not a reason to change someone else's post. I've gone ahead and re-edited my post to include a pastebin link.

How can I help with this issue? This is a major roadblock for us and would love for it to be resolved.

There's a patch in #35, it fails 5 of Drupal's automated tests, but it still might fix this issue.

So you could read http://drupal.org/patch/apply, and try it out on your system to see if it fixes the bug for you or not.

If it does, then it's mainly a matter of trying to fix the 5 test failures and some additional review of the code in order to get it actually committed and into the next 7.x point release (or the one after, since they're monthly at the moment).

Nevermind, #33 and #35 are close together and I misclicked.

A test view in a clean dev environment still fails when using a term relationship and term name contextual filters just as I had included in my view export.

Another note - attached image is irrelevant and I don't know how to take it off...sorry!

After considering the subtle but crippling effects for several days I deem this critical. I will fix it this week. Edit: provided the Views expotrs above indeed break stuff.

Untested patch, implementing the basics of what I think we need.

How does this differ from

  public function uniqueIdentifier() {
    return spl_object_hash($this);
  }

If the only place you change the unique identifier is the constructor and clone then i fail to see the difference.

spl_object_hash() -- has terrible documentation. I updated http://drupal4hu.com/node/224 which explains it better. debug_zval_dump() will show the data spl_object_hash creates a hash from
php -r 'class foo{};$x = new foo; $x = new foo; debug_zval_dump($x);' results in object(foo)#2 ... and that (foo)#2 is what spl_object_hash encodes.

Regarding spl_object_hash(): those hashs can and *are* reused. We just cannot rely on them here.

Damien, you are right. Those hashes are reused. How dumb of PHP!

Rerolled, ran database tests. Ran the view from #59 seemed OK. Edit: please disregard this comment.

Patch is 0 bytes

Sorry. There's no real patch. As said: please ignore. The only reason I got passes is because i failed to apply the patch i thought i did.

Fixed the patch, let's see how test plays.

Also, the proper arguments for http://www.php.net/manual/en/function.uniqid.php are $prefix and $more_entropy. We might use $prefix for something, but I left it as '' for now.

The patch in #70 doesn't solve the issue with the attached view timing out when an argument is supplied for the contextual filter.

We had a comment (by myself) in the code that this was going to bite us in the ass one day.

Note to self: the whole SelectQueryExtender needs to be reviewed, it is full of BS right now.

javier, what about trying 7.7? if the patch applies, good. Otherwise, may I recommend http://drupal.org/node/707484 ? Thanks for helping to test.

The docs need some work in this patch [leaving status at Needs Review so that people will continue to test the functionality though] [but PLEASE don't commit this patch until the docs are cleaned up!]

a)

+  /**
+   * Check if a condition has been previously compiled.

Should be "Checks whether a condition..."

b)

  /**
+   * Returns an unique identifier for this object.

Should be "Returns a unique..." Note: this line appears twice in the patch, wrong both times.

c) Some functions with no doc:

+
+  public function uniqueIdentifier() {
+    return $this->uniqueIdentifier;
+  }
+
   public function nextPlaceholder() {

There are quite a few others without doc. EVERY function/method needs a docblock starting with /**.

The functions without doc are doxygen'd in the interface. Based on testing passed and #76 reporting it works with Views, this is ready. Note that there is quite some cleanup required around this topic but this is a critical to be fixed ASAP

Edit: I only fixed grammar mistakes pointed out by jhodgdon. Thanks for reviewing our critical!

According to the (proposed? accepted?) "docs gate", we're not supposed to be committing patches without complete docs, or with docs that don't meet our standards.
http://drupal.org/node/1203498

So I would appreciate it if someone who has time could address #79 (if I had time to do it, I'd make a new patch, but alas, not today...). All functions and methods are supposed to be documented -- no exceptions.

We have standards for how to document ones that are implementations of interfaces. See
http://drupal.org/node/1354
for standards.

I addressed #79 a) and b) because it's valid. However, most of SelectQuery, for example, is already not doxygen'd because it comes from SelectQueryInterface. If this is not enough please open an issue to fix DBTNG docs. That's a change of standards -- I am not against it -- but I am against it blocking this critical. See http://api.drupal.org/api/drupal/includes--database--select.inc/class/Se... for eg.

On further investigation i see a lot of Implements QueryConditionInterface::compile(). already so -- what can I say? Let's reroll this with those.

We are not going to add all the doxygen for select.inc in this issue. I fixed the rest of the files IMO. I am adding a test , however. Filed #1255524: select.inc needs more doxygen for select.inc.

Test-only, will fail.

This one, OTOH, passes because it includes Damien's fixes. Edit: Doxygen fixes are also included but I did not and will not touch select.inc doxygen as said above. There's dozens upon dozens of methods needing to be doxygen'd already.

Add an issue summary

Docs are much closer, thanks! I wouldn't hold up this patch for the doc fixes at this point either. Thanks!

Do we know whether this patch fixes #1202416: Search is not working with node access turned on too?

I confirmed that #1202416: Search is not working with node access turned on is resolved by the patch here. Since this is critical, jhogdon suggested this should go in first and then we add a test for the other as a followup.

I called this critical because it surely messes up lots.

Also note the patch applies to D7.x cleanly.

It's 3am in London, and so I'm not sure I totally understand what's going on here, but this has sign-off from the right people and fixes a critical problem with Views.

Committed and pushed to 8.x and 7.x.

That's dedication! Thanks so much!

Guys, sorry to resurrect such an old topic but this does not seem to be fixed, I still have problems with cloning an already executed query with subqueries. Drupal 7.

This is an illustration of my query structure with argument placeholder counters applied by '$this->getArguments();', as it processes arguments one by one in the program flow:

:0 (main query)
:1
:2
-- :3 (subquery)
-- :4

After it has been executed, in my program flow I clone whole query and add one more condition to the outer query. The outer query gets marked as changed, but inner query is not! During the next $query->execute() the "$this->getArguments();" counts the placeholders correctly before the subquery starts. Since the subquery is not changed, it gets skipped, and placeholder counter continues where it was, producing wrong placeholder numbers:

:0 (main query)
:1
:2
-- :3 (subquery)
-- :4
:3 (main query continues, producing same placeholder number as in skipped inner query. Should be :5)

I have manually applied the second part of #6, which seems to solve the problem:

diff --git a/includes/database/select.inc b/includes/database/select.inc
index 43f9267..fcf8d16 100644
--- a/includes/database/select.inc
+++ b/includes/database/select.inc
@@ -1566,6 +1566,18 @@ class SelectQuery extends Query implements SelectQueryInterface {
     foreach ($this->union as $key => $aggregate) {
       $this->union[$key]['query'] = clone($aggregate['query']);
     }
+    // Additional clone subqueries, because you can't use them in more than in
+    // one query.
+    foreach ($this->fields as $key => $field) {
+      if ($field['field'] instanceOf SelectQueryInterface) {
+        $this->fields[$key]['field'] = clone($field['field']);
+      }
+    }
+    foreach ($this->tables as $key => $table) {
+      if ($table['table'] instanceOf SelectQueryInterface) {
+        $this->tables[$key]['table'] = clone($table['table']);
+      }
+    }
   }
 }

Does it make any sense? Should we apply this part of patch here to D7 as well?