Block Options

Hi sauce71,

This looks useful and the code looks pretty good. I noticed one issue. Please move this back to "needs review" when that is complete.

There are a few minor formatting issues that I noticed:

• *Comments should not exceed 80 characters: line 122 Zen themes uses the $classes variable in block.tpl.php, so themers just have to create classes
• *all comments should start capitalized, there should be a space after "//" and a "." at the end: line 71 (and others) // rippet from CCK content.module
• lines in README.txt should not exceed 80 characters
• remove extra line before block_options_schema()
• Add "Implementation of hook_install()" for block_options_install()

It appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.
Review of the master branch:

• Run coder to check your style, some issues were found (please check the Drupal coding standards):
  

  Severity minor, Drupal Commenting Standards, Internationalization, Drupal Security Checks, Drupal SQL Standards, Drupal Coding Standards
  
  sites/all/modules/pareview_temp/test_candidate/block_options.module:
   +6: [minor] There should be no trailing spaces
   +60: [minor] There should be no trailing spaces
   +66: [minor] There should be no trailing spaces
   +85: [minor] There should be no trailing spaces
   +91: [minor] There should be no trailing spaces
  
  sites/all/modules/pareview_temp/test_candidate/block_options.install:
   +47: [minor] There should be no trailing spaces
   +60: [minor] There should be no trailing spaces
   +68: [minor] There should be no trailing spaces
  
  sites/all/modules/pareview_temp/test_candidate/block_options.admin.inc:
   +5: [minor] There should be no trailing spaces
   +34: [minor] There should be no trailing spaces
  
  Status Messages:
   Coder found 1 projects, 3 files, 10 minor warnings, 0 warnings were flagged to be ignored
  

• All comments should start capitalized.
  

  block_options.module:116:    // if the option is set.
  

• All comments should end with a ".".
  

  block_options.module:115:    // Use !empty($block_options['my_option']) in the template file to check
  

This automated report was generated with PAReview.sh, your friendly project application review script. Please report any bugs to klausi.

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

• block_options.install in block_options.info: It's only necessary to declare files[] if they declare a class or interface.
• block_options.module in block_options.info: It's only necessary to declare files[] if they declare a class or interface.
• block_options.admin.inc in block_options.info: It's only necessary to declare files[] if they declare a class or interface.

This automated report was generated with PAReview.sh, your friendly project application review script. Please report any bugs to klausi.

manual review:

• block_options_settings_form(): wrong doc block formatting, see http://drupal.org/node/1354#functions
• "$form = array();": there should only be one space before "="
• "$form['settings']['block_options_title'] = array(": the closing ")" of the array should be on the same level. Also for the other arrays.
• block_options_schema(): "'data' => array(": indentation errors for the items of this array
• block_options_install(): no need to set the variables here, you can specify default values with variable_get() anyway. Remove this function.
• "function block_options_conf_filter( $key ) {": there should be mo space after "(" and before ")". See http://drupal.org/node/318#functdecl
• "// Only saves options that are checked (array_filter).": do not filter user data on input, but on output. Save the input as is, but make sure that you sanitize on output, i.e. in block_options_preprocess_block() and block_options_form_alter(). See http://drupal.org/node/28984
• There is an XSS security vulnerability in block_options_form_alter(), if I enter <script>alert('XSS');</script> for block_options_title or description. Make sure to sanitize that settings before outputting them!

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

• Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards):
  

  
  FILE: ...ew/sites/all/modules/pareview_temp/test_candidate/block_options.install
  --------------------------------------------------------------------------------
  FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
  --------------------------------------------------------------------------------
   44 | ERROR | Space before closing parenthesis of function call prohibited
   51 | ERROR | Inline comments must end in  full-stops, exclamation marks, or
      |       | question marks
  --------------------------------------------------------------------------------
  
  
  FILE: ...iew/sites/all/modules/pareview_temp/test_candidate/block_options.module
  --------------------------------------------------------------------------------
  FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
  --------------------------------------------------------------------------------
   32 | WARNING | A comma should follow the last multiline array item. Found:
      |         | $delta
  --------------------------------------------------------------------------------
  

• All text files should end in a single newline (\n). See http://drupal.org/node/318#indenting
  

  ./block_options.install ./block_options.admin.inc ./block_options.info ./block_options.module ./README.txt
  

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

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

Switched back to needs review, so in-depth reviews won't be blocked by coding standart issues.

There still seem to be coding standard issue on your module.

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

• Bad line endings were found, always use unix style terminators. See http://drupal.org/coding-standards#indenting
  

  ./block_options.info:      ASCII text, with CRLF line terminators
  ./README.txt:              UTF-8 Unicode (with BOM) English text, with CRLF line terminators
  block_options.admin.inc
  block_options.info
  block_options.install
  block_options.module
  README.txt
  

• 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.

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

I have two tiny issues and then it's perfect IMO:

1. Notify the user if a key (class name) is entered more than once (in the "Available options" textarea).
2. Maybe move the admin page to Structure > Blocks > Block options? Feels more natural to me.

Pareview also found something:

FILE: ...ew/sites/all/modules/pareview_temp/test_candidate/block_options.install
--------------------------------------------------------------------------------
FOUND 5 ERROR(S) AFFECTING 5 LINE(S)
--------------------------------------------------------------------------------
29 | ERROR | Array indentation error, expected 6 spaces but found 4
30 | ERROR | Array indentation error, expected 6 spaces but found 8
31 | ERROR | Array indentation error, expected 6 spaces but found 8
32 | ERROR | Array indentation error, expected 6 spaces but found 8
33 | ERROR | Array closing indentation error, expected 4 spaces but found 6
--------------------------------------------------------------------------------

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

Added git clone for D6 and D7

Removing unused tag.