drupal_write_record can't update a column to NULL

I didn't apply this patch and look at it in situ, but the indentation of this comment seems off:

+      // Build arrays for the fields, placeholders, and values in our query.
+    if (isset($object->$field) || array_key_exists($field, $object)) {

There is a test for Simpletest module for the issue.
But the patch #2 hasn't passed it.
It looks like the test maybe wrong and I'd missed something.

class TestCase227677 extends DrupalTestCase {
  function get_info() {
    return array(
      'name' => t('[227677] drupal_write_record can\'t update a column to NULL'),
      'desc' => t('In update mode, drupal_write_record skips entries set to NULL, and thus leaves the previous value in place. There\'s no way to set a column back to NULL.'),
      'group' => t('Drupal 7 Tests'),
    );
  }
  
  function testIssue() {

    // test on scheme 'users'
    $user = $this->drupalCreateUserRolePerm();
    $data = array($this->randomName() => $this->randomName());
    
    $user->data = $data; // 'data' field can be NULL

    drupal_write_record('users', $user, 'uid');
    $result = db_result(db_query("SELECT data FROM {users} WHERE uid = %d", $user->uid));
    $this->assertTrue(!empty($result), 'Check not empty field');

    $user->data = null;

    drupal_write_record('users', $user, 'uid');
    $foo = db_result(db_query("SELECT data FROM {users} WHERE uid = %d", $user->uid));
    $this->assertTrue(empty($result), 'Check empty field');

  }

}

    $foo = db_result(db_query("SELECT data FROM {users} WHERE uid = %d", $user->uid));
    $this->assertTrue(empty($result), 'Check empty field');

Looks weird, you set $foo and test for $result.

On the patch:

1. isset($object->$field) || array_key_exists($field, $object) (either "the key is set to something not null" or "the key is set") is strictly equivalent to just array_key_exists($field, $object).
2. What do we do about that isset($object->$field), also from drupal_write_record()?
   

       // For inserts, populate defaults from Schema if not already provided
       if (!isset($object->$field) && !count($update) && isset($info['default'])) {
         $object->$field = $info['default'];
       }
   

   Shouldn't the !isset($object->$field) also be !array_key_exists($field, $object)?

Damien

@yched, this seems like the NULL vs isset() vs is_null() issue in Drupal. drupal_write_record() is IMHO written to ensure to only deal with data provided by the caller, and it requires NOT NULL fields be set to '' for example instead of being omitted. Your code might fail there as it does not inspect the schema on non-null fields. Or am I mistaken somewhere?

I have no idea what isset vs is_null could be 'cos !isset and is_null is the same. Next up, the current database layer simply does not support NULLs, however http://drupal.org/node/225450 does so I would say, let's fix this once that one is in. Putting a NULL into the placeholder IMO is not nice. In the unlikely case the new database layer is not committed, we can still come back to this patch, there is plenty of time in this cycle. I do not want to derail an issue, blocking it with such a huge patch, but really, there is still time. The question is, do we need to fix this in Drupal 6...?

Re-rolled test case for the issue (thanks damz for the point)
BTW, the patch hasn't worked against HEAD (diff -u -r1.756.2.5 common.inc, but current is 1.756.2.8)

class TestCase227677 extends DrupalTestCase {
  function get_info() {
    return array(
      'name' => t('[227677] drupal_write_record can\'t update a column to NULL'),
      'desc' => t('In update mode, drupal_write_record skips entries set to NULL, and thus leaves the previous value in place. There\'s no way to set a column back to NULL.'),
      'group' => t('Drupal 7 Tests'),
    );
  }
 
  function testIssue() {

    // Test on scheme 'users', 'data' field can be NULL
    $user = $this->drupalCreateUserRolePerm();
    // sample 'data' for $user->data
    $data = array($this->randomName() => $this->randomName());
   
    $user->data = $data;

    // Sould pass so 'data' is not empty
    drupal_write_record('users', $user, 'uid');
    $result = db_result(db_query("SELECT data FROM {users} WHERE uid = %d", $user->uid));
    $this->assertTrue(!empty($result), 'Check not empty field');

    $user->data = null;

    // Sould pass so 'data' is NULL
    drupal_write_record('users', $user, 'uid');
    $result = db_result(db_query("SELECT data FROM {users} WHERE uid = %d", $user->uid));
    $this->assertTrue(empty($result), 'Check empty field');

  }

}

@chx: !isset and is_null is not the same (there will be a notice):

ini_set('error_reporting', E_ALL);

$obj = new stdClass();

$obj->foo = 'foo';
$obj->bar = 'bar';

unset($obj->foo);
$obj->bar = null;

if (is_null($obj->foo)) echo("foo is NULL\n");
if (is_null($obj->bar)) echo("bar is NULL\n");
if (!isset($obj->foo))  echo("foo does not set\n");
if (!isset($obj->bar))  echo("bar does not set\n");

produces:

<br />
<b>Notice</b>:  Undefined property:  foo in <b>sample.php</b> on line <b>13</b><br />
foo is NULL
bar is NULL
foo does not set
bar does not set

The patch is very clean, and solve the critical issue faced by CCK.

I guess this has been reviewed enough. Marking as ready to be considered for inclusion.

Well, this patch removes code after the comment: // Ignore values for serials when inserting data. Unsupported., which looks like adding serial value support??

Thanks, committed to Drupal 6. The PDO patch did not land yet, so I am moving to D7 to RTBC, and Dries will decide about it.

This caused the following errors on simple node submissions (eg. page node):

Column 'vid' cannot be null query: INSERT INTO node (vid, type, language, title, uid, status, created, changed, comment, promote, moderate, sticky, tnid, translate) VALUES (NULL, 'page', '', 'dfskld lkjdflkdsjflkj', 1, 1, 1214391049, 1214391049, 0, 0, 0, 0, 0, 0) in common.inc on line 3308.

Column 'nid' cannot be null query: INSERT INTO node_revisions (nid, uid, title, body, teaser, log, timestamp, format) VALUES (NULL, 1, 'dfskld lkjdflkdsjflkj', 'sdlkdflkj dsflkjd slkfjds lkjdsf l', 'sdlkdflkj dsflkjd slkfjds lkjdsf l', '', 1214391049, 1) in common.inc on line 3308.

So I rolled it back. I suspect this was due to the recently committed http://drupal.org/node/230029 which I think is a more important issue to fix now, so I kept that in and rolled this one back.

Patch does not apply.

About the discussion, there is no doubt that I would go the way #1.

Sorry, I was on 7.x... back to CNR.

This bug bit me today as well. Subscribing.

I'm ready to commit this, the moment we have tests for it. :)

Just wanted to replace stupid hand crafted save_profiie function in signwriter module as i just now heard of the existence of d_w_r... well not doing NULL field updates is uncool but not the end of the world after all. Still, the amount of time it costs the drupal community to resolve minor code issues like this is astonishingly long (judging from code complexity). Either the massively parallelled issue review/ patch queueing process is fundamentally inefficient, or the core team of skilled committed developers is too small...
Edit: that said, being on the WINE devel/patches lists since last year's soc, that process seems a lot more fluent. Patches either get in at once (mostly within 48 hours) or are silently (that has improved *g) dropped by Alexandre. Dan has also implemented a thing called patchwatcher that automatically checks patches for basic saneness (compiling & no test failures)...
When i look to the right, D7 Patch queue has 1064 items. I so totally do not understand what the plan is to ever bring this down, it just keeps raising. Hardly do i even want to imagine how huge the PITA must be checking all these against collisions.

@earnie well i didn't mean to just complain, i am really interested if the core devs are consciously aware of the issue i wrote about, or if all is on track and it is just my distorted perception.
And simpletest, i have not yet looked into that and am busy fixing other people's modules right now to get hfopi.org ready for launch on thursday, so sorry, won't do.

@eMPee584: there's no such thing as "core devs". There are core committers (Dries and webchick right now) and there are people who work on core when they have the time. Drupal is a meritocracy. If you need some feature, you'll have to work on it. *Everybody* has got some site to launch. ;) :)
That's why some things go in very fast and some things take a lot of time: it depends on the people working on the patch. yched is probably too busy or has lost interest. Either way, he, nor anybody else can be blamed for being slow. "Scratch your own itch" is the motto.
You can help get this patch in by writing the corresponding test.

