Posted by mcurry on June 1, 2011 at 2:47pm
2 followers
| Project: | Amazon Store |
| Version: | 7.x-1.x-dev |
| Component: | Code |
| Category: | task |
| Priority: | normal |
| Assigned: | mcurry |
| Status: | patch (to be ported) |
Issue Summary
Dealing with missing Browse Node name fields is something that will need to be done in multiple places. Currently it's done in in the amazon_store_browsenodes_panel.tpl.php module, and now I need to do it in a separate module I'm developing.
In keeping with the DRY principle, I'm going to add a utility function to deal with malformed Browse Node names.
<?php
/**
* Format and clean up a Browse Node name.
*
* This is a good place to handle <a href="http://drupal.org/node/1172442" title="http://drupal.org/node/1172442" rel="nofollow">http://drupal.org/node/1172442</a> and
* <a href="http://drupal.org/node/1172106" title="http://drupal.org/node/1172106" rel="nofollow">http://drupal.org/node/1172106</a> (Amazon API sometimes returns empty
* Browse Node names).
*
* This function uses t() function in conjunction with the 'amazon_store_missing_browsenode_name_text'
* variable. All occurances of the symbol @id will be replaced with the Browse
* Node Id.
*
* @param $id The current Browse Node Id.
* @param $name The name.
* @return A 'cleaned up' name.
*/
function amazon_store_cleanup_browsenode_name($id, $name) {
if ($name == null || strlen($name)==0) {
$name = t(variable_get('amazon_store_missing_browsenode_name_text', t('Unknown Category')), array('@id'=>$id));
}
return $name;
}
?>
Comments
#1
Proposed patch attached. Touches amazon_store.module and amazon_store_browsenodes_panel.tpl.php.
This is a very minor change and has been well-tested locally.
#2
Patch committed to 6.x; marking as to be ported to 7.x.
http://drupalcode.org/project/amazon_store.git/commit/5f40a61
#3
Is the panel the only place that needs to call the cleanup? Or are there others?
#4
Excellent question.
There are others in other modules I'm building. For example, I'm creating my own variant of the Browse Node info panel (with different links for the Browse Node navigation), and I need to duplicate most of the code in the original amazon_store_browsenodes_panel.tpl.php code.
I expect to add enhanced Browse Node support to the Amazon Store module in the near future, and I will need to deal with empty Browse Node names.
Also consider what happens when people need to copy and modify the existing amazon_store_browsenodes_panel.tpl.php file to customize it for their theme. Now we have the same 'cleanup' logic replicated in several places. Granted this is a minor example.
I made the addition because, in my judgement, the method of dealing with blank Browse Node names is likely to change in the future, and needs to be regular across this and other modules/themes that may rely on the Amazon Store module. And we need to demonstrate the proper method in the single example extant today (the Browse Info panel).
That, and I'm frankly amazed at the amount of code duplication I find in most Drupal modules. Especially when it comes to reading variable values using variable_get(). I find those cases troublesome, because every invocation of variable_get() requires that you provide a default value, and therefore multiple calls to variable_get() on the same variable creates a maintenance problem (the possibility of inconsistent use of the default value). This is a seemingly trivial case, but I've gotta start somewhere. :D
It's a judgement call, and a coding style thing. My preference is to avoid duplicating logic wherever possible, even if the logic isn't all that hairy.