http://drupal.org/node/130366 fixed a major complaint with the signatures system: it made signatures dynamic, so that they weren't coupled to the comment body and so that users could update their signature once and have it affect the entire site.

Now comes the second part: addressing two remaining issues with the signatures system:

  • Signatures are rendered on forum replies, but not forum topics.
  • There is no way to just plain turn signatures off.

My solution to both of these problems is going to be adding the ability to turn signatures on and off per node type. If they're turned on for a node type (e.g. forums), then both the initial node and its replies will have user signatures.

Comments

Status:Active» Needs review
StatusFileSize
new10.17 KB

Ok, here's a patch...

1. Where possible, I removed $comment->signature from comment.module in favour of $comment->type. Signatures are a user.module thing anyway, so it makes sense for the logic to be there.

2. With $comment->type, I can do a check user.module's hook_comment to either set or unset the signature as appropriate. The node's already in scope in most places. The only place I believe I had to add a node_load call was in comment_form_add_preview().

3. Now, each node type form has a signature enable checkbox. This is turned off by default. I've altered forum.install to enable it for forum nodes. If it's enabled, both the initial post and all subsequent replies get signatures.

4. The little grey signature separator line from user.css didn't make it into the last patch, so that's added back in now.

Status:Needs review» Needs work

This will need to be re-rolled:

1. I forgot the $signature variable like we gave to comments in the other patch.
2. I need to remove the user.css stuff
3. There are some changes to the original patch that will affect this one.

Status:Needs work» Needs review
StatusFileSize
new15.14 KB

Let's try this.

StatusFileSize
new14.78 KB

Ressin' fressin... :P

*without* the user.css hunk, I said! ;)

StatusFileSize
new15.17 KB

Missed the conditional in chameleon.theme.

I'm convinced about the need to disable signatures, but I'm not convinced that we need to have per node type signatures.

Either way:

+ if ($signature) {
+ $output .= theme('user_signature', $comment->signature);
+ }
This looks a bit weird: we check for $signature and then we use $comment->signature.

+ $node = node_load($comment->nid);
+ $comment->type = $node->type;

Throughout the patch, you assign $comment->type to $node->type. Why is that? Also, there is an additional node_load() which may or may not have a performance penalty (comments should be optimized for speed).

Status:Needs review» Needs work

Thanks for the review! :)

The main bug we need to fix (from a making Drupal forums kick ass standpoint) is for forum posts to display signatures, along with forum replies. I thought a way to do that would be to enable/disable per node type, but maybe instead we just put some special logic into forum.module and do a global "off" switch for forums... hm....

I'll re-roll with this approach instead. :)

The $comment->type stuff was to get the logic for signatures out of comment module and only added in user.module, since that's where signatures go, in order to enforce separation. But maybe this is not really necessary.

Title:Fix signatures, part 2 of 2: Enable signatures per node typeFix signatures, part 2 of 3: Append signatures to forum posts

Ok, let's try this.

So this does a few things:
1. Removes all the duplicate code in comment.module that's check_markuping the signature, and moves it to user_comment. This was a hold-over of the last patch.
2. Puts signature stuff in a theme function. The $signature variable for comment.tpl.php is retained.
3. Only forum module, no other types, appends the signature to the node body on post. When I was thinking about this before, you probably _don't_ want a signature on your blog post, even though you _would_ want signatures on users' comments. So we make a special case for forum module now.

It _doesn't_ include the ability to disable signatures entirely; that'll be an easy subsequent patch though.

Status:Needs work» Needs review
StatusFileSize
new7.23 KB

Heh. :) how about a patch? ;)

+ // Append signature to initial post.
+ $node->content['body']['#value'] .= theme('user_signature', check_markup($node->signature, $node->format));

Not sure we want to do this -- can't we pass the signature in a separate variable so it can be _positioned_ properly? Some sites may want to re-position the signature?

Plus, if we make it a separate variable, we don't need a kill switch to disable forum topic signatures. You'd just modify the theme not to print the signature variable.

Lastly, this approach looks more consistent with the approach taken by the comment module?

Otherwise the patch looks great. I'm perfectly happy with making this a forum only thing (for now).

Status:Needs review» Needs work

Cool!! :D

And yes, you're right, this should be made a variable and each core theme should ship with a node-forum.tpl.php that uses it. Will try and re-roll this later tonight.

StatusFileSize
new10.47 KB

