Restore the ability to add Log Messages to Nodes

in principle, this is a good thing. no time now to review/test this carefully, but i will when i get the chance.

Yup, this looks good. +1

My only comment is the patch has Windows "line endings". The patch I attach is identical to the original save for the ^M at each line end.

dww, when you return and review, RTBC ;)

regards,
--AjK

Switching to feature request. Don't see where the log message would actually get saved either, so the patch could use some work. With form_alter it would also be possible to make a fully featured module to add log messages to all nodes.

revision_moderation module does this, albeit only for 4.7 at the moment. The form alter is indeed very easy and should probably get moved into node.module for the next release cycle.

Oh, and incidentally, node_revisions has a "log" field already in it just for this purpose. So no worries on the data storage end.

exactly, that's my point (from a previous issue somewhere). we already have this field, we just have no UI to get to it anymore except for book nodes... :(

dww is right, and that imo makes this a bug rather than a feature request.

Here's a patch which doesn't work, but I don't know why. :P It simply adds a log field to a node if either its node->revision property is set (true of any node types that have "Create new revisions" on by default) or the "Create new revisions" checkbox got checked by someone with admin node privileges.

It produces the error:

warning: implode() [function.implode]: Bad arguments. in /Applications/MAMP/htdocs/head/includes/form.inc on line 585.

Which is from this line in form_get_error():

  $key = implode('][', $element['#parents']);

?!?!

Btw, the code's in #after_build because I can't check checkbox values in hook_form_alter.

this is an utter hack, but at least it works. where's chx when we need him? :(

since we're creating the $form['log'] in #after_build, it's impossible for FAPI to know about our field and do anything smart with it for us. so, we have to get the value directly from $_POST, and call form_builder() ourselves.

as i said, it works, but it's evil... hopefully there's a better way.

Ok, if that's the alternative, then it's probably better to check for $_POST['revision'] in node_form.

sigh. :P

yeah, if we're going to be checking $_POST directly, this is much cleaner than #after_build. i was just trying to get webchick's initial approach working. ;) this is much better...

We spoke at length over what to do about the fact that someone can both check "Create new revisions" and click "Submit" in the same move. Then they don't have the opportunity to enter a log message.

Eaton came up with an awesomely simply idea, which is just to always show the log field for people with administer nodes permission. So here is a patch which does that, and also gets rid of nasty $_POST checking.

dww found an obscure bug; if the node used to have a log message, but its node type is not set to "Create new revision" and thus this option gets reset, its log message gets clobbered with a blank value.

He's currently working on a fix; in the meantime, setting this to code needs work.

as promised, here's the patch. we just check in node_save() that $node->log has data before we try to write to the DB. this prevents destroying the current revision's log message if you edit a node without creating a new revision, but leave the log field blank. since the "log" field is default '' in the {node_revisions} schema, this causes no problems when creating a new revision with a blank log.

Ran through the following test cases:

a) Set "Create new revisions" as default option for blog posts and created a new blog post with normal user. Log field appears. Filled in log, submitted node, and click edit. Log field appears. Enter log and check revisions tab; both log messages are there.

b) Had normal user create a "page" type which did _not_ have "Create new revisions" enabled. Confirmed that log field does not appear.

c) As uid 1, created "page" type. Confirmed that log field appeared. Entered log message. Hit Edit. Did _not_ fill in a log message this time. Hit save. Checked db, original log message still was there.

Thus, I mark this RTBC.

I, for one, am happy that the log thingy doesn't show up for nodes.

I'm a little bit confused by the "this prevents empty log messages from overwriting existing values" comment in the patch. I'm not sure I understand that

Dries, they won't show up unless revisions are enabled. And revisions are basically useless without log fields. See for example: http://drupal.org/node/27362/revisions

Now imagine how completely useless that page would be if there was no ability to add why you changed something. It would be nothing more than a series of dates, times, and names.

That is what we're facing with the inclusion of CCK into core without this functionality.

Ugh, except that they _will_ show up on all node types if the user has "administer nodes" privileges, which of course you do on drupal.org.

