google_image_sitemap

Manual Review

1. Use 7.x-1.x branch instead of master for development. Check Release naming conventions
2. In your .install file, i did not find any install or uninstall hooks.
3. use t() functions for menu descriptions, form_set_error msgs.
4. Lot of whitespacing and unnecessary breaks are present in your module. Please do cleanup your module.
5. in .info file, please use a separate package to avoid confusion with other module packages.

coder Review

Please find the attached file

formatted text.

It appears that there have been multiple project applications opened under your username:

Project 1: https://drupal.org/node/1416240
Project 2: https://drupal.org/node/1413866

As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue.

Please close one of them!

Firstly, this is a great idea for a module, keep up the hard work.

Couple of things I noticed:

• Your comment reads Implements hook_perm() when the hook used is actually `hook_permission`.
• I'm still a little unsure what range start and range end mean. Are these dates, node id's or something else? Consider adding some text to clear this up
• I also got an error below when clicking on the URL of my sitemap. I wasn't able to continue from here. I'm not sure if this is my fault or a problem with the module at this stage.

PDOException: SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '-1' at line 4: SELECT n.nid AS nid, n.created AS created, n.title AS title, f.uri AS uri FROM {node} n INNER JOIN {file_usage} fu ON n.nid = fu.id INNER JOIN {file_managed} f ON fu.fid = f.fid WHERE (n.type = :db_condition_placeholder_0) AND (n.status = :db_condition_placeholder_1) AND (f.filemime IN (:db_condition_placeholder_2, :db_condition_placeholder_3, :db_condition_placeholder_4, :db_condition_placeholder_5)) ORDER BY n.created DESC, f.timestamp DESC LIMIT 19 OFFSET -1; Array ( [:db_condition_placeholder_0] => article [:db_condition_placeholder_1] => 1 [:db_condition_placeholder_2] => image/png [:db_condition_placeholder_3] => image/jpg [:db_condition_placeholder_4] => image/gif [:db_condition_placeholder_5] => image/jpeg ) in _google_image_sitemap_build() (line 269 of /home/sam/Sites/drupal7/sites/default/modules/google_image_sitemap/google_image_sitemap.module).

The URL in question, that I was linked to was as follows: http://drupal7.sam/#overlay=%3Fq%3Dadmin%252Fconfig%252Fsearch%252Fgoogl...

Just a quick fix needs to be done :
Don't wrap menu titles & description in hook_menu in t() http://drupal.org/node/140311

Not changing status, so that more-in depth review chances dont' get lost.

Thanks

manual review:

• _google_image_sitemap_list(): why do you need to hash the ids with md5 here?
• _google_image_sitemap_create_form(): "article" is hard coded.
• "'#options' => $node_types": you need to sanitize node type names before printing them to avoid XSS vulnerabilities. #options is only sanitized for select boxes by the form API. See http://drupal.org/node/28984
• _google_image_sitemap_create_form_submit(): use drupal_write_record() instead of db_insert() and db_update().
• _google_image_sitemap_delete_form(): use db_delete() here. Same in google_image_sitemap_node_type_delete().
• _google_image_sitemap_build(): You are printing user provided input (i.e. the node title) unsanitized to XML here. XSS is possible in XML, too. See for example http://sla.ckers.org/forum/read.php?2,15296
• _google_image_sitemap_build(): You query the node table here without taking care of node access grants. When adding a node listing to your module, be sure to use a dynamic query created by db_select() and add a tag of "node_access". This will allow modules dealing with node access to ensure only nodes to which the user has access are retrieved. http://api.drupal.org/api/drupal/modules!node!node.module/group/node_acc...

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

I agree with @DHSamB, this is a great idea for a module, and I plan to use it on a few sites I manage! A few things I noticed:

• The _google_image_site_map function is pretty difficult to read. Line 88 has over 300 characters, and generally lines should not be over 80 characters in length. IMO it would be nice to add some commenting and break up some of the logic you are using to build row arrays.
• The validate and submit functions for the _google_image_sitemap_create_form are not implementing hooks. To be honest, I'm not sure if those have functions have a better description, since they are simply the form name followed by _validate or _submit.
• The logic for _google_image_sitemap_create_form_validate is also difficult to read and understand. From the link below it looks like the standard for multi-line conditions is to not indent past the first line, or they can be broken up into multiple conditions. http://drupal.org/coding-standards#linelength
• When you use the dynamic query api, you often indent your calls by several spaces. I believe the standard is to indent two spaces like on other lines. Ex. line 316 of the module file.
• The form for adding site maps tells the user to choose which content types the map should be for, but only allows one to be selected. You should either accept multiple types or change the wording to choose only one type.
• The nid ranges need an upper bound. While testing I put several 9s as the max range value, and got a mysql error that the value was too large to insert in the database. Also IMO it would be nice to either not require the min and max values, or provide default values that include all nodes currently in the database.

Overall, it seems really nice. Keep up the good work!

I forgot to add the security tag in my previous review, doing that now (we keep that for statistics and to show examples of security problems).

manual review:

• "$query->condition('n.type', check_plain($form_state['values']['node_type']), '=');": why are you using check_plain() here? You are not printing anything to the user?
• _google_image_sitemap_delete_form(): indentation should always be 2 spaces per level, also for chained DB API usage. Also in other places.
• google_image_sitemap_node_type_delete(): this shouls use db_delete().
• _google_image_sitemap_create_form_submit(): instead of copying to the object couldn't you just cast $form_state['values'] to an object?
• _google_image_sitemap_list(): indentation errors on the theme('table'...) call.

Sorry to bump this back to needs work, but you need to know where to use check_plain() and where not. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

manual review:

• some dates of your commits are in the future. What is going on with your date settings on your computer?
• _google_image_sitemap_list(): still indentation errors for the table theming, always use 2 spaces per indentation level, not more.

Otherwise looks RTBC to me. Assigning to tim.plunkett as he might have time to finally review/approve this. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Still some clean up to be done, I've attached a patch to finish it up.

Please review http://drupal.org/node/52287 before committing the fix.

Post back here when you've done so, you're very close to being finished.

I would have gone with Issue #1413866 by tim.plunkett: Final clean up patch before promoting to full project. to better follow the guidelines for commit messages.

Also, out of curiosity, how did you manage to commit things 2 weeks in the future? Look at the date on http://drupalcode.org/sandbox/anilbhatt/1413754.git/commit/db4c660!

That said, welcome to the community of project contributors on drupal.org.

I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects depending on which you feel is best.

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.

As you continue to work on your module, keep in mind: Commit messages - providing history and credit and Release naming conventions.