cleaver [cleaver]

Hello, and thanks 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.

• The points reported in this review are not in order or 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. The module doesn't implement hook_uninstall().
2. Hook implementation comments should be like the following one:

   /**
    * Implements hook_menu().
    */
   

3. function tweetrss_enable() {
     variable_set('tweetrss', array('tweetrss' => 'Tweet RSS Regex'));
   }
   
   

   What is the purpose of changing the value of that Drupal variable each time the module is enabled, or disabled? If the module is disabled, that Drupal variable is not even used.

4. Menu descriptions and titles, schema descriptions are not passed to t().
5. Remove any debugging code.

6. function _tweetrss_feed_row($feed) {
     $form['#feed'] = $feed;
     $form['screen_name'] = array(
       '#type' => 'markup',
       '#value' => $feed['screen_name'],
     );
     $form['title'] = array(
       '#type' => 'markup',
       '#value' => $feed['title'],
     );
     $form['feedtype'] = array(
       '#type' => 'markup',
       '#value' => $feed['feedtype'],
     );
     $form['edit_link'] = array(
       '#type' => 'markup',
       '#value' => l(t('edit'), "admin/settings/tweetrss/edit/{$feed['tfid']}"),
     );
     return $form;
   }
   
   

   Why is the code using a form builder, if there are no form fields?

7. function tweetrss_filter_xss($string) {
     // Only operate on valid UTF-8 strings. This is necessary to prevent cross
     // site scripting issues on Internet Explorer 6.
     if (!drupal_validate_utf8($string)) {
       return '';
     }
     // Remove Netscape 4 JS entities
     $string = preg_replace('%&\s*\{[^}]*(\}\s*;?|$)%', '', $string);
   
     // Defuse all HTML entities
     $string = str_replace('&', '&', $string);
     // Change back only well-formed entities in our whitelist
     // Decimal numeric entities
     $string = preg_replace('/&#([0-9]+;)/', '&#\1', $string);
     // Hexadecimal numeric entities
     $string = preg_replace('/&#[Xx]0*((?:[0-9A-Fa-f]{2})+;)/', '&#x\1', $string);
     // Named entities
     $string = preg_replace('/&([A-Za-z][A-Za-z0-9]*;)/', '&\1', $string);
   
     return $string;
   }
   
   

   Why isn't the code using filter_xss()?

1.   $schema['tweetrss_feed'] = array(
       'description' => t('Record a list of tweetrss feeds'),
       'fields' => array(
         'tfid' => array(
           'description' => t('tweetrss feed ID'),
           'type' => 'serial',
           'not null' => TRUE,
         ),
         'twitter_uid' => array(
           'description' => t('Twitter user ID'),
           'type' => 'numeric',
           'not null' => TRUE,
           'default' => 0,
           'precision' => '20',
           'scale' => '0',
         ),
         'fid' => array(
           'description' => t('Feed Id'),
           'type' => 'int',
           'not null' => TRUE,
           'default' => 0,
         ),
   
   

   Menu descriptions and titles, as well as schema descriptions, are not passed to t().
   The strings used as description should start with a capitalized word, end with a period, and contain a sentence (not a phrase).

2. The uninstall hook was there already

   I should have better reported what I meant: the module doesn't implement the code to remove the Drupal variables it defines in hook_uninstall().

   The Drupal variable is used to store an array containing a list of the available filter submodules. As add-on filter modules are enabled and disabled, this array will have elements added and removed. The array is used to generate a drop drop down in the edit screen--this is used to select the filter module to apply.

   In that case, the module should use a custom hook that allows filter submodules to report information about themselves.
   Furthermore, it doesn't make sense for the main module to delete the Drupal variable it uses for that purpose to signal to itself that there are no filter submodules; since the main module is disabled, its code will not be executed, and it cannot verify if there are filter submodules.

3. /**
    * Implements hook_perm()
    */
   function tweetrss_canweather_perm() {
     return array('administer tweetrss');
   }
   
   

   Two modules cannot declare the same permission.

4.   if (preg_match("/Current Conditions|Conditions actuelles/", $data)) {
       $include = TRUE;
     }
   
   

   Why is the code checking only for English or French strings?

5.   // Remove Netscape 4 JS entities
     $string = preg_replace('%&\s*\{[^}]*(\}\s*;?|$)%', '', $string);
   
     // Defuse all HTML entities
     $string = str_replace('&', '&', $string);
     // Change back only well-formed entities in our whitelist
     // Decimal numeric entities
     $string = preg_replace('/&#([0-9]+;)/', '&#\1', $string);
     // Hexadecimal numeric entities
     $string = preg_replace('/&#[Xx]0*((?:[0-9A-Fa-f]{2})+;)/', '&#x\1', $string);
     // Named entities
     $string = preg_replace('/&([A-Za-z][A-Za-z0-9]*;)/', '&\1', $string);
   
   

   All those calls to preg_replace() can be replaced by a single call.

6.       $sql =
           "INSERT INTO {tweetrss_feed} (
             twitter_uid,
             fid,
             feedtype,
             regex_match,
             regex_replace,
             checked )
           VALUES (%d, %d, '%s', '%s', '%s', %d)";
         db_query($sql,
           $feed['twitter_uid'],
           $feed['fid'],
           $feed['feedtype'],
           $feed['regex_match'],
           $feed['regex_replace'],
           $feed['checked']);
   
   

   Why isn't the code using drupal_write_record()?

7. The code, in some points, is not formatted as reported by the coding standards.

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.