db_query() should whitelist queries

The last submitted patch, 783814-whitelist-queries.patch, failed testing.

Why should we have an unchecked mode? I'm afraid this will be the easy way out for module authors. Just flip the flag to TRUE and you're done updating.

The last submitted patch, 783814-whitelist-queries.patch, failed testing.

@catch, if we don't enfore SELECT only db_queries, we may end up with lots of modules that do not work across the different database drivers.

This doesn't fall in the "Babysitting broken code" category. If you issue an INSERT query using db_query() it might not fail under MySQL, but might fail under PostgreSQL or SQLite. We are making something that doesn't fail (but should) fail.

This is a critical *task*, not a bug.

Adding a regexp here should not incur any performance hit (but this will have to proven).

Come on, this is over the top. You are trying to enforce your view of the world on other (non-core) modules.

I can't believe we still had a direct UPDATE query in core!

Yeah, how could a regexp for every single db_query() possibly have a performance penalty?

@ksenzee: I believe preg_match() will be faster in every case. Regexps are fast and compiled, and generally faster then dumb string functions as soon as you are doing something more then once.

For the regex to match the comment, shouldn't the regex have anchors:

Also, why are only single quotes evil? Why not double quotes?

What would prevent anyone encountering a problem from just flipping the flag to TRUE? Can't we make a non API function (eg _install_query) that allows this? (Then again, some people would use that without thinking twice).

I'm against babysitting developers who don't know what they're doing and throwing roadblocks in front of developers who do.

@nevets, I'm a little curious why you consider this a blockade to development. The new db layer is quite usable - easier than doing db_query() right, if you ask me. This is more like putting a "Road Closed" sign on the smaller, bumpier road, so people look up and notice the freshly paved highway right in front of them.

There are a million ways in Drupal to write code that works on some systems but breaks on others. ... Why is the database layer special?

This is a special case because it's way too easy to do this wrong. To do it right, contrib authors would have to be even more aware of the coding standards than core contributors, apparently, since we currently have a spot in core where it's done wrong. And if either core or contrib uses the DB layer wrong, we're putting huge barriers in front of anyone who wants to use Drupal on anything but MySQL. Those people's needs are important, and we've made a public commitment to them.

In hindsight, it might have been useful to rename db_query() to something else - maybe db_simple_select(), although that's a dorky name - to make it blindingly obvious that you're only supposed to use it for simple select queries. We didn't do that, and so what we have is a deceptive API. This is a simple change that will help correct that.

If you force developers trying to use Drupal for purely in-house applications to jump through hoops to support multi-database capablities they have no interest in then they may well go somewhere else.

Or they will write:

So, really, what is the point? Developers who don't want to heed advice won't, all of a sudden, read the documentation if you throw a DatabaseInvalidQueryException; they will just look for the quickest, simplest work-around.

Valid point about people trying to do it right. Still, I'm not convinced throwing exceptions is the best way to reach these developers.

@Crell - what's wrong with the hook_requirements() + hook_modules_enabled() (or equivalent) from #19.

OK let's not do it at all then, I think this is won't fix, and a coder rule.

Its not Drupals job either to make sure people from database portable code, it's job is to facilitate portable database code.

EDIT: I spoke too soon @ksenzee. You're correct that it does at least offer an option as a way out of the exception.

@Damien, I'm not sure if you spoke incorrectly, but your assertion illustrates why this patch doesn't belong in core, IMO.

Making sure that your modules are using the API correctly and that you issue portable queries is the job of *every single* module maintainers [sic].

This should be documented and put in coder.module. That's how we've solved "making sure that module maintainers are aware of those issues and don't write queries the wrong way unknowingly" this far, as I can see.

I'm surprised that no one in this thread has mentioned the simpletest framework, which, BTW tests every patch on d.o. Funny enough, d.o is that same place we host contrib modules. So why not use the test bots to test every recommended release of a contrib module and provide on the project page a box with crosses or ticks next to each database driver indicating driver compliance for that release. This will:

• Encourage contrib maintainers to write tests for there modules. (Yay!)
• Encourage contrib maintainers to write compatible code for there modules, or show that there module is pretty poor and anti-compliant. Same on them

The coder module is not a solution as there are many developers who don't use it. The thing about open source is that we cannot control how people use or develop Drupal solutions. But since most people go to the project page to download a module. We can give them guidance on whether to use that module or not. We already do this with usage stats.

