Drupal Tough Love

chx and Morbus reviewed Signatures for Forums 5.x-2.3 which "provides user signatures that will be familiar to users of popular forum software" such as "the administrator can choose the input filter for signatures", conditional signatures that are hidden "if a post is under a particular length", and showing the signature only once per conversation. Liam McDermott, the developer, requested this review.

Standard disclaimers: If you have a suggestion on how to better these reviews, or have eyeballed one of our own mistakes, please let us know by leaving a comment. Our reviews are not necessarily security audits: if we don't mention any problems, it doesn't mean we assert the module is Security Team approved (of which we're both members). Finally, if you'd like us to review your own (contributed) module code, don't hesitate to let Morbus or chx know and we'll add it to our queue.

The Reviewed Files

• signature_forum.info
• signature_forum.install
• signature_forum.module

The Review

1. There's no README.txt. One would assume that, if a developer is going to duplicate a "popular forum" software's functionality, they should also provide handholding on how to migrate users of that software to Drupal. The lack of a README.txt negatively impacts perceptions: the potential migrator asks "do all Drupal modules lack documentation?" or "am I a second-class citizen because I merely want to do what I've done before?" A module's project page on drupal.org should not be used for this purpose. Create a README.txt in the same text format as core: see bot.module's README.txt for an example.
2. Is it "Signatures for Forums" (the module's project page; uppercase F) or "Signatures for forums" (inside signature_forum.info; lowercase F) or just "signature" (inside hook_perm())? Our inclination would be "Signatures for Forums", since it's a proper name (as opposed to most other strings used inside a module).
3. The signature_forum.info uses a package of "User goodies" and this is discouraged: "If your module comes with other modules or is meant to be used exclusively with other modules, enter the name of the package here ... In general, this field should only be used by large multi-module packages, or by modules meant to extend these packages, such as CCK, Views, E-Commerce, Organic Groups ... All other modules should leave this blank."
4. We'll gladly confess ignorance, but we've not personally used other forums that a) hide a signature if the post's content is small or b) only shows the signature once each thread or conversation. It seems that, for example, phpBB needs a mod to show the signature once per thread; vBulletin also needs a change one way or another. We recommend reconsidering "familiar to users of popular forum software". These two features are mentioned in the module description and also implied by signature_forum.info: "Manages users signatures in the style most forum users will be familiar with." Descriptions and documentation should strive to be free of opinions: we'd revise this to something like "Tweaks signatures in ways inspired by other traditional forum software." It says much the same thing, but doesn't make us feel out of touch. This would also apply to the definition in hook_help().
5. Coder found no errors. Awesome.
6. Core uses "Implementation of hook_NAME()." - some definitions are missing the ending period.
7. One of Drupal Tough Love's primary tenets is "to do what core does" and this applies equally to every aspect of module creation. A large number of Drupal 5 contributions don't support PostgreSQL because their developers "don't have it installed", even though their SQL would run just fine if only there were a proper hook_install(). Drupal 6's SchemaAPI solves this issue (for table definition, at least), but there's still column retardation, where a column representing a user's ID (or any other standardized data) is marked up in a different way than in core. For example, in signature_forum.install, we see:
   <?php
case 'mysql':
case 'mysqli':
case 'pgsql':
  db_query("CREATE TABLE {users_signature} (
    uid integer,
    signature text default NULL,
    PRIMARY KEY (uid)
    ) /*!40100 DEFAULT CHARACTER SET UTF8 */ ");
?>

   Not only does this force all three databases into one definition (causing the MySQL UTF8 workaround on the final line to be erroneously applied, but thankfully ignored, by PostgreSQL) but uid integer is (letter-for-letter) a different definition than what core uses. When in doubt, steal liberally from core's own .install files. The above would more correctly be written in Drupal 5 as:

   <?php
case 'mysql':
case 'mysqli':
  db_query("CREATE TABLE {users_signature} (
    uid int NOT NULL default '0',
    signature text,
    PRIMARY KEY (uid)
    ) /*!40100 DEFAULT CHARACTER SET UTF8 */ ");

  break;

case 'pgsql':
  db_query("CREATE TABLE {users_signature} (
    uid int NOT NULL default '0',
    signature text,
    PRIMARY KEY (uid)
    )");

  break;
?>

   Eagle-eyed viewers will note we left out the default NULL on the definition for signature text. MySQL "BLOB and TEXT columns cannot have DEFAULT values", but it will gladly ignore the attempt. Since there's no plausible default on MySQL, we did not bother providing one for PostgreSQL. Sure, this is more code for little actual gain, but it's also more accurate and standardized. Even the Signature for Forums .install for Drupal 6's Schema API isn't "like core"; compare the definition of a uid column in node.install with this module's 'uid' => array('type' => 'int', 'unsigned' => TRUE, 'not null' => TRUE, 'default' => 0) - there's no description, but there is an unnecessary unsigned.

   Steal liberally from core's definitions and patterns: if you mean the same thing, say the same too.

8. Drupal core uses constants to give human-readable names to numeric values, and Signature for Forums does the same, even going the extra distance by properly documenting them. Thank you!
9. This module contains another example of "store our module-specific settings in a single array". We're still not entirely sure where this pattern is coming from (if you know, tell us!). There seems little apparent rationale besides "I'm too lazy to variable_del() each of them in my hook_uninstall().", "I'm gonna forget to update it when I add a new setting.", or the unarguable "I like it!" By forcing your module's settings into an array, you waste a line loading all your settings to check just one value, and also require an extra function to build the defaults (seen also in Printer-Friendly Pages #6, only in slightly different form). We're not seeing how this is such an advantage that inventing something "not like core" is warranted. A healthy amount of code is reinvented in signature_forum_admin_settings_submit() too.

   Take a look at, pseudo-roughly, what Signatures for Forums currently does for its configuration:

   <?php
function signature_forum_defaults() {
  return array(
    ...
    'signature_forum_template' => " [default value] ",
  );
}

function signature_forum_form($default_values) {
  $form['template'] = array(
    ...
    '#default_value' => $default_values['signature_forum_template'],
  );

  return $form;
}

function signature_forum_admin_settings_submit($form_id, $form_values) {
  if ($form_values['op'] == 'Reset to defaults') {
    variable_set('signature_forum_settings', signature_forum_defaults());
    return;
  }

  ...
  $settings['signature_forum_template'] = $form_values['template'];
  variable_set('signature_forum_settings', $settings);
}
?>

   And, compare it to proper approach for use with system_settings_form():

   <?php
function signature_forum_form() {
  $form['signature_forum_template'] = array(
    ...
    '#default_value' => variable_get('signature_forum_template', ' [default value] ');
  );

  return system_settings_form($form);
}
?>

   The second code sample is literally all you need when using system_settings_form() and module configuration variables properly. One doesn't have to worry about resetting the default values, creating an extra function to hold them all, or renaming them from the Form API definitions to what they actually get stored as. (For Signatures for Forums, an extra #submit function would also be required to handle the deletion of hardcoded user signatures in existing posts.)

   Inventing new approaches to established Drupal patterns merely walls your module into its own little garden, requiring even more special knowledge to enter. Drupal already requires a lot to learning - help folks out by attempting to keep the established patterns prevalent in your modules. One may argue, and many have, that the core approach needlessly replicates the duplicate value across every use of that variable. And, they'd be right, but the proper way to enact change is to get a patch into core so that everyone benefits, such as the proposed hook_variables() to control and define default values.

10. Use elseif not else if for your conditionals.
11. The $section argument of hook_help() doesn't need a default value - one is always passed through by core. Also, the documentation within needs to be rewritten: besides a missing grammatical semi-colon, it suggests that users can use BBCode in their signatures, but doesn't provide any help on how.
12. user_access() properly restricts administrative configuration in your menu definitions, but the same permission is erroneously checked again within signature_forum_admin_settings(). The approach to that function is slightly odd too: we could see the rationale if signature_forum_form() (the actual form definition) was called multiple times throughout the code, but it isn't, instead serving as just another layer of abstraction. We'd rewrite signature_forum_admin_settings() to something like the below, which removes signature_forum_form() entirely. Combining this with #9 (above) will push your module configuration firmly into the established patterns inherent in core.
    <?php
function signature_forum_admin_settings() {
  $settings = variable_get('signature_forum_settings', signature_forum_defaults());

  $form['signature'] = array( ... );

  // include the rest of signature_forum_form() here.

  return system_settings_form($form);
}
?>

13. Generally speaking, all form elements should have a #title. If the #title isn't descriptive enough, a #description can be used to further explain what's going on. Fieldsets are used to collect similar fields together (literally, a fieldset is a "set of fields"). signature_forum_form(), however, has a fieldset with no #title and only "one" item (checkboxes for a site's node types). While this approach is OK here because it models the "Display post information on" fieldset in Drupal's theme configuration (admin/build/themes/settings), your current #description should move to the #title.
14. The module's configuration page has a fieldset entitled "Show signatures once per conversation" - this is action text best reserved for an interactive form element, such as a checkbox or radio button. Rename the fieldset to something more descriptive of the fields within, such as "Per-conversation signatures".
15. t('<strong>%s</strong> will be replaced with user\'s signature.'). Once upon a time, PHP was a bit slower when using double quotes for its strings, so people tried to avoid it all costs. This eventually became "scripture", disregarding the fact that noone even remembers which PHP version cured the lag; we can't bother to look up whether it was PHP 4.0 or PHP 4.1. Use double quotes: they are not your enemy! Needlessly escaped apostrophes, on the other hand, make code hard to read. signature_forum_help(), signature_forum_menu(), signature_forum_form(), and a few others, all contain escaped apostrophes. Lastly, in this particular example, you need "the user's signature", not just "user's signature". UPDATE: You should also remove the <strong> - not only is the use of HTML in t() questionable, but core doesn't markup placeholders in this way.
16. Signature for Forums offers a signature template that the admin can customize, placing a %s where they want a user's signature to appear by default. To interpolate the user's signature into this template, the code uses sprintf() in signature_forum_get_signature():
    <?php
$signature = sprintf($settings['signature_forum_template'], trim($signature));
?>

    The Drupal way to do this, exemplified in situations like mails sent out to users (admin/user/settings), is with strtr(), which also allows the use of named placeholders (i.e., !signature). Consider replacing your existing %s with !signature and the sprintf() with:

    <?php
$signature = strtr($settings['signature_forum_template'], array('!signature' => trim($signature));
?>

17. We use FALSE and TRUE, not the lowercase variants.
18. A critical bug exists with the use of check_markup() in signature_forum_get_signature(), which is rigged to err on the safe side by defaulting its $check attribute to TRUE. This means that, for every single visitor, they're being checked for whether they have access to the specified format. For example, if the submitter has access to, say, the <img> tag but the visitor does not, then the signature will be filtered into an empty string. You need to add a third parameter, FALSE, to your check_markup(). filter_form() takes care to show only the filter formats the submitter has access to, so that's not a problem.
19. Replace the longer code in signature_forum_nodeapi() with $node->content['body']['#value'] .= signature_forum_get_signature($node);. The matching logic in signature_forum_comment() is correct.
20. Some of your SQL uses column=%d. The proper style is a space on both sides of the equal sign.