_dala_ [_dala_]

Hi. Please read all the following and the links provided as this is very important information about your CVS Application:

Drupal.org has moved from CVS to Git! This is a very significant change for the Drupal community and for your application. Please read the following documentation on how this affects and benefits you and the application process:
Migrating from CVS Applications to (Git) Full Project Applications

• The status of this application will be put to "postponed" and by following the instructions in the above link, you will be able to reopen it.
• Or if your application has been "needs work" for more than 5 weeks, your application will be marked as "closed (won't fix)". You can still reopen it, by reading the instructions above.

Hi _dala_ I signed up to review your code and help you to get your code, and your coding skills, ready to qualify for the Drupal git repository. I will post my findings shortly.

The items below are not in order of importance. More review may follow after fixing these items. The purpose is to help you learn to use the Drupal API better and to improve your (Drupal) coding skills.

1. Info file

The .info file is missing vital data. See http://drupal.org/node/542202

2. Code style

Always check your code for code style before committing. Use the Coder Module to check.

3. Usability

Use collapisble fieldsets with care, they hide data. No need to use collapsible here.
Add comment to the check box to explain the contradicting (!) 'delay' and improvement of page load performance.

4. Checkbox never unset

The checkbox status is set in block_delay_save_settings(), but it can never be unset. The code you use here seems complex to me. Search for other module that use block config settings and use it as example code.

5. theming

Don't mix html and code in block_delay_footer(). Use theme() and the Drupal theme system to allow others to override the ouput.

    $output .= '<div id="content-'.$module.'-'.$delta.'" style="display:none;">'. $block['content'] .'</div>';
    $js .= "blockDelaySwap('".$placeholder_id."', '".$content_id."');\n";
  }
}
if ($js) {
  $js = '<script type="text/javascript">'. $js .'</script>';

6. Load js conditionally

Don't use drupal_add_js() in hook_init. Use it conditionally when required.

7. hook_footer() ?

hook_footer does not exist. Does block_delay_footer() ever get called?

#3 Usability

You don't have to agree with me on the use of collapsible fieldsets. But if it this setting is not important why don't you collapse it by default?

#4 Checkbox code

I'm not looking for the best possible code. The point is that you realize that you can learn from examples and use them.

Re. 5 Theming

Using the theme layer is not an option, it is a requirement. It's what makes Drupal flexible.

Re. 6 Conditional JS

In block_delay_preprocess_block() where you empty the block you could add the js conditionally.

Re. 7 hook_footer

Ah, this is a Drupal 6 module, that changes things (hook_footer does not exist D7).

8 Fail on module_name

list($module, $delta) = explode('_', $block_id);

This code will fail if the module name contains underscores. Use a different separator.

9 variable names

function blockDelaySwap(a,b) {
  var content = $('#'+b);
  if (content) {
    $('#'+b).children("script").remove();
    $('#'+b).children("noscript").remove();
    content.parent().remove('#'+b);
    var pholder = $('#'+a);
    if (pholder) {
      pholder.replaceWith(content.html());
    }
  }
}

Use descriptive ('a', 'b') and English variable names ('pholder'). Descriptive names will make maintenance soo much easier.

10 Documentation

See http://drupal.org/node/161085
Add some more documentation: README.txt, with the JS code. Optional: @file in .module, .install and .js ('do like core does').

Re. 2 Code style

The code still does not apply to Drupals coding standard. Use the Coder module to check. Code style is very important to Drupal developers. It helps to read and recognize code. This makes modules and code better maintainable, exchangeable between developers and helps co-operation.

The above is the only critical remarks I have on the module. After this it should be ready for release. Further I have evaluated the module's security. The module uses the block content from hook_preprocess(). At this point the content should already be safe. Moving this safe content it from one region to another does not introduce any safety risk.

Re. 6 Conditional JS

Nice solution introducing block_delay_active_queue()

I have a few more points for improvement, but not critical. It won't block the module from RTBC

11 Module description

The module description is exceptionally long (.info). One or two sentences are usually sufficient.

12 CVS keyword $Id$

The CVS keyword $Id$ is no longer required (even when coder tells you to add it). Some files still contains one.

13 Block id

  $block_id = str_replace('_', '~', $form['module']['#value']) .'_'. $form['delta']['#value'];

I would have done:

  $block_id = $form['module']['#value']) .'-'. $form['delta']['#value'];

However you may need to solve another problem. Because block delta's may contain more than just [a-z|0-9] you even may run into spaces :(

14 empty lines

You could use empty lines in the code to separate parts within the code. For example:

function block_delay_active_queue($block_id=null) {
  static $active_ids;

  if (!isset($active_ids)) {
    $active_ids = array();
  }
  if (!is_null($block_id)) {
    $active_ids[$block_id] = $block_id;
  }

  return $active_ids;
}

But agreed this is personal. Search for examples on this in core.

15 non-js fallback

Users without JS should ideally have a fallback situation. I see no way the module can provide it. But a remark could be added to the README.txt for the site builder. For example suggesting to add an backgound image to the emptied block.

That's all folks ;)

Sutharsan made an absolutely fabulous review. I checked for basic issues with the code and would say it's good to go. Setting to RTBC but I would suggest you don't create stable releases for a while. I would say that the apparent issue with the module is that it won't work without Javascript. Try asking the community for help.

Congratulations, you know can promote sandbox projects to full status ones!

Nice work and when you would like further review for any module please do ask for one at http://groups.drupal.org/peer-review/requests (or hire Sutharsan!)

One minor remaining coding style issue, the first line of comment blocks for functions should be followed by a blank comment line.

Thanks for your contribution and i look forward to its continuing improvement!

_{benjamin, agaric}