Lols hey wim i *know* how this works ;)
Fact is, many simple fixes like this one rot in the queue either not being reviewed or not being applied because noone dares to just commit... and i am itching my own scratch (very much so ^^) but wonder if this can not be made any more efficient. imho, faster commit times not only for core but also for contrib modules would be a worthy improvement as some maintainers do not live up to the 'be a responsible maintainer' standards (for whatever reason, no offence intended) and the - well, core committers have a very conservative committing policy (not necessarily the wrong strategy). I know patch reviewing is one huge bottleneck, i myself have posted one or two core patches, and it is quite annoying not to see twentyfive people test and approve them within a week (in fact, to the poll module patch there was not a single response yet).. but then again i did not yet review any core patches myself because i am running D6 only atm and have yet to set up a D7 (i tried once but my other host does not fulfill the requirements).
But PHP is a nice and simple language, and even though some of the drupal code flow is quite tricky to figure, many issues of this kind maybe could be solved quicker by... well how?
Is the whole process plan for D7 atm really just 'get simpletest done, then finish D7'?

Fair enough. What you say is true: many simple things that ideally should go in fast, aren't. And indeed the main bottleneck is the lack of (incentive for?) patch reviews.
Discussion/thoughts on how to make this more efficient don't belong here though. We've spammed this issue enough as it is. Continue in the appropriate g.d.o group :) Let's make my reply the final off-topic comment, shall we? ;) :)