I don't know what to do then. Go back to the one that checked for $_POST['revision'], so it only showed up if the revision control was specifically checked, even though just clicking "submit" won't allow someone to submit a log message with their change? I guess then #11 would do it, with the inclusion of dww's bug fix (which I assume would still be necessary).

Does this sound acceptable?

Bottom line is we really need this functionality _in core_ for revision control to be useful to anything other than books.

webchick: the $_POST trick sounds ugly. The current patch is the better solution, although it might annoy many administrators. ;-)

webernet: it would be great to see that explanation added to the patch. It does make me wonder why we aren't checking the value of the checkbox then, rather than checking whether the log message is non-empty.

Thanks for the clarification, but it says more or less the exact same thing, just using different words.

Original:

// Only store the log value if there's something to store; this
+    // prevents empty log messages from overwriting existing values.

New:

+    // Only store the log message if there's something to store; this prevents existing
+    // log messages from being unintentionally overwritten by a blank message.

I think we need to explain why and when this happens.

What if I want to leave the message blank? The message of the previous revision would carry over to the new revision? That sounds like a bug to me. (Haven't test this patch.)

I tested the patch and log messages were stored correctly. Committed to HEAD.

Awesome! Thanks, drumm!!

@webernet: That's cool, except can we have the collapsed property based on whether $node->revision is true or not? If "Create new revisions" are the default for a node type (such as for book nodes on drupal.org), then we basically want to always enter something in here, and having to click the fieldset to expand it first means people will miss it.

-1 to patch #26. "hiding" this field by default defeats the purpose, especially for non-admins. logs for revisions is a huge part of what makes the revisions useful in the first place. always showing this field when revisions are enabled and for content admins is totally reasonable, IMHO. if some admin is really up in arms about this, they can make a trivial form_alter() contrib to move this field into a collapsed fieldset, or even making a profile setting for if you want to see log messages, etc, etc. but all of this is feature creep for 5.x, and the code as it currently is in CVS (thanks, drumm!) fixes the bug we were trying to slay. setting this back to fixed. if someone wants a new setting for this, let that be a separate issue.

that said, the comment about the obscure bug fix is slightly better. ;) sorry it's confusing. i'll attempt to clarify the bug itself (as it existed in 5.x before this patch):

1. enable revisions by default on book pages
2. create a book page
3. edit that page with a new revision including a log message
4. edit the page again, but leave the "log" field blank and de-select "create new revision"

you just clobbered the log message stored with this revision.

the behavior of the patch is that if you do *not* make a new revision when you're editing a node, and you leave the log message blank (e.g. you just went in to fix a minor typo and you didn't think it was worth a whole new revision with a log message), the log message of the revision you're editing is preserved...

this is an obscure side-effect of http://drupal.org/node/39124 ("previously entered log message is shown by default") which we thankfully fixed. ;) but, fixing that bug revealed this one.

make sense?

thanks, everyone.
-derek

hehe, sorry for the simultaneous reply that clobbered what webchick said. ;) i guess she's right... using $node->revision to decide if the fieldset should be collapsed by default isn't the end of the world... ;) moving to a usability task that needs work (since the bug is in fact fixed).

I merged in some of this stuff manually (patch got committed a tad to early). We still want to discuss the fieldset ... not sure it is a good idea.

This is getting a little bit confusing. A power users has to open two fieldsets to make revisions work. First, you need check 'create new revision' in the 'publishing options' fieldset. Secondly, you need to open the log revision fieldset to type a log message. Furthermore, the placement of these things does not patch the logical order -- i.e. the checkbox is all the way down, above the actual log message.

It might make sense to group the 'create new revision' checkbox and the 'log message' textarea in a single fieldset. They somewhat belong to each other although they can be used seperately.

my personal feeling is to not spend any more time on this issue. the bugs are fixed. the resulting UI is usable as is: regular users only see Log if they'll be making a new revision, and node admins always see it, which is fine since they're node admins and are at least familiar with node revisions. we've now restored the behavior of previous versions of core. i say this is fixed and we should move on.

if we want to start messing with the whole UI for node revisions, the eve of the RC seems like a bad time. i say submit that as a UI feature request for 6.x and let's leave this alone. ;)

thanks,
-derek