Taxonomy Term Google Maps

In file term_gmap.module
Line 46-47
Maybe in most installations, it's common that when a path starts with "taxonomy" then the full path is taxonomy/term/[:tid] but it might not actually be the case. I would replace lines 46-47 to the following:
if (arg(0)=='taxonomy' && arg(1) == 'term' && $termid = intval(arg(2)) {

Line 76
Even though not required in Drupal7, I think it would be best if you used a default value for variable_get('term_gmap_large').

Line 120
You could use the t() function for the content of the #markup element.

Line 124-126
You use the vocabulary name as loaded from the database and then call the t() function upon that variable. Are you sure there is no other way? You could use the i18nstrings() function if available. It's the equivalent of Drupal 6 tt() function.

That's all from me right now. Great idea for a module though! :)

The install file and info file look fine, except for coding standards. In the file term_gmap.module:

Drupal Coding Standards:
1. Extra Whitespace on lines: 36
2. Incorrect number of space indents on lines: 46-55, etc
3. Spaces should surround an equals sign on lines: 156, 159, etc
There are many more problems than just these. See coder and coder_reviews projects to get an idea on what the code should look like.

Security Problems:
- Line 59, $title should use check_plain - Similar on lines 75, 77, etc

Multithreading:
- term_gmap_admin_settings_submit does not appear thread-safe. Could use a single call or transactions.

Code Placement:
- Javascript should be placed a js file and using Drupal.settings to pass the $title variable.

1. You must delete the variables when uninstalling the module. E.g. variable_del("term_gmap_width").
2. You must check the data filled by user using the function check_plain().

manual review:

1. term_gmap_uninstall(): drupal will uninstall the schema automatically for you, so remove drupal_uninstall_schema().
2. term_gmap_block_view(): put your javascript in a dedicated JS file and use Drupal.settings to pass down the variables from PHP to javascript. See http://drupal.org/node/756722
3. term_gmap_block_view(): this is vulnerable to XSS exploits. Term names are user provided input and need to be sanitized before printing. If I have a term name foo'; alert('XSS'); var x ='x I get a nasty javascript popup. Please read http://drupal.org/node/28984 again. That would not happen if you pass down the variables with Drupal.settings ;-)
4. term_gmap_term_settings(): this is also vulnerable to XSS: "'#title' => t($value),". Vocabulary names are also user provided text.
5. term_gmap_term_settings(): that will not scale. For large vocabularies you will get a very long checkbox list that could result in memory exhasution/WSOD.
6. term_gmap_get_maped_terms(): use db_select() und ->fetchColumn() or similar instead, then you don't need to foreach loop.

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

@ramsonkr,

Here some results of my Manual Review:

Unconsistant module folder name: it should be "term_gmap" : instead of "taxonomy_term_google_maps"

Lines' length: In term_gmap.module, there are too long lines like 49, 58, 60, 71, 108, 160. You coud make them more readable by going to next line.

link function: In term_gmap.module / Line 60 :
You better use l() function to build the link instead of ...

Use theme: In term_gmap.module on Lines 58 to 61 :
$block['content'] = xxxx
should go through a theme function in order to allow themer to change the design.

More info on variable_del: You must delete the variables when uninstalling the module. E.g. variable_del("term_gmap_width"). Your function term_gmap_admin_settings() creates 4 variables in variable table.
You need hook_uninstall() as term_gmap_uninstall() in term_gmap.install and delete all these 4 variables.

Menus:
Using local task menus will enhance the usability: See MENU_DEFAULT_LOCAL_TASK

Hope this helps.

Changing status base on comment #11. At least the variable_del() issue should be fixed.

the folder name is determined automatically for sandboxes by the project page title, so that is not an issue. Once this gets promoted the short name should be term_gmap, then the folder name used in git clone will also be term_gmap.

Line length: Drupal coding standards do not forbid code lines longer than 80 characters. Only comments have to be below that limit. So that is fine as well.

@klausi,

Thks for these very useful details.

@ramsonk,

I think you should also add variable_del('term_gmap_large');

I still think that using local task menus will enhance the usability: See MENU_DEFAULT_LOCAL_TASK, because today use or links to go from one settings page to another is quite different of the usual Drupal interface.

@ramsonkr,

Tried to install and use it:

in README;
- Now go to block administartion = typo

I followed the README and did every thing but I don't see any map.
Could you add more details, so every one may understand what to do to make it work?

On a related subject, could you enhance your Project page by following "Tips for a great project page"?

Looks RTBC to me. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

no objections for more than a week, so ...

Thanks for your contribution, ramsonkr!

I updated your account to let you promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and get involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.