When update a node, without creating a new revision, entering a revision log of "0" is ignored. This is because of code:

<?php
elseif (empty($node->log))
?>

in node_save() - PHP treats "0" as empty. This bug has been present since at least 6.x, and there are presumably lots of other instances of the same issue.

Files: 
CommentFileSizeAuthor
#11 node-revision-log-1282956-5007962.patch580 bytespgrond
PASSED: [[SimpleTest]]: [MySQL] 33,003 pass(es).
[ View ]
#8 node-revision-log-1282956-5006222.patch581 bytesrickmanelius
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in modules/node/node.module.
[ View ]
#5 node-revision-log-1282956-5005882.patch581 bytesrickmanelius
PASSED: [[SimpleTest]]: [MySQL] 32,980 pass(es).
[ View ]

Comments

Everett Zufelt’s picture

Issue tags:+Novice

This is the third related empty causes '0' to fail issue I've run across recently. I believe the others were for radio / checkbox #title and fieldset #title (legend).

The correct code should be:

elseif (isset($node->log) && $node->log !== '')

Presuming that isset($node->log) isn't tested / verfied somewhere earlier in the logic.

Everett Zufelt’s picture

Whoops,

realized that it is empty() and not !empty(), so comment #1 is wrong.

Can you please point to the function where this occurs?

lyricnz’s picture

Yes, it's a very common bug (I did a quick check now, found several in common.inc alone - will create a few more issues until I get bored)

Bug is in node_save() which is in modules/node/node.module - around line 1076

rickmanelius’s picture

I believe the line is solved by converting it to:

elseif (!(isset($node->log) && $node->log != '')) {

It's also possible with the following, but I think this may be more confusing

elseif (empty($node->log) && $node->log != 0.0) {

Patch coming soon...

rickmanelius’s picture

StatusFileSize
new581 bytes
PASSED: [[SimpleTest]]: [MySQL] 32,980 pass(es).
[ View ]

Patch attached

rickmanelius’s picture

Status:Active» Needs review
lyricnz’s picture

Status:Needs review» Needs work

That condition would probably be clearer if it was:

<?php
   
elseif (!isset($node->log) || $node->log === '')) {
?>

i.e. "if ($node->log is not set, or it's literally an empty string) then ..."

rickmanelius’s picture

Status:Needs work» Needs review
StatusFileSize
new581 bytes
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in modules/node/node.module.
[ View ]

Resubmit with suggestions in #7

Status:Needs review» Needs work

The last submitted patch, node-revision-log-1282956-5006222.patch, failed testing.

edb’s picture

You have an extra bracket, should be:

<?php
   
elseif (!isset($node->log) || $node->log === '') {
?>
pgrond’s picture

Status:Needs work» Needs review
StatusFileSize
new580 bytes
PASSED: [[SimpleTest]]: [MySQL] 33,003 pass(es).
[ View ]

Resubmit with fix from #10

rickmanelius’s picture

Thanks edb. I was moving too quickly on such a simple thing. Thanks to pgrond for the new patch.

rickmanelius’s picture

Status:Needs review» Reviewed & tested by the community

Thanks for the syntax fix in #11. Tested and it works. Logs of "0" and "0.0" (without creating a new revision) now save. Reviewed and tested.

lyricnz’s picture

Backport would be useful, at least to D7

rickmanelius’s picture

Sure. What's the protocol for including backport patches in the same issue? The patch above says nothing of being D8 specific except the tag in this issue. Do we roll the patch, change the status, and then attach?

lyricnz’s picture

Patch will probably apply to D7, and maybe D6 without modification. Let's get it in D8 first.

catch’s picture

Version:8.x-dev» 7.x-dev

I can't think of a reason you'd want to add a revision with 0 (so am not asking for a test here), but can't think of a reason to prevent that either.

Moving back to D7 for webchick.

webchick’s picture

Status:Reviewed & tested by the community» Fixed

Seems a bit silly, but I guess it doesn't hurt anything.

Committed and pushed to 7.x. Thanks!

lyricnz’s picture

Oh, totally silly! After fixing an empty() bug in my code, I checked a couple of places in D7 for the same bug. Yup, lots of 'em. Meh.

Status:Fixed» Closed (fixed)

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