joe casey [joecasey]

Hello, and thank you for applying for a CVS account.

As per requirements, the motivation should include more than 2 sentences of module features, and a comparison with the existing solutions.

Perhaps I am missing something, but what does this offer that Panels doesn't?

Joe,

Looks like you've followed the coding and documentation standards really closely - this is one of the cleanest-looking modules I've reviewed!

1. Please make sure all inline comments start with a capital and end wit a period:
   // no blocktable and no blocks, ignore this region
2. You can remove your require_once 'blocktable_ui.inc'; snippet, and just place it in your hook_menu() implementation, so that it is only included on calls for your configuration form. ie:
   

   $items['admin/build/blocktable'] = array(
     'page callback' => 'drupal_get_form',
     'page arguments' => array('blocktable_multiform'),
     'title' => 'Blocktable',
     'description' => 'Put blocks within a region into an HTML table',
     'access arguments' => array('administer blocktable'),
     'file' => 'blocktable_ui.inc',
     'type' => MENU_NORMAL_ITEM,
   );
   

That's all I can really find at this point, though once those are fixed, I'll re-review to double check everything.

I've re-reviewed the module and everything looks good.

It looks like you did a really nice job on it.

1. t() is available inside hook_install(); there is no need to use st() instead. If would not be available, then the code should use the following snippet (as used in <code>hook_requirements()).

     $t = get_t();
   
     drupal_set_message($t(/* ...*));
   

2. l() should not be used together t(). As reported in the documentation for t():

   Here is an example of incorrect usage of t():

     $output .= t('<p>Go to the @contact-page.</p>', array('@contact-page' => l(t('contact page'), 'contact')));
   

   Here is an example of t() used correctly:

     $output .= '<p>'. t('Go to the <a href="@contact-page">contact page</a>.', array('@contact-page' => url('contact'))) .'</p>';
   

   To notice that the correct placeholders for links don't start with !.

3. To get the path where a module is installed, Drupal has the function drupal_get_path().
4. The help text returned from hook_help() is normally returned directly without to pass it through any filter functions.

5. /**
    * Get all blocks for a region, regardless of user or role.
    * Adapted from block_list in block.module.
    *
    * @param $region
    *  Regions for the current theme.
    * @param $theme
    *  The current default theme.
    * @return
    *  Array of block objects.
    */
   

   The first sentence of a Doxygen function comment should be in third person.

6. See http://drupal.org/coding-standards to understand how a module should be written; in particular see how Drupal variables, global PHP variables, classes, functions, and local PHP variables defined from the module should be named.

7.     $error_2 = t(' Actual: ') . $actual_string . "<br />";
       $error_3 = t(' Defined:') . $markup_string;
       $error_string = $error_1 . $error_2 . $error_3;
   

   Use t()-placeholders, in such cases; in that way, the string passed to t() gives to translator users the contenxt in which the phrase is translated (in many languages, a English word is translated differently basing on the context provided from the sentence).
   When possible, it is better to not split a sentence in phrases that are passed separately to t().

8. The module define the permission use PHP for blocktable visibility, but it doesn't seem it checks the user has that permission where it should (e.g. right before to call drupal_eval()).

Point #6 was referring to the function get_bids(), which doesn't have the correct name; any function defined from the module must have a name prefixed by the module name. The same is true for Drupal variables, and global PHP variables.

Thank you for your contribution! I am going to update your account.

These 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 more contributors chatting on the IRC #drupal-contribute channel. So, come hang out and stay involved.
Thank you, 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.

I thank all the dedicated reviewers as well.