AllForGood

Quick scan looks pretty good.

• Remove $Id tags - these are a leftover from our CVS days, but are no longer required after we moved to Git.
• Comment out your unused theme functions, so that they aren't logged in the theme registry
• Might not be neccessary, but it's probably a good idea to run the returned results through a sanitization function (check_plain or filter_xss) ... treating the external data as 'untrusted'.

I'd suggest the theming implementation could still use a bit more cleanup:

/**
 * Implements hook_theme().
 */
function allforgood_theme() {
  return array(
    'allforgood_results' => array(
      'arguments' => array('content' => NULL),
    ),
  );
}

You define a theme hook, but don't have a theme_allforgood_results function ... and I don't see anything that would tie in your tpl.php file.

Otherwise, things generally look pretty good.

• project page is a little short, see http://drupal.org/node/997024
• 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.
• install file: doc blocks: the closing "*/" should be on a new line. See this for formatting: http://drupal.org/node/1354#files
• allforgood_help(): indentation errors, the "case" statement should be 2 spaces further to the right.
• allforgood_help(): no need to wrap the case string in "()"
• "@param $url": the parameter description on the next line should be indented 2 spaces. Same for @return.
• "$msg = 'No simplexml_load_string() function present.';": all user facing text should run through t() for translation.

@forestmonster has been contacted to ask if the application is abandoned.

After ten weeks with a status of needs work: the applicant may be contacted by a reviewer to determine whether the application was indeed abandoned. The action taken by the reviewer should be documented in the project application issue.

http://drupal.org/node/894256

The application has been marked as need work since October. Doing commits on a sandbox project is not the same as wanting a review for full project access. If you want us to review it, you must mark it 'needs review'.

The response time for a review is now approaching 4 weeks. Get a review bonus and we will come back to your application sooner.

Good stuff, this module looks pretty darn close to perfect to me. PAReview.sh throws no errors and the code seems to be well documented / well organized.

Two small notes:
1) I don't think its proper to have tags inside t(''). See http://api.drupal.org/api/drupal/includes%21bootstrap.inc/function/t/7#c... for how to insert a link into translatable text. You're doing this on line 56 in a Form API '#description'

2) Your module relies on the simple XML extension to PHP and it doesn't mention this on either the project page or in README.txt. If the error is thrown because the user doesn't have the function to parse the XML response. The error reads: 'No simplexml_load_string() function present.' Which wouldn't be very informative for a user who doesn't have this extension on, but doesn't know about php modules / doesn't know that specific one. I'd note that your module requires it in the README or on the project page and update the error message to say that not having that php extension may be the cause of the problem.

Other than that it looks good!

Jease, if I'm going to be nit picky I should at least get it right! ;) I didn't know there was exception for a tags within t('') but it makes sense to me.

I'm marking this RTBC but someone with real git powers will have to come knight thee.

manual review:

• "'access arguments' => array('access administration pages'),": this permission is too generic, create your own.
• _allforgood_get_search_results(): the check for simplexml should not done here, use hook_requirements() for that.
• _allforgood_block_content(): Generating markup should be done in the theme function only, so that it can be overridden. This function should only prepare the list of items in an array.

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

Did a basic functionality check and code looks also good for me.

One point I found is that you maybe should use some other function name for your admin settings as (I can't remember where it was) it could be invoked with something like hook_admin(). Generally forms are suffixed with _form, so maybe you make that small change just to be sure.

Thanks for your contribution and 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.

Thanks to the dedicated reviewer(s) as well.