Complex cloned query dependent on __toString() call

Thanks a lot for debugging so thoroughly, this is a huge help!
However, I can't really say I understand the problem either. I know that outputting the query like this triggers it to be compiled internally (see SelectQuery::__toString()). Otherwise, it would only get compiled when executed. It seems that this earlier compiling fixes some bug that otherwise occurs later on. It's probably related to being cloned when computing the facets. (To test, could you maybe comment the $this->getFacets(…) line and see if the query is correct then?)
My theory would be that the __clone() implementation is not completely correct and some changes to the cloned query are in fact reflected back to the original one. By compiling the query before cloning it, this is somehow prevented (through internal caching of the arguments, or whatever).

In any case, this is almost definitely a core bug of some sorts. Outputting the query shouldn't have any effect, a cloned query should also not interact with the original, and since we are definitely not setting the fulltext key as a status filter, there seems to be a bug there as well.
First off, are you using the latest version of Drupal core (dev would be ideal)? There were bugs like this before (see, e.g., #1792380: DatabaseCondition not cloning SelectQuery value object (documentation followup)) but I thought those were all fixed.

In case this is really a problem in the latest version, I'm moving this to the core issue queue now. Maybe someone can help there. (The problem might be DBMS-specific, I guess (mostly since no-one else has complained so far), so I'm using "postgresql db driver" as the component instead of "database system". If it is a general bug, feel free to move it. Also, instead of closing please move back to the "Search API Database Search" project if the issue is incorrect and there is no bug.)

Oops, wrong version.

Drupal 7.24 is good enough, I'd say, it would just have been a problem if it was 7.10 or something similarly old. A quick git diff furthermore shows that there are no (non-comment) changes to the database system at all in dev compared to 7.24, so we can be fairly sure that the problem will be in both dev and 7.24.

By the way in the last test both variables ($db_query and $db_query2) were changed. If this was already expected please forgive my ignorance.

I wouldn't call it ignorance, but yes, it was expected. This is because in PHP 5, objects are always handled by reference. That's why we have to explicitly clone it when passing it into the getFacets() method.
Normally, the clone operation should be implemented (using the __clone() magic method) in a way that also clones all objects that are referenced by the cloned object, so that the two copies are really independent. (See the PHP manual.) This doesn't seem to be the case here, which leads to the problem.

What do you tnink about raising the priority of this issue to major?

I'm always a bit hesitant to do that. I'd say, let's just wait for any Postgres or DB maintainer to read this and let them decide. (If this doesn't happen for a few days, we can still raise the priority.)

Maybe we can contact someone able to help us... I'm not very confident that this post hit them.

I know, it's (nearly) always the same thing when reporting Drupal core bugs, especially for niche functionality (which most bugs are, of course, so long after 7.0). It's frustrating.
The best thing you could do is probably to go into the #drupal IRC channel and ask around there, whether anyone has encountered the problem, has some idea or would look at it for you.
If that fails, however, you're probably left to either fix it yourself or pay someone to do it – and even then it might need a fair amount of IRC campaigning to get the patch reviewed and committed.

In any case, good luck! Sorry I can't be of more help to you here.

Pinging one of the Postgres maintainers...

This cannot be PostgreSQL specific. Nobody has complained about it on MySQL because the engine silently cast column types (so a comparison between status that is a number and the parameter that is a string would not trigger an error).

There are at least two (partially related issues here):

• SelectQuery::__clone doesn't clone the sub-query tables at all
• SelectQuery::__clone doesn't call Query::__clone, so as a consequence its "placeholder generator id" doesn't get reset

Also, none of the other query types actually implement __clone, even if all of them have at least a WHERE condition that should be cloned. Of course, nobody bumped into this yet because cloning those is way less useful.

Another issue reported recently that is possibly a duplicate of this issue:
#2233605: Search query getting confused about which values go with which conditions
I marked the other as "related" because I wasn't sure if it was a duplicate or not? There is a clone happening in that query as well.

To hopefully move this forward finally: the attached patch works for me, fixing, e.g., #2299385: Search results not shown in view using several terms and facet filter .

Your patch works !

Hi, I have tested only with 7.28. We are going to update the core, I'll tell you the result.

For me it is not solved without the patch after updating the core to 7.31

@ gilsbert: It's definitely not been applied, and I wouldn't know a reason why it should now be fixed. Please re-test!
Also, if the problem is fixed with this patch, could someone set it to "Reviewed & tested by the community"?

