Drupal Tough Love

chx and Morbus reviewed Printer-Friendly Pages 5.x-3.4 which "allows you to generate printer friendly versions of any node by navigating to www.example.com/print/nid, where nid is the node id of content to render. A link is inserted in the each node (configurable in the content type settings), that opens a version of the page with no sidebars, search boxes, navigation pages, etc."

First-post disclaimers: As the first post, we're still figuring out the best format and approach to distilling the information we proffer. If you have suggestions, concerns, or have eyeballed one of our own mistakes, please let us know by leaving a comment. Likewise, also note that there was no ulterior motive to picking Printer-Friendly Pages as our first review: we literally just browsed through the 5.x taxonomy term until we found a recent release of a module we knew was in actual use. Our reviews are also not necessarily security reviews: 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 Review

1. $nodetype should be $node_type. Additionally, the use of $nodetype has a slight problem:
   <?php
$nodetype = isset($node->type) ? $node->type : '';
$print["submitted"] = theme_get_setting("toggle_node_info_$nodetype") ...
?>

   Your use here, and similarly on line 55, allows a theme_get_setting() check on a variable that would never exist ("toggle_node_info_"). It'll be a few more lines of code, but you're better served with an if that checks if $node->type exists, then calls the proper display options. Your final use on line 508 ($print["type"] = $nodetype;) could be moved into the same if.

2. Coder only caught 3 errors (great job!) involving camelCase variable names (specifically, $queryArr). camelCase and mungedwords should be replaced with lower case variable names, split on word boundaries with a "_".
3. As the PHP documentation says on strstr(): "If you only want to determine if a particular needle occurs within haystack, use the faster and less memory intensive function strpos()..." Instead of strstr($module, 'book_printer'), use strpos($module, 'book_printer')) !== FALSE instead.
4. There's plenty of text files shipped with the module, and nearly all of them need work. Besides the standard "string retardation" (the name of the module is listed as "print", the module's filename, as opposed to "Printer-Friendly Pages", the module's actual name), and the README.txt and INSTALL.txt not using core style (in the absence of a style guide, Do What Core Does), they also can't agree on which format they themselves would like to use (README.txt uses numbered lists with either a), b) or i., ii., sub-items, whilst INSTALL.txt uses a dashed-list with i., ii., etc.). More egregious is the lack of hard newlines (depending, instead, on an editor's default word-wrapping capability, which browsers don't have, making these docs difficult to read online). In some cases, the docs are also slightly wrong (suggesting that it only works on nodes when it currently defaults to working on every page, an error that print.info also perpetuates). Finally, there's not a whole lot of rationale to have a CREDITS.txt and a MAINTAINER.txt in a contrib module - certainly, yes, it's "like core", but core is a lot bigger and maintained by far more people, which justifies their existence. Move the contents of your versions into your README.txt and get rid of 'em.
5. print.css has some missing semi-colons and its formatting could be improved.
6. Your print.install is great; thanks for deleting your variables. However, it's a bit odd to store most of a module's configuration inside a single variable (print_settings, etc.). Ideally, you'd only do that for things like lists, not for the entire batch of variables offered on a module's admin page. It's entirely possible this was a misstep or misuse of Forms API - there shouldn't ever be a real need to use #tree on a form intended for system_settings_form(). If you get rid of the #tree, you'll also be able to remove/collapse your various "default" functions (like print_robot_settings_default()).
7. The print.module starts off with a spurious use of a constant - there's no real need for define("PRINT_PATH", "print"), and if one is concerned about having a single place to customize the root path for all printer-friendly pages, move it into a variable for the admin pages. It's only four or five extra lines of code to make the module friendlier, and more inline with what core does (no part of core hardcodes a menu path to a constant).
8. A module that overrides hook_help() without providing its own help documentation is amusing.
9. Another bad habit we often see is "string first on equality tests" such as lines 128 and 137:
   <?php
