Authoring information is never updated.

Sounds like we could use a test for this!

As far as I can tell the problem is in node_load_multiple(). Select fields are not set up properly.

Let's see if this passes. Apparently when loading a node you should get the node->uid and when loading a revision you should get the node_revision->uid.

I guess some artifact of the new db api? Apparently when you pass an array of fields into $query->fields() it has to be in the form array(1 => 'nid', 2 => 'vid') so $node_fields['vid'] would be meaningless unless you array_flip()

This is a bit cleaner. However, maybe this fix should be made on the API side? i.e. drupal_schema_fields_sql() itself calls drupal_map_assoc(). I believe all tests would pass in that case.

Ok here's an alternate patch which modifies drupal_schema_fields_sql() directly.

With 7x from CVS today I was experiencing the same issue, the UID is set to the current user rather than that specified in the authored by field.

I applied the patch from #12 successfully and now when updating the node the original author is maintained

Also when creating multiple revisions the author is correctly updated.

Patch worked for me manually - what next?

#patch -p0 < patches/node-load-mulitple_0.patch
patching file includes/common.inc
Hunk #1 succeeded at 4399 (offset 156 lines).
patching file modules/node/node.module
Hunk #1 succeeded at 762 (offset 7 lines).

Heres the patch rerolled

Benno and I found that the patch mostly solved this problem -- once the patch was applied, in manual testing (couldn't do simpletest because there isn't one yet - btmash is writing one at this moment), the author information was properly maintained. A manual edit of the original author by an editor also worked successfully and the information was properly updated.

However, we found that when we looked at the "Revisions" list, revision authors were correct, but for some reason the author name on the original node under the revisions list contained the last user who had edited the before revisions began to be kept instead of the original author.

Or is that how it should have worked?

Benno and I found that the patch mostly solved this problem -- once the patch was applied, in manual testing (couldn't do simpletest because there isn't one yet - btmash is writing one at this moment), the author information was properly maintained. A manual edit of the original author by an editor also worked successfully and the information was properly updated.

However, we found that when we looked at the "Revisions" list, revision authors were correct, but for some reason the author name on the original node under the revisions list contained the last user who had edited the before revisions began to be kept instead of the original author.

Or is that how it should have worked?

I applied #17 and I was still unable to change the author of a node on today's D7 dev tarball.

I am assuming the patch took as I received no errors:

patch -p0 < node-load-mulitple_0.patch
patching file includes/common.inc
Hunk #1 succeeded at 4529 (offset 130 lines).
patching file modules/node/node.module
Hunk #1 succeeded at 760 (offset -2 lines).

Strange thing is that now the nodes display the author I was trying to change the nodes to prior to applying this patch. But I am unable to change them back.

Revision history shows all revisions by my logged in (user/1) account, not the user designated as author. Not sure if that's by design, but thought I'd mention it here.

