[D7] Little Helper

There are some errors reported by automated review tools, did you already check them? See http://ventral.org/pareview/httpgitdrupalorgsandboxcaptainpants2045971git

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then 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 @captainpants,

I've reviewed your module and I have the following comments:

1. Re-factor the install hook.
Do not make variable_set() there cause it is not necessary. It will just add some unnecessary overhead in this table when you don't have settings for the variables. Instead of variable_set() use variable_get(VAR_NAME, DEFAULT VALUE);

function variable_get($name, $default = NULL) {
  global $conf;

  return isset($conf[$name]) ? $conf[$name] : $default;
}

As you can see this will only return the value.

2. The variable 'little_helper_config_img' has been set but nobody delete it on module uninstall.
Please check your way you are defining submit callback for the settings form. The default save of the settings form will save all the variables by default with the key they are coming from. This means that if you don't remove the default submit of settings form it will sore the variable little_helper_config_img into database no matter that you didn't do it in your submit callback.
Better it will be to add a validate hook where you can do the things you need and leave the default submit callback to do the rest (this is only mine opinion).

3. Hook little_helper_permission() should be defined in the .module file. Otherwise you should use hook_hook_info().

4. 'access arguments' => array('little_helper_has_permission') - this should be 'access callback' instead, otherwise it will check user_access('little_helper_has_permission') which is not defined.

Feature you can make:

1. You can add image style option (preset) for image display.

Hi captainpants,

here my suggestions and comments to your module.

Manual review

1. The little helper image should not be mandantory.

2. Perhaps you could add a width: 100% to the image style on line 6 in little_helper.css to make sure the image show up correctly in most cases.

3. You don't have to provide a function little_helper_has_permission() to check user access rights. Just use user_access as access callback and provide the access arguments in the array right away.

  $arr_adminpages['admin/config/user-interface/little-helper'] = array(
    'title' => 'Little Helper Settings',
    'description' => 'Configure your little helper\'s appearance',
    'file' => 'little_helper.admin.inc',
    'menu_name' => 'management',
    'page callback' => 'drupal_get_form',
    'page arguments' => array('little_helper_generate_settings_form'),
    'access callback' => 'user_access',
    'access arguments' => array('config_little_helper'), 
    'type' => MENU_NORMAL_ITEM,
  );

But you can remove the line with access_callback because the function user_access() is the default callback function here.

Some other notices

I don't find another module similuar to that one, but a example how to do the same without a module: https://drupal.org/node/58973. But of cause it's not useful for people without knowledge in php.

Kind regards

Hi captainpants,

Module looks fine to me except one thing. You can load the little_helper.css file conditionally as its is loading for all pages whether block is available or not.

Sorry I havn't changed the status in my comment, so I am doing it.

You don't need to provide defaults for all your variables in little_helper_install(), just when you read the variable with variable_get.
We don't generally use hungarian notation like $o_file, but I guess it's ok.

Otherwise I can't find any major problems.

----
Top Shelf Modules - Crafted, Curated, Contributed.

Added some modules I've reviewed.

It's been a month without any problems reported, so I'm promoting this myself as per https://drupal.org/node/1125818.

Thanks for your contribution, captainpants!

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.

----
Top Shelf Modules - Crafted, Curated, Contributed.