For some (unknown) reasons, {node_revisions}.body, {node_revisions}.teaser and {node_revisions}.log does not have a default value in the current schema. Thus, node_save() have to ensure that these fields are here on inserting a new row. If it does not, the row could be rejected by the database engine (PostgreSQL rejects the row in all cases, and some MySQL configurations do too).

But the current logic in node_save() is flawed: it only add required field when a new node is created, and not when a new *revision* is created.

I have identified two critical bugs that are directly linked to this bug:

Marking this issue as critical, because it is needed to solve critical issues. Patch enclosed. Please review.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

David_Rothstein’s picture

I looked through this and the code makes sense. (If I'm not mistaken, though, logically speaking this patch introduces a slight change in the conditions under which $node->old_vid = $node->vid; gets set? However, I'm not sure it has a practical effect.)

A couple quick comments, though:

First, why not just move the logic for checking whether these fields are set outside of the if statement altogether and into the main body of node_save()? If we check it every time node_save() is called, then bugs like this will never pop up again...

Second, I did a little research and it looks like some of the original motivation for this code is at #188462: null value in column "log" violates not-null constraint and #195169: NOT NULL fields need default value. Seems like these columns don't have default values in the schema because MySQL doesn't support that for text fields (?). However, based on what is said in those issues, as well in as the code comments, it sounds like an appropriate fix (for D7, at least) is just to change the schema so those fields are allowed to be NULL. Maybe that would be a better solution to this problem?

Damien Tournoud’s picture

FileSize
1.18 KB

Thanks for the review, David. Here is a new patch that should address most of your concerns.

All node tests pass before and after the patch, which also fix an E_ALL exception when $node->vid is not set when creating a new revision (such as when testing revision creation in node.test).

First, why not just move the logic for checking whether these fields are set outside of the if statement altogether and into the main body of node_save()?

I took some time to think about that, and it seems like a good idea (implemented in the patch). The log field still requires special treatment.

It sounds like an appropriate fix (for D7, at least) is just to change the schema so those fields are allowed to be NULL. Maybe that would be a better solution to this problem?

It should be for D7, but we need a fix for D6 urgently: the book module is completly broken of PostgreSQL, while the poll module does not play well with revisions... My plan is to first fix the current logic (both in D7 and D6), and only then think of a better solution.

David_Rothstein’s picture

Status: Needs review » Needs work

Sounds like a reasonable plan to me.

Only problem is that the second patch you attached is actually the exact same as the first one ;)

Damien Tournoud’s picture

Status: Needs work » Needs review
FileSize
2.62 KB

Correct, sorry, this is the good one.

David_Rothstein’s picture

This looks good to me.

I attach a revised version that fixes a spelling error in the code comments and also slightly reorganizes the logic to change an empty() to a !isset(). This is in keeping with the previous behavior and is more technically correct for the (very obscure edge) case where $node->log = '0', I believe.... Plus, it makes the code easier to understand.

I also confirmed that all node tests pass both before and after this patch.

I'm tempted to just mark this RTBC since the changes I introduced to the patch were tiny and it's such a critical fix, but I will leave it as is for at least a short while...

R.Muilwijk’s picture

Status: Needs review » Needs work

I think the patch is reasonable and it doesn't break the tests.

However to prove this patch.... can you write a test that shows the bug in the old system which should be fixed by applying this patch?

VladSavitsky’s picture

I have the same bug in 6.2 too.

Damien Tournoud’s picture

Status: Needs work » Needs review

@R.Muiwijk: there is a perfectly working test in #223820: Book module tests for postgres error. It may require a reroll, thou.

R.Muilwijk’s picture

Status: Needs review » Needs work

Patch should be added to the node tests not the book tests because it is a general node fix.

erle’s picture

Just wanted to know if anyone has rolled a patch against 6.x . I need to apply this for a project now. I will go ahead and roll a patch against 6.4 using this if not (since this fixes a lot more that mine)

* I had hit this same bug in 6.x, I had rolled a very simple patch for the case where body is null. That issue is marked as a duplicate here #296615: revision fails when body field is null (that patch for 6.4 is in there)

Damien Tournoud’s picture

Status: Needs work » Needs review
FileSize
1.34 KB
2.22 KB

Rerolled to correct some fuzz.

Added a unit non-regression test.

webchick’s picture

Adding this to my todo list.

