dwr() fails when a NULL value is passed in a serial column (on PostgreSQL and SQLite)

This is due to the change in drupal_write_record() that got in with #303965: Enhanced data import - node_save() and user_save() (see the description in #44).

In a nutshell: callers should not pass NULL values for serial columns in drupal_write_record(). It used to be harmless, because dwr() used to ignore all values passed in serial columns. The change that got in with #303965: Enhanced data import - node_save() and user_save() removes that, and revealed bugs in the calling functions at several places in core (including the field API, and apparently _node_save_revision()!), but *only on PostgreSQL and SQLite*. MySQL considers that setting a NULL value in an autoincrement column is the same as not passing the value.

We will have to fix the calling functions themselves. But, because the behavior is inconsistent across engines, we will probably have to throw an explicit exception in dwr() when a NULL value is set for a serial column.

Fixing incoming NULL values is one thing.
The bug described in #303965-50: Enhanced data import - node_save() and user_save() (which actually breaks node_save() on SQLite) is a separate one, *not* caused by $node->vid = NULL, but by $node->vid = 0.

The patch fixes it because, although the comment claims 'Ignore NULL values', it actually relies on empty($object->$field).

Either we state that 0 can never be an intended value for a serial, and the fix here unbreaks node_save() - but we need to update the comment so that it is clear we want to catch NULL and 0, and some good soul doesn't revert it back later to 'only check NULL' based on the comment.
Or we accept explicit 0 in serials, just like any other explicit value - then this patch needs to check only for NULL, and node_save() needs its own fixing (unset($node->vid) before _node_save_revision() on node or new revision creation).

The bug described in #303965-50: Enhanced data import - node_save() and user_save() (which actually breaks node_save() on SQLite) is a separate one, *not* caused by $node->vid = NULL, but by $node->vid = 0.

Good point!

I really believe that we should use the NO_AUTO_VALUE_ON_ZERO SQL mode on MySQL: MySQL standard behavior (assuming 0 in an autoincrement column means "whatever the next value is") is inconsistent with PostgreSQL and SQLite, which makes it harder to write proper cross-platform code when developing on MySQL.

I'd love to use NO_AUTO_VALUE_ON_ZERO on MySQL. IME, though, that's a server-level setting that you don't always have access to. A lot of the shared servers I work on won't let me set it, as it's a server-wide property.

Admittedly I've not tried setting it per-connection through Drupal, so it's worth trying; but we should be careful that we don't inadvertently raise the DB permission requirements in the process.

@Crell: I don't see anything in http://dev.mysql.com/doc/refman/5.0/en/server-sql-mode.html that could imply that NO_AUTO_VALUE_ON_ZERO is any different from TRADITIONAL and ANSI that we already use. It seems to be a per-session parameter.

Hm, OK. Let's give it a shot. If you roll a patch I can test it on a server I know has been problematic in the past. If that works, we can commit that. Even if it isn't the ideal fix for this issue it's something we should be doing anyway. :-)

+1 for making MySQL break on the same stuff that other db backends.
Note that once all dbs break consistently, we'll still need to decide what's the right fix :-) - see 2nd part of comment #6

   // Generate the node table query and the node_revisions table query.
   if ($node->is_new) {
+    unset($node->nid);
     drupal_write_record('node', $node);

reverts #303965: Enhanced data import - node_save() and user_save()...

node multiple load dies an early death (10 passes, 0 fails, and 1 exception) without the patch and passes (38 passes, 0 fails, and 0 exceptions) on SQLite.

I'd like Moshe to give this a once-over. He authored the patch that re-worked drupal_write_record() which ended up causing this issue, and also was the original author of the function.

Sorry, I have no expertise to know if this is a proper fix or not. The 'enhanced data import' patch went in with tests so i am certain that this patch does not break it, nor does it break node_save() and user_save() in general. So, I personally would be comfortable going ahead with it.

What once was a fairly simple function, is now becoming increasingly complex. ;-)

+    // If a serial column does exist with no value (i.e. 0) then remove it as
+    // the database will insert the correct value for us.
+    elseif (isset($serial) && array_key_exists($serial, $fields)) {
+      unset($fields[$serial]);
+    }

Why would one pass a serial with value zero? If you do, I think it should be interpreted as "I want to insert a value with ID 0". I don't think silently modifying the parameters of a call is appropriate. If 0 is not a valid value, let's return an error message instead.

+      // If the serial value was set before the insert, then the database
+      // wouldn't have returned the last insert id, so we populate it instead.
+      if (array_key_exists($serial, $fields) && $fields[$serial] > 0) {
+        $object->$serial = $fields[$serial];
+      }
+      else {
+        $object->$serial = $last_insert_id;
+      }

I think this is reversed. If the database returns a serial ID, we should return that one because it is the only one that is guaranteed to be correct. I don't like how this code makes assumptions about the underlying database behavior -- it is prone to mistakes. We could return 'A' while the database actually returned 'B' which data loss and/or data inconsistencies as a result. I don't want my abstraction layer to 'guess' what the database does, I want to get the database to return me what really happened. If we start making assumptions about how the database works, we'll end up adding more and more special cases.

Two reasons why a serial column might get to this point with a 0 value :
- as Josh Waihi says, $fields['serial_colmun'] = NULL does get typecasted to 0 a few lines above. As I said in #6 above, I'm not sure it's the most common case - how much code calls d_w_r() with a serial vlaue explicitly set to NULL ? At least, it is *not* the reason node_save is broken on SQLite.
- current node_save workflow, described in #303965-50: Enhanced data import - node_save() and user_save(), calls d_w_r('node_revision') with vid = 0, and expects it will insert [next_vid]

IMO the main question to answer is "is it valid to call d_w_r with a serial set to 0 while you in fact expect the [next_id] value to be inserted ?". If no, node_save workflow should be fixed.

@Dries : "What once was a fairly simple function, is now becoming increasingly complex. ;-)"
Well, if the code in there gets more complex to handle db backends subtleties so that I don't have to care about them, that's fine IMO.

Yeah that's fine now.

Committed to HEAD. Thanks!

Yes, it's too bad this function gets more complex, but yched has a good point at #33. It's nice to get this logic encapsulated in one place so it doesn't have to be repeated in every X_save() function. This also helps fix some PostgreSQL failures.