drupal_write_record can't update a column to NULL
yched - February 27, 2008 - 22:25
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | base system |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Dave Reid |
| Status: | reviewed & tested by the community |
| Issue tags: | contrib dependency, XML sitemap |
Description
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.
CCK HEAD currently uses it's own altered version of the function.
Marking as critical, since it's a serious limitation to the function, and also because I'm really reluctant to maintaining a copy of this function in CCK...
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| d_w_r.patch | 1.18 KB | Idle | Failed: Failed to apply patch. | View details | Re-test |
#1
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)) {
#2
True.
#3
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.
<?php
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');
}
}
?>
#4
<?php$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
$fooand test for$result.On the patch:
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 justarray_key_exists($field, $object).isset($object->$field), also fromdrupal_write_record()?<?php// 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
#5
@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?
#6
@vladimir.dolgopolov :
thanks for rolling the test - however, damz is most likely right in the error he points :-)
@damz :
1 -
isset($object->$field) || array_key_exists($field, $object)=array_key_exists($field, $object)True, but array_key_exists() is reported to be *much* slower than a simple isset().
So the patch 1st catches the most frequent case ($object->$field set to an actual value) with a speedy isset, and then catches the edge-case ($object->$field === NULL) with array_key_exists.
I should probably have added a line of comment about that.
2 - Right. My initial thought was that we didn't need to support $object->$field === NULL on 'insert' mode, and so let those lines replace with the default value. But after all, there might be cases where the column has a (non NULL) default value and yet we want to create a record with NULL.
So you're right, these lines should be affected as well.
I'll try to roll a new patch shortly, won't be before a couple days, unfortunately.
@Gabor :
I'm not sure I get what is this thing you call 'the NULL vs isset() vs is_null() issue in Drupal'.
Agreed that d_r_w is built to only deal with caller-provided data. Current problem is precisely that if the caller explicitely sets $object->$type = NULL (instead of omitting it), it *should* be considered as actual and intentional information, instead of overlooked currently. It's the only way I can update NULL into a column that already has a value.
I don't think it's necessary to inspect the schema here. If the caller tries to set a NOT NULL column to NULL, then he is doing something wrong, and d_r_w doesn't need to babysit this more than it babysits the 'omitted NOT NULL column' case you mention.
I'm not sure i answer your concerns here :-)
#7
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...?
#8
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)
<?php
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):
<?php
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
#9
Updated patch, as per damz's remark in #4 (do not replace explicit NULL values with column default on insert).
The patch also avoids sticking 'NULL' in the placeholders aray, as pointed by chx in #7.
After discussion with chx on IRC, the PDO patch will most probably make drupal_write_record obsolete in its current form, so he advises fixing the issue in D6 independantly. I suggest we don't wait until the PDO patch lands in D7 to get this fixed in D6 (so that CCK can remove its current version soon)
Resetting to D6 version - the new patch is against latest D6.
#10
The patch is very clean, and solve the critical issue faced by CCK.
#11
Sorry to bump, but as I wrote earlier, it would be best if this was in when 6.2 is rolled, so that CCK doesn't need it's own separate copy of the function.
#12
I guess this has been reviewed enough. Marking as ready to be considered for inclusion.
#13
Well, this patch removes code after the comment:
// Ignore values for serials when inserting data. Unsupported., which looks like adding serial value support??#14
Sorry for the late answer.
The patch does keep the current behavior for serials : the 'continue' statement will jump the enclosing foreach loop to the next field in the schema, thus overlooking the serial field in the generated query (current code does the same thing for serials in UPDATEs a few lines above). This change simply avoids a superfluous call to array_key_exists() in
if (isset($object->$field) || array_key_exists($field, $object)) {on the next line.New patch simply changes the order of the clauses in:
if (!isset($object->$field) && !array_key_exists($field, $object) && !count($update) && isset($info['default'])) {to
if (!count($update) && isset($info['default']) && !isset($object->$field) && !array_key_exists($field, $object)) {to further reduce the number of calls to the slow array_key_exists().
Really minor change, so I'm setting back to RTBC.
BTW, the whole of drupal_write_record() could be slightly optimized by storing
$is_update = count($update) > 0;once and for all instead of the various checks againstcount($update)in the loop. Since the patch does currently not make things worse or better on this regard, I'd rather not delay it further.#15
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.
#16
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.
#17
This is because of the behavior mentioned in #4 (item 2), added in #9, and that is new to this patch (because current d_w_r overlooks the case of incoming NULL values, and we had to make a choice) :
"On insert, when the incoming $object has an explicit key set to NULL (instead of just not being there), we consider it to be the caller's intent to actually insert NULL, and don't override with the schema's default for this field."
That's the 'insert' counterpart of the 'update' behavior that is the initial point of this patch (explicit NULL means 'update to NULL', not 'keep the current db value'). It keeps d_w_r consistent between insert and update, and it also makes David Strauss's ideal node_save sequence (http://drupal.org/node/230029#comment-850316) possible.
In the new node_save process introduced by http://drupal.org/node/230029, the incoming $node does a NULL nid (set by FAPI #value), whereas the code actually expects the schema default to be inserted.
Attached patch fixes it by simply unsetting the NULL $node->nid before calling _node_save_revision().
The way I see it, we have 3 options :
1) declare this new patch acceptable and check the other places the new 'insert' behavior might break.
Elements to consider IMO :
- The patch only affects an edge case (inserting NULL on columns that have a schema default)
- d_w_r use cases in core are fairly rare yet, so the number of cases to check is minor. I'm currently scanning D6 contrib (that is, modules that have a DRUPAL-6 branch, others can't be considered 'released').
2) Forget about changing the behavior on insert, and just keep the 'update' changes of the patch (which is everything we need to fix CCK)
This leaves us with :
'd_w_r can't insert NULL into a column that has a default',
instead of currently
'd_w_r can't insert NULL into a column that has a default'
AND 'd_w_r can't update any column to NULL'.
A pity because then we precisely can't have the ideal node_save sequence.
It also breaks consistency between 'insert' and 'update' behaviors.
3) Leave d_w_r as is, with CCK and other modules that possibly need to update a column to NULL use their own code (not something I'd vote for, obviously :-) )
#18
Patch does not apply.
About the discussion, there is no doubt that I would go the way #1.
#19
Sorry, I was on 7.x... back to CNR.
#20
This bug bit me today as well. Subscribing.
#21
I'm ready to commit this, the moment we have tests for it. :)
#22
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.
#23
So stop complaining and get busy. This one needs a SimpleTest written. So take the patch in #17 and run with it.
#24
@yched: What's with the node.module change to unset $node->nid?
#25
@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.
#26
@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.
#27
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'?
#28
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.
#29
> 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
#30
#325025: drupal_write_record() fails with NULL object members. was a duplicate.
#31
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...
#32
As a note for anyone writing a patch for this, if you use
array_key_existsto 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.
#33
Or one could write array_key_exists($key, get_object_vars($object));
#34
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.
#35
This is biting me pretty bad too.
#36
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.
#37
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.
#38
Looking good!
#39
Simplified a bit with using
property_exist.#40
This seems to be in order. Best wait for the test-bot to do its thing again, though.
#41
The last submitted patch failed testing.
#42
Reroll against HEAD.
#43
Not correct patch.
#44
Without merge errors.
#45
The last submitted patch failed testing.
#46
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
#47
@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.
#48
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
#49
@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 NULLarray_key_exists($key, $array)is TRUE as soon as there is a $key key (that can be null) in $arraySo 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).
#50
Thanks for the explanation, Damien.
#51
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
#52
Putting this into my todo list for D7.
#53
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().
#54
The last submitted patch failed testing.
#55
New patch with updated tests (due to a change in the schema for the vocabulary table that was used in the tests).
#56
Reroll.
#57
The last submitted patch failed testing.
#58
Reroll.
#59
The last submitted patch failed testing.
#60
Dave Reid requested that failed test be re-tested.
#61
Tagging since XML sitemap module maintains its own copy of drupal_write_record() until this is fixed.
#62
Forgot this tag as well, sorry.
#63
The last submitted patch, , failed testing.
#64
Re-test of from comment #2385142 was requested by @user.
#65
The last submitted patch, , failed testing.
#66
Reroll.
#68
Looks great and love the fact that we're using the dbtest tables instead of taxonomy_vocabulary.