I've been following this signature thread closely and as it happens I am looking to slip in these changes to a Drupal 4.7.x installation. I've attached a patch against 4.7.6 for the changes to comment.module, node.module, and user.module. I have some other modifications to forum.module at the moment so I did not include that portion. Note I made one change to the user_comment function in user.module to test if a signature not only isset but has a string length. Otherwise users with no signature still show the ndash and paragraph tags.

Wow. This is not easy. :(

So creating a $signature variable in phptemplate.engine and printing it in after the $content variable in node-forum.tpl.php results in:

Note title
Node body
---------------
Forum navigation block
---------------
*Signature*
Links
Taxonomy
---------------
Comments...

Which is... sub-optimal. The signature needs to appear directly after the person's post for the 99.9999% use case.

I can abstract out the forum navigation into a variable as well (which might be nice, so you can leave it out entirely if you don't want to print it at all), but every other module that adds content to the $node->content array is still going to appear before the signature. :( Not sure how to tackle this without basically re-working the entire node rendering system. :( And I'm thinking that eaton already has something for this in the works....

Dries, is it still possible to get this in without the variable? It's inconsistent with the way signatures are done in comments; however, it's in a themeable function so you could add an ID to position the thing off to the left corner or something if you want...

Solving this in a one-off manner for signatures is a bad idea, IMO. The patch as it currently stands at least makes it possible to override the default behavior -- the lack of an easy to use $signature variable in forum nodes is annoying but not insurmountable.

The real solution will be the planned refactoring of node-rendering, which will pass on the individual elements of each node on to the theme templates as discrete variables. ;-)

Ok. That being the case, I'm going to tentatively set this back to code needs review.

@jaydub: I'm not seeing that behaviour. If I erase my user's signature, the div doesn't get displayed?

Status:Needs work» Needs review

Can't we use weights for node content arrays as well?

I haven't looked at this in detail yet (still on vacation, limited internet time), but it looks like this is something we want to fix properly. It strikes me as a shortcoming of the forum module's theme functions? It would be great if we could 'simplify' this -- it aligns well with the desire to improve the forum module in core. Not?

Can't we use weights for node content arrays as well?

Nope. The issue is that we want to pass a $signature variable on to the theme/template level, just like comment.tpl.php. Unfortunately, *all* the content for the node has been collapsed to a single string by that time and there's no way to slip text into the middle of it.

The long-term answer, IMO, is to hold onto the structured array until the theme engine itself processes theme_node. There, we could simply map all the top-level array elements to discrete variables for the template. That would give us the best of all worlds -- the ability to print $content, or to print $signature, etc.

That does need to be done, and I'm working on the initial pseudocode for it, but it's definitely outside the scope of the signatures patch itself.

I agree with Eaton's analysis. If it's really outside the scope of this patch, we can do two things:

1. Postpone this patch as there is a dependency on Eaton's work. Committing this patch might complicate his work.

2. Commit this patch, and add a TODO or document this limitation. Ask Eaton to fix this up as part of his forthcoming patch.

I leave this up to webchick and Eaton to decide. I don't know when Eaton plans to have his patch ready, but it strikes me as an important improvement for Drupal 6.

Status:Needs review» Postponed

I really hate to do this ;) but the best solution is indeed to postpone it at this stage... otherwise we introduce inconsistency between the way signatures are done between comments and nodes.

I'll do what I can to help Eaton with his node rendering plans so that I can still get this sucker in this version. :D

I, for one, am shocked that webchick would voluntarily set this patch of patches to 'postponed.' I take that very seriously. As such, I've posted the first shot of explaining my proposal for the refactoring of node building and rendering: http://drupal.org/node/134478.

Of the three 'phases' outlined in that issue, only the first would be necessary to expose the dedicated $signature variable we're discussing in this patch.

Version:6.x-dev» 7.x-dev
Status:Postponed» Needs work

Subscribing and trying to figure out the status of everything... Catch, isn't it still postponed pending http://drupal.org/node/134478 ?

hmm, well #134478 was posted after this was postponed, so I think the original intention was "next version" rather than a dependency. I don't know if the new theming stuff that's got in since then would help at all? Like forum.module being able to expose it's own node-forum.tpl.php file including the signature maybe? If it doesn't, then yeah it should probably go back to postponed.

Status:Needs work» Postponed

Yeah, postponed is the correct status. See #20.

Basically, the problem is that there's no way to append signatures to the end of the node content. there's only a way to append it to the end of the node body, which is not necessarily where you want it.

