node_submit resets $node->uid

+++ modules/node/node.module	30 Oct 2009 19:53:08 -0000
@@ -897,12 +897,12 @@ function node_validate($node, $form = ar
- * Prepare node for save and allow modules to make changes.
+ * Prepare node for save by generating UID, if needed, and creation date.

Please revert this.

+++ modules/node/node.module	30 Oct 2009 19:53:08 -0000
@@ -897,12 +897,12 @@ function node_validate($node, $form = ar
-  if (user_access('administer nodes')) {
+  if (user_access('administer nodes') && !isset($node->uid)) {

Please add an inline comment right before the condition that does not repeat the condition, but explains the reasoning for the condition.

I'm on crack. Are you, too?

yes, we want to move + extend that comment instead. A note about the actual condition/reasoning wouldn't hurt. I mean, this issue talks about a problem in programmatic creation of nodes, and the existing condition in the code presumes that there is some magic around "administer nodes" permission involved, and somewhere, I saw you talking about a special case when $node->name is defined... So just adding another condition will increase the WTF. Hence, we need a proper inline comment here that properly explains why & when this is happening. To the point, no novel required.

Regarding 1), ok, if that is the case, then yeah, good catch.

+++ modules/node/node.module	6 Nov 2009 18:07:27 -0000
@@ -897,13 +897,15 @@ function node_validate(stdClass $node, $
- * Prepare node for save and allow modules to make changes.
+ * Prepare node for save by generating UID, if needed, and creation date.

"Prepare node for saving by populating author and creation date."

+++ modules/node/node.module	6 Nov 2009 18:07:27 -0000
@@ -897,13 +897,15 @@ function node_validate(stdClass $node, $
-
...
+  

Trailing white-space introduced here.

+++ modules/node/node.module	6 Nov 2009 18:07:27 -0000
@@ -897,13 +897,15 @@ function node_validate(stdClass $node, $
+  // A user with administer nodes permission might assign the node author by

The permission name should be in quotes.

This review is powered by Dreditor.

Thanks!

Tests failed. :(

looks good.

in another patch, i'd be up for a rename of node_submit(), or merge it into some other function. it is confusing with fapi submit handlers and all

Let's get some tests for this. This definitely sounds like an edge case we're bound to break again someday.

And I don't think a rename of node_submit() at this stage makes sense. We've been confusing it with form submit handlers since D4.7, so another release won't kill us. Would be happy to discuss cleaning this entire node pseudo-hook mess up in D8 tho. :)

+++ modules/node/node.test	23 Nov 2009 20:28:32 -0000
@@ -841,6 +841,11 @@ class NodeSaveTestCase extends DrupalWeb
+    node_submit(&$node);

This should have led to an exception, because variables cannot be passed by reference. Please remove the ampersand from $node.

+++ modules/node/node.test	23 Nov 2009 20:28:32 -0000
@@ -841,6 +841,11 @@ class NodeSaveTestCase extends DrupalWeb
+    
+    // Verify UID, per #398110.
+    $this->assertTrue($node->uid == $this->web_user->uid, t('Function node_submit() preserves UID'));
+    

1) (minor) Trailing white-space here; blank lines should be blank.

2) The inline comment should properly describe what is being verified here; without issue number.

3) You want to use assertEqual() here.

This review is powered by Dreditor.

+++ modules/node/node.module	24 Nov 2009 17:46:25 -0000
@@ -905,13 +905,15 @@ function node_validate($node, $form = ar
+  // User ID, unless we've already been provided a UID by other means.

Can we consistently use the same term (e.g. user ID) instead of UID?

+++ modules/node/node.test	24 Nov 2009 17:46:25 -0000
@@ -840,7 +840,11 @@ class NodeSaveTestCase extends DrupalWeb
+
+    // Verify that node_submit did not overwrite the UID.
+    $this->assertEqual($node->uid, $this->web_user->uid, t('Function node_submit() preserves UID'));
+

See above.

Committed to CVS HEAD. Thanks.

In node_submit function (node.module,v 1.1179 2009/12/06 18:27:23)

  if (user_access('administer nodes') && !isset($node->uid)) {
    if ($account = user_load_by_name($node->name)) {
...

The statement !isset($node->uid) would be false by default.

• For new node, $node->uid is default to $user->uid in node_object_prepare().
• For existing node, $node->uid would have been populated in node_form().

As a result, even with administer nodes access, user cannot assign alternative author from node form. Please review...

My bad.

OMG everyone's hated node_submit dies. Quick commit soon before it resurrects.

+++ modules/node/node.pages.inc	8 Jan 2010 18:17:09 -0000
+++ modules/node/node.pages.inc	8 Jan 2010 18:17:09 -0000
@@ -453,7 +453,23 @@ function node_form_submit_build_node($fo

Actually, I'm not very happy of this move... this is the god-forsaken #builder_function for nodes, and we all know what we don't know, but we all know that we want to eliminate those #builder_functions entirely, because literally no one on this earth really groks that Form API + Field API integration _entirely_.

For example, this builder function is also invoked for AJAX submits and elsewhere. Intended?

+++ modules/node/node.pages.inc	8 Jan 2010 18:17:09 -0000
@@ -453,7 +453,23 @@ function node_form_submit_build_node($fo
+  
...
+  
...
+  

+++ modules/node/node.test	8 Jan 2010 18:17:10 -0000
@@ -245,6 +245,17 @@ class PageEditTestCase extends DrupalWeb
+    
...
+    

Trailing white-space here.

5 days to code freeze. Better review yourself.

It's too late for more. We need to fix #24 and be done with it.

Looks like this was all fixed by the latest commit at #492186: Authoring information is never updated., although the bug report here was older.

The problem described in OP just bit me using Drupal 6.19

Has this issue been fixed in the latest D6?

D6 backport.

#47 does this still suffer from the issue raised in #24? After that approach was rolled to D7 there was this issue that came up: #659962: Nodes with a $node->uid assigned can not have author changed. D7 seems to have been fixed here. For D6, perhaps this would be the simplest approach.

#47 does not work as we could see in D7. So I continue in #492186: Authoring information is never updated. with a backported patch similar to the D6 patch pointed out in #48, but we keep the code in sync with D7.