So I don't agree with checking for illegal use - we don't do that in parts of the drivers. But I think it would be a good idea to rename the db_query function so that module maintainers do need to re-code there modules which will force them to implement DBTNG correctly, or rename the db_query function if they are lazy.

I propose db_static_query, as it is used for more than just SELECT queries and with good reason, as the drivers don't cover 100% of SQL requirements.

The placeholders change from D6 to D7 ought to ensure that queries don't work without some kind of tweak anyway, unless they have no placeholders at all, so I'm not sure that a change which breaks all existing D7 modules, and breaks D6 modules a little bit more, just for the sake of breaking them, is much help.

I'm also not clear why multi-database type test bots aren't mentioned on here either, I remember that being discussed in irc and the argument, irrc, was that it was going to be too resource-intensive - I don't see how we can get away without that eventually, so I'd rather have the resources being expended in test slaves than in runtime code on every Drupal 7 site.

As an example of all the messing we are getting ourselves into, see Randy's surprise that double quotes in static queries don't work consistently in #813310: db_query(): Static query with double quotes is not properly parsed.

Randy is far from being the random module author. I stand that we definitely need to automatically fail those queries, or our cross-database support will remain a pipe dream.

The patch.

Wrong patch, sorry.

I tested my case and this works for me. Consistent, predictable. Needs to be documented still, of course.

I benchmarked this, and there is (as expected) no significant difference made by that additional check.

I tested the following:

• full benchmark + 50 static queries per run
• 100 runs total

The difference is below the noise level.

The last submitted patch, 783814-whitelist-queries.patch, failed testing.

And this one should fix the "Exception thrown without a stack frame" (triggered from DatabaseConnection_mysql::nextIdDelete() which is registered as a shutdown function).

Fixing the remaining issues (temporary tables queries should be relaxed, one missing query in menu tests, and one typo in the action tests).

What's going on in DatabasTasks_pgsql? I don't know Postgres, so what is this patch doing there with the LANGUAGE='sql'?

The exception thrown by the new regex check say a bad query was passed to db_query(). That's technically not true, since we do support calling $connection->query() as a first-class API; that's the better way to handle DB switching now.