Based on what rainbreaw and all have been working on, I am appending the test to the patch (which verifies the patch works and the current version does not). The patch needs to be checked through since its my first time writing a simpletest (and there might be better ideas on how it should be written.

Second pass at the patch. I do not seem to get the issue that Rain is getting after running the simpletest, but I do see the discrepancy she sees during manual testing.

Subscribing.

Tested the patch in #22 again. While the issue is mostly fixed, the lingering discrepancy in the revisions list is still present in manual testing.

Fixed in both simpletest results and in manual testing:
- original author information is correctly retained in node display and content list display
- original author is correctly changed to new author in both displays when a user with correct privileges changes the author information intentionally

Under the revisions listing, however, if someone makes an edit to the node without storing it as a new revision, the author information changes to that person, instead of the original node author.

Perhaps this is how that display is meant to work?

If so, then this patch is REVIEWED AND TESTED BY THE COMMUNITY.

If not, then there is still an issue that only shows up in manual testing in the Revisions area.

I think that author should not change unless it is specifically set by the user editing a node or a node revision... the "administer nodes" permission is required to change the author and it should be given only to trusted users, so I do not think it should change author uid automatically when editing nodes or node revisions.

I can confirm the discrepancy between {node} and {node_revision}, although I did not test the patch extensively (with effective revisions of the node), if you look at the table in the the database after you've edited a node and changed the author, it is updated in {node} and not in {node_revision}. I do not know if it is the expected behavior, but it sounds weird to me.

As far as I know the behavior is expected, if not ideal.

There is something of a deficiency in the drupal node object. We want to store both the user who actually edited the node for revision log purposes, and the user who is presented as the "author". The latter would ideally be stored per-revision similar to title.

For those two types of information all we have is a uid column for the node and node_revision tables, and one $node->uid property EDIT: and a $node->revision_uid property which was present in Drupal 6.

We're storing the user who made the revision in node_revision.uid and the node author (unversioned) in node.uid, and loading either one or the other into $node->uid depending on if we're loading a node or node revision. When you load a node revision you do not load the author and when you load a node you do not load the user who made the current revision. EDIT: The user who made the revision should be loaded into $node->revision_uid and the node author (unversioned) should be loaded into $node->uid.

Based on mfb's note, which basically clarifies that it is working as expected, I'm changing the status of the patch in comment #22 and this issue to "Reviewed and Tested by the Community"

But as TheRec points out, the lingering question of how the author is stored in node_revision does seem a little odd. If others feel this way, perhaps a new issue should be opened specifically for that functionality? (As it seems to both be a different matter entirely and be a far less critical behavior and is more a matter of opinion about how it should work.)

I will test tonight. One thing I noticed before on this issue was seeing one author on the node display, another author in the admin content screen.

Indeed laura s, this has been fixed according to the tests created by BTMash (which passes) and also according to manual tests, but you're welcome to test it too and report if you see problems Authors with revisions :)

I would like to clarify that this was my first time writing a test and as I try to figure out what is going on (it definitely feels like there is a discrepancy somewhere), I would welcome more testing to narrow down the issue.

Well. The easiest way to answer the question "how is this supposed to work" is "how does D6 work?" since D6 is a stable release on its 13th point version used in 100K+ sites.

It's not clear to me: does this patch retain the same behaviour upon editing as D6?

I have tested with two clean install, one of D6 and one of HEAD with this patch, the behavior is the same. If you edit a node and do not change the Author, your uid is used to update the revision (in {node_revision}), but the uid in {node} stays the same (which is what this patch corrects). If you change the user, it changes it in both places. So it seems that this discrepancy is the expected behavior, as this was the concern making the issue CNR, I set it back to RTBC.

Actually there is an API difference from D6, which has a $node->revision_uid property. New patch on the way, please stand by.

We now load the node_revision.uid into the $node object as revision_uid, as Drupal 6 does.

The included test fails without the patch. It tests that the original node author is preserved when a new revision is created by a second user, and also tests that a revision_uid property is set in the $node object.

In case you're wondering, I avoided using the drupalCreateNode() method because it does "hack"-ish things with uid, which is one of the things I'm testing here.

Sounds like a weird fail.

Fixed the Cancel account test failure.

It works for me... same behavior as D6 and it follows the same code logic. Waiting for another positive review to mark it RTBC.

I tested this and it seems to work as advertised, or better. In fact, I was able to demonstrate correct behavior with the patched version, switch to the current trunk and demonstrate misbehavior on the same node, then switch back and the node was actually repaired by the non-author user adding another revision.

I think it's RTBC.

Gave it a try right now and it looks like the winner :)

Ok, I've no problem committing this since it fixes a bug and unblocks the content screen, but given how much confusion there was in this issue over the correct way to do things, I *really* think this fix needs better documentation.

Particularly here:

