Fix signatures

Here is a very simple patch that just does this the same way as it's done in comment.module.

should implement for all nodes, not just forum ... ideally we move sig code out of comment and into common.inc or user.module

Assigning this to myself to ressurect it from the dead. I'll try rolling a patch today or tomorrow.

Here is a patch that removes the user signatures from core, and moves the functionality to a contrib module (which I'll post right after this).

You'll notice there are *no* database changes here. This is because one of the mandates of Drupal is that users should never lose any data. If I dropped the signature field from the users table, this would happen. So this field instead remains dormant and unused. Not an ideal situation, of course, but there's no other way to salvage the information in the signature field without making signature.module a core module (which I'm assuming no one would like very much).

And here is the contrib module.

Features:
- Lets you permit signatures on a per-node type basis
- Lets you control the format of how signatures are displayed
- Renders the signature on node/comment view, so that they are automatically updated when a user updates his/her signature
- The signature no longer appears in the comment box, messing up your cursor location when you go to type

I should mention that this is for the most part a port of the signature module posted by ultraboy, with the code cleaned up and aimed toward 4.7. The advantage of this one is if the above patch gets in, there is no need for core patches at all to run this module, thanks to hook_comment.

As much as l like the functionality, I think we now we have split personalities: the database in core has the column, but core never populates that column.

This is a bit inconsistent, and hence un-Drupalish.

Yes, you are very right. Ok, I'll try a second approach which keeps these options in core without the need for a contrib module. Post back soon. ;)

Ok, let's try this on for size.

Since the "signature" field is in the users table, it makes sense for all signature-related information to be in under user settings. So this patch does the following:

- Removes signature bits from comment.module
- Places options to enable signatures by node type under administer >> settings >> users (same place as the user picture options, as Robert suggested)
- Places new hook calls user_comment and user_nodeapi to add the signatures at content view
- Moves the user signature textfield under user >> edit directly

Basically, everything the old patch+contrib module combo did, except now it's incorporated directly into core.

Let me know what you think!

nice patch. personally i like moving this functionality to contrib but no big deal. over 3 years ago i posted some wishes for signatures: http://drupal.org/node/233. back to this patch ...

1. you should not need nodeapi('load'). instead, edit the node.module query which retrieves the node info. it already retrieves a few columns from users table - just add signature. this saves a query for every node view.

2. signature_is_allowed() should probably be user_signature_is_allowed()

Awesome! Thank you moshe for the tips.

This one incorporates moshe's changes, and a couple others I found:

1. Renamed signature_is_allowed to user_signature_is_allowed
2. Also renamed theme_signature to theme_user_signature
3. Added u.signature to node module's node_load call
4. Also added u.signature to comment module's comment_render call

So that now eliminates an extra query per node *and* per comment (which would have added up fast!).

The only other possible place to eliminate crossover is is user_comment, I have to load the comment's node ID in order to determine its type. The query to retrieve comments from the database only joins on comment and users, so this would mean joining on a third table (nodes) to retrieve type information. I've been told though that node_load($nid) gets cached, so maybe this is not a performance hit? Which way should I go?

Re-roll for current head

Re-roll for current HEAD again. :)

Here's an updated patch that check_markup's the output. Else it allows users to put unfiltered input into their sigs. not good!

For the patch-ily challenged, here is a screenshot also of the signature settings under administer >> users so you can see what features this patch adds (obnoxious red box not included ;)):

http://webchick.net/soc/user_signatures/signature-settings.png

I really like this functionality. A more flexible way of managing user/author signatures as described above would make Drupal a much more acceptable out of the box solution for "online magazine" type website, just to name one example. Those communities offer "exposure" for the author as an alternative to paying for content. A flexible "article byline" or signature and being able to turn this functionality on/off per node type is a necessary piece of that proposition.

I would love for this patch to make it into Drupal. If there's anything I can do to help facilitate that process (testing?), please let me know.