Of course, that then immediately raises the question in my mind, would we be better off putting this check in db_query() and allowing $connection->query() as the bypass? That would dramatically reduce the amount of code we have to change within the DB drivers themselves to set the bypass flag all over the place. (Some of them are kinda ugly if we don't have any placeholders.)

I would still be open to a coder.module rule instead if we can show it will be sufficiently effective at weening people off of doing things in a non-portable way.

jesus, 10k db_query()'s resulted in 350k function calls.

Although I'd tend to agree with catch, adding the preg_match seems to have a miniscule impact on performance in the grand scheme of things. I don't see it hurting given these benchmarks. I hereby release jpegs into the world.

A first pass at answering concerns here:

@Crell: What's going on in DatabasTasks_pgsql? I don't know Postgres, so what is this patch doing there with the LANGUAGE='sql'?

We are not doing anything here, just adding the relax check.

@Crell: Of course, that then immediately raises the question in my mind, would we be better off putting this check in db_query() and allowing $connection->query() as the bypass? That would dramatically reduce the amount of code we have to change within the DB drivers themselves to set the bypass flag all over the place. (Some of them are kinda ugly if we don't have any placeholders.)

I actually floated the idea in DCSF, and you rejected it on the basis of API consistency. And I agree with the you from DCSF.

@Crell: I would still be open to a coder.module rule instead if we can show it will be sufficiently effective at weening people off of doing things in a non-portable way.

For all those that think that social means (like the coder module, which might, one day, be deployed on the drupal.org test infrastructure) will solve this issue, think again. The support of PostgreSQL on Drupal core and modules has been a joke for years.

This patch aims at encouraging module developpers to do the right thing from the start. Once something is broken, it's generally very hard to unbreak. Of course, this complements other tools, like the Coder module.

Even if I'm maintaining the PostgreSQL and SQLite drivers in core, *I will not make sure that every module on Drupal.org works on PostgreSQL and SQLite*.

@catch: The patch has to rewrite valid db_query() calls where '' is used as a string literal in the query due to adding those to the regexp (presumably to account for mistakes like ':placeholders').

Those queries are using string literals. As a consequence, they are not valid.

The separate issue, if one is created, should also correct that fact that those query strings overall are still double quoted, which doesn't match coding standards - in fact if we decided to make such a change for consistency (with or without the regexp) one advantage would be that any normal calls to db_query() would use single quoted strings per coding-standards.

Indeed. The fact that those queries uses double quotes is a symptom that they need to be fixed. I considered doing that but rulled against it, as it would significantly increase the size of this patch.

I don't remember string literals being invalid in db_query(), was unable to find any discussion prior to this issue, or docs either. Double quoted strings, or placing quotes around placeholders yes, but not the examples which are removed in this patch (which is probably why there's so many of them). So if they're invalid we need better documentation than "use the appropriate placeholders to pass string values.".

On this:

Even if I'm maintaining the PostgreSQL and SQLite drivers in core, *I will not make sure that every module on Drupal.org works on PostgreSQL and SQLite*.

We can equally change that sentence to:

I will not make sure that every module on Drupal.org works

And it would be even more reason not to add safety checks to core functions which run on every request.

This is getting repetitive on both sides, but to reiterate, I can't think of a single example where contrib modules being broken has resulted in the addition of a lot of runtime code to core, with the exception of security issues which are their own category (but even when core is extremely helpful the responsibility is on contrib modules to use the API correctly).

@dhthwy: thanks, that looks like 2.2% cpu usage added to db_query(), since db_query() cpu usage itself is only 2-5% of most requests in terms of CPU that confirms a negligible performance difference, so it's really down to whether the change is desirable in itself or not.

I suspect the quote issue is due to the way the DB itself treats " vs. ' in query strings. It's not something that the Drupal code cares about either way at the moment.

I believe string literals in queries were considered "bad" starting with D6, on the grounds that they made cross-DB support harder. I cannot find a reference to that in the D6 update guide, though. I thought it was there: http://drupal.org/update/modules/5/6

In practice I believe it's only an issue for BLOB and LOB fields, since those require special handling, including secondary queries, on some databases (Postgres and Oracle in particular). varchars and ints should be safe in practice, but as always it's easier to document "just don't do it" than "just don't do it on these types of fields in these cases under these conditions".

Single quotes and double quotes have never been the same in SQL (some discussions about this):

• single quotes are ANSI SQL for string literals
• double quotes are ANSI SQL for identifiers

That said, there are several benefits in removing all string literals from queries, which is the point of this issue:

• Without string literals, all queries are guaranteed to contain only identifiers, so third party drivers can quote reserved identifiers (that even Drupal core still uses) using (more) simple regexps
• Without string literals, drivers can massage the strings, as needed for Oracle to workaround the empty string == NULL bug
• Without string literals, we don't risk SQL injections anymore, #win

Ensuring that only SELECT queries are passed to db_query() and that all DML queries (INSERT, UPDATE, DELETE, TRUNCATE, MERGE) are using our query builders also has the following additional benefits:

• Module writers don't write queries they think are valid, but really aren't,
• BLOB / LOB fields can be properly massaged (see InsertQuery_pgsql, UpdateQuery_pgsql),
• The database engine can workaround bugs or semantic differences in those queries (see UpdateQuery_sqlite, DeleteQuery_sqlite),
• The database engine can implement constructs that are not supported by some database engines (see TruncateQuery_sqlite),

We have a nice database abstraction layer, which will make true database portability a reality, assuming module authors uses the API properly. Relying on social pressure to do that will fail, as we fail to support PostgreSQL for years.

I'm neutral on removing single quoted literals from a code style perspective. However it's valid ANSI SQL and removing it is a very late API change which judging by the patch will affect a reasonable number of modules. Absolutely protecting against SQL injection might make it worth doing regardless (although we don't have a history of lots of SQL injection issues compare to XSS or access bypass), but adding the API change tag here either way so it gets documented properly if this goes in.

Once again on the social pressure argument, I still don't see what we get here that's not covered by automated testing, which feels like the proper place to be doing these kind of checks rather than in the critical code execution path.

cha0s, I think you're misinterpreting Damien's comments. This patch is not intended to be social pressure. It is intended to make people say, "Whoa, I'm doing something wrong, guess I'll check the docs." See my road analogy in #26. What we have *now* is social pressure, and it's basically useless.