Fact is that to avoid regressions, the new rule starting with D7 core is that we only commit patches to core that have a test case. That's what preventing this patch from being committed.
Therefore I'm marking this as CNW, to give it the proper attention.

> Let's make my reply the final off-topic comment, shall we?
So be it :)
i guess webchick is the fix anyways. and that commit policy does sound sound, however it means that this also applies to fixes for D6 which need to go into D7 first, so *some people* need to have a first look at simpletests n stuff *g

#325025: drupal_write_record() fails with NULL object members. was a duplicate.

This issue affects Ubercart during code porting to 6 to use this feature. I am currently working around it, but could we please get the test and patch in there? Thanks...

As a note for anyone writing a patch for this, if you use array_key_exists to check the property on the object, then this code will no longer work in PHP 5.3.

The alternative is to use property_exists, however this requires PHP 5.1.

This should be fine for D7, but any D6 versions will need to decide which approach to take, which will then effectively fix D6 to require either PHP < 5.3 or >= 5.1 to work.

Seems like a tough call if this patch does make it in.

It's amazing how there's so little activity around this patch. It's probably because few people know of drupal_write_record() *and* because most Drupal devs don't use NULL in their modules' db tables.

I'll try to write the test, but it won't be any time soon.

This is biting me pretty bad too.

I know this patch still needs work, because I'm not sure where to create a new test for this. Should it go in includes/tests ? I noticed that at least in my CVS, this directory is still empty. I'd appreciate some guidance on this one. =)

Here's the patch so far though, incorporating the idea by earnie to use get_object_vars(). I'm really not sure how this does performance-wise, but it should keep us PHP-compatible across the board.

We actually have a basic test already for drupal_write_record in modules/simpletest/tests/system.test ("Data API functions" test). I expanded on that test with fields that can and cannot be NULL. I found a few bugs in the process and everything seems to be working perfectly. Now to let the test bot run the suite and report back.

I rolled back my changes to drupal_write_record() but left the test changes and the only test failure was setting a field to NULL that could be nullable, so the test seems good.

Looking good!

Simplified a bit with using property_exist.

This seems to be in order. Best wait for the test-bot to do its thing again, though.

Reroll against HEAD.

Not correct patch.

Without merge errors.

I believe setting NULLs on Updates by CCK is the source of those problems: http://drupal.org/node/368956 and http://drupal.org/node/486206

I solved this issue on my site by ignoring NULL fields on Update http://drupal.org/node/368956#comment-1935600 that is reverting back to the drupal_write_record way (if I understand correctly).

Cheers,
Jakub

@zaczek: content_write_record() is the function that was implemented to workaround the limitations of drupal_write_record() described in this issue. The CCK version do *not* set NULL on a field that is not provided, it sets NULL only when passed explicitly NULL.

Thanks, Damien for your reply.

I must have misunderstood this piece of code starting in line 1172:

   if (isset($object->$field)) {
        $placeholders[] = db_type_placeholder($info['type']);
        ...
      }
      else {
       $placeholders[] = 'NULL';
      }

The object reference is the data passed to the function. I understood that if the isset($object->$field) condition is not fulfilled, that would mean that no data is provided? Then, the placeholder with a NULL value is inserted.

For a value set to NULL, shouldn't we test with $object->$field===NULL ?

Sorry if I sound naive - I'm quite new to this. Thanks for your help.

Cheers,
Jakub

@zaczek: the chunk of code you pasted is inside a isset($object->$field) || array_key_exists($field, $object) condition. And:

• isset($value) is TRUE if $value is anything but NULL
• array_key_exists($key, $array) is TRUE as soon as there is a $key key (that can be null) in $array

So as soon as you are in this condition, you are guaranteed that $object->$field is defined (but it can be NULL). In the case it is NULL, content_write_record() uses an explicit SQL 'NULL' in the query.

If $object->$field was not defined, you would not match the condition in the first place, and content_write_record() simply ignore the field (in case of an update) or uses its default value (in case of an insert).

Thanks for the explanation, Damien.

I have now found a proper solution for Media Mover, without the need to change CCK: http://drupal.org/node/486206#comment-1942328