catch’s picture

Status: Needs review » Reviewed & tested by the community

The regression test is pretty self documenting, fails without the patch, passes with it. I've read through both the patch and the discussion, and this seems fine to me. We should get it into 7 and 6, then look at actually allowing NULL values for those fields for 7 (which is in the TODO).

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Hm. I spent a long time looking at this, comparing it to node_save before the patch. It made my brain hurt. Since this is the 60,000th time or so that we've adjusted the logic in this function, and since it took me 35 minutes to grok what was going on in this patch (and I still am not totally sure), I think we could stand to document this part of the code a little bit more. I realize some of these things were not changed by your patch, but it would really help to make sure that these tweaky bits are not broken again in the future.

  // Insert a new node.
  $node->is_new = empty($node->nid);

This comment is wrong; we are not actually inserting a new node.

  if ($node->is_new || !empty($node->revision)) {

Isn't $node->revision a binary flag indicating whether revisions are turned on for this node type or not? If so, can we use something like if $node->revision == 1 instead? Or at least document what exactly we're checking here? This line as-is is pretty confusing.

    // When inserting a node, $node->log must be set because
    // {node_revisions}.log does not (and cannot) have a default
    // value. 

Really? Why can it not?

    // value.  If the user does not have permission to create
...
  // $node->body.  We should consider making these fields nullable

(minor) two spaces before If and We.

  elseif (empty($node->log)) {
    // When updating a node, however, avoid clobbering an existing
    // log entry with an empty one.
    unset($node->log);
  }

Clobbering an existing log entry? What does that mean? If the node log is already empty, why do we need to unset it? Didn't we just get done saying that $node->log /had/ to be set?

We should consider making these fields nullable
    // in a future version since node types are not required to use them.

Let's call that out as a specific TODO. (and aim to fix it in D7) Probably also true of {node_revisions}.log, no?

  // Save the old revision if needed.

How do I know it's needed?

  // Generate the node table query and the node_revisions table query.

This comment is wrong, too. We don't seem to be generating a query at all, but rather actually saving the node at this point. I'd also like to see at least a one-liner comment on each one of those logic branches there.

Then, a review of the .test:

+   * Regression test for #261258.

I'm not a fan of this kind of PHPDoc. I'd prefer it to actually describe what's being tested.

+   * Checks that it is possible to create a revision without all fields being set.

It seems to me there are specific fields (log, body, and teaser) that are deliberately not being set. We should make it clear what those fields are and why that is. I'm fine if these comments go down below above the + $updated_node = (object) array( line though, since that bit is especially puzzling and can use some explanation.

Pancho’s picture

I came across this issue while re-doing the node_revisions split. Getting this here fixed would be awesome!

Damien Tournoud’s picture

#353301: {node}.body and {node}.teaser should be nullable is related for D7. This now actually fails on MySQL on Drupal 7, because we started using MySQL in a very strict mode.

Damien Tournoud’s picture

pwolanin’s picture

subscribe

Freso’s picture

Eric_A’s picture

Thanks to Damien Tournoud I arrived here after posting in this duplicate: #434562: Node revision does not get created when BODY field is omitted or left blank

I feel this critical issue can be resolved quickly in a very simple way and that perhaps too many things are being dealt with in the current patches. If you ask me, the simplest and clearest way to fix this bug is to copy the existing code for new nodes (see below) and paste it ("When inserting a node" => "When inserting a node revision") just before the second _node_save_revision($node, $user->uid), where new revisions are being dealt with.

Sure, node_save() is not getting prettier, but this does fix a critical issue for D7 and D6. The code duplication and other stuff can be dealt with in separate issues. I suspect that drupal_write_record(), it having knowledge of the schema, is a better place to detect and fix illegal INSERTs but we can deal with that and everything else after the bug is actually fixed.

    // When inserting a node, $node->log must be set because
    // {node_revision}.log does not (and cannot) have a default
    // value. If the user does not have permission to create
    // revisions, however, the form will not contain an element for
    // log so $node->log will be unset at this point.
    if (!isset($node->log)) {
      $node->log = '';
    }

    // For the same reasons, make sure we have $node->teaser and
    // $node->body. We should consider making these fields nullable
    // in a future version since node types are not required to use them.
    if (!isset($node->teaser)) {
      $node->teaser = '';
    }
    if (!isset($node->body)) {
      $node->body = '';
    }
jbeall’s picture

I was having trouble with the revisioning module try to insert a NULL value into the node_revisions table. The one-liner fix I figured out is as as follows (it adds a default value of an empty string to the node_revisions.body field):

Open modules/node/node.install and change it from this this (starting on line 213):

'body' => array(
'description' => 'The body of this version.',
'type' => 'text',
'not null' => TRUE,
'size' => 'big'),

To this:

'body' => array(
'description' => 'The body of this version.',
'type' => 'text',
'not null' => TRUE,
'size' => 'big',
'default' => ''),

You can see that I added a "default" key with the value of an empty string.

I got here from bug #434562: Node revision does not get created when BODY field is omitted or left blank

webchick’s picture

iirc you can't give text fields default values. it breaks either in pgsql or mysql strict.

jbeall’s picture

webcheck -- you're right, you can't give them a default value in the table's DDL statements (e.g., CREATE TABLE...) that give the column specifications. But you can do it here no problem, this is all inside PHP and will affect the construction of the INSERT statement. I explained that further in my comments on #434562: Node revision does not get created when BODY field is omitted or left blank.

Damien Tournoud’s picture

@jbeall: and you have tried reinstalling? ;)

