wadmiraal [wadmiraal]

Hello, and thanks for applying for a CVS account. I am adding the review tags, and some volunteers will review your code, pointing out what needs to be changed.

• .module line 10 & 11: title and description shouldn't be run through the t() function
• Use Coder module to make sure your module follows Drupal coding standards.
• Add an .install file that implements hook_uninstall, to perform the necessary variable_del() calls to clean up variables set by your module
• Preferably add the settings page in a separate file to avoid unnecessary code execution. Also make use of system_settings_form to save variables.

  In short:
  - change hook_menu:

    return array(
      'admin/settings/i18n-book-navigation' => array(
        'title' => t('i18n Book navigation'),
        'description' => t('Settings'),
        'page callback' => 'drupal_get_form',
        'page arguments' => array('_i18n_book_navigation_settings_form'),
        'access arguments' => array('access administration pages'),
        'type' => MENU_NORMAL_ITEM,
        'file' => 'i18n_book_navigation.admin.inc',
      )
    );
   

  - add i18n_book_navigation.admin.inc and move function _i18n_book_navigation_settings_form() there
  - at the end of _i18n_book_navigation_settings_form(), use return system_settings_form($form); instead of return $form;
  - remove function _i18n_book_navigation_settings_form_submit() as it's obsolete now

Kind regards,
Sven

I am changing status as per previous comment.

Hi,

Yes, it's quite quiet here I know. I'm just doing a bit of basic module review whilst...waiting to have my module reviewed.

A couple of things I noticed:
a) module should include a README.txt. This for me is the most important file because 90% of users won't be able to read the code. I'd suggest to look at other README.txt files to see what they include. I would suggest: what the module does, how to install [assume yours is the first module anybody has ever installed], how to administer, how to use.

b) I see you use t() on #title strings:

  $form['i18n_book_navigation_settings'] = array(
    '#type' => 'textfield',
    '#title' => t('Generate different navigation blocks'),

I don't think this is needed. Can anybody else clarify?

Form field titles need to be passed to t(); it's the menu titles and descriptions that don't need to be passed to that function.
The README.txt file is preferable, but it is not required (nor is it reported in http://drupal.org/node/539608).

Ha, ha! Yes, that's the problem. We're all busy folk. Too many folk who want their module reviewed and not enough who want to give their time to reviewing :-(. Hooray at least for kiamlaluno!! If it weren't for him, I think this queue would pretty much stop.

Ditto on the Kudos for Kiamlaluno. Awesome work and commitment. :-)

Welcome on board!

subscribe