Support from Acquia helps fund testing for Drupal Acquia logo

Comments

socialtalker’s picture

subscribe

sbsimmons86’s picture

subscribe

majortom’s picture

I need this module to actually upgrade to Drupal 7. Any plans for a release?

Cray Flatline’s picture

Subscribe

RikiB’s picture

subscribing :)

Fidelix’s picture

Subscribing...

randompants’s picture

Subscribing!

Niklas Fiekas’s picture

Subscribe.

cparker15’s picture

subscribing

Niklas Fiekas’s picture

Of the features

  1. when a user changes their signature, all their posts will be updated;
  2. signatures are automatically added to posts, instead of being inserted into the post text;
  3. the administrator can choose the input filter for signatures, allowing BBCode -- if the BBCode module is installed -- or HTML to be used;
  4. signatures are longer than the the Drupal default
  5. conditional signatures, these are hidden, or rel=nofollow'd if a post is under a particular length;
  6. show signature only once per conversation;

1, 2 and 3 (use better_formats for more fine grained control?) are supported by D7 core, I think.

cparker15’s picture

3 is supported by D7 core... just checked on my local dev install.

1 and 2 unfortunately only apply to comments, not to forum topics themselves... again, just checked.

From the better_formats project page:

Some of the features in BF are in Drupal 7 core now. Ability to order formats to give a default is in D7 core. BF in D7 will focus on filling in the gaps that are not in core and is under heavy development and is NOT yet ready.

Drupal 7: Under development. Not ready for use on production sites.

Niklas Fiekas’s picture

1 and 2 unfortunately only apply to comments, not to forum topics themselves... again, just checked.

Oh, that's an important point I wasn't aware of.

Niklas Fiekas’s picture

I experimented with this in a sandbox project: https://github.com/niklasf/better_signatures / http://drupal.org/sandbox/niklas/1253374

Signatures in nodes work pretty well.

(Careful, backup your database before testing - especially when altering the maximum signature length in the admin settings, because that's not well tested and has some TODOs.)

BeaPower’s picture

sub, any other successful tests to #13?

Liam McDermott’s picture

It would be better if you supplied a patch for this module than writing an entirely new one from scratch.

I'm busy upgrading another module at the moment, but will get to upgrading this one. It's just a matter of time (unfortunately).

Niklas Fiekas’s picture

Liam McDermott, of course, and I will, if I have some more time. I was just experimenting and learning, because I am not sure what I tried is the way to go. For example: Use hook_field_extra_fields() to display the signature in nodes?

One point is, that there might be a very direct way to port the module and one way making use of new Drupal 7 features. While the last one would make a big patch and two very different versions of Signatures for Forums, I think (but I am again not sure) that this is the way to go.

Liam McDermott’s picture

I see what you're saying now, thanks for posting.

I was just experimenting and learning, because I am not sure what I tried is the way to go. For example: Use hook_field_extra_fields() to display the signature in nodes?

I've mostly been getting into DBTNG and haven't had a chance to look at the fields stuff yet, but this does sound interesting. I'm definitely going to have a look at what you've done and post some feedback here.

One point is, that there might be a very direct way to port the module and one way making use of new Drupal 7 features.

Good point, might be best to create a 7.x-1.x branch for a straight port and a 7.x-2.x branch for a rewrite. Although the 6.x-1.x-dev branch isn't in a great state anyway, so perhaps it would be better to just do the rewrite instead of trying a straight port. Hmmm.

Sorry this is taking so long guys!

Niklas Fiekas’s picture

Great, looking forward to your review. The more intresting part is hook_field_extra_fields() and hook_node_view(). The rest is just an experiment with updating the database field length of signatures.

My suggestions for a rewrite:

  • Use hook_node_view() and hook_comment_view() to add signatures to the render array of nodes and comments.
  • Use hook_field_extra_fields() to allow the user turning on and off signatures for the different content types, with reasonable default settings. I.e. signatures enabled in forum post nodes and their comments by default.
  • Integrate the settings form into the existing sytem form. I actually found this todo in the code:
    TODO: Move admin form to existing signature form under admin/user/settings.

    What happens with the "administer Signatures for Forums" permission? Most likely that should be removed, if extending the default admin form.

  • Remove the 'signature_forum_template' => "__________________\n<p>%s</p>", setting and leave that for the theme layer. There is theme_user_signature().
  • Don't use an extra table for signatures. Drupal 7 has a signature field in the users table. Alter the field length using hook_schema_alter(). Otherwise only 255 characters would be supported.
  • I am not sure about the "display signatures once per thread" thing. I'll need to investigate how you would do that.

I'll create another sandbox or a branch in my existing one and try some of this out, trying to stay closer to Signatures for Forums. I'll submit a link or patch once I have something to show.

Liam McDermott’s picture

The more intresting part is hook_field_extra_fields() and hook_node_view().

Need to do some more reading to understand hook_field_extra_fields() works but using hook_node_view() seems like a good idea.

The rest is just an experiment with updating the database field length of signatures.

Altering the size of the field in {users} was also suggested by merlinofchaos some time ago. Although it needs to be a MEDIUMTEXT (or whatever the schema equivalent of that is) to be compatible with vBulletin signatures.

Just a note: There are performance implications (in MySQL) with including blobs/text fields in select queries, but I noticed there's already a blob in the {users} table (that is included in the select query run to load the user entity) so it shouldn't be a problem.

Use hook_field_extra_fields() to allow the user turning on and off signatures for the different content types, with reasonable default settings. I.e. signatures enabled in forum post nodes and their comments by default.

Sounds good to me.

Integrate the settings form into the existing sytem form. I actually found this todo in the code: [...]

Been wanting to do that for a long time but didn't want to confuse users, hopefully they'll expect changes in a major upgrade though and agree we should use this opportunity to introduce major changes.

What happens with the "administer Signatures for Forums" permission? Most likely that should be removed, if extending the default admin form.

Good point, let's ditch it. :)

Remove the 'signature_forum_template' => "__________________\n

%s

", setting and leave that for the theme layer. There is theme_user_signature().

This is probably the last holdover from the original Signatures module this is based on. Been itching to get rid of it but was worried someone might be relying on it. This is the time to introduce breakage though, so agreed, let's get rid of it.

I am not sure about the "display signatures once per thread" thing. I'll need to investigate how you would do that.

Probably a good idea to write tests for this before coding, since it has to take into account the different threading modes in comment.module and testing each of them manually to make sure they work takes longer than just writing a test. :)

