Subselect with placeholders causes invalid/duplicate placeholders

I suspect this is a side effect of #496500: Remove global placeholder counter. Can you check to see if the outer delete query is getting passed to the subselect getArguments() method properly?

hass, can you provide a unit test that fails from this bug? The provided unit tests include a fair amount of sample data you can work from. If you can replicate it from that, we should be able to track it down and squish it.

Subscribing, I have also run into this "subselect" issue when porting the biblio module to D7.

Here is a patch that addresses this problem and passes justinrandell's test. I decided to use this issue as a way for me to learn about the DBTNG backend and PDO in general, so apologies for neglecting the Assigned field, as I wasn't sure if I could take it until I got knee-deep.

The only way I could think of doing this was to add getter and setter functions for the placeholder variable, as we need to provide the select subquery with an updated placeholder count, and then pass it back to the master query. Technically, this could also make the nextPlaceholder function irrelevant, but I think it's still useful as a lean function. Also, I'm not sure if I'm following standards with the names of the new abstract functions.

Hm. What confuses me is that select and update queries were fixed a long time ago by passing a placeholder generating object along the compile chain. Delete queries are doing the same, so I don't get why it's breaking here. I don't think the extra methods added by the patch in #9 should be necessary. Unfortunately my debugger is being cranky and not letting me debug properly. :-( Still investigating.

I found the code you are referring to, but it's actually addressing another issue -- using a subquery as a table ("INNER JOIN [subquery] AS s"), rather than using a subquery in a condition ("WHERE [subquery] > :val"). As far as I can tell, without the above patch, the latter kind of subquery is broken on selects, updates, and deletes - and has been since the removal of global placeholders.

And in fact, it appears as if delete and update queries don't allow joins at all, which means that the getArguments solution isn't even needed here. I'm assuming there was a previous community decision to eschew joins in delete and update queries? It seems like more than a year ago it was determined that it didn't need to be supported since since SQLite doesn't support them: http://drupal.org/node/225450#comment-781151 -- Not sure if that was the end of the discussion.

Joins in UPDATE and DELETE are MySQL-specific extensions, so no we don't support them. However, subqueries in WHERE clauses *should* work cross-DB so those are supposed to work properly. Hence this issue being a legit bug. :-)

But we do have unit tests for WHERE subselects on Select queries already, don't we? Do those work? If not, then this is a bigger bug than just DELETE. :-)

This is indeed a bigger bug than just DELETE, as the problem is in DatabaseCondition. There is a test (testConditionSubquerySelect) that's supposed to cover this for SELECT queries, but it only uses one placeholder so it didn't run into this problem.

So like any good tricky bug, the actual fix is one line of code that took several hours to track down. :-)

The problem is that in order to keep placeholders consistent within dependent queries, we pass the outer-most query object through the entire compilation chain, no matter how many layers deep it recurses. That should work for all queries.

However! Long story short, in the specific case of a select query in a DatabaseCondition object (aka WHERE or HAVING clauses) the object wasn't getting passed through. Thus, the subquery thought it was the top-most object and used itself as the placeholder object, and when it then passed itself to its DatabaseCondition dependent objects (its own WHERE clause), the placeholder counter was different, and so we got 0 twice. Fail!

The solution is a single extra line to pre-compile the subselect object before we cast it to a string, into which we are then able to pass the placeholder object. And then the bug goes away.

While at it, though, I discovered the line right above it had a coding style error. It was doing an instanceof check on a class rather than an interface. So I fixed that, too. I also added a comment to an earlier code block a few lines away because I spent 10 minutes wondering if that's where the bug was because I forgot that no, it's processed elsewhere. :-)

Whew! I think we got this one. Thanks all. bot, what do you think?

I thought I removed some blank lines in this patch... :-)

@hass: Yeah, I'm not really worried about SQL micro-optimization in tests. For production code it may matter, but not for tests.

Simplicity wins -- very nice.

It was a joy to read up on this issue. Great team work. Committed to CVS HEAD.

Re-opening, since something very similar happened to me just now. Reproducing code:

$query = db_select('node', 't')->fields('t', array('uid'))->condition('t.uid', 2);
$query = db_select($query, 't')->fields('t', array('uid'))->condition('t.uid', 1);
(string) $query;
print_r($query->execute()->fetchAll());

Edit: Just tested the code again, to be sure, and suddenly the bug was gone! It seems, it only appears when the third line is present, i.e. when __toString() is called before the query is executed. This makes the bug less harmful, but more unpredictable.
This should of course return nothing, as it selects all nodes having uid = 1 AND uid = 2. However, nodes are returned, namely all with uid 1.
Outputting the SQL query shows why:

SELECT t.uid AS uid
  FROM (
    SELECT t.uid AS uid
      FROM {node} t
      WHERE  (t.uid = :db_condition_placeholder_0)
    ) t
  WHERE  (t.uid = :db_condition_placeholder_0)

Placeholder names are not unique when using a condition and a subselect. Other combinations might also not work.
Note that it doesn't matter that I use the same field in both conditions, the placeholder will always be the same.

I think this still deserves the "critical" priority, as it makes the placeholder system almost useless for complex queries.

Another related issue which I'm not sure is a bug (but annoying in any case): When assigning your own placeholders and using subqueries, the placeholders also aren't checked for uniqueness across queries, so when dynamically building a complex query I have to use a different name every time for each placeholder (to be on the safe side). Ideally, the Database API would take care of that for me.

Maybe there is a solution that solves all these issues once and for all?

That's an edge case that core has not run into. That's not critical.

Can you provide a patch with a failing test case? It sounds like the placeholder tracker propagation logic may have a hole in it somewhere.

We're not going to be able to mutate user-provided placeholders, though, without some exciting string and array parsing and maybe not even then. The DB layer can't do everything. If you're specifying your own placeholders then the onus is on you to not screw it up. :-)

Attached is a patch that adds such a failing test. As already mentioned as an edit above, the real problem here seems to be something else entirely. (OK, maybe this means I should re-close this and open another, fitting issue, but as I'm unsure and this is the more easily reversible option, I'll just change the title to reflect this.)
In my opinion, the __toString() method should refrain from altering the query object at all (safe an optional caching of the return value) – when this doesn't always produce the exact same result as executing the query without first casting the object to a string, this is highly inexpected behaviour. Who would think a __toString() method alters an object?

You are of course right about the priority.

That's an entirely separate matter. Select queries are non-reusable, unlike the other builders, for various reasons. That's by design in this case, and I don't see how that has anything to do with the original issue which is duplicate placeholders.

And really, yes, this issue is long dead. If there's other issues, please open a new ticket and reference this one. Reopening old tickets just makes everything harder to track.

Restoring original title and status.

#2509358: Erpal search fails on postgresql proves that this still needs work