I was told that this patch prevents the signature from appearing in the text area when you make a comment. In the hopes that it gets applied, I've re-rolled it against 4.7.0-beta2

i played around with this patch a little bit, and I don't think it prints signatures on comments anymore.

+1 Anything is better than the current signature implementation.

Bumping this up again... once 4.7's out I plan to re-roll this patch cos man does this annoy me. ;)

Any idea when this will be out? I jsut switched to drupal and that is very very annoying. Thanks

Whenever I get around to it or someone else beats me to it.

In the meantime, apparently there is a signature module which does a lot of this. Check for it in the downloads section.

I was not able to find a signature module in the downloads for 4.7

My 4.7.0 version is out: http://drupal.org/node/64670

Why, why, nobody pointed me this thread and these patches? I'm so *sad* :( I'll apply these patches tomorrow, but I'd like to maintain the code by myself. I think everybody wants this.

Since core developers are intrested now in separating signature functions from the core, I'll think over how to do it.

If I was given a CVS record, I would check out bug reports particulary for my module... I asked, but it was declined... So sad...

Ultraboy, if you'd like to take the lead in getting this functionality implemented in core, go for it. I've unassigned myself from the issue. I will still try and re-roll my last patch eventually, but there's a good chance you could get to it first. ;)

http://drupal.org/node/16726 has just been marked as a duplicate to this one.

What about using a <span>instead of a <span> for the sig?
Would that be more flexible for styling?

to Beginner: as I mentioned here http://drupal.org/node/64670 , currently some tags (such as ) are filtered out from nodes' signatures. I think it's possible to fix that still without a path to the core. If you need that right now, try playing with this by yourself (and please do a report to other users here http://drupal.org/node/64670).

to webchick: I think signature.module should go to the contribs (and all the corresponding functionality should be stripped out from comment.module). I'll do a report and post it to the mailing list.

ultraBoy: Moving signagutures to contribs was my first inclination too, but Khalid makes a good point (#8)... one of Drupal's core values has always been, "We can break people's code, but not people's data." That means that existing signature data in that database can't be deleted. Which means moving this functionality to a contrib modules leaves stale data in the table. Not good.

Therefore, I think the answer is to just make the core bahaviour of signatures more inline with the signature module, which is what I attempted in later patches.

Reviving this again. This is not identical to the last patch but pretty close:

- Removes coupling with comment.module - everything related to signatures is now with users where it belongs.
- Allows you to specify that signatures should be displayed on certain node types (ex. forums).
- Adds a setting to enable/disable signatures for the entire site.
- Renders the signatures dynamically on node/comment view. This means that when a user changes their sig, it will reflect in all their past posts as well.

A patch would be nice, huh? :P

Marking back to code needs work -- Moshe had some additional feedback that I plan to address in another version later tonight.

Also, re-assigning to me.

Remaining todos:
- Need to assign an input format for users' signatures
- Need to figure out what to do about existing posts with signatures already appended... hm...

2 more:
- Add in site-wide option to either append to bottom of all nodes/comments on view, or only add them on insert (per killes)
- Make comments respect node type signature on/off setting.

Ok, new patch, much simplified over last patch. Marking back for review.

- Got rid of the global "disable user signatures" setting; if you don't want signatures, just don't enable them for any node types.
- Decided against having alternate options for adding comments on view vs. insert. The logic just gets too complicated (ex: comments are objects sometimes and arrays other times, etc.). The patch as-is just appends them to nodes and comments on view.
- No more setting for "signature separator" -- instead, this is handled with the theme function.
- Added input format (default to Filtered HTML) for user signatures.

Ahem. Marking back for review. :P

I think the PreCCK patch has probably broken this.

...and so it did.

Changes:
- Name of node settings forms changed to 'node_type_form.'
- Getting node type information from $form['orig_type'] ... $form['type'] no longer exists, and orig_type should hopefully not change so will work in this context.
- Had to create a custom submit function to set variables.. that no longer seems to happen by default.
- Added signature stuff under user settings to a fieldset for consistency with other options there.

Been thinking about this, and marking back to code needs work. There really needs to be an option for appending signatures on insert or on view. Will try and come up with something workable...

Also, while I'm brainstorming ways to improve this patch, I should add signature to the list of fields grabbed in the comment and node queries as Moshe suggested a long time ago, to eliminate the need for an extra user_load per view.

Another update. Still needs work to get the insert thing on there and there's some weirdness in comment view still, but I've switched the logic around hopefully for better performance.

Ok, I think this puppy's ready for review again.

Implemented the choice between adding signatures on insert vs. adding signatures on view. It defaults to the old behaviour so if you're not interested in "dynamic" signatures (or if you have an old site with a bunch of signatures already in the body), you don't have to enable that option.

I'd prefer two patches; one that fixes the problems and one that introduces new functionality.

That line is a bit hard to define, as it kind of depends on what you consider fixing the problems and what you consider new functionality... the new functionality in this patch was introduced in order to fix problems. ;)

