Remove global placeholder counter

One problem i can see from this is when you have subqueries or otherwise "combined" queries, so the placeholders have to be unique for each sent query, and not for each query object/statement.

Hm. That means we have to make it re-entrant. That's highly ugly.

Random brainstorming: Could we have execute() reset the global counter before each compile? To make it re-entrant, we could have a semaphore static that gets set to TRUE at the start of execute() and FALSE at the end of execute()?

Ok, the attached patch moves that $next_parameter thing to the DatabaseConnection class and does reset it after *every* sent query. It is also not static anymore, because it just needs to be unique per database connection imho.

It *seems* to work as expected, from looking at devel's query log.

Note: by *every* query, I mean *every*. This needs to be improved to only be called it when necessary (queries that use DatabaseCondition), but this was the fastest way to have something working ;)

We use dynamic placeholders in a lot of places all over the DB layer, not just in condition objects. If we're going to move to a centralized counter per-connection object, we should use it in those other places, too.

Yes I know, I thought about that too, but...

a) Only conditions have the recursion problem, INSERT/UPDATE/MERGE are just a foreach
b) INSERT/UPDATE/MERGE do not have access to $connection when the Query string is built, so it would need to be static again, and I'm not sure if that is a good thing.

Edit: Oh, I was wrong regarding b, they do have access :)

At minimum, Select queries can also recurse. You can also have Select queries inside an insert/update/delete via the condition, which compile themselves separately from the condition object. Plus, I really don't like having different parts of DBTNG track their indexes differently.

If we just have the counter take a type param, we can then have each class simply submit its __CLASS__ as the counter type. They could be tracked together or separately at that point; shouldn't make a huge difference.

It also occurs to me that the array handling of parameters also uses an auto-counter, so that should get centralized as well.

OK I started working on this, and it's going to be more complex than I thought. Most of the query builders double-generate their placeholders: Once in execute(), once in __toString(). So we can't just use the connection's placeholder counter as we'd end up calling it twice.

... Except that is OK, because those aren't static to begin with. Hang on a minute here!

Right, so, on further investigation Berdir is right. We only need to centralize the placeholders on conditions, not because other query objects don't use them but because they don't use static versions of them. In fact, as noted in #9 trying to centralize those would break because we have to double-generate them.

So the attached patch is mostly just a reroll. :-) Hey bot, how do you like it?

Looks great, and I think the removed comments don't need to be replaced beyond what's there, unique placeholders per query are how I'd expect it to work.

New code makes lots of sense.

Do we need to expand our test coverage to cover some of the "Oh, what about..." things brought up in this issue?

Due to the if ($this->changed) { check in DatabaseCondition::compile(), the previous query string is cached. Is this safe when placeholders are reused?

I couldn't reproduce the exception. Bot, are you sure?

The bot wasn't sure.

Committed to CVS HEAD. Thanks for driving this one home! :)

With this patch the behaviour of the following code changed:

$query = db_select('cache')->fields('cache', array('cid'))->condition(db_or()->condition('cid', 'foo')->condition('cid', 'bar'));
var_export($query->execute()->fetchCol());
$query->condition('cid', 'variables');
var_export($query->execute()->fetchCol());

Before, the output was two empty arrays. Now the output is array ( )array ( 0 => 'variables', ), i.e. appending another condition to the default AND clause generates more results instead of less results as expected.

The query string generated in the latter query is this: SELECT cache.cid AS cid FROM {cache} cache WHERE ( (cid = :db_condition_placeholder_1) OR (cid = :db_condition_placeholder_2) )AND (cid = :db_condition_placeholder_1)

Re-using select objects isn't supported to begin with. Insert queries are the only ones that we explicitly support reusing. The combination of query_alter and extenders makes the select query compiler single-use.

This is broken. If you are using a complex query with simple conditions and db_or both this is broken because this is per condition not per query.

I'm pretty sure it is per query (It is actually per-connection and reset after a query has been executed). I'm pretty sure the tests would fail if it would be per condition.

But reverting this made #394182: DBTNG search.module pass.

Also? I read the code and placeholders are reset in the query method. If you happen to run that while building another query that is also broken. This definitely needs to be rolled back and a better approach worked out. I believe the placeholder needs to be part of the query object and probably db_or() needs either the query object or it needs to be a method.

OK, here is the rollback patch.

The exact thing that chx describes in #25 happens with the search patch: we are building a big db_select() query that uses the result of another big db_select() query.

Sorry.

More precisely, what happens is that the new search DBTNG patch is *sharing* condition objects between queries. This used to work flawlessly when the placeholders where guaranteed to be different during the lifetime of the request, but that cannot work anymore. I'm not convinced the main issue is in the placeholder reset counter.

Hm. Yuck. We do want to support multiple active query objects at once, so that's an issue. Unfortunately, I can't think of another approach that would also support Moshe's use case. I agree that search module is higher priority. Fortunately, I wouldn't describe this as an API change so we don't need to have a solution this week. We do need a solution, though.

I'm OK with rolling back if it makes search.module work.

Failed to run tests? WTH? But then the very same patch just worked inside the DBTNG patch. And, this is a straight roll back of this patch which is the last patch committed to core...

Committed. Thanks!

Marking back to active now HEAD is back to working in case there's another way to do this.

we worked on this with Crell.

That included search dbtng which is a great test case btw. :)

Seemingly this not a trivial issue.

Edit: I ran the problematic DBTNG search tests and they pass too.

$query in the compile() method should be called something else, since it's not always a query.

$query should not allow NULL to be passed into the compile() method, as we're not handling NULL and that would break anyway.

There's no need to re-specify the $placeholder variable in InsertQuery, SelectQuery, etc. All of the placeholder behavior can be handled in the Query base class. Also, $placeholder should probably be called $nextPlaceholder. (It will be needed in SelectQueryExtender, though, since that doesn't extend Query.)

I think that covers it. Just nitpicky stuff. It's actually overall simpler than the size of the patch makes it look. We're just providing better context to parts of the query build system through passing in context rather than just assuming it. That's generally a good thing anyway.

Renamed to queryPlaceholder everywhere, i can't help condition compile getting a NULL as most other compiles (like InsertQuery compile) wont get a query , renamed the variable too.

That should do it. Again, it's a much simpler patch than it looks at first glance. :-)

Committed to CVS HEAD. Thanks.

Ok, this broke SQLite and PostgreSQL.

This fixes it.

Thanks, committed to HEAD. :)

#564852: Subselect with placeholders causes invalid/duplicate placeholders may be a regression by the placeholder changes made here. The result in the specific case is a data loss!

No, this is closed. The other issue is a bug that is still open.