Grassroot Interests

Hi,
Some manual review of the code.
README.txt
Please make the README.txt in a proper format. Read here for more details. sun's README.txt style is really cool, I follow that.

grassroots_interests.info file
description should end with a full stop.

grassroots_interests.module file.
Line #16 to #21. Since the implementation of hook_help() is empty. Either remove it or implement the functionality.

Line #15

  require_once DRUPAL_ROOT . '/' . drupal_get_path('module', 'grassroot_interests') . '/includes/grassroot_interests.inc';

Use module_load_include() instead. So the corresponding code would be

  module_load_include('inc', 'grassroot_interests', '/includes/grassroot_interests');

In the implementation of hook_permission() adding description to the permissions would be good as it clarifies what actually the permission is meant for.
Line #45

 $items['admin/grassroot_interests'] = array(
 .....
 );
>

Avoid underscores in menus, instead use "-". So the corresponding function would be

 $items['admin/grassroot-interests'] = array(
 .....
 );

grassroot_interests.install file

/**
 * Implements hook_install().
 */
  function grassroor_interests_install() {
  }

But in your code the implementation is incorrect.

/**
 * Implements hook_install().
 */
  function grassroot_interests_schema() {
  }

Please make the changes accordingly. Please make the changes as described especially the implementation of hook_install() and it is highly recommend to get a review bonus so we can come back to your application sooner.

Changing the status to needs work and would be great if you follow the review bonus stuff mentioned above.

Hi,

First off; useful idea with clean execution into a drupal module, it works as expected with very few bugs. On the automated side of things,

Line 163: Potential problem: FAPI elements '#title' and '#description' only accept filtered text, be sure to use check_plain(), filter_xss() or similar to ensure your $variable is fully sanitized.

      '#title' => $title,

This looks like a false positive as your setting the variable; not a user.

Line 291: The $message argument to drupal_set_message() should be enclosed within t() so that it is translatable.

  drupal_set_message("Keywords Deleted.", "status");

Small fix required.

Manual notices;

• Installing your module was simple and straight forward however I found your configuration link not linking to the correct location, should this link to admin/grassroot-interests?
• When editing a preexisting keyword the keywords title would append itself to the top of the list upon submit. While this is intended functionality it occurs even if the title is already added as a keyword.

Cheers,

Hi,

A few points from a manual review:

1) In grassroot_interests.module: in grassroot_interests_add_keywords_form() you set $title to a string literal (line 137) and then conditionally overwrite it with another literal (line 139) - I believe that both of these literals should be translatable ie. should be wrapped with t().

2) When the above $title is used (line 163) it is put through check_plain() - but I don't think this is needed since the only way that $title gets set is from a literal as far as I can see?

3) In grassroot_interests.info you have the line for specifying the core version twice for some reason?

4) As per the automated review, there is still a master branch which should be removed (see http://drupal.org/node/1127732).

Would be great in case you could review other projects and get a review bonus.

Hi,

# there is no hook_install , you need this to install schema

there is no need to install or uninstall schemas in drupal 7, this happens automatically

Hi,

some suggestions
# add dependency for specific 'Page manager' in .info than panel
# in project details under requirement , specify optional modules like drupal search (i didnt enabled drupal's default search), solr etc

tested the module , followed instruction and create a keyword and a panel page, when viewing panel page got "Notice: Undefined offset: 0 in grassroot_interests_content_type_render() (line 29 of D:\wamp\www\drupal-7.4\sites\all\modules\contributed\grassroot_interests\plugins\content_types\search_interest.inc)."

Hi,
now error is gone . probably i didnt added keywords . but i am not getting anything on my test panel page! (sorry i didnt get the idea how this module works). do you have any demo page?

already followed instruction check my comment

created panel page, added search, but nothing displayed on that page

i think you misunderstood , i have followed exactly same way.. i just wondering how display page will look like.. i am getting a blank page!

Hi Srejas,
Adding screenshots to your comments maybe helpful.

Thanks for your time reviewing.

hi now its working fine.. you know what made me confused? this line

Create or enable the page you want to add GI results.

# actually you cant create a page , you have to enable 'search-node' and 'search-user' panel
# then you have to add a variant in this panels
# then add GI search to panel region

now perform a search, its displayed in search page :)

Works fine for me.

All looks good, and really a useful module. Cheers! Should be promoted.

It's works for me should be promoted as full project.

Wow, I like screenshots too! :)

; Information added by ravyg started writing script on 9th april 2012
You don't have to add such a line, this is only for packaging purposes, and GIT already takes care about logging who has done what and when.

don't use module_load_include() in global spec, only use it within functions and it's also only necessary for including files of foreign modules. Use require_once for files you always include and include_once for conditional inclusions.

Please indent method calls, like db_selects:

anyfunction()
  ->anotherfunction()
  ->yetanother()

I think you missunderstood the hook-thing.
There's a hook_form() but there's no hook_form_submit().
Use the "Implements ..." documentation text ONLY for hooks. What you use it for are Form and Submission Callbacks! not hooks.
If you define function Callbacks - make sure there's no hook that you collide with!

Whenever possible do not use arg() function, make use of the "page arguments" functionality of hook_menu() so you can get your needed information directly as function parameter -> please re-read hook_menu documentation.

$items['admin/help/grassroot-interests']
There's no reason to create this menu item, the help module will take care of this.

Implements hook_perm().
It's called hook_permission(), so document it properly.

grassroot_interests_settings_form()
There are no settings, just more documentation and a redirecting submit button -> let help module take care of giving help.

check_plain(filter_xss($post_data['name'][1]['first'])),
Sorry, this code makes no sense and gives me the feeling that you hardly have an idea how and when to filter strings for security.
Please read Writing secure code documentation

$newline = "\n";
What about PHP_EOL?

variable_del('grassroot_interests_string');
Do you even use this variable?

Please initialize arrays before using them to prevent having notice's all over my watchdog logs.

Never ever use t() in global space - except you want to kill the overall site's performance by 50%.

Sorry, the module may work good, but it's code needs some cleanup. Also I'd like to be sure you understood the correct usage of filter functions as security is the most major thing in this process. Also you need to understand the difference between hooks and callbacks.

looks good.

Thanks for your contribution, rayg!

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.