jbeall’s picture

Yes, actually. I apply the fix after installation, though. I'm assuming it would cause installation problems.

graper’s picture

While I know that this topic is marked for 7.x, this issue is in 6.14, and unfortunately means that the revision system is broke. I recently turned on revisions on a 6.18 version of drupal just to find that when I edited a node that was made with CCK, it then set the revision to 0 and and wasn't able to update the node_revisions table.

in my search, I not only found this issue, but another one that made a great suggestion of making body and teaser field changes to allow for NULL values.

#353301: {node}.body and {node}.teaser should be nullable is the issue and, while I didn't use the patch I was able to change the node.install and node.module properly and then made changes to the DB to allow nulls for those fields and everything is working fine. Would that not work for the next version of 6, 7 and head?

beauz’s picture

subscribing as I have this issue as well...

crea’s picture

subscribing... This is critical since it affects 6.x

markus_petrux’s picture

Just marked an issue in the CCK queue as a duplicate of this one. #599244: CCK node without 'body' lost after creating a new revision / WSOD.

I have never seen this error, though. I guess it depends on something related to MySQL.

hass’s picture

Very bad... my D6 CCK node without a 'body' is no cluttered, no more editable after I checked "create new revision"... need to investigate how to fix the MySQL DB inconsistencies on DB level :-(

@markus_petrux: You may need to enable MySQL strict mode...

hass’s picture

Patch in #11 works for me in D6.

hass’s picture

This is really CRITICAL and I do not understand why this need to wait 1.5 years. Marking as blocker to get this fixed.

dogbertdp’s picture

I am experiencing this issue as well. Here is the run down:

I am running Drupal 6.13 and php 5.2.

  1. A user creates a node from a content type that has no body field.
  2. The user returns to edit the node.
  3. The user makes changes, but when the user saves the node, it apparently all but vanishes.

In the database there are references to the node and there are errors in Recent Logs. Views still "find" the node, but when you click on a link to the node or try to get there directly using what should be the URL you get a Drupal 404.

The following is logged in the recent logs:

Type: php
Message: Field 'body' doesn't have a default value query: INSERT INTO node_revisions (nid, uid, title, teaser, log, timestamp, format) VALUES (68, 28, 'Node Title Goes Here', '', '', 1256006161, 0) in [path]/apache/htdocs/includes/common.inc on line 3436.
Severity: error

Type: php
Message: Column 'vid' cannot be null query: UPDATE content_type_sop SET vid = NULL, nid = 68, field_sop_instructions_value = '<ol><li>Node Title Goes Here</li><li>Node Content Goes Here</li></ol>', field_sop_prereqs_format = 1, field_sop_purpose_value = '<p>Node Title Goes Here</p>', field_sop_purpose_format = 1, field_sop_scope_value = '<p>More Node Content Goes Here</p>', field_sop_scope_format = 1 WHERE vid = 0 in [path]/apache/htdocs/sites/all/modules/cck/content.module on line 1213.
Severity: error

The taxonomy module will also now throw an error similar to the following on each cron run:

Type: php
Message: Invalid argument supplied for foreach() in D:\SRVApps\itdoc-prod\apache\htdocs\modules\taxonomy\taxonomy.module on line 1214.
Severity: error

If I go into the "node_revisions" table in my database, grab the `vid` for the node id (`nid`) in question, and then edit the "node" table by changing the `vid` for the `nid` in question from "0" to the value I grabbed, I can resurrect the node. But the next time the node is edited, it returns the node to the previous orphaned state, and the changes are lost.

There are several suggestions above for correcting this issue, but I don't want to hack core in a way that will be overwritten on upgrade.

Can someone tell me the fix that will make it into core, and can we get a fix for this in the next version after 6.14?

Thanks,
Mike Hays

Update: I "resurrected" an affected node as I explain above on my test server, and applied the patch in #11 above and it resolved this issue. I will do some testing to see if it causes any other issues.

Damien Tournoud’s picture

This tag is for the branch maintainer only.

hass’s picture

Do we need to wait another year for a patch that works well? Not sure why the D6 patch needs work. For me it works well.

markuskb’s picture

subscribing. Can't believe this issue is going on for 1.5 years now...

Damien Tournoud’s picture

To all the subscribers since #14: if nobody rolls a patch taking into account webchick's review in #14, this will never be fixed. Ahoy!

pwolanin’s picture

hey DamZ - I'll see if I can make time for a re-roll. Is Mikkel pulling your fixed core git repo yet?

pwolanin’s picture

patch in #11 fails to apply for me.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
1.74 KB

here is a re-roll of the DamZ patch from #11 - setting to CNR just for the testbot, it does not yet incorporate suggestions from webchick.

also - The patch from #5 does apply cleanly to D6 with just some offset.

iwkse’s picture

I'm attaching the same patch for D6 applying smoothly

iwkse’s picture

pwolanin’s picture

ok, what the hell - this code is so broken in 6 and 7:

       global $user;
 ...

      _node_save_revision($node, $user->uid, 'vid');

why do we force the revision to be saved with the uid of the current logged in user?! Isn't this an API function?

pwolanin’s picture

ok, re: webchick's comments. I've revised the doxygen. however, we *should* be checking $node->revision as empty() though, since we have no assurance whether or not it is set. I don't think == 1 is correct for that reason.

Also, the recent patch to allow is_new to be passed in is a bit dangerous since I can both set that true AND pass in a node with $node->nid set to an existing nid.

pwolanin’s picture

Eric_A’s picture

This looks good.
When commenting in #20 I had misread parts of the original patches. But it's actually a clean version of what I was aiming at.
I've done no actual testing, though, just looking at the code.

pwolanin’s picture

Combined D7 patch with DamZ's test from #11. I needed to rewrite the test to account for the node title being a field.

eft’s picture

I feel like I've entered Drupal's sanctum sanctorum but.... could anyone help a relative newbie understand how best to fix this in D6? Is the patch applicable to both D7 and D6 and, if not, how should I fix D6 i.e. Do I need to edit core code, turn off strict mode in MySQL etc.

pwolanin’s picture

#42 should work for D6

eft’s picture

thanks a lot - will try it out

[Edit] Works great - too bad this wasn't included in D6.15

pwolanin’s picture

@eft - please post more details of your testing so we can get this in the next release.

eft’s picture

sure - what kind of details would you like?

pwolanin’s picture

Details of how you observed the bug before and how you tested/confirmed that it was resolved.

David_Rothstein’s picture

Updated patch:

  • Changes FIELD_LANGUAGE_NONE to LANGUAGE_NONE
  • Gets rid of the $node->body and $node->teaser parts (I don't believe that's relevant anymore?)
  • Expands the test a bit
  • Adds more code comments to finish addressing @webchick's points in #14
pwolanin’s picture

@David - ok, well we'll still need body and teaser for the D6 version

David_Rothstein’s picture

Indeed, that's true.

Attached patch has a couple very minor fixes to the test.

pwolanin’s picture

joshmiller’s picture

Status: Needs review » Reviewed & tested by the community

Testing has been turned back on. Bot is happy. Code has tests. RTBC.

Josh

webchick’s picture

Version: 7.x-dev » 6.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Wow, great job on this, folks!

I'm sure it's only a matter of hours before someone yells that this change broke some other weird edge case they were depending on, but now we have tests they can expand to prove it. ;)