Thanks,
Jakub

Putting this into my todo list for D7.

Updated patch.

I think the existing code is very hard to follow. Because of the way e.g. $fields[$field] is initialized inside an if block, it is difficult to know whether it has been initialized further down. Likewise with $object->$field.

In this patch I first initialize $object->$field or escape the current foreach iteration (using continue), so for the rest of the iteration we are sure that is is initialized. Likewise, if we get this far, we always initialize $fields[$field].

With the previous patch, a NULL value was simply ignored if the column did not allow it. I think it makes more sense to cast it to the expected value, just like e.g. FALSE is.

The comment about casting was inspired by #299088: [DBTNG]: simplify drupal_write_record() with db_insert() and db_update().

New patch with updated tests (due to a change in the schema for the vocabulary table that was used in the tests).

Reroll.

Reroll.

Tagging since XML sitemap module maintains its own copy of drupal_write_record() until this is fixed.

Forgot this tag as well, sorry.

Reroll.

Looks great and love the fact that we're using the dbtest tables instead of taxonomy_vocabulary.

Cool, I'm glad to see movement on this (painful) issue. Am I correct thinking we missed the window for D7 with this? Should we bump it up to 8 (Though that makes me sad:P)

No. It's a bug that does not change APIs and should be backported as well.

Thanks for the delicious tests! Committed to HEAD.

Marking 6.x to be ported.

If I want to patch the D6 version, do I do that here or do I open a new issue?

Attaching D6 version. Not sure the best way for handling array_key_exists/parameter exists problem... suggestions?

Updating to the correct version.

What could I do to help get this to RTBC? I'd like to review, but am not sure what would be required.

Trying on 6.17, sadly without succes...
Back to dev?

Tried the patch and it doesn't seem to work on D6.

Running into this issue as well, where we have columns that can be NULL in a custom module. As is, it means we either can't use drupal_write_record() or have to delete the row and do an insert.

I've stumbled on this issue as well.
This is how I fixed it for my drupal 6 installations (patch should apply on both 6.19 and 6.20).

Since property_exists() only works for PHP >= 5.1.0, people using older versions of PHP might want to implement the missing function like this:
http://www.php.net/manual/en/function.property-exists.php#85359

Our requirements page says:

Recommended: PHP 5.2 for Drupal 5 and 6, PHP 5.3 for Drupal 7
Required: PHP version 4.4.0 or higher for Drupal 5 and Drupal 6, PHP 5.2.5 or higher for Drupal 7

So that means we probably have to add a drupal_property_exists() to cover older versions.

Here's a patch based off of #79 that adds drupal_property_exists() and fixes a minor comment style issue.

Works great.

Thanks for the review. It'd be great if one other person could review and mark RTBC if they don't have any improvements or issues with the patch in #81.

Tested #81 on installation with many Data supported custom tables with NULL values enabled. Works well! Thanks a lot!

#84 is one more review and sounds like a fairly thorough one too. I'd say this is RTBC.

Does D8 have the same problem?

Git merge-base tells me that 8.x branched off 7.x at:

commit f1ba363a8c3b5185e4e663bd2c3b7eb7b0fb9983
Author: webchick
Date: Mon Mar 21 11:34:00 2011 -0700

Webchick committed a patch further up this issue on:

Posted by webchick on January 8, 2010 at 6:07am

That being prior to the branching, I'd say it's fixed in D8 :)

Re-roll of #81 such that it applies cleanly to d-6.25.

Re-roll of #81 such that it applies cleanly to d-6.25. (fixed)

Let's see if the latest patch passes tests...

This was RTBC a long time ago, just got out of date. Then nobody got the testbot working on the reroll.

Thankyou for this patch! Brilliant work

> @Dave Reid Did this patch ever get into D7?

It would seem so:

commit e07b9d35a1f4dcb1678c4d3bb6482daaebea6350
Author: Angie Byron <webchick@24967.no-reply.drupal.org>
Date:   Fri Jan 8 06:07:03 2010 +0000

    #227677 by c960657, yched, cha0s, Dave Reid, et al: Fixed drupal_write_record() can't up

which git describe says is:

> 7.0-unstable-10-531-ge07b9d3

However, I think I am still seeing this problem in D7. Will investigate. My mistake; I am thinking of a slightly different problem.

Re-roll of #81 that applies to 6.x-dev.

Works for me in a 6.34 site.

Hi,

I'm seeing this issue in a 6.37 site.

From the conversation it looks like the patch has passed tests in the 6.x branch.
I'm confused as to whether the patch has been applied or not.

It has not been applied to D6. Those of us who have run into the bug have applied the patch to our own sites.