comment.tpl.php has this neat $signature variable in Drupal 6, and we want a similar $signature variable for node.tpl.php, so they're consistent. The only way to do that is to refactor how node properties are stored and presented.

Status:Postponed» Needs work

I think we can un-postpone this now - #350984: Kick comment rendering out of node module

Title:Fix signatures, part 2 of 3: Append signatures to forum postsSignature support for nodes

Marked #363878: Signatures for nodes (which has an active patch) as duplicate.

Status:Needs work» Needs review
StatusFileSize
new6.3 KB
Unable to apply patch node_signatures_1.patch
[ View ]

i don't know why webchick says

comment.tpl.php has this neat $signature variable in Drupal 6, and we want a similar $signature variable for node.tpl.php, so they're consistent. The only way to do that is to refactor how node properties are stored and presented.

but here is a patch which adds display of signature per contenttyp

Status:Needs review» Needs work

contenttype.
should be content type.

+ '#title' => t('Attach signature to the content'),
+ '#default_value' => variable_get('node_signature_' . $type->type, TRUE),
+ '#description' => t('Enable the display of the author signature at the node.'),

Should probably be
"Display user signatures"

"Append author signatures to content"

<div class="content"><?php print $content?><?php if ($signature): ?><div class="user-signature clear-block"><?php print $signature ?></div><?php endif; ?></div>

This should be split over multiple lines - I think I'd also reather see it in it's own div outside content, but not sure on that.

More generally- could signaturesbe added in user_preprocess_node? Also user module should add the fields to the content type forms as well - and only if signatures are allowed at all from admin/user/settings, which probably means making the variables user_signature_node_$type or something.

wow thx.

this old themes are really bad...

i agree with you, that signatures should live in there own div, but so we also have to change comment.tpl.php

to user_signature_node_$type, it makes sense to have this stuff in user module.

i would like to hide signatures from comments also if its not allowed on the nodes.

dereine - that'd be amazing. We could probably make the per-node type setting apply to both comments and nodes for that type - then if people really want to get fine grained about it they can do so in the theme. Otherwise it's two settings for every node type, which'd be far too many.

StatusFileSize
new9.04 KB

so here is the next patch,

still needs tests :(

i'm not sure wether to preprocess for node and comment in the user module to add the signatures, any thoughts about it?

Assigned:webchick» Unassigned

Unassigning myself, since I'm obviously not going to be working on this anymore. :)

I agree that it makes more logical sense for user.module to preprocess in the variables, since it's the one providing these values. The reason this stuff was added to comment module initially was that the various comment queries are already pulling in values from the user table, and pulling out u.signature at the same time eliminated a join. I wonder if there's a way to do both?

For nodes, we need to shift $node->picture/name/signature from node_load to user_nodeapi_load() anyway - which currently has a similar issue in terms of the preprocess stuff. That will only be possible once #347250: Multiple load users gets in.

So maybe we should keep these where they are for now (pictures have a similar issue) - then when both this patch and user_load_multiple() are committed, we can work on centralising everything in user.module.

For comments - I agree with chx's 'non-fieldable API' issue - in that we need to kill hook_comment entirely, and offer either or both of an ultra lightweight item and 'comments as nodes' in core. Which if that happens will mean no comment_load_multiple() - so we can leave the current irritations in comment module until those issues are resolved.

Ok let's please not get signatures wrapped up in all the higher architectural crapola that needs to happen. That's exactly what killed this issue in D6. ;)

@dereine: Go for the solution that mirrors what comment module does now, and we can re-work it later once the underlying tools become available.

webchick: agreed 100% - just pointing to the other issues so we know what needs to go in before revisiting the preprocess stuff in a followup issue.

So IMO the main changes are - let user.module handle the form_alter() - depending on the site wide signatures settings, and namespace the variables accordingly - that's as much as user module can handle for now, but no harm putting it in like that ready for later changes. Plus check the variables in template_preprocess_comment (per node type) so we have uniform behaviour between nodes and comments.

@webchick: Can this be done for 7.x (API changes, string freezes, etc..) or should we bump this to 8.x?

Version:7.x-dev» 8.x-dev
Assigned:Unassigned» webchick

grumble grumble. ;)

Subscribing.

Title:Signature support for nodesUser signatures for nodes
Component:user system» node.module
Status:Needs work» Postponed

Assigned:webchick» Unassigned

Component:node.module» node system
Issue summary:View changes

(Merging "node system" and "node.module" components for 8.x; disregard.)