That said, the logic probably will not need to be changed much to go from Drupal 6 to 7.

BeaPower’s picture

You can just add a signature field in Profile 2 and show...

Niklas Fiekas’s picture

Probably a good idea to write tests for this before coding, since it has to take into account the different threading modes in comment.module and testing each of them manually to make sure they work takes longer than just writing a test. :)

Test driven development with Drupal (or at least with DrupalWebTestCase) is painful on my machine, because tests take minutes to run. Maybe, once there are some tests, you can apply for PIFT testing #689990: Contrib projects to be included in beta stage of automated testing for modules, so that patches in the issue queue are automatically tested.

Need to do some more reading to understand hook_field_extra_fields() works but using hook_node_view() seems like a good idea.

Yes. hook_node_view() does what hook_nodeapi() does before: Adding stuff to the render array of a node, say "signature". In hook_field_extra_fields() you can say: "signature" is part of the render array of this content type. Then the user can set #weight and visibility for that part of the render array in field_ui.

 

@BeaPower: yes, but Signatures for Forums has more features you don't have with Profile 2 alone.

 

Edit: I came across another problem with the minimum content length. Now that nodes are fieldable, users might remove the body field of nodes and add their custom named ones. Even Drupal core has problems with this new flexibility (see #1038652: Notices in comment_submit() and comment_admin_overview() when body field does not exist or is not required for example). What would the behaviour be, if a node has no body field? What would the behaviour be if a node has no default body field, but one or more other textfields attached (considering, that once a user removes the core body field named "body", he can only add a new body field named "field_*")?

 

Edit 2: Work in progress at https://github.com/niklasf/signature_forum

Niklas Fiekas’s picture

Assigned: Unassigned » Niklas Fiekas
Status: Active » Needs review
FileSize
28.77 KB
79.52 KB

Pfew. I have a working port / rewrite. It's a giant patch, so it will need - as you said - a lot of testing and review.

Especially the update function from D6 to D7 needs testing, as I had no real D6 data to try it with. (It is self-evident not to use this in production. Backup your database, because the old tables will be dropped. You'll loose everything if the upgrade fails.)

Looking forward to getting some feedback.

Edit:
If it's good enough to be a base for further work, consider creating a branch for it.

Niklas Fiekas’s picture

As always, once I have submitted my files, I remember a problem I didn't think of then. There is a core bug I patched on my local machine: #879580: States fail when using integer values for select/radio dependencies. So I forgot to use a workaround.
Here are new files - workaround included.

Liam McDermott’s picture

Epic patch, thanks Niklas! Should I be testing the patch in #22, since that core issue you referred to is fixed in D7.8?

At first glance this certainly does look good enough to build upon; when you say 'create a branch for it.' Do you mean just 7.x-1.x-dev branch or some other branch ('signaturesasfields' for example)? I'm happy to make this the basis of our D7 version, if you don't mind. :)

Thanks again for the hard work!

Niklas Fiekas’s picture

It's only two strval()'s that work around the bug so I don't know - maybe just include the workaround.

Both 7.x-1.x-dev or signaturesasfields would be aswesome. I realize it's a giant patch and might be painful to review - so the alternative would be coming up with smaller, incremental parts. Only, if you do base 7.x-1.x on it (and I would like that) we should have a period of bug fixing before a first release.

Liam McDermott’s picture

Title: Drupal 7 version? » Drupal 7 version
Category: feature » task

Updating description to match the git commit message. Also, I've created a Drupal 7 dev version, it's going to take a while before Drupal.org packages the new code, creating a download though.

The -dev release is based on this patch, and I've put a note on the front page that people use it at their own risk, hopefully we'll get some useful feedback.

Niklas Fiekas’s picture

Status: Needs review » Fixed

Awesome! Then I believe we can close this one and open seperate issue for everything we find.

Liam McDermott’s picture

Awesome! Then I believe we can close this one and open seperate issue for everything we find.

Thanks for that. Are you subscribed to the project via e-mail? I'm already coming up with questions to create issues for. :)

Also, the admin area looks great, you did a really good job on the vertical tabs and auto-hiding magic.

Niklas Fiekas’s picture

Ok, I just subcribed to all issues.

It was the first or secound time a coded something with auto hiding in D7, but #states makes it pretty easy ;)

Status: Fixed » Closed (fixed)
Issue tags: -#D7

Automatically closed -- issue fixed for 2 weeks with no activity.