constant values in db_query

Can you clarify in what way point 4 does not comply with D6 coding standards http://drupal.org/node/2497#formatting? I can't see the discrepancy :-?

Because as I see drupal 6 core does not follow this standard. I will try to count all the examples in drupal core, and all issues where such modification in core was rejected..

>as I see drupal 6 core does not follow this standard.
Ahhhh...!

>count all the examples in drupal core, and all issues where such modification in core was rejected.
Hmmm curious..!

counts in drupal core
rule broken vs compliance with the rule

includes dir:
13:8

modules dir:
20:1
I just stopped counting, the one was for better visibility not because the developer wanted to follow this rule. You can hardly find any occurrence in modules directory where this rule applies..

So, this rule is not a coding standard for drupal 6, never was..

quick search for rejected issues
#175704: SQL cleanup: /includes
#175825: SQL cleanup: aggregator
#175826: SQL cleanup: block

This was added to the page by hswong who was also the one trying to get the cleanup patches in. See the revision diff here: http://drupal.org/node/114774/revisions/view/189503/190765

It looks like even though he conceded this did not need to change (http://drupal.org/node/175704#comment-605038) he never went back and cleaned up the handbook page edits he made.

So basically yes, his changes didn't go in and that info needs to be removed from the page since it never really belonged there. I don't have time to do this myself right now.

Removed.

Should this also be fixed (http://drupal.org/node/2497):

User-supplied arguments should be moved out of the query body and passed in as separate parameters to db_query(), db_query_range(), and db_query_temporary(), etc. The query body should only contain placeholders specifying the type of the arguments (%d|%s|%%|%f|%b). This ensures that the data will be properly escaped and avoids SQL injection attacks.

Avoiding SQL injection is one thing, but I think we are saying that it's currently OK to have literal values *in* the query body?

@gpk - looks like it. More stuff added by hswong3i - revision diff here: http://drupal.org/node/2497/revisions/view/190378/190800

Well it's not a patch but I'd be grateful if someone could check that my edits make sense!
http://drupal.org/node/2497/revisions/view/303850/320192
Ta.

the db_query example is still incorrect on that page (2479)
should be changed to a real example from drupal core

From a quick grep I'd say core is not 100% consistent about whether or when literals should be included in the body vs. substituted in via placeholders.

Maybe this example from php.install is a more useful illustration:

yes, that's better

OK try now http://drupal.org/node/2497.

Looks good to me. In general that page reads a bit abruptly, but that's not one for this issue (and wasn't introduced by it either).

OK great :)

thanks Giles

Automatically closed -- issue fixed for two weeks with no activity.