But if I had to break down this patch to its barest components, these are the main "bugs" with signatures that I see currently:

1. The initial issue: signatures only show up on comments; not on their parent nodes. This is very confusing for forums in particular. However, you also don't want to universally turn this feature on for _all_ node types, as on static pages it would look kind of silly.
2. Because signatures should be placed on both nodes (in certain circumstances) and comments, it no longer makes sense (if it ever did) to store signature settings with comment.module. They should be moved to user.module, as the signature field is in the user table to begin with.
3. (my biggest pet peeve) Signatures appear in the comment box before you type anything. This means when tabbing into the box, your mouse cursor ends up at the _end_ of the text, and you have to either click the mouse or hit a bunch of arrow keys to move it to the beginning of the text. It also is confusing for people who are used to this being appended automatically to their posts, as this is standard in other web apps.
4. There is no way to differentiate a user's signature from the surrounding text; it's just smooshed in there at the end. Additionally, there is no way to specify a signature separator, or make signatures look different HTML-wise from the rest of the post.

This new patch addresses the above bugs in the following ways:

1. Append signatures to node types which have comments read/write under their settings pages, not to those which have comments disabled.
2. Moves signature settings to user.module.
3. Appends the signature during submission, taking them out of the bleeping comment box.
4. Adds a theme_signature function, so signatures can be both differentiated from surrounding text and themed.

If you'd prefer these as separate patches, I can oblige. :)

In addition to these, there were three other features in the old patch:

1. Allow signatures to be displayed on view rather than insert, so updates to a user's signature would instantly appear
2. Ability to turn on signatures per node type
3. Ability to turn off signatures altogether (by not having turned them on any node type)

However, I'm pretty comfortable with this level of fix-age and having something like signature.module in contrib handling these extra features. It was pretty messy to implement these in core anyway.

Thanks a lot for the review, Dries. :) It's allowed me to take a hard look at what's here and filter out the cruft. I'm much happier with the patch now, and I hope you will be too. ;)

Actually, I should do the adding during form submission, so it's possble for other modules to override this and cancel it if they want to change the way it works.

New patch coming up shortly.

Ok, one more try. :) This move actually cleaned it up a lot, so yay! And now contrib modules such as signature can unset the $form['#submit'] callbacks that this functionality has and replace it with their own. This allows disabling of signatures completely, for those who are so inclined, or making them go to a render-on-view model, or whatever.

Please let me know what you think! This has been my biggest itch for the past year+ ;)

