mikedance [mikedance]

We just review a module / theme per applicant. Please choose the ne you want reviewed and let us know.

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

As per http://drupal.org/cvs-application/requirements, the motivation message should be expanded to contain more details about the features of the proposed module, and it should include also a comparison with the existing solutions.

I will review the code tomorrow morning.

1.   $sql = "DELETE FROM {variable} WHERE name LIKE 'meerkat_%'";
   
     db_query($sql);
   

   The code could delete variables used by a module whose name starts with meerkat_ (there is no rules that blocks somebody from creating a module named meerkat_addons, in example).

2. /**
    * Implementation of hook_enable().
    */
   function meerkat_enable() {
     drupal_set_message(t('The meerkat module has been enabled.'));
   }
   
   /**
    * Implementation of hook_disable().
    */
   function meerkat_disable() {
     drupal_set_message(t('The meerkat module has been disabled.'));
   }
   

   I would rather remove those messages; they simply report to who disabled the module (or enabled it) that the module has been disabled (or enabled). Those messages would make more sense if they would report, in example, that the node permissions must be rebuild.

3. Strings used in the user interface should be in sentence case.

4.   $result = db_query($sql, $input);
   
     if ( $result ) {
       while ( $row = db_fetch_array($result) ) {
         $row['data'] = unserialize($row['data']);
   
         return $row;
       }
     }
   

   Such code is not used from any Drupal core modules, which don't check first the result returned by db_query(); db_fetch_array() (as well as db_fetch_object()) is able to handle the case the passed parameter the boolean FALSE.

5.     $link = l(t('Please click here to add a new default.'), 'admin/settings/meerkat/defaults/add');
   
       $output = t('There are no defaults at this time. !link', array('!link' => $link));
   
   

   l() should not be used together t(); see the documentation for t(), which reports such code as wrong code.

6.   if ( $result ) {
       while ( $row = db_fetch_array($result) ) {
         $output[$row['id']] = $row['name'];
       }
     }
   

   The code is not formatted as it should; the space after the opening parenthesis, or before the closing parenthesis in the control statements are not required.

I checked only the merkat.module code.

There have not been replies in the past week. I am marking this application as won't fix.