Google Books

Automated review:

It appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.
Review of the master branch:

• Remove "project" from the info file, it will be added by drupal.org packaging automatically.
• googlebook.module in googlebook.info: It's only necessary to declare files[] if they declare a class or interface.
• ./googlebook.module: all functions should be prefixed with your module/theme name to avoid name clashes. See http://drupal.org/node/318#naming
  

  function filter_googlebook_tips( $filter, $format, $long = FALSE ) {
  function make_html_link( $address ) {
  function set_param( $params, &$flag_var, $value, $set ) {
  function add_googlebooks_javascript() {
  function add_googlebooks_viewer_inline( $isbn ) {
  

• ./googlebookapi.module: all functions should be prefixed with your module/theme name to avoid name clashes. See http://drupal.org/node/318#naming
  

  function bib_field_array() {
  function get_googlebook_data( $id, $version_num ) {
  function json_extract( & $barr ) {
  function assign_bib_array( & $bib_array, $index, $value ) {
  function get_googlebook_version_count( $id ) {
  function clean_search_id( $id ) {
  

• Run coder to check your style, some issues were found (please check the Drupal coding standards). See attachment.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. Go and review some other project applications, so we can get back to yours sooner.

Severity minor, Drupal Commenting Standards, Internationalization, Drupal Security Checks, Drupal SQL Standards, Drupal Coding Standards

sites/all/modules/pareview_temp/test_candidate/googlebook.module:
 +315: [minor] in most cases, replace the string function with the drupal_ equivalent string functions
 +329: [minor] in most cases, replace the string function with the drupal_ equivalent string functions
 +360: [minor] There should be no trailing spaces

Status Messages:
 Coder found 1 projects, 1 files, 3 minor warnings, 0 warnings were flagged to be ignored
ERROR: the "DrupalCodingStandard" coding standard is not installed. The installed coding standards are MySource, PEAR, Zend, Squiz and PHPCS

Took out link that was not useful.

Looking better...still a few issues found.

Automated review:

It appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.
Review of the master branch:

• Run coder to check your style, some issues were found (please check the Drupal coding standards). See attachment.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. Go and review some other project applications, so we can get back to yours sooner.

Severity minor, Drupal Commenting Standards, Internationalization, Drupal Security Checks, Drupal SQL Standards, Drupal Coding Standards

sites/all/modules/pareview_temp/test_candidate/googlebook.module:
 +103: [minor] Use an indent of 2 spaces, with no tabs
 +319: [minor] in most cases, replace the string function with the drupal_ equivalent string functions
 +333: [minor] in most cases, replace the string function with the drupal_ equivalent string functions

Status Messages:
 Coder found 1 projects, 1 files, 3 minor warnings, 0 warnings were flagged to be ignored

I tested it and worked as described. Did a manual review of the code, and it looks correct. So for me it is thumbs up.
EDIT: Just to be clear, did not find any security issues, and could not find a similar module for drupal 7 that does the same thing. Code is well written and uses drupal functions, and is well documented.

Next step is, if no other reviewer here thinks otherwise, is to be granted git acces to grant full project release by one of the Git administrators.

The following git branches do not match the release branch pattern, you should remove/rename them. See http://drupal.org/node/1015226

* 7.x-1.x-dev
  remotes/origin/7.x-1.x-dev

Review of the 7.x-1.x-dev branch:

• Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards). See attachment.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. Get a review bonus and we will come back to your application sooner.

manual review:

• googlebookapi.install: remove the copyright notice, all code on drupal.org is GPL anyway + downloadable tarballs get an extra LICENSE.txt file. Also in the other files.
• googlebookapi_uninstall(): empty function, so remove it.
• googlebookapi_get_googlebook_data(): for documenting function parameters you should use the @param syntax, see http://drupal.org/node/1354#functions . Also on other functions.
• "json_decode($bookkeys, TRUE);": you should use drupal_json_decode() instead. Also elsewehere.
• googlebookapi_get_googlebook_data(): $obj is a bad variable name, use something more descriptive, e.g. $cached_books, $cache etc.
• googlebookapi_get_googlebook_version_count(): useless docblock, please at least explain the function in one sentence. Also elsewhere.
• "'#title' => 'Google Book Data Fields',": all user facing text should run through t() for translation. Please check all your strings.
• googlebook_filter_process(): you could use a foreach() loop here, then you don't need the counter. You can add new array elements simply with $book[] = ...
• googlebook_retrieve_bookdata(): what is you strategy to sanitize the third party provided data before printing it? You have to use the appropriate sanitization function (check_plain(), check_url() etc.) before embedding any user provided property (or google provided in this case) into your HTML output. See http://drupal.org/node/28984 and http://drupalscout.com/knowledge-base/drupal-text-filtering-cheat-sheet-...
• Also you should split up googlebook_retrieve_bookdata(): this function should only assemble the necessary data and pass it to a theme function that actually renders the output. So other modules/themes can easily override and customize it.

Updated instructions for 7.x-1.x-dev branch.

So I approve again, hope that other reviewers feel the same :-)

blockers

googlebook_filter_settings

When user can enter anything they want, but you want them to enter an integer, validate for it instead of accepting and storing anything and then having to do output processing on it everywhere. It looks like all the textfield inputs need to be integers - use a full blown #validate function, or #element_validate on each of them.
It seems these values are used verbatim later on so they should be validated on input.

googlebook_retrieve_bookdata, googlebookapi_get_googlebook_data, googlebookapi_cached_request

None of these functions takes into account the request failing for whatever reason. The last one even caches the faulty result.

googlebookapi_cached_request

cache id can easily be longer than the 255 limit on the cid. Usually cid is set to module specific prefix, sub-area specific prefix, and then a hash of the user provided data so that the length of the cid is always known to be less than the limit.

theme function - preprocess_method

Name is generic and does not indicate at all that this is a google books theme function. Perhaps it would be better called 'googlebook'?

non-blockers

too much growth hormone

Please remove the 7.x-1.x-dev branch from the d.o git repository if you are no longer using it. 7.x-N.x is the normal major version branch naming convention and you seem to have one of those too. Both branches seem to be identical. Just in case you hadn't seen it before - Release naming conventions

git commit messages and attribution

Please refer to Commit messages - providing history and credit about giving yourself some credit and properly formatting commit messages.

googlebook_filter_process

It seems the module should be looking for a marker, not just everything inside {{}}. Other modules may use this same "signal" elsewhere. Most other modules use square brackets - eg [node:5] in node link. {{googlebooks:drupal+7+development}}

googlebookapi_json_extract

API duplication? http://api.drupal.org/api/drupal/includes%21common.inc/function/drupal_j...

Here I go again.
Seriously, @ELC, I think this is to picky. It is obvious that @darrellulm has an understanding of Drupal standards, and the module works, and is easily to setup. Improvements could always be done when you release your project as a full project, how many sandbox projects is a stable release on the first day they become full projects?

@misc This is not the correct forum for such arguments. If you have issues with my reviews you may ask for a second opinion or talk to me directly. This is not your application?

The project has a security issue which it seems I forgot to add the tag for. The unvalidated user input is entered directly into the HTML output. This is a canary for understanding the security implications of trusting user input and using the forms API. Security and understanding the Drupal API are some goals of this process.

The unhandled error states on http requests, and cache id key length are not as important but are serious enough that they will cause unexpected errors and warnings, and could be potentially difficult to diagnose at a later date as they will not be easily reproduced.

@darrellulm

googlebookapi_json_extract

That seems perfectly reasonable.

googlebook_filter_process

This can be changed to [googlebooks:drupal+7+development]. Is this what you are indicating? Yes, that would work. It then makes that tag uniquely identifiable as being from googlebooks. You can stick with {{}} if you want, but yes, most modules seem to use the [].

I look forward to the other fixes.

@ELC, I am also doing review of this applicant, and raised my concern in the issue there I think we are to picky. Further discussion on the subject could be done here: http://groups.drupal.org/node/208403

Main security issue addressed; admin input is now validated as a number before being accepted and included in output html.

The second blocking issue listed in #15 is still at large. I'll open a bug on the project page as they will cause errors to displayed to the user, not outright break or be a security issue. Hmm.. one of them doesn't cause errors. It appears that access an non-existent index on a NULL value does not throw an E_NOTICE. The property still throws one though.

Caching; fixed. I agree that it doesn't need a module specific identifier. Perhaps you should make the search string lower case before hashing it so that differences in case don't mean keeping a cached copy for each.

Theme name; it's certainly module specific!

Also fixed the non-blockers.

other mild suggestions

googlebook_make_html_link

this function should be using the l() function to build a link, allowing it go through the theme engine for other modules to manipulate. http://api.drupal.org/api/drupal/includes%21common.inc/function/l/7

out of date reference

Put a Google Book search term between double brackets like this:
[googlebook:The Hobbit] or [googlebook:9780618154012] or [googlebook:Rucker+Software]
and this will filter the input to replace with Google Book data
and images from http://books.google.com

So, the next step from here is for another git admin to come along and review it again and if they find it to be all good, they'll set it to "fixed" and grant the git vetted status. If they find any issues, it will be bouced back to needs work with appropriate instructions.

• Drupal Code Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. Get a review bonus and we will come back to your application sooner.

There is a git tag that has the same name as the branch 7.x-1.x. Make sure to remove this tag to avoid confusion.

manual review:

• "'#default_value' => isset($filter->settings['googlebook_link']['worldcat']) ? check_plain(...": The form API will run check_plain() on #default_value, so you should not use check_plain() here. Double escaping is bad.
• function doc blocks: The first line e.g. "Implements: googlebook_filter_info()" does not contain any information. You are just repeating the function name here. The first line should contain the short description, see http://drupal.org/node/1354#functions
• "array_walk($params, create_function('&$val', '$val = trim($val);'));": why can't you use $params = array_map('trim', $params); ?
• googlebook.tpl.php: a template file should be HTML code that has PHP code snippets embedded, not the other way around. So you would not need all that print statements. On the other hand, if there is more theming logic than HTML code you should consider using a theme function instead.
• "htmlentities($info_link)": you should use check_url() here instead. Even better: use the l() function to generate link markup and read its documentation, it will sanitize things for you if you want.
• $html_coverimage: do not create the image markup yourself, use theme('image', ...) instead.
• Do not embed javascript yourself in the HTML output, use drupal_add_js() instead. This is required so that Drupal is able to aggregate javascript, other modules could alter it, etc.

Sorry to bump this back to needs work, but you need to know where to use check_plain() and where not.

Changed information to indicate 7.x.1.x normal release branch for naming conventions.

re: that javascript

After looking at what it is and asking for some advice about it, here's MESHO on the subject ;)

There are two parts to the javascript for a book; a file which is pulled in from google - define('GOOGLE_BOOK_EXTERN_JS', 'https://www.google.com/jsapi');, and a dynamic chunk of script based on the ISBN of the book.

The file from google should be included on any page that needs it - teaser or full node display, or random entity. Due to that being stupendously hard to figure out, just always add it during hook_init() with drupal_add_js().

The dynamic piece of script is entirely based on the exact entry of text being replace, it should remain there as inline javascript. There is no way to add it to the top of the page without ruining caching and generally getting in the way.

Correct me if I'm wrong: shouldn't googlebook be google_book and googlebookapi be google_book_api?

I won't set it to needs review because I just glanced at the code, but I found this line in googlebookapi.module
277 drupal_set_message($message = t('Googlebooks: Could not retrieve data from google.com. @err', array('@err' => check_plain($url_data->error))), $type = 'error');
Using the @err will already ensure that the string is passed through check_plain(), and assigning the $message variable is kind of pointless.

Looks good, however you have processing and logic in the template file and template function. The theme functions are meant to only be printing out things that have been completely prepared already. Or looping through and array of completely prepared stuff, or conditionally including wrapper divs around prepared stuff.

In googlebook.tpl.php, the call to theme('image') and theme('googlebookbiblio') should have already been done in the preprocess funtion and their output should simply be "print $whatever;", or "if ($whatever) .. etc." Or if you setup render arrays in the preprocess function, if ($whatver) drupal_render($whatever).
http://drupal.org/node/223430

Please note that full render arrays for everything is new to Drupal 7 and so not many modules will have this implemented.

Still back on needs work because of the logic in themes.

manual review:

• google_books_theme(): this is a hook and should be documented as such, see http://drupal.org/node/1354#hookimpl . Same for google_books_init(). Please check all your hook implementations.

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

your application was already RTBC, read about the workflow of applications.

Thanks for your contribution, Darrell! 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 (as you have). Please consider reviewing (more) other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

That last paragraph seems like it might need a different template for people who have actually participated in the review process ;)