sigh. :( theme function isn't going to work out. :( also, signature settings show up in user registration form. ;)

Oh, good. :D That "showing stuff on the registration form" thing wasn't my patch. ;) http://drupal.org/node/80085

Ok, here we go. I think this is both as simple and as close to fixing all of the major issues as we're going to get.

The theme function didn't work out, because it's throwing the

chx had a good idea for the render-on-view option so that it doesn't screw with existing comments...

on update, do a variable_set('user_signatures_start_nid', $current_highest_nid)

then only render them where nid > variable_get('user_signatures_start_nid', 0)

However, Dries asked me to remove features so I will leafve this for a second patch.

These seem to conflict:
"Append signatures to new comments, and to new nodes that have comments enabled"
"Your signature will be publicly displayed at the end of your posts and comments"

Spelling:
"Customize the separator between user cotent and their signature."
and I think something more like "Formatting for users' signatures below posts and comments. Use %si..."

Or, better yet, make it a themable function since it is about controlling the display of something. Why won't that work?

Cool, thanks for the review, Drumm.

1. I changed that line of text to read: Your signature will be publicly displayed at the end of your comments, and posts which allow comments.
2. Also put it in a check for if (module_exists('comment')), since without that it doesn't really make sense to have the box. :) For some reason this was in a foreach loop of node_get_types() before; I think this was a hold-over from a patch awhile back.
3. I went back to the theme function route. I was initially thinking that when "regular" users edited their old posts/comments, that the div would get taken out since it's not in the Filtered HTML list, but it appears I was wrong. It shows up in the content edit box which is a little awkward, but I think that's ok.

Putting back to review again.

Also, just to reiterate, there is a known issue where the signature box will appear on the user registration form, and also on the admin user create form. That is not related to this patch: http://drupal.org/node/80085.

Sorry, just don't want to see this marked "code needs work" again for something unrelated. ;)

sigh. :(

OK just found this having opened a duplicate issue so reviving this instead.

I rolled this patch (first ever try at one) to remove signature handling from comment.module against 5.0. Will do it again against HEAD as soon as I can.

I also opened a seperate issue about handling signatures in profile.module http://drupal.org/node/113089 which I'll leave open for now. Profiles seem to me to be the natural place for signatures to live since it's about extending users in general and the functionality is pretty much already there (would just need a default optional field and a way to display it).

ack forgot the patch

So essentially remove signatures from core (at least until a better solution comes about)? I like it.

Given the myriad of ways that people want to see signatures implemented (see the above 60,000 attempts at a patch that pleases everyone), this functionality is much better moved to a contrib module unless we can do it in a smarter way in core.

In any case though, you're right; signatures should have nothing to do with comments. It seemed to me there were other places where the signature field was referred to (user.module?) but I'll try and do a more thorough review later.

So essentially remove signatures from core (at least until a better solution comes about)?

Yeah I think that's easier to remove the current implementation completely, then it can be re-introduced (in user or profile or a contrib or some combination of the three) cleanly, and an upgrade path provided to take existing ones out of the user table. I don't think there's much worth saving in the way they work at the moment.

You're right about user.module - signature is included in user_fields but it's only one word in an array I think (quick look at 4.7).

New patch - removes signature from both comment.module and user.module against the 5.x development tarball, just to get some practice in. Looks like I have to browse manually to the individual files for 6.x so that'll have to be later on.

Hey, catch! What are you using to make these patches? I notice from your other patch you're on Windows... I just wrote up instructions on how to make patches under Windows with TortoiseCVS, which might be helpful. Otherwise, there are lots of other options under that section of the handbook.

Marking code needs work because the patch is in kind of a weird format, and because it needs to be rolled against Drupal HEAD.

Btw, I would re-roll this myself, but it's better that new people learn. :D Let me know if you run into any problems.

Left it on review for your patch, not mine...

This one hits both user and comment, and is against HEAD - so marking back to review optimistically.

Thanks for the instructions for Tortoise, first go at cvs as well, not bad for 24 hours.

Marking RTBC. Removes the UI for signatures and any references to it in the code. Honestly, this functionality is currently so horribly broken that this really is the best way.

Patch does not remove the signature field from the user table. This is by design; we don't destroy peoples' data. We'll either find a better thing to do with this as a result of something like http://drupal.org/node/113089, or will leave it in one release cycle for contrib signature modules to migrate out to a secondary table.

Sorry, we're not removing signatures from core at this point.

Dang. ;) Well, a girl has to try. ;)