Hi,
Here we also use search API 7.X-1.13 et Database search 7.x-1.4 and have this issue. We also use Biblio search API (which allows to search in special fields created by Biblio module), but I don't think this is the problem because we had the same problem on another drupal without Biblio module.
Regards
Stéphanie

@ gilsbert: You can find old releases of modules under "View all Releases" on the project page.
For Search API
For Search API DB

It's of course possible that some change there could lead to that issue not being triggered in your setup anymore.

Looks like a testbot fluke.

Testbot fluke.

Testbot fluke.

Took a look at this, and actually, doesn't this need to go into Drupal 8 first?

The code in this method for Drupal 8 is the exact same as the code in Drupal 7, so I would assume the same bug exists there.

Just another report that the patch in #16 fixed the issue for me on a site running Drupal 7.43 with Search API 7.x-1.18 and Facet API 7.x-1.5.

This is a very hard-to-troubleshoot bug, and it would be great if we could get the fix committed to both Drupal 7 and Drupal 8 soon.

Thanks!

And another report that patch #16 fixed the issue with Search API 7.x-1.18 and Facet API 7.x-1.5 too. Thanks.

And another report - patch #16 fixed the issue with Search API 7.x-1.20 and Facet API 7.x-1.5. Thanks much!

Here is the same patch for Drupal 8 (8.2 and 8.3 should both work).

This needs test(s) to make sure that the problem is fixed and will stay fixed.

Thanks @drunken monkey :)
Patch in #16 works perfect. I was getting nuts with this :)

@ drunken monkey, your patch works great! You're a lifesaver #16

Ta-da.

I am happy to confirm that #16 solved #2842853: Strange behaviour of tokenized fields for me! Thank you @drunken-monkey. You are the best, and still going strong in Drupal :)

Without patch:
php ./core/scripts/run-tests.sh --class "Drupal\KernelTests\Core\Database\SelectCloneTest"

Drupal test run
---------------

Tests to be run:
  - Drupal\KernelTests\Core\Database\SelectCloneTest

Test run started:
  Friday, September 14, 2018 - 06:36

Test summary
------------

Drupal\KernelTests\Core\Database\SelectCloneTest               0 passes   1 fails                            

Test run duration: 32 sec

With patch applied:
php ./core/scripts/run-tests.sh --class "Drupal\KernelTests\Core\Database\SelectCloneTest"

Drupal test run
---------------

Tests to be run:
  - Drupal\KernelTests\Core\Database\SelectCloneTest

Test run started:
  Friday, September 14, 2018 - 06:41

Test summary
------------

Drupal\KernelTests\Core\Database\SelectCloneTest               2 passes                                      

Test run duration: 32 sec

To add some context, @wla_g was my mentee today and worked on testing/reviewing this as he's got personal experience battling this bug for years on his D7 sites. He knew the fix works very well in production and it looks like a clean solution. To ensure it still applies cleanly and the D8 tests are correct we ran them again locally and verified the same fix is needed there.

Thanks to @wla_g for making me confident in mentoring my first sprint!

+++ b/core/tests/Drupal/KernelTests/Core/Database/SelectCloneTest.php
@@ -34,4 +38,31 @@ public function testSelectConditionSubQueryCloning() {
+    $sub_query = \Drupal::database()->select('test', 't');

we can use $this->connection->select('test', 't'); here now.

EDIT - also a couple of lines below.

A dubious reason to hold up a bug fix that’s been needed for five years, but alright.
Please just review again and set back to RTBC!

Back to RTBC.

Fixed these on commit:

1. +++ b/core/tests/Drupal/KernelTests/Core/Database/SelectCloneTest.php
   @@ -34,4 +38,31 @@ public function testSelectConditionSubQueryCloning() {
   +   * Test that nested SELECT queries are cloned properly.
   

   This should be 'Tests that...'.

2. +++ b/core/tests/Drupal/KernelTests/Core/Database/SelectCloneTest.php
   @@ -34,4 +38,31 @@ public function testSelectConditionSubQueryCloning() {
   +    // Make sure the cloned query has not been modified
   

   Missing a period on the end.

Committed/pushed to 8.7.x and cherry-picked to 8.6.x. Thanks!

7.x issue here: #3000191: Complex cloned query dependent on __toString() call.

Here's a re-roll of patch #16 for drupal 7.80.

I'm having the same issue, and this patch fixes it. My current applicable module versions are:

* search_api: 7.x-1.28
* search_api_db: 7.x-1.8
* facetapi: 7.x-1.6

Closed #1808624: SelectQuery __clone method does not recompute the Query uniqueIdentifier while other QueryPlaceholderInterface does as a duplicate, adding credit.