Authoring information is never updated.

Just to clarify, it's updated in the {node} table, but not in the {node_revision} table - and the query in node_load_multiple() ends up querying node_revision{} for uid.

Sounds like we could use a test for this!

I think this code probably also breaks if you edit the authoring information while creating a new revision. I've got a feeling this should actually be done in _submit() somewhere, not node_save() at all. And yeah we need a test - this issue/patch was a product of tracking this down after scor asked in irc while working on another patch.

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

The last submitted patch failed testing.

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.

Why do we have to array_flip the fields twice? I'm pretty sure this query building was copied direct from the Drupal 6-style node_load() (not to say I might have missed something along the way).

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()

The last submitted patch failed testing.

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.

Doing this directly in drupal_schema_fields_sql() seems better if that's the only way for it to be useful with built queries.

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.

The last submitted patch failed testing.

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:

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.

The last submitted patch failed testing.

Sounds like a weird fail.

The last submitted patch failed testing.

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:

But also here:

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 :)

I'm not clear why we need this - so it could use some comments (would also be more readable if rolled with -up - can't see which function is being changed).

This should be "...to avoid a name collision.... otherwise {node.uid} is never updated when editing a node."

Similary other references to 'author' in comments should be lowercase.

This review is powered by Dreditor.

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.

I just realised despite reviewing this already that this patch will conflict with #460320: Standarized, pluggable entity loading (nodes, users, taxonomy, files, comments) (which fixes the unset($key) issue with an array_flip() but may not fix the actual bug here) - either way whichever goes in first, the other will need re-rolling.

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

Committed to HEAD.

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