Committed to HEAD! Looks like this applies to 6.x as well?

babbage’s picture

Title: Fix node_save() insertion logic » [TESTING BROKEN] Fix node_save() insertion logic
Version: 6.x-dev » 7.x-dev
Status: Patch (to be ported) » Needs work

HEAD is broken as a result of this patch. :(

babbage’s picture

Title: Fix node_save() insertion logic » [TESTING BROKEN] Fix node_save() insertion logic
Version: 6.x-dev » 7.x-dev
Status: Patch (to be ported) » Needs review
FileSize
6.9 KB

Attached patch corrects the two tests in the patch that broke HEAD, which hadn't taken account of #571654: Revert node titles as fields.

EDIT: This presumably won't apply as it assumes the committed patch above was rolled back first.

webchick’s picture

Version: 7.x-dev » 6.x-dev
Status: Needs work » Patch (to be ported)

Oh, crap. :( Sorry. :( I didn't read the date on the patch closely enough. My kingdom for #635334: Patches are not being re-tested automatically to be deployed. :(

Committed to HEAD. THANK YOU!

catch’s picture

Title: [TESTING BROKEN] Fix node_save() insertion logic » Fix node_save() insertion logic
madlee’s picture

Title: [TESTING BROKEN] Fix node_save() insertion logic » Fix node_save() insertion logic
Version: 7.x-dev » 6.x-dev
Status: Needs review » Patch (to be ported)

#42: This worked for me in 6.14. Thanks!

http://drupal.org/node/261258#comment-2276860

ealtman’s picture

Hi -- I'm running 6.15, and tried to apply the #42 patch, but got this command-line output :

patch: **** malformed patch at line 70: // Set some required fields:

It looks like something's missing from this version. Will one of the other versions work with 6.15?

ealtman’s picture

I still have not applied any patch. In 6.15 I have found this (failure to insert a new row in node_revisions) only to be problem for the book page node type and only when the author has been reassigned. The workaround was to turn on "create new revision" for the book page node type, which we otherwise have turned off. Not sure if this is a bug or a best practices issue. I will post elsewhere.

David_Rothstein’s picture

It looks like there was an inadvertent change introduced in the patch in #62 that caused one of the new node module tests to not actually test what it was supposed to...

Since this issue is now for backporting the (critical) bugfix to D6, I've posted a fix for the test elsewhere: #700862: Test for empty node log messages doesn't actually test what it's supposed to

gost_gost’s picture

Samme error about malformed line when applying patch in 6.15
tried applying with --verbose --binary
on both Gnu/Linux and Window$ machines.
Fix needed please!

kehan’s picture

subscribing

pwolanin’s picture

Status: Patch (to be ported) » Needs review
FileSize
2.59 KB

Reroll and minor tweaks of the patch at #5 (essentially the same as #11 also).

andypost’s picture

Status: Needs review » Reviewed & tested by the community

Patch works fine!!!

It was hard to check this! This require mysql server in strict mode http://dev.mysql.com/doc/refman/5.0/en/server-sql-mode.html

1) enable strict mode on mysql
2) when editing node, check "Create new revision" and leave log message empty
3) hit Save

You got "Page not found" with following

user warning: Field 'body' doesn't have a default value query: INSERT INTO node_revisions (nid, uid, title, teaser, log, timestamp, format) VALUES (3, 1, '1234', '', '', 1267476654, 0) in /var/www/d6cvs/includes/common.inc on line 3467.
notice: Undefined property: stdClass::$vid in /var/www/d6cvs/modules/node/node.module on line 925.

also {node} table will contain record {nid=NODE_NUMBER vid=0} - so you lost information about current revision and only one possible way to fix this

update {node} set vid=nid where vid=0
brenda003’s picture

Patch works good.

Replicated issue by turning on mySQL strict mode and editing nodes w/o body fields, error occurs:

user warning: Field 'body' doesn't have a default value query: INSERT INTO node_revisions (nid, uid, title, teaser, log, timestamp, format) VALUES (39, 1, 'revisions test 2 changed 3', '', '', 1267646017, 0) in /xxx/includes/common.inc on line 3468.

Which completely fails to enter anything into the node_revisions table, while node table is updated fine.

Applied patch in #71, retested revisions on multiple content types w/ and w/o body fields, with info properly saved in the node_revisions table.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Thanks, committed.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.