Radios Select

So this looks really cool. :) I've run the automated test bot on your module, and it's picked up some minor issues, but I like the look of what it can create :)

There are still files other than README.txt in the master branch, make sure to remove them. See also step 5 in http://drupal.org/node/1127732
Review of the 7.x-1.x branch:

• README.txt is missing, see the guidelines for in-project documentation.
• Remove "version" from the info file, it will be added by drupal.org packaging automatically.
• Run coder to check your style, some issues were found (please check the Drupal coding standards). See attachment.
• Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards). See attachment.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. Go and review some other project applications, so we can get back to yours sooner.

Severity minor, Drupal Commenting Standards, Internationalization, Drupal Security Checks, Drupal SQL Standards, Drupal Coding Standards

sites/all/modules/pareview_temp/test_candidate/./examples/radios_select.examples.inc:
 +34: [minor] use <br /> instead of <br>
 +37: [minor] use <br /> instead of <br>
 +39: [minor] use <br /> instead of <br>
 +168: [minor] use <br /> instead of <br>
 +550: [minor] use <br /> instead of <br>
 +626: [minor] use <br /> instead of <br>
 +730: [minor] use <br /> instead of <br>
 +731: [minor] use <br /> instead of <br>
 +732: [minor] use <br /> instead of <br>
 +733: [minor] use <br /> instead of <br>
 +734: [minor] use <br /> instead of <br>
 +735: [minor] use <br /> instead of <br>
 +736: [minor] use <br /> instead of <br>
 +737: [minor] use <br /> instead of <br>
 +738: [minor] use <br /> instead of <br>

Status Messages:
 Coder found 1 projects, 1 files, 15 minor warnings, 0 warnings were flagged to be ignored

FILE: ...odules/pareview_temp/test_candidate/examples/radios_select.examples.inc
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AND 1 WARNING(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
  35 | WARNING | Avoid backslash escaping in translatable strings when
     |         | possible, use "" quotes instead
 264 | ERROR   | Array indentation error, expected 6 spaces but found 8
 264 | ERROR   | Array closing indentation error, expected 6 spaces but found 8
--------------------------------------------------------------------------------

Source: http://ventral.org/pareview - PAReview.sh online service

I think this is one of the times when this should be OK. Whatever you do, you would end up escaping single quotes or double quotes - so I would argue that this is an exception case.

One thing I noticed when I enabled the module is that when I enable the module I receive the following warning from the PHP compiler:

Strict warning: Only variables should be passed by reference in radios_select_help() (line 16 of /private/var/www/sites/ddu12/sites/all/modules/radios_select/radios_select.module).

You should set the form from drupal_get_form to a variable first and then pass it through to drupal_render();

If you can fix this issue, then I'll mark this as RTBC.

Thanks for updating and almost there! The changes you have made are still not quite right. PHP does not require you to pass a variable by reference to a function, this is handled in the function declaration. You should modify your code to read:

<?php
/**
 * Implements hook_help().
 */
function radios_select_help($path, $arg) {
  switch ($path) {
    case 'admin/help#radios_select':
      if (module_load_include('inc', 'radios_select', 'examples/radios_select.examples')) {
          $example_form = drupal_get_form('radios_select_example_form');
          return drupal_render($example_form);
      }
  }
}

It's a good idea during module development to set your PHP error_reporting level to E_ALL and display_errors to On so that you can see what your module does in every case. Obviously you shouldn't do this in production :)

Good work. Don't stress over the repeated mistakes - it's all a learning experience, and a good one.

rtbc by the way

manual review:

• Regarding the backslash escaping: the last t() call on that line culd use "" quotes instead of '', then you do not need the backslash to escape the quote.
• radios_select_example_form(): you should really use some more line breaks for your $rows array, as some lines are really looooooong.
• "t('<h2>Radios Select attributes and examples</h2>'),": the markup should be outside of t() where possible.
• radios_select_help(): indentation errors, always use just 2 spaces per level.
• "preg_replace("/((?<=>)|(?<=--)|(?<=.))[\s\n\r\t]+((?=--)|(?=<))/U"": why is this needed and what does it do?

But that are just minor issues, so ...

Thanks for your contribution, OldWarrior! 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.