In that case, I think we need a series of patches:
1. Remove signatures from comment module, move to user module, since that's the table where the field is stored. Keep existing (broken) functionality otherwise. Should be a pretty simple patch.
2. Add in the ability to turn on/off signatures per node type; when signatures are turned on, _both_ nodes and comments get signatures. This solves the original bug about forum nodes.
3. Remove signatures from the body of the comment / node and put inside a theme function instead that's appended when the post is made. This solves signatures' biggest usability problem, and also makes their output themeable, so you can add something to separate signatures from post body, for example.

The one part of the previous patches that was a feature was the ability to display signatures at "view time" rather than "post time" -- this one is tricky, because it requires an update to strip existing signatures out of node posts so they're not displayed twice, may bring about performance concerns, and there are people who want to keep the existing behaviour. Therefore, this is probably best handled with a contrib module.

Dries, does this sound like a plan? If so, I'll get a-patch-rollin'!

In that case, I think we need a series of patches:
..snip...

I still think all of this could be done easier by moving signatures to profile module.
Much of the functionality is there already, any additional features could apply to the whole of profile module, it'd keep signatures in core, and my patch could still get in as the first step ;)

If you can come up with a workable plan/patch, sure. I lean against that atm because of all the various issues I outlined in reply #1 there; I don't think these are easily solved problems. Plus, an argument could be made for not having the overhead of profile.module for something as simple as signatures, but benchmarks would be needed in order to prove which way was better.

Above is how I intend to fix the problems, though, pending input from Dries. Moving signatures to profile module could be done as a 4th step or as one of the middle steps if it makes sense, and don't really impact any of the above except #1. However, signatures would still be disable-able (the feature you're requesting) by not enabling signatures on any node type after patch #2.

No that all sounds fair enough. I have no idea how to remove old signatures from old comments or alternatively prevent theme-level display on old comments conceptually, let alone in terms of code. And that's a major issue I'd not given much thought. So I'll leave this one to those more capable. Thanks for the heads up on patches and Tortoise and the encouragement though.

No problem! :D And I'll give you a heads-up on the other issue if I come to any epiphanies. ;)

Here's my patch to solve this issue. It moves signature away from the comment's body and adds a "signature" (binary) column to the comments table. If signature is 1, the user's signature (the user table is joined anyway) is added to the comments object. The theme just needs to handle the signature appropriately.

This patch is not yet done: Input validation for the signature field is still missing! Ideally, you should be able to select an input format for the signature, but I currently don't see a clean way to implement this. Perhaps you've got better ideas.

"I have no idea how to remove old signatures from old comments..."

We can do it like this: do NOT add new signatures to old comments! So there won't be duplicate signatures.

And still, I'd like to see signatures in a separate module (*and that module can be in core*, so Dries would be happy). Otherwise, we'll deal with *multiple* patches which are much more difficult to test and review compared to standalone module.

We can do it like this: do NOT add new signatures to old comments! So there won't be duplicate signatures.

A lot of people would like them displayed at the theme level though - and if you do that, you'd need to not display signatures at the theme level for old comments (since they're already there), which would require something clever like making display conditional on cid or something, too clever for me!

I, too, think it makes for a theme system thing. +1 for moving it to the user module, and making it available to the theme.

Not sure how to deal with old comments that already have a signature. That is going to be tricky -- and it shows that the current system is flawed (we lose important semantic information). :/

While this issue was indisputably first, all the various replies make it impossible to follow. Therefore, I've split this out into:

http://drupal.org/node/130366 - Make signatures dynamic (and themable)
http://drupal.org/node/132446 - Enable signatures per node type

Marking this as a duplicate.