Module Instructions

Welcome!

Please take a moment to make your README.txt follow the guidelines for in-project documentation. Also it's really only necessary to put the installation documentation into an extra file if it's multiple site long.
Have a look at the tips for a great project page.

t($module . ' (' . $file . ')'); - have a look at the t() functions documentation and use placeholders instead.
Also, as this is not really a translatable string you should use the similiar but non-translating format_string() function here.

Be aware of module_filter module, it'll change the whole form and do it the none-#tree style, you should handle that case.

We do really need more hands in the application queue and highly recommend to get a review bonus so we can come back to your application sooner.

regards

It would be better to put the link http://git.drupal.org/sandbox/alesr/1526218.git as your GIT URL.

/**
 * Implements hook_menu().
 */
function module_instructions_menu() {
  $items = array();
  $items['admin/modules/module_instructions/%/%'] = array(
    'title callback' => '_module_instructions_custom_title',
    'title arguments' => array(3, 4),
    'page callback' => '_module_instructions_show',
    'page arguments' => array(3, 4),
    'access arguments' => array('administer modules'),
  );
  $items['admin/modules/module_instructions/%/%'] = array(
    'title callback' => '_module_instructions_custom_title',
    'title arguments' => array(3, 4),
    'page callback' => '_module_instructions_show',
    'page arguments' => array(3, 4),
    'access arguments' => array('administer modules'),
  );
  return $items;
}

1. Why do you set the same menu-entry twice?
2. In Drupal you don't need the wildcards % at the end of a menu entry, they will be appended automatically, check the documentation, you may also check the project "Directory based organisational layer." @ http://drupal.org/sandbox/luxpaparazzi/1301730 for having a working example:

...
    $items['odir'] = array(
    'title' => 'Directory',
    'page callback' => 'odir_filelist',
    'page arguments' => array(),
    'access callback' => 'odir_control',
    'access arguments' => array('view', 1),
    'type' => MENU_LOCAL_TASK,
    'file' => 'odir.inc',
  );
...
function odir_filelist($path = '') {
  odir_current($path);
  $text = theme('odir_filelist_header', array('splitted_path' => odir_split_path($path)));
  $text .= _odir_render_file_output();
  _odir_js_set_filecounter();
  return $text;
}

An empty array is passed to 'page arguments', any arguments will then automatically be past to the callbackfunction odir_filelist ...

3. "_" represents a hidden function, Page callbacks and menu callbacks should not be hidden

return t($module . ' (' . $file . ')')

4. You should never put variables into the t() function, they should be used as placemarks, please consult the api documenation.

5. _module_instructions_system_modules_fieldset: It is recommanded putting all HTML into theme functions, or even better into theme files, please consult the theme documentation.

Typ:

Helped reviewing: http://drupal.org/node/1364772

should point to the comment directly, not the project, so add the part #......

return format_string('@module (@file)', array('@module' => $module, '@file' => $file));

would be better using t() which uses the same syntax, see http://api.drupal.org/api/drupal/includes!bootstrap.inc/function/t/7

Added git url: http://git.drupal.org/sandbox/alesr/1526218.git

would be better using t() which uses the same syntax, see http://api.drupal.org/api/drupal/includes!bootstrap.inc/function/t/7

why? this "( )" is not really translatable, IMHO this would just create unnecessary workload for translators.

@alesr, ok @patrickd was write, I did not correctly read his review, nor did I check the contents correctly, sorry for bothering you with that one ;)

You may consider getting a review bonus and one will come back to your application sooner, but in fact I suppose your reviews should be little more complete.

If you wish you may check my project @ http://drupal.org/node/1302786

Links of reviewed projects.

Automatic review is OK.
Well-documented.

You could make your code smaller and clearer, which prevents coding errors:

