Currently, signatures are coupled with comment module. The signature field is stored with the user table. Logically, the code for signatures should be included there as well.

Note that comment module still prints $user->signature explictly in the comment. The goal is to remove that and display it in a theme function instead, but baby steps. :P See http://drupal.org/node/10938 for (much) more discussion. :P

CommentFileSizeAuthor
#24 signature-cleanup.patch4.41 KBwebchick
#22 separator.patch3.76 KBwebchick
#17 sig_5.patch14.28 KBwebchick
#16 sig_4.patch12.49 KBwebchick
#15 sig_3.patch12.49 KBwebchick
#14 Picture 3_5.png42.69 KBwebchick
#13 sig_2.patch11.19 KBwebchick
#12 sig_1.patch11.19 KBwebchick
#5 sig_0.patch9.13 KBwebchick
#2 sig.patch3.72 KBwebchick
fix_signatures_6.patch2.42 KBwebchick
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Title: Fix signatures (part 1 of 4): Move signature settings to user module » Fix signatures (part 1 of 2): Make signatures themeable
Status: Needs review » Needs work

After talking w/ Steven, he recommended I do this in two steps instead, so adjusting title.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Title: Fix signatures (part 1 of 2): Make signatures themeable » Fix signatures (part 1 of 2): Make signatures dynamic (and themable)
FileSize
sig.patch3.72 KB

Note: this is not scope creep! :)

If we run signatures through a theme function, there is no way they can be appended to the comment body as they currently are, because then you get <div ... etc. in the comment body on edit. Therefore, we must now render signatures dynamically on view, which is fine, because that was the next thing I was going to fix. ;)

There's no update path.... Steven said he would help me out with that.

Steven’s picture

Steven CreditAttribution: Steven commented

Okay so I had a nice chat with webchick about this... and it's an incredibly hairy problem. Here's what we agreed upon:

  • Retro-active signatures (i.e. edit your signature, and it will show on all your old comments) is what pretty much all BB software does. There is a saying that "people spend more time on other websites than on yours" and given that signatures are mostly used for forums, we should really follow the dominant behaviour by default.

    The best way to do this is to split off the signature from the comment body completely, and just add it when viewing.

  • However, by properly abstracting and separating signatures, it also becomes completely possible to make a signature.module that has a signature table with nid/cid and signature (and probably uid too for speed). This module can override the default signature when viewing comments (with hook_comment) and keep track of old signatures using a submit handler.
  • The tricky part is an upgrade path... in theory there is a working solution, namely to remember when the admin upgraded to Drupal 6, and only tack on signatures to comments created after that upgrade timestamp.

    However, I think this is a really bad idea. So far, we've had an unofficial rule that all updates involve one-time operations. Once you've upgraded, there should be no left over cruft in the code that runs. This code would affect even fresh 6.0 installs.

    The only solution that follows this rule would be a best effort solution that only removes the user's current signature from their comments. Comments with outdated signatures would show up with two signatures. The question here is how much of an issue this would be. Personally, I don't think people care that much about outdated content, and if a site does have some often frequented forum topics (like a FAQ), the admin can still edit them and manually remove the signature.

    In the end, having the signature not be separate from the comment body is a very bad idea, regardless of whether signatures are retro-active or not, so I think this move is inevitable any way.

eaton’s picture

eaton CreditAttribution: eaton commented

I agree with Steven's comments and webchick's proposal. Changing the look-and-feel of our forums to match other forum systems isn't that hard. Our implementation of signatures, though, makes it almost impossible to achieve similar *workflow*.

This patch makes it possible, and leaving slightly messy data in old comments (doubled signatures, etc) is by far the lesser of two evils.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Needs work » Needs review
FileSize
sig_0.patch9.13 KB

Different approach, based on Steven's awesome feedback. Thanks, Steven! :D

This method removes the theme function and hook_comment in favour of adding a new variable to comment.tpl.php. This way, the output is totally themeable, even by people who don't know PHP. :) Also, the legacy issue can be resolved via the theme, so I'm moving this to needs review.

BioALIEN’s picture

BioALIEN CreditAttribution: BioALIEN commented

best effort solution that only removes the user's current signature from their comments

To be honest, even this is a little OTT. Just implement the proposed functionality and forget all about previous comments. If a user bothers to edit an existing post, then they should remember to remove any signature they see at the footer.

Users will quickly learn that the only place they will ever see their signature in a text area would be their My Account > Edit page.

1) This will make it invisible with no penalty or extra code for fresh Drupal 6 installations.
2) Adding extra code that solves a (questionable?) part of a problem isn't required in this instance.

Users that post comments will quickly realise Drupal comments & signature system will function in line with what they expect at any BB system and will adapt accordingly. The only thing we need to do is state this clearly in the UPGRADE.txt and on the Drupal 5.x > 6.x upgrade pages to inform site admins. They can then take necessary measures under their discretion whether adding a global announcement to their users or in a mailing list to their members etc.

Gman’s picture

Gman CreditAttribution: Gman commented

+1 on including it in the comment.tpl.php of the themes. A great way to let admin/designers who know some CSS to theme the sigs. I was trying to contemplate a solution since I hadn't read Steven's reply, but without any noticable break between the end of a comment, and the included old signature, there is no way get rid of those old non-current comments sigs.

Mostly I am subscribing, as I hope to try this out soon. Also, I like how this leave open the ability for a contrib module to extend/transform the sigs at a later date. Keep the core slim, but with ability to expand.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented

