Revisions are associated with node owner, not node editor

Yes, this is clearly a bug. Being able to see who made a change is a basic requirement for tracking revisions.

Bug confirmed in 6.0-beta2.

Not sure if this is a regression from D5, but node_save has been rewritten to use drupal_write_record - maybe an extra argument for uid is needed?

I believe that yes, this is related to the use of drupal_write_record. We hand in the $node object for the object even though the proper value to write in node_revisions is the current user.

I have a solution for this which is that in node_save I set the $node->uid to the current $user->uid write before calling drupal_write_record('node_revisions').

I added a comment because it seems quite odd that we need to do this. It might be necessary to make the comment the second time.

Also, I'm not sure if I should store the current $node->uid in a temporary variable and set it back as soon as the save is done. Thoughts?

slight update. If it's not a revision, we shouldn't mess with this value.

This is critical.

I can confirm this fixes the problem. But the current fix passes nodes into hook_nodeapi and node_access_require_grants with the current user ID which seems dangerous. Then again, switching the uid back after the drupal_write_record also seems just hacky. I'm wondering if we should make a new function like node_save_revision($node) that sets the uid on a copy of the node, rather than the node itself?

I agree it's critical.

Here is a patch that takes Webchick's advice on splitting this into a new function and cloning the object prior to changing the uid.

Minor coding style issue that there is too many space before the function comment added and there is no dot at the end of sentence.

A better question might be why the third node_revisions write requires no uid amendment?

What about the case where a site administrator needs to make a change to a node created by a user, but wants to leave that user as the author of the node? For example the editor of a news site needing to fix a type in an article written by a contributor.

How do we distinguish between the owner of a node and the owner of a revision? Suppose we associate the node with the owner and the revision with the person who makes the modification, how can we log changes of ownership? For example the editor of our news site needing to reattribute an article from contributor A to contributor B.

I realize that these are edge cases which should probably be handled by contrib modules, but drupal core needs to make sure that we provide the best generic case that will make these possible.

Node ownership does not change as the revision is saved, but the node ownership value is reused in the revision to store who updated the node in that revision. Look into lines 598-648 in Drupal 5.3 for the implementation. Whether this is good or bad is a good question. It definitely has a side effect that when you revert to a previous revision, the editing user of that revision does take on ownership of the node (as far as the revert logic implementation looks to me).

In other words, this is a regression, Drupal 6 does not do what Drupal 5 used to do.

No, these are not edge cases!

The author of the node must NOT be changed, unless the user changes the Authored by field in Authoring information.

@salvis: that's what happens with or without the patch, so there should not be any problem there.

I've now checked more into the behavior of Drupal5 to make sure that the patch is consistent with that. Here are a couple scenarios. Assume revisions are enabled in all scenarios. "admin" is an admin and "user2" is a normal authenticated user.

Scenario 1:
user2 creates content
user2 edits content

user2 is author and is shown as the user who performed both revisions.

Scenario 2:
user2 creates content
Admin edits content

user2 is shown as author and user2 is shown as user on first revision while Admin is shown on second revision.

Scenario 3:
Admin creates content, marks "user2" as author.
Admin edits content

user2 is shown as the author and Admin is shown as user who performed both revisions

Scenario 4:
user2 creates content
user2 edits content - at this point user2 is author and shown on both revisions
Admin edits content - at this point user2 is author and performed first revision, admin is shown on second revision

I re-state these because it's important to understand them. I believe this shows that as Gabor hinted at in comment #7 we need to perform this same type of write for all three versions of this node_revisions record writing.

Now - I stopped doing the clone of the object in this version and switched to a temporary UID variable. The reason I did this is that when I used the clone my sequences were getting messed up and node/NID would give me a page not found. I didn't dig far enough to understand why that was happening, but this new style of the patch doesn't have that fun side effect.

I also removed the single space before the comment and added a period at the end.

$node->uid = $temp_uid doesn't do anything AFAICT. Perhaps you just like to put things back in order before you leave a function :)

code looks good.

I believe that in PHP5 always-pass-objects-by-reference it does make a difference for edits of a node (but not creation, where that is just an array).

But yes, I do like to put everything back in order in general. I consider myself a crusader against entropy :)

Patch applies cleanly..

I've tried each scenario from #13, and with patch everything works as expected.

Mmm, should we roll back the drupal_write_record() patch? With the new function, it is no longer saving us code -- and it certainly becomes less transparent.

I don't know which way it should go.

Well, drupal_write_record() already saves as some code elsewhere, or at least it got used in later patches being committed:

$ grep -r "drupal_write_record" *
includes/common.inc:function drupal_write_record($table, &$object, $update = array()) {
includes/file.inc:    drupal_write_record('files', $file);
modules/block/block.module:          drupal_write_record('blocks', $block);
modules/node/node.module:    drupal_write_record('node', $node);
modules/node/node.module:    drupal_write_record('node_revisions', $node);
modules/node/node.module:    drupal_write_record('node', $node, 'nid');
modules/node/node.module:      drupal_write_record('node_revisions', $node);
modules/node/node.module:      drupal_write_record('node_revisions', $node, 'vid');
modules/taxonomy/taxonomy.module:    drupal_write_record('vocabulary', $edit, 'vid');
modules/taxonomy/taxonomy.module:    drupal_write_record('vocabulary', $edit);
modules/taxonomy/taxonomy.module:    drupal_write_record('term_data', $form_values, 'tid');
modules/taxonomy/taxonomy.module:    drupal_write_record('term_data', $form_values);

Rolling this back might be a little painful now.

Confirmation that patch applies cleanly and things work as expected.
If the drupal_write_record() rollback should happen, it can go in a new issue.

Having the temp_uid is not exactly nice, but cloning the whole object is certainly not required here, so this is a compromise we can live with IMHO. I am not entirely sure I should commit this though, in light of Dries' comments, so awaiting his newer thoughts.

It's an ugly hack, but I guess that's what we're forced to use now. I've committed this to CVS HEAD. Thanks.