Posted by hswong3i on August 12, 2007 at 5:46am
Jump to:
| Project: | Drupal core |
| Version: | 6.x-dev |
| Component: | database system |
| Category: | bug report |
| Priority: | critical |
| Assigned: | hswong3i |
| Status: | closed (fixed) |
Issue Summary
minor update to queries handling, which will run though, installation. P.S. we need to use db_query() correctly for cross database concern :)
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| drupal-6.x-dev-query-20070812.diff | 6.25 KB | Ignored: Check issue status. | None | None |
Comments
#1
Why is this an improvement?
#2
all 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 :)
#3
There 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.
#4
i 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 :)
#5
First 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.#6
so let's take system.install as example:
<?php- db_query("INSERT INTO {variable} (name,value) VALUES('theme_default', 's:7:\"garland\";')");
+ db_query("INSERT INTO {variable} (name, value) VALUES ('%s', '%s')", 'theme_default', 's:7:"garland";');
?>
here we are assuming all databases use <code>\
', 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 :)#7
sorry about this example, it is only a escape for starting
"... BTW, it is still a good idea to use our db_query() correctly :)#8
since 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.
#9
This 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.
#10
OK, committed. Let's document this in the update and coding style docs. I leave this at "needs work" until someone does that.
#11
document the changes in following book pages:
#12
Yes, you documented those there, so it's all done.
#13
thanks chx :)
#14