[D7] Context States

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

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 Evan,

Great looking module, just a few pointers before it is ready for release:

1. You can help other developers find the module configuration by adding a 'configure' line in your .info file, check out this page for more information: https://drupal.org/node/542202
2. 'admin/structure/states' and 'admin/structure/states/define' can only be reached directly through the menu. You can add tabs to the interface by adding 'type' to hook_menu for local tasks. For more info on this, check out hook_menu and specifically 'Rendering Menu Items As Tabs'
3. There is no need to clear the cache in hook_install(). The cache is cleared by default after a module is enabled.
4. You can add your page callbacks for the admin interface to a seperate file, so they can be loaded when needed. Your .module file should be a small as reasonably possible to reduce memory load. You can read more on how to include files for page callbacks in hook_menu

Also, you are correct in not sweating the notices from the automated tests, since you are following the Context API correctly

Hi Evan,

Your changes look great! I have just a few more nitpicks, then you are good to go :)

1. You can put the administration forms in the same file, the most important thing is they are not in the .module. Sorry I didn't make that clear in my previous comment, my bad! By convention, this file is often called [modulename].admin.inc, so in your case it would be context_states.admin.inc

2. context_states_form.inc:8 should say 'Implements hook_form().'

3. context_states_define_states_form:8 should say 'Implements hook_form().'

4. system_settings_form($form) already saves each field as a variable in its submit function, so in context_states_define_states_submit() you create a duplicate variable. You now have 'context_states_defined_states' as well as 'defined_states' in your variable table. You can rename the form field from $form['defined_states'] to $form['context_states_defined_states'] and it will save automatically into a variable. For more information on this, check out system_settings_form() and system_settings_form_submit()

Ah yes, I see what you mean. In the notification e-mail for changes in the issue, links are appended with a footnote and I added a link to the hook_form() documentation. You can replace 'Implements hook_form [1].' with 'Implements hook_form().' like you did correctly for hook_menu() in context_states.module:11. The function docblock lets other developers know what hook you implemented, so they can look up the documentation.

A last remark about the field name you changed in context_states_define_states_form(). Since you changed the field name to 'context_states_defined_states', it gets saved automatically to a variable of the same name by the system_settings_form(form) callback. This means that you can now safely remove the context_states_define_states_submit() function in context_states.admin.inc:28 and the variable will still be updated.

Great work! I am not sure on the procedure on who can set the status to 'Reviewed and tested by the community', but I am sure one of the moderators will correct me if I am wrong :)

You can safely remove function context_states_define_states_submit() on context_states.admin.inc:28 once you remove the additional submit callback $form['#submit'][] = 'context_states_define_states_submit'; on context_states.admin.inc:20

Best of luck with your submission :)

Hi evanmwillhite, thank you for nice code.

Here is my suggestions after manual review:
1)In this file context_states.admin.inc as doc you are using hook_form. In fact hook_form is only for modules that define a content type. To put right docs, please look here https://drupal.org/node/1354#forms;
2)also line 60 in the file above
'#markup' => t('No states have been defined'),
It seems to me that it would be better to put tag out of t() function;
3) Your submit handler is empty context_states_define_states_form(), Maybe it would be better not to declare it at all;
4) In hook_install you could use cache_clear_all('*', 'cache', TRUE);
5) In hook_uninstall you could use
$nodes = db_delete('variable')
->condition('name', db_like('context_states_states_') . '%', 'LIKE')
->condition('name', 'context_states_defined_states')
->execute();

That's all thank you!

#10:2 refers to this code on context_states.admin.inc:58

$form['states']['no_state'] = array(
  '#type' => 'item',
  '#markup' => t('<em>No states have been defined</em>'),
);

The t() function typically only prints plain text.

To help site administrators find the right course of action, I would suggest changing it to:

if (user_access('define states')) {
  $empty_text = t('No states have been defined. <a href="@define_url">Add new states</a>.', array('@define_url' => '/admin/structure/states/define');
}
else {
  $empty_text = t('No states have been defined.');
}
$form['states']['no_state'] = array(
  '#type' => 'item',
  '#markup' => $empty_text,
);

manual review:

• context_states_install(): All caches are cleared when a new module is installed, so this should not be necessary.

But otherwise looks good to me, so ...

Thanks for your contribution, evanmwillhite!

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.