[DBTNG]: simplify drupal_write_record() with db_insert() and db_update()

Reroll patch with db_update() but not db_merge(), which should be more suitable.

BTW, it is also known as buggy when working with MySQL, as like as another issue (http://drupal.org/node/299385#comment-980272). The new query builder db_insert() is buggy when working with db_last_insert_id() (just after db_insert()) due to INSERT DELAYED. The programming logic of this patch should be correct: when I mask the 'DELAYED' from InsertQuery_mysql(), all cases work fine.

Reroll patch without db_last_insert_id(), fix the idea of serial field(s), and a bit code cleanup.

Patch via CVS HEAD, tested with MySQL and PostgreSQL, pass both node and user INSERT/UPDATE.

Catch the bug when working with http://drupal.org/node/371. db_insert() and db_update() can't handle invalid integer and float content, if input value is not in correct type, e.g. input '' for integer field.

For traditional db_query() most int/float will be filtered as correct data type but now we need to handle that manually.

Patch reroll via CVS HEAD, tested with MySQL, simpletetest + node and taxonomy, no error generated :D

Please checkout CVS HEAD includes/database/database.inc function _db_query_process_args() (line 1824) for more information:

/**
 * Backward-compatibility utility.
 *
 * This function should be removed after all queries have been converted
 * to the new API.  It is temporary only.
 *
 * @todo Remove this once the query conversion is complete.
 */
function _db_query_process_args($query, $args, $options) {

  // A large number of queries pass FALSE or empty-string for
  // int/float fields because the previous version of db_query()
  // casted them to int/float, resulting in 0.  MySQL PDO happily
  // accepts these values as zero but PostgreSQL PDO does not, and I
  // do not feel like tracking down and fixing every such query at
  // this time.
  if (preg_match_all('/%([dsfb])/', $old_query, $m) > 0) {
    foreach ($m[1] as $idx => $char) {
      switch ($char) {
        case 'd':
          $args[$idx] = (int) $args[$idx];
          break;
        case 'f':
          $args[$idx] = (float) $args[$idx];
          break;
      }
    }
  }

I have no preference for handling the casting within drupal_write_record(), but else we will need to go though:

hswong3i@dc:~/projects/drupal/drupal-7.x-dev$ find | xargs fgrep -nH drupal_write_record
./sites/default/modules/devel/devel_generate.inc:360:  drupal_write_record('files', $file);
./includes/file.inc:647:    drupal_write_record('files', $file);
./includes/common.inc:3258:function drupal_write_record($table, &$object, $update = array()) {
./modules/contact/contact.admin.inc:105:    drupal_write_record('contact', $form_state['values']);
./modules/contact/contact.admin.inc:111:    drupal_write_record('contact', $form_state['values'], 'cid');
./modules/node/node.module:946:    drupal_write_record('node', $node);
./modules/node/node.module:951:    drupal_write_record('node', $node, 'nid');
./modules/node/node.module:981: * Node is taken by reference, becuse drupal_write_record() updates the
./modules/node/node.module:988:    drupal_write_record('node_revisions', $node, $update);
./modules/node/node.module:991:    drupal_write_record('node_revisions', $node);
./modules/simpletest/tests/file.test:89:    $this->assertNotIdentical(drupal_write_record('files', $file), FALSE, t('The file was added to the database.'));
./modules/user/user.module:261:    $success = drupal_write_record('users', $edit, 'uid');
./modules/user/user.module:313:    $success = drupal_write_record('users', $edit);
./modules/user/user.module:335:      drupal_write_record('users', $data_array, 'uid');
./modules/taxonomy/taxonomy.module:219:    drupal_write_record('vocabulary', $edit, 'vid');
./modules/taxonomy/taxonomy.module:231:    drupal_write_record('vocabulary', $edit);
./modules/taxonomy/taxonomy.module:324:    drupal_write_record('term_data', $form_values, 'tid');
./modules/taxonomy/taxonomy.module:332:    drupal_write_record('term_data', $form_values);
./modules/block/block.module:252:          drupal_write_record('blocks', $block);

Some other potential cases should take in consideration (e.g. http://drupal.org/node/300788, http://drupal.org/node/299385 and http://drupal.org/node/300203), too. Any idea?

Sorry that we shouldn't apply type cast (!?) within drupal_write_record() internally, or else will generate more error message in simpletest :S

I found that after rollback to previous version, we only buggy in dblog and node, with sticky (int) and promote (int) input as '' which are invalid. Apply a very minor type casting can solve the problem. I also give a (almost) complete set of simpletest twice and found there is no additional error generated with MySQL.

Please try to run simpletest with: block, contact, dblog, node, taxonomy and user, compare result with CVS HEAD. It should now be safe.

@catch: I add the following code in includes/database/database.inc, run simpletest and check syslog for debug:

@@ -352,6 +423,11 @@ abstract class DatabaseConnection extend
       }
     }
     catch (PDOException $e) {
+      // TODO: remove debug
+      openlog("siren", LOG_PID | LOG_PERROR, LOG_LOCAL0);
+      syslog(LOG_ERR, "PDOException: " . $query . ' ' . $e->getMessage());
+      closelog();
+
       if (!function_exists('module_implements')) {
         _db_need_install();
       }

@moshe: Yup I catch this bug too. Patch updated with type casting. Tested with MySQL + dblog, node, taxonomy and forum. PostgreSQL seems a bit buggy with node revisions.

Actually I have no preference with drupal_write_record(), but from my point of view it is completely different from db_*() and so not interchangeable.

What the main duty of drupal_write_record() is to check schema definition, e.g. serial? default value? serialized? last insert id? For sure it is much high level than using db_*() directly without any checking and bonus benefit.

On the other hand, in some rear case, e.g. insert query of watchdog, we are not able to use drupal_write_record() because common.inc is not even load in yet. Therefore using db_insert() is a must.

Updated version. Correctly type casting input values, based on schema definition (only field with not serial and not allow null). Similar as legacy _db_query_callback(). Also update input variable $update as $primary_keys for better naming. So now no longer need to type casting before each call of drupal_write_record().

Patch via CVS HEAD, tested with MySQL. Run parallel simpletest (node, taxonomy, dblog, comment) and coming with identical result (P.S. CVS HEAD is also buggy in simpletest...)

Updated version. Coming with better type cast handling.

Patch via CVS HEAD, tested with MySQL and PostgreSQL. Run parallel simpletest (blog, node, taxonomy, dblog, comment) and coming with identical result.

P.S. Since this patch is just a wrapper of db_insert() and db_update(), the "empty string => NULL, in case of BLOB field" bug for PostgreSQL declared in http://drupal.org/node/316095 will still happening.

Cross test with following issues and all looks fine. Should we have more discussion for this issue, or move it as RTBC?

#299385: [DBTNG + BLOB] remap {aggregator_item}.description
#300203: Remap field as BLOB: {boxes}.body
#300788: [DBTNG + BLOB] remap {comments}.comment
#318650: Remap field as BLOB: {menu_router}.access_arguments and {menu_router}.page_arguments

Edit: Moshe changed this message to use our issue number hyperlinks method - [#n].

Just recall that patch from #23 still apply to CVS HEAD, and the hidden PosgreSQL + BLOB bug (#316095: Raise PHP requirement to 5.2.12 for PostgreSQL only) is belongs to PHP pdo_pgsql but not this patch.

Since PosgreSQL + BLOB bug (#316095: Raise PHP requirement to 5.2.12 for PostgreSQL only) is now coming with solution, so change of this patch is now safe. Even combine with #300802: [DBTNG + BLOB]: remap {node_revisions}.body and {node_revisions}.teaser is now also safe for PostgreSQL. All path should be now cleaned so this patch can commit without any problem.

Maybe we still need some more discussion, or is time for this issue to be mark as RTBC? Patch from #23 still apply clean to CVS HEAD.

Patch reroll via CVS HEAD.