Jovean [jokoveltman]

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

My module leverages the ChessTempo PGN Viewer (www.chesstempo.com), an excellent and very flexible PGN viewer. Though is is not covered by the GPL, my module is not comprised of any of the ChessTempo PGN viewer code, but simply calls the JavaScript and CSS as provided by ChessTempo.com and permitted under it's license.

I guess you mean that the PNG viewer is not licensed under GPL, but your module is.

• The points reported in this review are not in order of importance / relevance.
• Most of the times I report the code that present an issue. In such cases, the same error can be present in other parts of the code; the fact I don't report the same issue more than once doesn't mean the same issue is not present in different places.
• Not all the reported points are application blockers; some of the points I report are simple suggestions to who applies for a CVS account. For a list of what is considered a blocker for the application approval, see CVS applications review, what to expect. Keep in mind the list is still under construction, and can be changed to adapt it to what has been found out during code review, or to make the list clearer to who applies for a CVS account.

1. License files cannot be committed in Drupal.org repository. Projects committed in Drupal.org repository have the same license used by Drupal.

2. dependencies[] = filter
   
   

   Core modules that cannot be disabled should not put between the dependencies of a module.

3. The module doesn't implement hook_uninstall(), or doesn't implement it to remove the Drupal variables it defines.
4. Leave an empty line between a function, and the other.

5. /**
    * Valid permissions for this module
    * @return array An array of valid permissions for this module
    */
   function pgn_diagram_filter_perm() {
     return array('access pgn_diagram_filter');
   } // function pgn_diagram_filter_perm
   
   

   Hook implementation comments should be like the following one:

   /**
    * Implements hook_menu().
    */
   

   As reported in Documenting hook implementations:

   If the implementation of a hook is rather standard and does not require more explanation than the hook reference provides, a shorthand documentation form may be used in place of the full function documentation block described above:

   /**
    * Implements hook_help().
    */
   function blog_help($section) {
     // ...
   }
   

   This generates a link to the hook reference, reminds the developer that this is a hook implementation, and avoids having to document parameters and return values that are the same for every implementation of the hook. Optionally, you can add more information in a separate paragraph to describe the particular quirks of your hook implementation.

   In the case of hooks that have variables in the names, such as hook_form_FORM_ID_alter(), a slightly expanded syntax should be used:

   <?
   /**
   * Implements hook_form_FORM_ID_alter() for node_type_form().
   */
   function mymodule_form_node_type_form_alter(&$form, &$form_state) {
   // ...
   }
   ?>

   This generates a link to the hook reference, as well as to the particular form that is being altered. Again, optionally you can add more information in a separate paragraph to describe the particular quirks of your hook implementation.

   The comment at the end of the function is not standard in Drupal.

6. A module that implements a filter usually have settings that are set for each input format the filter is used in.

7.   $sizes = array(
       20 => 'tiny',
       24 => 'small',
       29 => 'normal',
       35 => 'medium',
       40 => 'mediumlarge',
       46 => 'large',
       55 => 'extralarge',
       65 => 'xxl',
       75 => 'insane',
     );
     $form['pgn_diagram_filter_piece_size'] = array(
       '#type' => 'select',
       '#title' => t('Default piece set'),
       '#options' => $sizes,
       '#default_value' => variable_get('pgn_diagram_filter_piece_size', 'normal'),
       '#description' => t('Choose the default piece size.'),
     );
   
   

   Strings used in the user interface should be translated.

8.   if ($has_diagram){
       $str .= t('<div id="chesstempo-attribution">The PGN diagrams displayed on this page use the ' .
       l('ChessTempo PGN Viewer', 'http://chesstempo.com/pgn-viewer.html') . '.</div>');
     }
   
   

   l() should not be used together with t(). See the documentation for t(), where this code is reported to be wrong:

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

   The correct code to use is the following:

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

9. /**
    * Valid permissions for this module
    * @return array An array of valid permissions for this module
    */
   function pgn_diagram_filter_perm() {
     return array('access pgn_diagram_filter');
   } // function pgn_diagram_filter_perm
   
   

   Modules that implement a filter don't define a permission to access a filter, as Drupal core code implements the permissions to use a input format (which is made of filters implemented by different modules).

The parameter $format passed to hook_filter() is the ID of the input format that includes the filter implemented by the module.
A filter module that has code for the operation 'settings' is returning form fields that are used to create a settings form that is specific to an input format. In fact, the settings handler uses Drupal variables like allowed_html_$format, not allowed_html.

function filter_filter($op, $delta = 0, $format = -1, $text = '') {
  switch ($op) {
    case 'list':
      return array(0 => t('HTML filter'), 1 => t('Line break converter'), 2 => t('URL filter'), 3 => t('HTML corrector'));

    case 'description':
      switch ($delta) {
        case 0:
          return t('Allows you to restrict whether users can post HTML and which tags to filter out. It will also remove harmful content such as JavaScript events, JavaScript URLs and CSS styles from those tags that are not removed.');
        case 1:
          return t('Converts line breaks into HTML (i.e. <br> and <p> tags).');
        case 2:
          return t('Turns web and e-mail addresses into clickable links.');
        case 3:
          return t('Corrects faulty and chopped off HTML in postings.');
        default:
          return;
      }

    case 'process':
      switch ($delta) {
        case 0:
          return _filter_html($text, $format);
        case 1:
          return _filter_autop($text);
        case 2:
          return _filter_url($text, $format);
        case 3:
          return _filter_htmlcorrector($text);
        default:
          return $text;
      }

    case 'settings':
      switch ($delta) {
        case 0:
          return _filter_html_settings($format);
        case 2:
          return _filter_url_settings($format);
        default:
          return;
      }

    default:
      return $text;
  }

function _filter_html($text, $format) {
  if (variable_get("filter_html_$format", FILTER_HTML_STRIP) == FILTER_HTML_STRIP) {
    $allowed_tags = preg_split('/\s+|<|>/', variable_get("allowed_html_$format", '<a> <em> <strong> <cite> <code> <ul> <ol> <li> <dl> <dt> <dd>'), -1, PREG_SPLIT_NO_EMPTY);
    $text = filter_xss($text, $allowed_tags);
  }

  if (variable_get("filter_html_$format", FILTER_HTML_STRIP) == FILTER_HTML_ESCAPE) {
    // Escape HTML
    $text = check_plain($text);
  }

  if (variable_get("filter_html_nofollow_$format", FALSE)) {
    $text = preg_replace('/<a([^>]+)>/i', '<a\\1 rel="nofollow">', $text);
  }

  return trim($text);
}

I will review the code tomorrow.

The version line needs to be removed from the .info file.

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.