[D7] forWhereiAm Widget Integration

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

Hi Naz

Automated review: I agree - PAReview comes up clean. Good stuff.

Manual review: I found a few bits in the install file that I thought the automated review would have picked up.

1. In the hook_requirements function, you use t() to translate your display message if the forWherIAm client code is not entered. Because that could be run without all of Drupal bootstrap in place, it may need to use t(), or it may need to use st(), depending on context. The correct approach is to use get_t() to retrieve the function to use, then use what you get back. http://api.drupal.org/api/drupal/includes!bootstrap.inc/function/get_t/7

2. That same translate function also contains some <a> tags to link both offsite and onsite. Have a look at the links box in the middle of http://drupal.org/node/322774

Hi,

forwhereiam_admin_form()

I am wondering about $block_name in forwhereiam_admin_form():

<?php
function forwhereiam_admin_form($block_name = '') {

  $form = array();
  $form['forwhereiam_client_id'] = array(
  [...]
?>

Normaly you get the $form array there:

<?php
function forwhereiam_admin_form($form, $form_states) {

  $form['forwhereiam_client_id'] = array(
  [...]
?>

forwhereiam_admin_validate()

This callback should handle validation opertions only.
You may splitup this function in forwhereiam_admin_validate() and forwhereiam_admin_submit().

In forwhereiam.tpl.php I found this:

guess my postcode

and

Powered by

This strings are not language aware.

You can use t() to solve.

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

I'm a robot and this is an automated message from Project Applications Scraper.

Hi Naslam,

Really nice coding. It is clear and well done.
I just made a manual and i don't have a lot to report :

-You should rename the file "forwhereiam_admin.inc" by "forwhereiam.admin.inc".

-I ran a jshint on you js file (where all the logic seems to be) and I've got some errors. You may want to consider it :

forwhereiam.js: line 47, col 49, Missing semicolon.
forwhereiam.js: line 71, col 20, Use '===' to compare with '0'.
forwhereiam.js: line 71, col 34, Use '===' to compare with '0'.
forwhereiam.js: line 135, col 8, Missing semicolon.
forwhereiam.js: line 208, col 10, Missing semicolon.
forwhereiam.js: line 235, col 35, Use '===' to compare with '0'.
forwhereiam.js: line 288, col 28, Use '===' to compare with 'true'.
forwhereiam.js: line 288, col 51, Use '===' to compare with '0'.
forwhereiam.js: line 292, col 43, Use '===' to compare with '0'.
forwhereiam.js: line 440, col 31, 'error' is already defined.
forwhereiam.js: line 477, col 42, Missing semicolon.
forwhereiam.js: line 773, col 25, Use '===' to compare with ''.
forwhereiam.js: line 814, col 43, Use '===' to compare with '0'.

- Rather than themed output use renderable array in hook_block_view()
- Your module should add a new permission string and use it in hook_menu()
- CSS files should be in `css` directory, JavaScript files should be under `js` directory, images should be inside `images` directory, tpl files under `templates`/`theme` directory, and inc files inside `includes` directory
- Your theme function should pass only some variable settings, passing variable values and form array is not desirable. You can produce them inside some theme preprocess function
- `forwhereiam_admin.inc` rename it to `forwhereiam.admin.inc`. Include files should be namespaced as per their requirement
- In `forwhereiam_admin.inc` instead of checking for empty values, use #required instead
- In `forwhereiam_admin.inc`, you should not use form validate callbacks to save variables. It should be limited to only validations
- In `forwhereiam_admin.inc`, since you are using system_settings_form(), there is no need to use variable_set(), Drupal will do that
- You should move drupal_render() to theme preprocess, in the tpl file
- Consider using the js file as library, so that other modules may use it
- I see that you are printing some text through js, make sure they are translated and sanitized
- Indentation in some places is not right in js file

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

I'm a robot and this is an automated message from Project Applications Scraper.

LGTM\

One sidenote, I got this warning:
Strict warning: Only variables should be passed by reference in forwhereiam_block_view() (line 92 of /var/www/drupal/sites/all/modules/contrib/forwhereiam/forwhereiam.module).

And this is easiliy solved by first storing the output of drupal_get_form() and then calling drupal_render($output_from_drupal_get_form).

I don't think this is a show-stopper though.

No issues found, checked for security, use of Drupal API, licensing issues and an individual account.

Non-blocking Issues

• There's some extra whitespace in this line in forwhereiam.info scripts[] = js/forwhereiam.js
• A few style issues reported by http://pareview.sh/pareview/httpgitdrupalorgsandboxnaslam1969738git
• In forwhereiam_disable(), what are you trying to do with that cookie? I think that will only end up being executed for the user that disabled the module
• The parameter in forwhereiam_block_view() is actually called $delta instead of $block_name
• Instead of using the addThis js library directly, can your module use https://www.drupal.org/project/addthis as a dependency?
• forwhereiam_form() is not an implementation of hook_form(). "This is a node-type-specific hook, which is invoked only for the node type being affected". This module is obviously not defining new node types, so consider renaming this function.
• In forwhereiam_findme_ajax(), instead of if (empty($output['error'])) you can just use else, since that value is defined in the if clause above
• In forwhereiam_search_ajax() you probably want to bail out earlier when $location is not valid - the rest of that code will still be executed before you check for an error
• There's no use of any theme functions, this may make users of this module unhappy as they will be unable to easily incorporate your blocks into their site's look and feel

Thanks for your contribution, naslam!

I updated your account so you can 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 stay 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.