Closed (fixed)
Project:
Drupal core
Version:
6.x-dev
Component:
database system
Priority:
Critical
Category:
Bug report
Assigned:
Reporter:
Created:
12 Aug 2007 at 05:46 UTC
Updated:
16 Sep 2007 at 12:31 UTC
minor update to queries handling, which will run though, installation. P.S. we need to use db_query() correctly for cross database concern :)
| Comment | File | Size | Author |
|---|---|---|---|
| drupal-6.x-dev-query-20070812.diff | 6.25 KB | hswong3i |
Comments
Comment #1
dries commentedWhy is this an improvement?
Comment #2
hswong3i commentedall due to Oracle and DB2 extra handling. they will use preg_replace_callbacl() to filter out all database specific reserved words and escape them, only if they are in lower case. for letting this functioning, we need to follow drupal's query coding standard strictly:
on the other hand, as different database come with different string escape handling (db_escape_string()), putting all values into %s can ensure codes are able to function among different database :)
P.S. the main idea is: just follow our query coding standard for "ALL" queries, therefore all new database drivers will able to function with no question. they will handling their database specific reserved words internally :)
Comment #3
gábor hojtsyThere are lots of places in Drupal, where we use literal values in the queries, and this is clearly not against our coding standards. So this patch is not actually enforcing our coding standards, and is most probably a small fraction of all the literal stuff, that would need to change.
Comment #4
hswong3i commentedi guess this patch just try to promote our coding standard (http://drupal.org/node/2497), which is VERY useful for other database implementation. on the other hand, this can improve our codes quality (we promote our coding standard, so at least we follow it, too). we grain something with no trade-off :)
Comment #5
gábor hojtsyFirst point me to the place where the coding standards say we should write
type = '%s' ..., 'module'and nottype = 'module', then call these changes coding standards enforcing, please.Comment #6
hswong3i commentedso let's take system.install as example:
as escape character: for Oracle and DB2, we only need to escape
', which replace it as''. if we are not placing string value into %s, it may be a bit difficult to ensure cross database compatibility. it is a hidden bug that may be happened, but can escape by following our coding standard :)Comment #7
hswong3i commentedsorry about this example, it is only a escape for starting
"... BTW, it is still a good idea to use our db_query() correctly :)Comment #8
hswong3i commentedsince both Oracle/DB2/MSSQL will preform A LOT OF reserved word rewrite handling to query BODY, this patch can greatly improve the ability of cross database compatibility. This is because all user input values are escaped, and will not capture by rewrite handling.
Comment #9
dmitrig01 commentedThis is much needed. Dries and Gabor, the reason for this is because MSSQL, Oracle and DB2 run many regular expressions on the queries (and large ones). They can catch most situations, however the parsers aren't perfect. They match SQL keywords and %s/%d/%f/%%/%b. Not just any text.
Comment #10
gábor hojtsyOK, committed. Let's document this in the update and coding style docs. I leave this at "needs work" until someone does that.
Comment #11
hswong3i commenteddocument the changes in following book pages:
Comment #12
chx commentedYes, you documented those there, so it's all done.
Comment #13
hswong3i commentedthanks chx :)
Comment #14
(not verified) commented