+++ modules/node/node.module	21 Aug 2009 10:52:51 -0000
@@ -810,6 +810,10 @@ function node_load_multiple($nids = arra
+    // Change revision uid to revision_uid.
+    unset($node_revision_fields['uid']);
+    $query->addField('r', 'uid', 'revision_uid');

But also here:

+++ modules/node/node.test	21 Aug 2009 10:52:51 -0000
@@ -218,6 +218,25 @@ class PageEditTestCase extends DrupalWeb
+    // Load the revised node and verify vid, uid and revision_uid properties.

The goal should be that someone who's never seen this issue before can come across that code and understand why it works that way, so we don't try and "fix" this again in the future.

Beer-o-mania starts in 9 days! Don't drink and patch.

I updated the comments the way I understand the code :)

Ok, well I do not know what is done in drupal_schema_fields_sql()... I think it is safer to let mfb (who introduced the changes in the patch #12) comment this... and review the comments I've added... Or if anyone else understand it, they are welcome to re-roll.

The reason I modified drupal_schema_fields_sql() to use the field as key is so that the later unset($fields[$field]); calls to remove a field by key actually work.

In Drupal 6, node_load() uses $fields = array_diff($fields, array($field)); to remove fields by value.

In this patch, you can see how it would work if we preserve drupal_schema_fields_sql() as-is and restore the Drupal 6 array_diff() syntax. I guess one version is some microseconds faster than the other if anyone wants to run a micro-benchmark. Also I revamped the comments to hopefully be more clear.

Also clean up the test comments a bit, changing "Author" to "author".

Heh, I was about to do that clean up :) Thank you for the explanation and also correcting my previous mistakes, indeed those two solutions are pretty similar.
The patch is still working fine (manual and automated tests). It has been tested before and the improved comments are clear enough, I feel this is RTBC.

Awesome! Great work on this folks! Those new comments are very easy to follow.

Committed to HEAD.

Not sure why ... still looking, but it appears this is still broked.I just tried this:

1) fresh D7 install
2) created article
3) edited article and changed author to another user and hit save
4) the original (or possibly the user that made the change, I havent tested this yet because in my case they were the same) user is still listed as the author.
5) sadness

I created a article under "user_a" (uid = 3).
I edited by user "admin" (uid = 1), at "authoring information" i selected "user_b" (uid = 4).

The node table says the uid = 3.
The node_revision table says the uid = 1.

Creation a new revision doesn't help either.

Changed

function node_submit($node) {
  global $user;

  

  // A user might assign the node author by entering a user name in the node
  // form, which we then need to translate to a user ID, unless we've already
  // been provided a user ID by other means.
  if (!empty($node->name) && !isset($node->uid)) {
    if ($account = user_load_by_name($node->name)) {
      $node->uid = $account->uid;
    }
    else {
      $node->uid = 0;
    }
  }
  $node->created = !empty($node->date) ? strtotime($node->date) : REQUEST_TIME;
  $node->validated = TRUE;

  return $node;
}

to

  if (!empty($node->name) || !isset($node->uid)) {

And it works.

There's still a problem though. If I edit the node and go to Authoring options, there's a option:
Authored by:
*Leave blank for Anonymous.*

This makes no sense, because if I don't edit this field, it should not be Anonymous, but the user it already was, "user_A".

This is still broken on a fresh install of 7 alpha 1.

Yeah this needs to be just isset($node->name).

See also #687608: Unable to set node author to "Anonymous".

Patch with tests to back it up. Tests changing to invalid username, to anonymous user, and to a different (not currently-logged-in) user.

Also removes some cruft from node_form() since we do this earlier, so the second time we do it, its pointless:

  // Basic node information.
  // These elements are just values so they are not even sent to the client.
  foreach (array('nid', 'vid', 'uid', 'created', 'type', 'language') as $key) {
    $form[$key] = array(
      '#type' => 'value',
      '#value' => isset($node->$key) ? $node->$key : NULL,
    );
  }

I applied the patch in #59 to a fresh install of D7-dev. It applied cleanly. I created two users and did a variety of authorship changing, all of them succeeding.

Simple human test: passed. :)

Shai

Works for me :)

Great. Thanks for the tests! Now hopefully we won't break this a third time. ;)

Committed to HEAD!

Backport to fix another D6 critical issue: #398110: node_submit resets $node->uid.