1. foreach ($form['modules'] as $module => $item) {
     // Check if array is really a module.
     if (is_array($form['modules'][$module]) && isset($form['modules'][$module]['links']))
   

   Why don't you use $item instead of $form['modules'][$module] ? It's the same for your other foreach loops, you could have a smaller and clearer code by using directly the value provided by the foreach loop, instead of the key. Sometimes you may even not need the key:

   foreach ($form['modules'] as $group)
   

2. This code is written both. It would be nice in a function, to ease future changes.

   $form['modules'][$module]['links']['readme'] = array(
     '#type' => 'link',
     '#title' => 'Readme',
     '#href' => 'admin/modules/module_instructions/' . $module . '/' . $readme,
     '#options' => array(
       'attributes' => array(
         'class' => array('module-link', 'module-link-readme'),
         'title' => t('Readme'),
       ),
     ),
   );
   

   Same for install link. And you also have redundant code in your theme functions.

I'm not sure your theme functions are properly named. I would start them with theme_. But I might be wrong, as these are core replacements.

I tested it with and without Module Filter, and it works like a charm.

I think this is good enough to be changed to RTBC, but I'm a newbie in reviewing. So I don't change status and don't remove PAReview bonus tag. I guess a more experienced reviewer will know what to do.

RE this module -
You know that http://drupal.org/project/advanced_help already does auto-detect README files and make them available through the admin UI in the same way you advertise here?

Have you compared that one with this?

advanced_help *prefers* that you provide full, structured HTML documentation, but also, as a Freebie, throws in this feature because it's a good start.

I instaled the module but I can not see how to use it. Nothing has changed on the modules list, or am I missing somthing. Are they suposed to be links to README and INSTALL files on the modules page?
----------------------------

in the README.txt you can add section how to use once is installed, For example. Once installed simply go to modules page ...

-----------------------

It looks as useful module, saves you going to file system.

Ok, I can see links now, sorry I was looking at core modules

It looks all good to me.

Review of the 7.x-1.x branch:

• Drupal Code Sniffer has found some issues with your code (please check the Drupal coding standards).
  

  
  FILE: ...tes/all/modules/pareview_temp/test_candidate/module_instructions.module
  --------------------------------------------------------------------------------
  FOUND 4 ERROR(S) AFFECTING 4 LINE(S)
  --------------------------------------------------------------------------------
   70 | ERROR | Array indentation error, expected 4 spaces but found 6
   71 | ERROR | Array indentation error, expected 4 spaces but found 6
   76 | ERROR | Array indentation error, expected 4 spaces but found 6
   77 | ERROR | Array closing indentation error, expected 2 spaces but found 4
  --------------------------------------------------------------------------------
  

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. Get a review bonus and we will come back to your application sooner.

manual review:

1. Project page does not mention where those links can be found. You should reference the modules page. Same for README.txt.
2. "'title' => t($type),": do not pass dynamic varibales to t() if possible (translation extraction tools cannot pick that up). You should use t() directly in module_instructions_form_system_modules_alter() when you call _module_instructions_create_link_array().
3. module_instructions_show(): Hm, that page does not really look good as most Readme files are written with fixed width fonts in mind. Better use something like '<pre>' . file_get_contents($fullpath) . '</pre>'? Also it would be really helpful if links would be made clickable, so that I don't have to copy&paste URLs in Readmes.
4. Also: if a module uses markup in the Readme, that will get interpreted. Even worse: if I have javascript samples in my Readme like <script>alert('XSS');</script> this will be executed. Not really a security issue, as I don't consider text from a Readme as user provided input, but you should sanitize it to plain text anyway.

Otherwise I think this is nearly ready. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Good job!

manual review:

• "check_markup(_filter_url(file_get_contents($fullpath), $filter), 'filtered_html')": that text format might not exist on every Drupal installation. Why can't you use check_plain()? I would do: '<pre>' . _filter_url(check_plain(file_get_contents($fullpath)), $filter) . '</pre>'.
• "_module_instructions_create_link_array($module, 'Readme', $readme)": all user facing text should run through t() for translation, right here. Not in _module_instructions_create_link_array().

Otherwise this looks RTBC to me. Assigning to tim.plunkett as he might have time to finally approve this. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

I have a couple nitpicks, which I've attached as a patch. Note, it adds a couple @todo's, fill those in.

That said, 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.