Core queries coding style cleanup for PDO development

patch file

This patch (apply on drupal6-head) generate a crash on cache system :

Warning: require_once(./includes/cache.inc) [function.require-once]: failed to open stream: Permission denied in D:\Serveur\www\drupal-6-dev\includes\bootstrap.inc on line 898

Fatal error: require_once() [function.require]: Failed opening required './includes/cache.inc' (include_path='.;C:\php5\pear') in D:\Serveur\www\drupal-6-dev\includes\bootstrap.inc on line 898

@lilou: Seems a bit strange... Your error message is all about permission denied, but not programming error nor conflict... Would you mind to check those patched files permission, if they are able to open/read by your web server owner? E.g. are they have permission 644? I test the patch once again, and it is function for both MySQL and PostgreSQL ;-)

2. use %s for empty string.

Why?

@mooffie: according to my personal research project progress, the correctly use of DB API can greatly improve cross database compatibility, even with PDO and Oracle supporting. For more detail, please refer to my recent blog post.

db_query() should use correctly, e.g. escape empty string '' with %s (http://drupal.org/node/194162).

I mark this into my personal research project issue.

Patch reroll via latest CVS HEAD. Test without ill effect. It should be able to handle within D6, in order to prepare for next generation development.

As Siren is not a project on Drupal.org but an unofficial fork, I am removing it from issue titles to avoid confusion.

This fails against Drupal HEAD.

@birdmanx35: thanks for review and pointing out. Some changes are already fixed in other issue. Here is the updated version.

@hswong3i, no problem, that's about the extent of my abilities. I'll review this later tonight.

@hswong3i, no problem, that's about the extent of my abilities. I'll review this later tonight.

I got this error when testing it...
$ patch -p0 < queries_cleanup-0.3.patch
patch: **** Only garbage was found in the patch input.

Is that me, or the patch?

Are you sure that != is really better? According to this tool, it's not valid SQL-92, -99, or -2003. I'm pretty sure that it won't work in MS SQL Server, for example, though I don't have an instance to test against at the moment.

And the Coder module's (beta) SQL checking also references replacing "!=" with "<>".

Thanks for suggestion. I search out most core queries, and replace (most) "!=" as "<>". I just search with following simple script, so may still miss some out:

This patch still applies cleanly, and has made the necessary revisions. I am marking it RTBC.

Well, this is a huge patch which needs more then a yes or no on whether it applies or not. Adhering to coding standards with replacing != with <> looks like possible in Drupal 6, but I would not pursue replacing %s usage all around especially that nothing is broken in the systems we support. The quoted http://drupal.org/node/194162 issue which makes sense out of that is for 7.x. Please understand that we are focusing on stabilization not rewriting stuff on grand scales.

After thinking and reading about this patch, I have come to the following conclusions:

1) The patch was created to change != to <>, amongst other things.
2) That change has proved, through review by more educated individuals, to be of no use.
3) The whole patch, therefore, is based upon a single incorrect change in thought.
4) If the other problems tackled by this patch are really problems, they should be moved to a new patch, one that hasn't been touched by the incorrect change.

Gabor also points out that we are focusing on stability at the moment: however, if these are truly problems that need to be solved (which I can't determine), they need to be handled in a new patch.

@birdmanx35: Your #2 is incorrect. The original patch was replacing "<>" with "!=", not the other way around. This replacement is the one that was mentioned as possibly not being valid (comments #16, #17). The updated patch is properly replacing "!=" with "<>". This eliminates your #3 and #4.

Gábor thought we might get the SQL query replacements in. I would suggest we move this whole issue to D7 since we're so late in the game. We know all of the current queries are working. Changing them now would require an additioanl major effort to check all of the affected functions.

Yep, typo on the first, and you are right. hswong3i did say earlier he may have missed all of the switches though, so I guess I'd check those.

Moving to D7 as suggested.

Since Drupal 7.x is now open for public development, this patch is reroll via latest CVS HEAD. No special change since #18.

P.S. In case of PDO, we will still need to clean up the '%s' syntax: change them into %s (with '') or else PDO placeholder replacement will not accept it. BTW, I will reactive this issue after the successful commit of this first stage clean up, as that will affect almost ALL core queries.

I was always and still against using placeholders for constants. Remember my tirade in the other issue about how not to burden the developer? Not to mention it's cruft.

chx: totally agree with you. Since I clone these handling from ADOdb (where ADOdb is a OOP implementation), they are handled as driver object method, etc. If we will revamp our drivers as OOP in 7.x, it is a good idea for giving a simple ADOdb-clone. But before that, I would like to handle these simple abstraction as constant temporarily, and so we will able to introduce other database drivers into core without any difficulties, e.g. SQLite, Oracle and DB2. These abstraction are always need, in whatever form of implementation :-)

You have not answered my problems.

@chx: Hmmm... so answer it once again: we may use function to wrap the return string, we can use OOP as object method, we can use predefined constant, etc. I choose predefined constant right now since it is the most simple as strict forward solution. We may seems it as temporary and reroll to a better solution whenever it is suitable.

Or may I have your suggested answer? If you hope to reroll it as function callback style, I am please to do so :-)

But why are you using %s-placeholders for these constants? Sorry, but neither of your answers to chx made that clear to me.

I still have absolutely no idea why we want this and why this is the best solution. What is your problem in the first that mandates us to write such ugly stuff?

The detail of '%s' stuff:

1. We can't use %% syntax because this is the requirement of PDO prepare statement (see Example#11 for more detail). This idea also applied to the replace of '%s' as %s.
2. We need %s for empty string since some database will handle this incorrectly, says MSSQL, DB2 and Oracle.

   In case of MSSQL, PHP driver will return a space ( ) for NULL value stored within database. This is a well know bug for PHP MSSQL driver, which due to the incorrect handling of MSSQL dll provided by Microsoft. In case of Oracle, empty string will seems as NULL in SQL level. This means Oracle will need IS NULL for normal != '' checking. The is a bug of Oracle by design.

   BTW, both of the above bug can be handled within database driver implementation internally. We can handle these edge case within db_escape_string(). So the use of %s for empty string is important, as this will trigger the call of db_escape_string() for further more special handling.

3. The second point is also apply to empty BLOB handling. Some database need to use EMPTY_BLOB() as placeholder during INSERT/UPDATE queries. We will then get its empty BLOB pointer, and feed in BLOB value. E.g. Oracle and DB2 both need this hook. So we will need to use %b for empty BLOB value, in order to trigger db_encode_blob() for special handling.

Wait! Up to this point, please forget those hidden and complicated reason, as that would belongs to the duty of database abstraction driver developers and designer. Normal developer only require to follow a simple guideline: always use placeholder correctly, for whatever case (even empty string). DB abstraction layer will handle all of the other crazy stuff :-)

*shrug* i have better things to do than battle this. Please do not set back to critical. Critical is when a) Drupal is fundamentally broken b) something we can't or don't want to release without. For those, see Dries' posts. I know that this is important to *you* but not to Drupal as a whole.

SO D7 *may* without PDO? I don't really think so, and so the %% and '%s' must be clean up. Are they PDO related stuff?

Patch via CVS HEAD.

No longer applicable since DBTNG landed and doesn't use %.