Hi everyone,

I wrote a module with some new functionality for our website and now I'm doing a review, before sharing it - I hope it will be accepted by the community. While writing the code, I had a problem with the SQL queries and NULL values - so now I want to ask for help and feedback, before contacting the maintainers and add some inutil work on their schedule.

I found, that drupal function db_query has problems with NULL values in the queries. Perhaps I chose the wrong keywords for searching - but I couldn't find hints on how to solve this problems.

An example:
With the following code, in the database it inserted 0 instead of NULL - and broke the whole database insert, because the $status got 0 as well (It should be 'premium') ...:

      if (empty($ts)) $ts = "NULL";
      $query = "INSERT INTO {db_table} (nid, status ,ts ) VALUES ( %d, %d, %d)";      
      db_query($query, $node->nid, $status, $ts);
      

My solution was:

      $query = "INSERT INTO {db_table} (nid, status ,ts ) VALUES ( %d, '$status', ";
      $query .= empty($ts) ? "NULL" : $ts;
      $query .= ")";
      
      db_query($query, $node->nid);
      

Now, the coding standards tell me: Use placeholders because SQL-Injections will be filtered with them. But it seems to me, I can't use placeholders, if I want to enter NULL values in the database.

Did I do something wrong? Do I have to add some attribute, to make NULL possible with the db_query function? Is there another function for inserting NULL values? Or - is my code OK and this is the right way to do it?

I'd really appreciate your feedback
anschinsan

Comments

Alan D.’s picture

Just scanning the forums for a reason why all %d numbers are case to integers myself. Looking at the _db_query_callback you will see that it is simply not possible with db_query.

    case '%d': // We must use type casting to int to convert FALSE/NULL/(TRUE?)
      return (int) array_shift($args); // We don't need db_escape_string as numbers are db-safe

Why on earth is NULL cast to an integer! This has been done since at least a version 4.7, so simply changing this now would possibly cause a cascading effect of bugs in other areas of the system.


Alan Davison
www.caignwebs.com.au

Alan Davison
Alan D.’s picture

You need to escape the integer value to avoid any sort of SQL injection attack. Use db_escape_string or cast $ts to an int (best).

For example;

<?php
  $ts = ($ts === NULL) ? 'NULL' : (int) $ts;
  $query = "INSERT INTO {db_table} (nid, status ,ts ) VALUES ( %d, '%s', %s)";
  db_query($query, $node->nid, $status, $ts);     
?>

Also just noticed that in your code you are using empty($x). If you have done this through out your module, just use 0 for NULL. Both return a TRUE result via the empty function. See http://www.php.net/manual/en/types.comparisons.php for a comparison of the different types of php type functions.

As per my own problem, I've defined up a flag that means NULL. So when I get 0 from the db, I know that it means NULL. If my db table field wasn't unsigned, I'd probably would have used -1 for the flag.


Alan Davison
www.caignwebs.com.au

Alan Davison
tny-2’s picture

I have changed db_query adding some new placeholders.

I rename it, in order to avoid any conflict.

when you need to insert NULL values, call my function instead of db_query(....)

the new placeholders for fields that could be NULL, are the upper case of actual ones.
Do not put quotation marks around %S.

  • %d==>%D
  • '%s'==>%S
  • %f==>%F
  • %n==>%N

...

I've done it today, and It's NOT TESTED ENOUGH.

I'll thank any bug reported.

THE CODE:

define('NULL_QUERY_REGEXP', '/(%D|%S|%F|%N|%d|%s|%%|%f|%b|%n)/');
function null_query($query) {
  $args = func_get_args();
  array_shift($args);
  $query = db_prefix_tables($query);
  if (isset($args[0]) and is_array($args[0])) { // 'All arguments in one array' syntax
    $args = $args[0];
  }
  _null_query_callback($args, TRUE);
  $query = preg_replace_callback(NULL_QUERY_REGEXP, '_null_query_callback', $query);
  return _db_query($query);
}
function _null_query_callback($match, $init = FALSE) {
  static $args = NULL;
  if ($init) {
    $args = $match;
    return;
  }
  switch ($match[1]) {
    case '%d': // We must use type casting to int to convert FALSE/NULL/(TRUE?)
      return (int) array_shift($args); // We don't need db_escape_string as numbers are db-safe
    case '%s':
      return db_escape_string(array_shift($args));
    case '%n':
      // Numeric values have arbitrary precision, so can't be treated as float.
      // is_numeric() allows hex values (0xFF), but they are not valid.
      $value = trim(array_shift($args));
      return is_numeric($value) && !preg_match('/x/i', $value) ? $value : '0';
    case '%%':
      return '%';
    case '%f':
      return (float) array_shift($args);
    case '%b': // binary data
      return db_encode_blob(array_shift($args));
  }
  $d=array_shift($args);

  if($d==='' || $d==='null' || $d==='NULL' || is_null($d)){
	return 'NULL';
  }
  switch ($match[1]) {
    case '%D':
      return (int)$d;
    case '%S':
      return "'".db_escape_string($d)."'";
    case '%N':
      $value = trim(array_shift($d));
      return is_numeric($value) && !preg_match('/x/i', $value) ? $value : '0';
    case '%F':
  }
}

PD: I know my english sucks, but I've tried to do my best.

rosell.dk’s picture

Here is another, simpler solution:

1) Set the database field to be NULL per default
2) Don't add the field to the query, when NULL

ad 1:
When you define your schema in hook_schema, define the field with something like this:
'ts' => array(
'type' => 'int',
'not null' => TRUE,
'default' => NULL,
),

ad 2:
like this:

if (empty($ts)) {
db_query("INSERT INTO {db_table} (nid, status) VALUES ( %d, %d)", $node->nid, $status);
else {
db_query("INSERT INTO {db_table} (nid, status, ts) VALUES ( %d, %d, %d)", $node->nid, $status, $ts);
}

mykmallett’s picture

As far as I'm aware this does not work when updating the database with a NULL value, only the first time.