Over aggressive escaping of table names

Looks good to me. Can't imagine why a dot would hurt other DBs but this way we know what we are doing.

That will fail horribly if there is a default prefix. Not sure how to workaround that.

How is this different than #302327: Support cross-schema/database prefixing like we claim to?

What about Damien's comment in #2? Just overriding the method without thinking of the impact on other weird things we're doing to the table name is a hack.

If we're going to allow periods, shouldn't that at least be allowed for all drivers, not just the MySQL one? We want to make certain that we can get the behavior of all supported databases as close as possible.

Er, wait. I just realized something I should have realized days ago.

The period needs to be OUTSIDE the table name. Prefixing happens on the table name, not the DB name. So if you're joining across databases, you should be doing this anyway:

db_query("SELECT * FROM {table1} INNER JOIN otherdb.{table2}");

Shouldn't you? Is there a detail here I'm missing?

Ah! Hm. Well, that's going to be a problem anyway if there's a prefix defined, because it will prefix the DB name, not the tablename, wouldn't it?

DBTNG was not designed for cross-DB joins in part because that's a MySQLism AFAIK, and very much an edge-case.

If we're going to allow periods in table names, then the patch in #8 looks correct with the caveat that it will still break with prefixing. I'm torn on whether or not that's a deal-breaking caveat.

I'd prefer to get input from the rest of the DB team here.

Considering: db_select('external.users)->execute(); I really don't see, considering {prefixing} how it would be fixed -- if you had, say a prefix of mydb_, that would be turned into "SELECT ... FROM mydb_external.users ..." I think that it would be better to somehow allow external db's; perhaps like db_select('users')->external('external')?

However, in that case, an external db might not want to have prefixes anyway. I think the whole thing is opening a big can of worms, and needs to be thought out in more detail. Committing this patch would achieve absolutely nothing, IMHO.

Well just to confuse the issue, I checked through the CVS records. And I found no period. I just did a cvs up, and the escapeTable() in database.inc has no period. In fact I found no record of a period in the CVS logs anywhere.

Someone please explain to me why I am going rapidly insane, because I don't know how that's possible if #8 above is passing the bot.

-    return preg_replace('/[^A-Za-z0-9_]+/', '', $table);
+    return preg_replace('/[^A-Za-z0-9_.]+/', '', $tabl

The patch in #8 is adding the dot to the characters that are not stripped.

OK, so I am just going crazy. That's what I get for trying to do 5 things at once this morning. *sigh*

In that case, the current whitelist has been the same going back to 4.7. Before that it doesn't seem to exist. I couldn't tell you why that was selected originally. For DBTNG I just carried that escaping logic forward as-is.

I suspect it was to avoid "whoops, wrong database!" problems, since a period is NOT a valid character in a table name.

Crell is referring to db_escape_table() no?

http://api.drupal.org/api/function/db_escape_table

If so, that was only applied to four functions in D6, not to db_query() or update_sql().

Looks like this might be a small design flaw in the API?

I'll leave it up to Crell to sign off on the final fix.

Maybe Crell will take another look if I mark this RTBC. We've confirmed that this is a new restriction in D7, given the old one didn't apply to select queries at all. I'm not sure what else there is to do here.

Since I don't know what the original logic was for db_escape_table() in the first place, I'm OK with this unless someone from the security team sees a reason not to.

(Sorry, this patch fell off my radar for a bit.)

Committed to CVS HEAD. Thanks.