if ('node_type_form' == $form_id) {
?>

   The implication, of course, is that a mistype of == to = would cause an error, since you can never assign anything to a string. The only rationale for using the above "workaround" is to indicate that not enough testing is happening. If a developer tests every line of code, obsessively and iteratively, they'd find "assignment instead of equality" bugs and could write and maintain code that doesn't use lazy tricks like the above. To make it worse, the module isn't consistent in this usage, with $type == 'comment' and count($matches) == 4 elsewhere. Replace these sorts of constructs, and don't rely on them to protect you from yourself - that's what testing is for.

10. Theme functions (here theme_print_format_link() and theme_print_text()) should never return an array. If you want data or configuration to be override-able, use a hook and module_invoke_all(); developers can decide the ordering of the overrides via the standard module weight capabilities of the system table. In Drupal 6, there's also the new drupal_alter(), which allows you to pass around data and get hooks for free. Theme documentation tends to start with "Generate the themed output", traditionally HTML, not "return a structured array of strings". The true intent of these functions seems to be to control the strings displayed in the generated printer-friendly version, such as on line 466:
    <?php
$print["printdate"] = $print_sourceurl_settings['date'] ? (" (". $themed['retrieved'] ." ". format_date(time(), 'small') .")") : "";
?>

    But this is also considered bad form: one shouldn't ever concatenate strings onto a t() (here, the t() is stored inside the $themed['retrieved'], which was generated with a call to theme('print_text')). Assuming that the above string is assigned to $print["printdate"], you end up with t('retrieved on') . $date. One would think this OK until they consider a language where the date would read before, or in the midst of, the translated "retrieved on" (such as Japanese). Because the date is always appended to the translated string, it would always translate incorrectly. This even assumes translation... in English, one wouldn't be able to rephrase this to "[DATE] (retrieved on)". A much better approach is:

    <?php
$print["printdate"] = $print_sourceurl_settings['date'] ? t('(retrieved on %date)', array('%date' => format_date(time(), 'small'))) : "";
?>

    Besides reading more clearly from a code perspective, it allows translators to put the %date where they need it. One could also argue that this gives themers more control over the strings displayed. While the conceit is nice, it's still wrong: the site's developers should have properly setup a locale toolkit to allow translating of strings stored in Drupal's t() or, in Drupal 6, you could modify the strings inside the revised settings.php (scroll all the way to the bottom for documentation on how to do this). The shortest guideline here is: don't override the theme functionality (or, for that matter, any programming construct that is meant to do only one thing). Theme functions are intended for HTML, not a piggy-back into developmental hooks (as your use of theme_print_format_link(), which replicates hook_link_alter() functionality, promotes).

11. print_generate_path() has very familiar menu code: this was committed to core for a few days and then rolled back (cf. this issue). This patch was one of those "unfixable" problems that lead to the menu system rewrite in Drupal 6. To sum up, _menu_append_contextual_items() was only ever designed to run once during a single page load: calling it a second time causes unpredictable results. One potential fix would be to set a flag in $_SESSION and do a drupal_goto() instead. Capturing the output passed to theme('page') is not easy in Drupal 5; you'd probably need to implement a phptemplate_variables() function or instruct your users to add your code there and use it to catch the content of theme hook page. You can check Advanced Forum for an example. Another trick you could pull is to provide an entirely new theme called 'print' and change to this theme in hook_menu() !$may_cache by tweaking the global $custom_theme (see Sections for an example).
12. Something else we see a lot of are bad #descriptions for form elements. Either they're wrong or out-of-date, an exact repetition of the element's #title, or they contain spurious information. There's a little bit of all three in the elements that the module adds to some of Drupal's standard forms:
    <?php
$form['viewing_options']['print_display_comment'] = array(
  '#type' => 'checkbox',
  '#title' => t('Show printer-friendly version link in individual comments'),
  '#return_value' => 1,
  '#default_value' => variable_get('print_display_comment', '0'),
  '#description' => t('Displays the link to a printer-friendly version of the comment. Further configuration is available on the !settings.', array('!settings' => l(t('settings page'), 'admin/settings/print' ))),
);
?>

    We'll start with the "Further configuration" sentence, which includes a link to the primary settings page. Not only is this dangerous (there's no check against user_access('administer print'), so a user who can administer comments may receive an "Access denied" message), but there's not a lot of benefit of going to the suggested URL: there's nothing additional that specifically affects the current decision. Removing that entire sentence leaves us with something that repeats the #title in a few more words and serves no useful purpose. This form element, as well as a similar checkbox for node types, should have its #description removed entirely. One final itch: integers don't use quotes (see #default_value).

13. Core uses "Implementation of hook_NAME()." - you've forgotten the ending period on a few of yours.
14. Use dangling commas on your arrays (From the coding standards: "Note the comma at the end of the last array element; This is not a typo! It helps prevent parsing errors if another element is placed at the end of the list later."). Some of your arrays do, others don't. Consistency, above all else, is key: if a developer is going to commit to something (mistake or otherwise), commit to it fully. Otherwise, it just looks sloppy.
15. If the module is named "Printer-Friendly Pages", use that as the title of the admin settings page, not "Printer-friendly". Also, the description should be revised to note that it can now function on more than just nodes. Decide if it's "Printer-Friendly" (the capitalization on drupal.org) or "Printer-friendly" (the more grammatically correct usage, and what most of your documentation properly repeats).
16. Lowercase AS in the foreach ($links AS $module => $link) on line 89.
17. Don't use '#return_value' => 1 on boolean checkboxes in Forms API. #return_value is one of those fun and obscure constructs where you can specify what value you want if the checkbox has been clicked. It defaults to 1 already, which is usually what people expect and want.
18. Standardize your form elements: under admin/settings/printer-friendly, you offer radio buttons ("Enabled" and "Disabled") for the printer-friendly link on nodes, but then change tactics by offering a single checkbox for the same feature, only for non-content pages. Get rid of the radio buttons and use the checkbox - for a decision involving merely "on" or "off" or "yes" and "no", a checkbox is always stronger than two radios.
    
    
19. Drupal core uses TRUE and FALSE not true and false.
20. Don't use PHP function aliases: use count(), not sizeof().
21. If an array has only one item, implode() does the right thing (i.e., no joining with the glue string). Don't isset($robots_meta[1]) ? implode(', ', $robots_meta) : $robots_meta[0];, just skip the ternary entirely and use implode() as normal.
22. The module's function documentation needs work - most functions have a one-line summary (good), but either skimp on the details (a @return string that depends on the one-liner to describe the results; best to remove the @return entirely) or leave them out wholesale (such as missing @params). Function summaries should always be one-line: "The first line of the block should contain a brief description of what the function does, beginning with a verb in the form "Do such and such" (rather than "Does such and such"). A longer description with usage notes may follow after a blank line. Each parameter should be listed with a @param directive, with a description indented on the following line."
23. We're not entirely sure why the code uses $base_url or $base_path in _print_var_generator() and print_rewrite_urls(). You should be able to do most everything you need with url() and its "absolute URL?" parameter. Part of our confusion lies in the fact that url() is used for some conditions, but not others. Regardless, if you absolutely did require $base_path, use base_path() instead.
24. $base_url is unused in print_generate_node() and the other _generate functions.