Better signature options to look more like PhpBB
| Project: | Signatures for Forums |
| Version: | 6.x-1.x-dev |
| Component: | Code |
| Category: | feature request |
| Priority: | normal |
| Assigned: | neclimdul |
| Status: | closed |
| Issue tags: | DruBB |
We're working on trying to get Advanced forum to look more like PhpBB (see #472590: Meta-issue: Make posting form more like phpBB's for some screenshots of the changes needed and #474716: Allow disabling signatures per-post for the signature issue.) One of the changes needed would be to add a checkbox to the bottom of the post where the user could choose whether or not to add a signature. This will require a fair bit of re-work of the way this module functions now since the option is not set per-post but per content type. We'll have to add in a table to keep track of the id of the post and what choice was made and then display or not display the signature based on that. We might also need to have a global default for each user and then have the checkbox default to their choice. And we need permissions for these options and a way for admins to turn them on or off globally.
We talked in IRC about this and other things that would be nice would be to have a way to choose whether or not to display other signatures and/or to load each signature only once per thread.
#1
tagging.
#2
Having a way NOT to display signatures would solve the upgrade problem of having to worry about double signatures. I'm *totally* on board with that.
#3
Oh! Me! Me me me! I want to work on this. :)
#4
stealing this one from you webchick - sorry
#5
Patch for the install file attached
Just creates a new table with comment ID and status of whether or not a signature should be attached to that comment/node
At this point it doesn't seem necessary to store the user id here too, because the user id will always be in the node/comment object. However if anyone feels it would be valuable then I'll add it too.
There's an install function that populates the table with the comment id/node id of all pre-existing comments/nodes and sets the comment status to disabled (0) to avoid duplication of comments.
This is a very insignificant step I know, but I just want to put all my progress up here today in case anyone wants/needs to pick up this over the weekend.
#6
working patch this time...
#7
The attached patch updates the .install and .module
I've tested this both with the DruBB setup, and with standard drupal comments, but would love some more testing with other setups.
Feedback and suggestions for improvements to the patch are very welcome.
#8
new attached patch fixes mistake in description in .install (thanks merlinofchaos) and correctly attaches checkbox to node forms, not just replies (thanks neclimdul).
neclimdul & merlinofchaos pointed out that both nodecomment and comment can now be enabled on a site at the same time, so I'm going to need to do some significant work on this patch so that the two don't clash - I was not distinguishing between comment ids and node ids.
#9
Some observations:
notice: Trying to get property of non-object in /home/liam/public_html/drupal6/sites/all/modules/signature_forums/signature_forum.module on line 320.notice: Trying to get property of non-object in /home/liam/public_html/drupal6/sites/all/modules/signature_forums/signature_forum.module on line 321.
$schema['signature_comments'] = array( ... );forgive the stupid question, but why does this table only relate to comments? Wouldn't this option be shown when a user is posting a node (or at least the first node in a thread) too?// deal with nodecomments and standard nodes// add option to disable/enable signature for this comment, if this option is availableThe coding standards say code comments should start with a capital letter and end with a full stop. The patch has lots of these small ommissions in the comments, the above are just examples. Kudos for writing comments though. :)
* A Boolean for whether or not the signature should be attached to this comment or node+ * @return+ * bool 0|1
This is rather ambiguous, do we use 'Boolean' or 'bool'? Also, should datatypes be capitalised? My guess is no, since they're not a proper noun.
+/**+ * Get the status for whether signature should be used for a particular node or comment
+ * @param $cid
+ * An integer containing the id of a comment or node
+ * @return
+ * bool 0|1
+ */
+
+function signature_forum_get_status($cid) {
+ return db_result(db_query("SELECT status from {signature_comments} WHERE cid = %d", $cid));
+}
That's enough for now. I'll admit that I haven't really got into/tried to understand the logic yet, expect more anality when I do get to it though!
#10
Interesting, even with all content types in Signature defaults unchecked I'm still seeing the 'Attach signature' checkbox on forms. I have 'Allow users to choose their own global default:' set to Yes, although switching it off doesn't seem to make any difference.
#11
#12
err forgot to comment. Previous patch should address the issues enumerated by Liam and some other I found.
#13
As discussed on IRC, problems with the patch are:
$schema['signature_comments'] should become $schema['signature_post']
coder.module found a few small issues, no full stop at the end of
Implementation of hook_update_n()and so on. No big problem and I can clear these up later if you're pushed for time.Per-post checkboxes, for content types, in admin/settings/signature_forum -> Per-post settings should be disabled by default.
+/**+ * Set all existing comments to not have a signature to avoid duplicate signatures.
+ */
+function signature_forum_build_signature_status() {
+ // Deal with nodecomments and standard nodes.
+ if (module_exists('nodecomment')) {
+ db_query("INSERT INTO {signature_comments} (cid, status)
+ SELECT nid, 0
+ FROM {node}");
+ }
+ // Deal with standard Drupal comments.
+ else {
+ db_query("INSERT INTO {signature_comments} (cid, status)
+ SELECT cid, 0
+ FROM {comments}");
+ }
}
Two issues with this code:
statusshould be inserted with an initial value of 1.$schema['signature_comments'] = array(+ 'fields' => array(
+ 'cid' => array(
'cid' should really be 'delta', since different types of id will be saved there.
#14
Additionally, here's what coder.module found:
signature_forum.module
Line 258: There should be no trailing spaces
}
Line 391: There should be no trailing spaces
'#default_value' => $use_signature,
Line 526: There should be no trailing spaces
// Check if signature is disabled for this comment/node
Line 689: There should be no trailing spaces
/**
Line 691: There should be no trailing spaces
* @param $cid
Line 707: There should be no trailing spaces
* @return
signature_forum.install
Line 83: There should be no trailing spaces
FROM {node}");
Line 115: Missing period
* Implementation of hook_update_n()
*** EDIT ***
Also, this elseif statement is pure evil:
+ // Add option to disable/enable signature for this comment, if this option is available.+ elseif (((($form_id == 'forum_reply_node_form' || (isset($form['type']) && isset($form['#node']) && $form['type']['#value'] .'_node_form' == $form_id)) && $settings['signature_forum_show_for_'. $form['type']['#value']] == 1 ) || $form_id == 'comment_form') && variable_get('user_signatures', FALSE)) {
Ouch! :)
Is there any way we could split this down a bit?
#15
Patch addressing the feedback from above #13/#14
#16
Few more things:
If 'Show signature by default' is off in the user's settings, the 'Attach signature' checkbox should default to unchecked on posts, however it is defaulting to checked.
Am seeing these errors after uninstalling/enabling the Signatures for Forums module:
warning: Invalid argument supplied for foreach() in /home/liam/public_html/drupal6/includes/common.inc on line 3297.warning: Invalid argument supplied for foreach() in /home/liam/public_html/drupal6/includes/common.inc on line 3218.
Even if none of the content types are checked in admin/settings/signature_forum -> Per-post settings, the 'Attach signature' checkbox appears anyway.
#17
Provide an initial value to deal with the first issue and remove some weirdness in hook_install() to deal with the second. Also fix some minor documentation inconsistencies I missed.
#18
Default option for 'Attach signature' is working, install seems to be fixed. Great!
However, the 'Attach signature' checkbox is appearing on comments (not nodes), even when no content types are selected under 'Show signatures with nodes and comments for' or 'Per-post settings' (in admin/settings/signature_forum). The problem is this logic in the elseif statement, on line 366:
(($form_id == 'forum_reply_node_form' || (isset($form['type']) && isset($form['#node']) && $form['type']['#value'] .'_node_form' == $form_id)) && $settings['signature_forum_show_for_'. $form['type']['#value']] == 1 ) || $form_id == 'comment_form')It's saying: if we've got these form elements and we're showing signatures for this node type, _or_ it's a comment, then show the 'Attach signature' checkbox. That's incorrect, since the content types checkboxes on the admin pages refer to both nodes and comments. :)
#19
Ok, I think this should address issues with core comments by providing its own settings checkbox. It is straightforward and simple though maybe a little hacky.
I rearranged the logic in that elseif to make it a little easier to read what's being checked as well(separate concepts on separate lines).
#20
#21
Committed to DRUPAL-6--1, thanks! :)
#22
Automatically closed -- issue fixed for 2 weeks with no activity.