The nice thing about this being included in the theme is that you could conceivably do code like:

if ($comment->timestamp > 121293945) { // timestamp of when you updated to Drupal 6)
  print $signature;
}

I actually kind of prefer this kind of "exact" fix to attempting a "best guess" upgrade path... the best we could do is go through all of the comments in the comment table, check to see if the author's current signature is found (at the very end of the signature), and remove it. This doesn't do anything for users who have had multiple signatures over time.

And yes, doing things this way in no way prevents a contrib module from extending signatures further (or even reverting to the old behaviour); rather, it should in fact be _easier_ now, because the signature is not coupled with the comment body.

Dries’s picture

Dries CreditAttribution: Dries commented

Just wanted to point out that I agree with the proposed solution.

I looked at the code and it looks good. I wasn't able to test it, but I'll do that soon. Feel free to help though.

One detail: typically two dashes (--) are used to separate the signature from the body. At least, that's what they recommend for e-mail, and is what e-mail clients pick up.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented

Yep, that's what I originally had (dash-dash-space-newline), but Steven pointed out that since it's the Web and not e-mail, we should use emdash instead.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Needs review » Needs work

Patch no longer applies. Feel free to RTBC this after you re-rolled it. (I still think two dashes are more 'natural' but I don't really care all that much about it.)

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
FileSize
sig_1.patch11.19 KB

So in satisfying the "other people use other sites more often than yours" rule, I went out to discover what other BB systems are using as signature separators. ;)

Phorum appears not to use a signature separator.

PHPBB 2.x and vBulletin use:

_________________<br />

IPB uses:

<br />--------------------<br />

SMF uses a <hr />.

PHPBB 3.x uses a div styled to look like a <hr />:

.signature {
  border-top-color: #CCCCCC;
}

----

It seems to me that all the forum systems using a bunch of ---s or ___s are in fact trying to simulate the look of a horizontal rule separating the text body from the signature. So why not just bite the bullet and use a horizontal rule? This way there isn't extra meaningless text in the post for robots and blind people to try and parse.

I looked up some stuff on the <hr /> tag itself, but it appears that generally it's frowned upon as being presentational rather than semantic.

Therefore, I'm going with PHPBB 3.x's approach, and styling the signature div. This allows to also ditch a non-semantic <br /> tag as well. Hooray!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Needs work » Reviewed & tested by the community
FileSize
sig_2.patch11.19 KB

thin doesn't appeat to be cross-browser compliant... here's a version using 1px instead for the border. Screenshot forthcoming.

Per Dries, I've marked this RTBC.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
FileSize
Picture 3_5.png42.69 KB

Screenshot

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
FileSize
sig_3.patch12.49 KB

kkaefer reviewed and found several bugs, which are fixed in this patch:

1. I forgot to change themes/engines/phptemplate/comment.tpl.php. Whoops!
2. I forgot to add the signature stuff into theme_comment
3. the correct class should be clear-block and not clear, which is Garland-specific.

Thanks, kkaefer!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
FileSize
sig_4.patch12.49 KB

Missed silly theme_comment. ;)

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
FileSize
sig_5.patch14.28 KB

kkaefer noted an XSS vuln in theme_comment.. moving the check_markup into the comment.module logic, which unfortunately means I need to duplicate it 12,000 times. :P

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

It looks good to me -- I've committed this part to CVS HEAD. We can refine this as necessary in part 2 of 2.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented

OH MAN! :D

This is officially the best day EVER! :D Thank you thank you thank you!!

Theming docs updated: http://drupal.org/node/132442

kkaefer’s picture

kkaefer CreditAttribution: kkaefer commented

Part 2: http://drupal.org/node/132446

Steven’s picture

Steven CreditAttribution: Steven commented
Status: Fixed » Needs work

Gray? In Garland? Please.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Needs work » Needs review
FileSize
separator.patch3.76 KB

Actually, looks like the user.css hunk was never committed.

But Steven schooled me in some design-fu over IRC, and he's right; an emdash looks much less cluttered than "yet another bar that goes all the way across the screen."

So here they are in emdashes. And even better, I kept the <br /> out of the patch... since the signature gets wrapped in <p>, we don't need it.

Steven’s picture

Steven CreditAttribution: Steven commented
Status: Needs review » Fixed

We should still wrap the dash in a div IMO, so that we always have it a separate block level element. Some input formats may not output paragraphs tags.

Other than that, this is much better. Commited to HEAD. Thanks, webchick!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Fixed » Reviewed & tested by the community
FileSize
signature-cleanup.patch4.41 KB

When I was working on the 'part 2' patch, I discovered a much better way to handle parts of this one.

Rather than doing the logic for $comment->signature 1500 times in comment.module, this one does it once in user_comment(). It also adds a theme function which is called from theme_comment, so that:

a) when/if this is eventually re-used for nodes we don't have duplicate code.
b) the output isn't hard-coded into comment.module, but in user.module where it belongs.

I marked this RTBC because this patch was already reviewed as part of http://drupal.org/node/132446 and the only complaints were related to the node stuff which isn't part of this patch. Hope that's ok.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented

Oh. I should also add that cleaning this up makes it MUCH easier to add in a "disable" option for signatures, as I only have to put the logic in user_comment(). :)

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

Anonymous’s picture

(not verified) CreditAttribution: commented
Status: Fixed » Closed (fixed)