Upgrade Your Drupal Skills
We trained 1,000+ Drupal Developers over the last decade.
See Advanced Courses NAH, I know EnoughPrinter-Friendly Pages 5.x-3.4
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
$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 anif
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 sameif
.- 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 "_". - 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 functionstrpos()
..." Instead ofstrstr($module, 'book_printer')
, usestrpos($module, 'book_printer')) !== FALSE
instead. - 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
andINSTALL.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, whilstINSTALL.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 thatprint.info
also perpetuates). Finally, there's not a whole lot of rationale to have aCREDITS.txt
and aMAINTAINER.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 yourREADME.txt
and get rid of 'em. print.css
has some missing semi-colons and its formatting could be improved.- 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 forsystem_settings_form()
. If you get rid of the#tree
, you'll also be able to remove/collapse your various "default" functions (likeprint_robot_settings_default()
). - The
print.module
starts off with a spurious use of a constant - there's no real need fordefine("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). - A module that overrides
hook_help()
without providing its own help documentation is amusing. - 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'
andcount($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. - Theme functions (here
theme_print_format_link()
andtheme_print_text()
) should never return an array. If you want data or configuration to be override-able, use a hook andmodule_invoke_all()
; developers can decide the ordering of the overrides via the standard module weight capabilities of thesystem
table. In Drupal 6, there's also the newdrupal_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, thet()
is stored inside the$themed['retrieved']
, which was generated with a call totheme('print_text')
). Assuming that the above string is assigned to$print["printdate"]
, you end up witht('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'st()
or, in Drupal 6, you could modify the strings inside the revisedsettings.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 oftheme_print_format_link()
, which replicateshook_link_alter()
functionality, promotes). 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 adrupal_goto()
instead. Capturing the output passed totheme('page')
is not easy in Drupal 5; you'd probably need to implement aphptemplate_variables()
function or instruct your users to add your code there and use it to catch the content of theme hookpage
. 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 inhook_menu()
!$may_cache
by tweaking the global$custom_theme
(see Sections for an example).- Something else we see a lot of are bad
#description
s 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 canadminister 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
). - Core uses "
Implementation of hook_NAME().
" - you've forgotten the ending period on a few of yours. - 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.
- If the module is named "Printer-Friendly Pages", use that as the
title
of the admin settings page, not "Printer-friendly". Also, thedescription
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). - Lowercase
AS
in theforeach ($links AS $module => $link)
on line 89. - 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. - 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.
- Drupal core uses
TRUE
andFALSE
nottrue
andfalse
. - Don't use PHP function aliases: use
count()
, notsizeof()
. - If an array has only one item,
implode()
does the right thing (i.e., no joining with the glue string). Don'tisset($robots_meta[1]) ? implode(', ', $robots_meta) : $robots_meta[0];
, just skip the ternary entirely and useimplode()
as normal. - 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@param
s). 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." - We're not entirely sure why the code uses
$base_url
or$base_path
in_print_var_generator()
andprint_rewrite_urls()
. You should be able to do most everything you need withurl()
and its "absolute URL?" parameter. Part of our confusion lies in the fact thaturl()
is used for some conditions, but not others. Regardless, if you absolutely did require$base_path
, usebase_path()
instead. $base_url
is unused inprint_generate_node()
and the other_generate
functions.
About Drupal Sun
Drupal Sun is an Evolving Web project. It allows you to:
- Do full-text search on all the articles in Drupal Planet (thanks to Apache Solr)
- Facet based on tags, author, or feed
- Flip through articles quickly (with j/k or arrow keys) to find what you're interested in
- View the entire article text inline, or in the context of the site where it was created
See the blog post at Evolving Web