$node->uid should come from node table not node_revisions
rcorsaro - December 5, 2007 - 21:20
| Project: | Drupal |
| Version: | 6.x-dev |
| Component: | node.module |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | closed |
Description
I am using Drupal 6.x beta3. When a user creates a blog entry, and then an administrator edits the blog entry, the breadcrumb for the blog shows the creaters name, but links to the administrators blog page. I have confirmed that this bug exists in a fresh beta3 install. Here is the code responsible for creating the breadcrumb, starting at line 104 of blog.module:
/**
* Implementation of hook_view().
*/
function blog_view($node, $teaser = FALSE, $page = FALSE) {
if ($page) {
// Breadcrumb navigation
drupal_set_breadcrumb(array(l(t('Home'), NULL), l(t('Blogs'), 'blog'), l(t("@name's blog", array('@name' => $node->name)), 'blog/'. $node->uid)));
}
return node_prepare($node, $teaser);
}I will research this further and hopefully supply a patch.
#1
The problem is the uid is set to the last node_revision uid and not the node uid because the node_revision uid overwrites the node. This patch changes the order of the fields in the query. I'm not sure what kind of side effects it might have. Perhaps a better solution would be to add a named field like creator_uid or something and assign the uid from the node table to that. Maybe someone who knows the system better can comment on this.
Index: node.module
===================================================================
--- node.module (revision 1051)
+++ node.module (working copy)
@@ -650,8 +650,8 @@
}
// Retrieve a field list based on the site's schema.
- $fields = drupal_schema_fields_sql('node', 'n');
- $fields = array_merge($fields, drupal_schema_fields_sql('node_revisions', 'r'));
+ $fields = drupal_schema_fields_sql('node_revisions', 'r');
+ $fields = array_merge($fields, drupal_schema_fields_sql('node', 'n'));
$fields = array_merge($fields, array('u.name', 'u.picture', 'u.data'));
$fields = implode(', ', $fields);
// rename timestamp field for clarity.
#2
#3
You might want to have a look at the Ugly Breadcrumbs issue as it might fix what you're experiencing.
#4
The last patch was from my repo. Here is a patch from head:
Index: node.module
===================================================================
RCS file: /cvs/drupal/drupal/modules/node/node.module,v
retrieving revision 1.916
diff -u -r1.916 node.module
--- node.module 6 Dec 2007 09:58:32 -0000 1.916
+++ node.module 6 Dec 2007 16:29:56 -0000
@@ -685,8 +685,10 @@
}
// Retrieve a field list based on the site's schema.
- $fields = drupal_schema_fields_sql('node', 'n');
- $fields = array_merge($fields, drupal_schema_fields_sql('node_revisions', 'r'));
+ // NOTE: If node_revisions comes after head then uid is the last person
+ // to edit the node rather then the person who created the node.
+ $fields = drupal_schema_fields_sql('node_revisions', 'r');
+ $fields = array_merge($fields, drupal_schema_fields_sql('node', 'n'));
$fields = array_merge($fields, array('u.name', 'u.picture', 'u.data'));
$fields = implode(', ', $fields);
// rename timestamp field for clarity.
#5
Changing title and component because this issue seems larger than the blog module. The patch may fix the problem in the blog module but not sure how this change will affect other modules. If $node->name is the original author of the node, it seems as though $node->uid should match that rather than coming from node_revisions table.
#6
New patch. This one replaces r.uid with u.uid in the node_load query.
#7
Lynn, your patch manually changes the CVS id tag.
I have a weird feeling this was introduced in Drupal 5 as a feature. I agree it's a bug though, authoring information is there for a reason, nodes shouldn't change ownership just because someone fixes a typo.
#8
Sorry about that. Is this patch correct now? (I am trying to get the hang of modifying Textmate's diff's to make patches.)
Comparing node_load in Drupal 5 and 6, it just looks like the way the query is formed was rewritten and that it was an oversight that the uid ends up now coming from node_revisions. I doubt it was on purpose. In drupal 5 the query is
<?phpSELECT n.nid, r.vid, n.type, n.status, n.created, n.changed, n.comment, n.promote, n.sticky, r.timestamp AS revision_timestamp, r.title, r.body, r.teaser, r.log, r.format, u.uid, u.name, u.picture, u.data FROM {node} n INNER JOIN {users} u ON u.uid = n.uid ...
?>
and in drupal 6 we first make an array of fields to grab and then right the query
<?php$fields = drupal_schema_fields_sql('node', 'n');
$fields = array_merge($fields, drupal_schema_fields_sql('node_revisions', 'r'));
$fields = array_merge($fields, array('u.name', 'u.picture', 'u.data'));
?>
#9
For some reason the patch was malformed and wouldn't apply as attached, so I removed a few of the contextual lines to get it to apply. I've attached this version.
#10
As for reviewing it, it works as advertised. However, I'm new to D6 and the drupal_schema_fields_sql function, so I'm not sure the string replace solution is good practice. The other solution would be to rewrite the node_revisions call to add all needed fields explicitly, and to add u.uid explicitly. I've attached a patch that does it this way.
#11
i think it is better to preserve the revison uid so add this instead:
$fields = str_replace('r.uid', 'r.uid AS revision_uid', $fields);
this will avoid clobbering the node uid ... my proposal needs testing.
#12
we cannot ship a broken node_load.
#13
I did a print $fields inside node_load and realized it actually was calling all kinds of duplicate fields:
<?phpn.nid, n.vid, n.type, n.language, n.title, n.uid, n.status, n.created, n.changed, n.comment, n.promote, n.moderate, n.sticky, n.tnid, n.translate, r.nid, r.vid, r.uid, r.title, r.body, r.teaser, r.log, r.timestamp AS revision_timestamp, r.format, u.name, u.picture, u.data
?>
#14
node_load is designed to incorporate fields from contrib modules, so explicitly can't specify each field individually.
edit: I don't see any duplicate fields in your example, and would array_unique solve it if there was?
#15
catch, it already is specifiying each field individually, just by virtue of the drupal_schema_fields_sql function, which includes every field in the table. Contrib content is added in elsewhere.
Some of the "duplicate" fields Lynn points out aren't a problem, because they have the same value, but it comes off to me as ambiguous to include duplicate fields just because we have a function that conveniently dumps all fields in.
The Drupal 5 query specifically uses the title and body from the node_revisions table, not the node table, and HEAD is now including both.
In other words, I think this is a bigger problem than just uid, if not in effect than definitely in clarity.
#16
I agree with mcarbone. I guess 'duplicate' was a poor wordchoice of mine. PHP sees n.title and r.title as two separate things but running the query like that is just confusion, since only one will survive. I think specifying the fields explicitly as in Drupal 5 is the clearest method.
#17
Wouldn't changing the fields returned be an api change and potentially effect many, many things? If something relies on uid being the last editor, and something else relies on uid being the creator, then an api change is what is needed. In the short term, adding revision_uid might help, but ultimately, the data structure is at at fault. It seems that there should be a latest_revision field that holds all the revision fields, or even an array of revision structures.
#19
rcorsaro: We should do what Drupal 5 did. If we don't do that, we are in error (unless someone knows about any intention of changing this behavior). To me this seems like a bug introduced with the schema columns automatically being used.
#20
I agree. Here's a patch that brings it back to what Drupal 5 did. Moshe's idea of adding revision_uid is a good one, but probably should be dealt with elsewhere, as it is a (minor) change of functionality.
This still needs to be tested, though, because it's possible that in the course of D6 development someone came to rely on one of the extra fields, or something similarly erroneous.
#21
#22
no, we should not revert to D5 behavior of specifying fields. It is desirable that the resulting SQL is the same as before, but we mustn't get there by specifying individual fields. This change is deliberate. The intent is that one can add fields to the node table and declare those fields with schema API and they will automatically get loaded by this code and automatically saved by drupal_write_record().
#23
Ah, now I understand why drupal_schema_fields_sql is used -- as I said in my first comment above, I'm new to this function. I've added a patch that removes/renames the conflicting fields depending on the case.
And in that case, since we're adding all fields, it seems like there's no good reason to throw away the additional fields that Drupal 5 never included. Plus, moshe's addition of revision_uid now makes sense. So I left in all fields except for those that could conflict (n.title) or are redundant (n.vid, r.nid).
Here are the new fields that get added via the drupal_schema_fields_sql call, and moshe's suggested addition:
n.language
n.moderate
n.tnid
n.translate
r.uid AS revision_uid
I see no harm in these being added, but I can remove them explicitly if there's a good argument against them being there.
#24
I just did the same kind of thing mcarbone, but I think using array_diff makes it a little more concise.
#25
True dat, Lynn, that's much more concise. Here it is with your concision, and moshe's suggestion to keep revision_uid.
#26
Patch tested and works- solves original problem and still allows for schema changes.
#27
IMHO the comment should include info on what happens after all. This one is not clear to me:
// Remove conflicts between node and node_revisions fields.So what happens then? Which value is used from which table after this? Document this please in the code.
#28
Rewrote comment for clarity.
#29
mcarbone: comments have to wrap at line 79/80
#30
I re-wrapped the comments and committed. Thanks!
#31
Automatically closed -- issue fixed for two weeks with no activity.