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:
- #223820: Book module tests for postgres error
- #260264: Creating a new revision of a poll gives error and causes corruption
Marking this issue as critical, because it is needed to solve critical issues. Patch enclosed. Please review.
Comments
Comment #1
David_Rothstein CreditAttribution: David_Rothstein commentedI 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?
Comment #2
Damien Tournoud CreditAttribution: Damien Tournoud commentedThanks 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 innode.test
).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 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.
Comment #3
David_Rothstein CreditAttribution: David_Rothstein commentedSounds like a reasonable plan to me.
Only problem is that the second patch you attached is actually the exact same as the first one ;)
Comment #4
Damien Tournoud CreditAttribution: Damien Tournoud commentedCorrect, sorry, this is the good one.
Comment #5
David_Rothstein CreditAttribution: David_Rothstein commentedThis 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...
Comment #6
R.Muilwijk CreditAttribution: R.Muilwijk commentedI 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?
Comment #7
VladSavitsky CreditAttribution: VladSavitsky commentedI have the same bug in 6.2 too.
Comment #8
Damien Tournoud CreditAttribution: Damien Tournoud commented@R.Muiwijk: there is a perfectly working test in #223820: Book module tests for postgres error. It may require a reroll, thou.
Comment #9
R.Muilwijk CreditAttribution: R.Muilwijk commentedPatch should be added to the node tests not the book tests because it is a general node fix.
Comment #10
erle CreditAttribution: erle commentedJust 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)
Comment #11
Damien Tournoud CreditAttribution: Damien Tournoud commentedRerolled to correct some fuzz.
Added a unit non-regression test.
Comment #12
webchickAdding this to my todo list.
Comment #13
catchThe 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).
Comment #14
webchickHm. 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.
This comment is wrong; we are not actually inserting a new node.
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.
Really? Why can it not?
(minor) two spaces before If and We.
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?
Let's call that out as a specific TODO. (and aim to fix it in D7) Probably also true of {node_revisions}.log, no?
How do I know it's needed?
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:
I'm not a fan of this kind of PHPDoc. I'd prefer it to actually describe what's being tested.
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.Comment #15
PanchoI came across this issue while re-doing the node_revisions split. Getting this here fixed would be awesome!
Comment #16
Damien Tournoud CreditAttribution: Damien Tournoud commented#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.
Comment #17
Damien Tournoud CreditAttribution: Damien Tournoud commented#330290: Node with revisions but without body corrupts node and node_revisions tables was a duplicate.
Comment #18
pwolanin CreditAttribution: pwolanin commentedsubscribe
Comment #19
Freso CreditAttribution: Freso commentedSubscribe, from #466586: Error on submitting new revision with empty (NULL) body.
Comment #20
Eric_A CreditAttribution: Eric_A commentedThanks 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.
Comment #21
jbeall CreditAttribution: jbeall commentedI 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
Comment #22
webchickiirc you can't give text fields default values. it breaks either in pgsql or mysql strict.
Comment #23
jbeall CreditAttribution: jbeall commentedwebcheck -- 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.
Comment #24
Damien Tournoud CreditAttribution: Damien Tournoud commented@jbeall: and you have tried reinstalling? ;)
Comment #25
jbeall CreditAttribution: jbeall commentedYes, actually. I apply the fix after installation, though. I'm assuming it would cause installation problems.
Comment #26
graper CreditAttribution: graper commentedWhile 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?
Comment #27
beauz CreditAttribution: beauz commentedsubscribing as I have this issue as well...
Comment #28
crea CreditAttribution: crea commentedsubscribing... This is critical since it affects 6.x
Comment #29
markus_petrux CreditAttribution: markus_petrux commentedJust 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.
Comment #30
hass CreditAttribution: hass commentedVery 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...
Comment #31
hass CreditAttribution: hass commentedPatch in #11 works for me in D6.
Comment #32
hass CreditAttribution: hass commentedThis is really CRITICAL and I do not understand why this need to wait 1.5 years. Marking as blocker to get this fixed.
Comment #33
dogbertdp CreditAttribution: dogbertdp commentedI am experiencing this issue as well. Here is the run down:
I am running Drupal 6.13 and php 5.2.
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:
The taxonomy module will also now throw an error similar to the following on each cron run:
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.
Comment #34
Damien Tournoud CreditAttribution: Damien Tournoud commentedThis tag is for the branch maintainer only.
Comment #35
hass CreditAttribution: hass commentedDo 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.
Comment #36
markuskb CreditAttribution: markuskb commentedsubscribing. Can't believe this issue is going on for 1.5 years now...
Comment #37
Damien Tournoud CreditAttribution: Damien Tournoud commentedTo all the subscribers since #14: if nobody rolls a patch taking into account webchick's review in #14, this will never be fixed. Ahoy!
Comment #38
pwolanin CreditAttribution: pwolanin commentedhey DamZ - I'll see if I can make time for a re-roll. Is Mikkel pulling your fixed core git repo yet?
Comment #39
pwolanin CreditAttribution: pwolanin commentedpatch in #11 fails to apply for me.
Comment #40
pwolanin CreditAttribution: pwolanin commentedhere 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.
Comment #41
iwkse CreditAttribution: iwkse commentedI'm attaching the same patch for D6 applying smoothly
Comment #42
iwkse CreditAttribution: iwkse commentedComment #43
pwolanin CreditAttribution: pwolanin commentedok, what the hell - this code is so broken in 6 and 7:
why do we force the revision to be saved with the uid of the current logged in user?! Isn't this an API function?
Comment #44
pwolanin CreditAttribution: pwolanin commentedok, 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.
Comment #45
pwolanin CreditAttribution: pwolanin commentedre-roll for #108818: Add transactions to key _save routines which changed node_save()
Comment #46
Eric_A CreditAttribution: Eric_A commentedThis 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.
Comment #47
pwolanin CreditAttribution: pwolanin commentedCombined D7 patch with DamZ's test from #11. I needed to rewrite the test to account for the node title being a field.
Comment #48
eft CreditAttribution: eft commentedI 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.
Comment #49
pwolanin CreditAttribution: pwolanin commented#42 should work for D6
Comment #50
eft CreditAttribution: eft commentedthanks a lot - will try it out
[Edit] Works great - too bad this wasn't included in D6.15
Comment #51
pwolanin CreditAttribution: pwolanin commented@eft - please post more details of your testing so we can get this in the next release.
Comment #52
eft CreditAttribution: eft commentedsure - what kind of details would you like?
Comment #53
pwolanin CreditAttribution: pwolanin commentedDetails of how you observed the bug before and how you tested/confirmed that it was resolved.
Comment #54
David_Rothstein CreditAttribution: David_Rothstein commentedUpdated patch:
Comment #55
pwolanin CreditAttribution: pwolanin commented@David - ok, well we'll still need body and teaser for the D6 version
Comment #56
David_Rothstein CreditAttribution: David_Rothstein commentedIndeed, that's true.
Attached patch has a couple very minor fixes to the test.
Comment #57
pwolanin CreditAttribution: pwolanin commentedapparently testing is shut off due to #667264: [TESTING BROKEN] FileFieldValidateTestCase fails randomly
Comment #59
joshmillerTesting has been turned back on. Bot is happy. Code has tests. RTBC.
Josh
Comment #60
webchickWow, 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?
Comment #61
babbage CreditAttribution: babbage commentedHEAD is broken as a result of this patch. :(
Comment #62
babbage CreditAttribution: babbage commentedAttached 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.
Comment #63
webchickOh, 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!
Comment #64
catchComment #65
madlee CreditAttribution: madlee commented#42: This worked for me in 6.14. Thanks!
http://drupal.org/node/261258#comment-2276860
Comment #66
ealtman CreditAttribution: ealtman commentedHi -- 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?
Comment #67
ealtman CreditAttribution: ealtman commentedI 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.
Comment #68
David_Rothstein CreditAttribution: David_Rothstein commentedIt 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
Comment #69
gost_gost CreditAttribution: gost_gost commentedSamme 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!
Comment #70
kehan CreditAttribution: kehan commentedsubscribing
Comment #71
pwolanin CreditAttribution: pwolanin commentedReroll and minor tweaks of the patch at #5 (essentially the same as #11 also).
Comment #72
andypostPatch 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
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
Comment #73
brenda003Patch works good.
Replicated issue by turning on mySQL strict mode and editing nodes w/o body fields, error occurs:
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.
Comment #74
Gábor HojtsyThanks, committed.