blaxbb [blaxbb]

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.

As much as I'd like to see more zombie in Drupal, this has some issues. Stuff that probably needs to be fixed before this can be approved:

• Per Drupal coding standards, line endings should all be \n. Many are not.
• hvz_set_status runs an update function, but there's nothing to update if the users were created before the module was enabled, so only users created after the module was enabled can play. This seems like a pretty big bug.
• hvz_set_status creates a variable named $user (risky variable name due to global $user, $account is better), and then does nothing with it.
• hvz_edit_form_validate sets some variables, but doesn't really do anything with them.
• hvz_admin_validate is an empty function. If it's not doing anything, it should be deleted.
• $l = t('<strong>An Original Zombie</strong>'); HTML in t() should be avoided, and is easy to avoid here.
• The entire module has one comment.

Stuff that would be good to fix, but maybe isn't necessary:

• hvz_admin_submit has a switch statement with 5 cases, only one of which does anything. Why not just use a single if()?
• $content = ''; immediately followed by $content .= can be shorted to $content =.
• A textfield input with specific options ("1=Human; 2=Zombie; 3=OZ(revealed); 4=OZ(hidden)") would be better as a select field.
• Indentation is inconsistent, hard to read.
• Almost no commenting.

This is all in the main module. The other modules both have similar issues. In general, this just seems kind of sloppy, not a finished project.

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

2.               $block['subject'] = 'Register a Tag';
   
   

   Strings used in the user interface should be translated.

3. See http://drupal.org/coding-standards to understand how a module should be written. In particular, see how the code should be formatted.

4.   $out = '';
     $query = "SELECT * FROM {hvz_cure} WHERE `cure`='%d'";
   
   

   The character ` is not used from all the database engines that Drupal is compatible with.

5. function hvz_cure_get_cure_exist($cure) {
   
     $out = '';
     $query = "SELECT * FROM {hvz_cure} WHERE `cure`='%d'";
     $query_result = db_query($query, $cure);
     while ($queryids = db_fetch_object($query_result)) {
       $ids .= $queryids->cure;
     }
     if ($ids != '') {
       return FALSE;
     }
     return FALSE;
   }
   
   

   The return value is always FALSE; the form submission handler would always report an error.

     if (hvz_cure_get_cure_exist($cure)) {
       hvz_set_status($user->uid, 1);
       drupal_set_message('You have been cured!  Try to do better this time....');
     }
     else {
       drupal_set_message('Invalid Cure Number!');
     }
   
   

6.   for ($i=1; $i<=$number; $i++) {
       $query = db_query("INSERT INTO {hvz_cure} (cure) VALUES ('%s') ", hvz_cure_generate_cure());
     }
   
   

   Why isn't the code using drupal_write_record()?

7.       'description' => l($user->name, 'user/' . $user->uid) .
                  ' tagged ' . l($killed->name, 'user/' . $killed->uid) . '.',
   
   

   The description should be a string passed to t() as every string used in the user interface needs to be translated (tagged is not translated).

8. Menu descriptions and titles, as well as schema descriptions, are not passed to t().

9.     case "admin/help#hvz":
         $output = '<p>' .  t("HvZ Module Help") . '</p>';
         break;
   
   

   That help string doesn't help much.

10.   $query = "SELECT uid FROM " . "{hvz} WHERE uid='%d'";
    
    

    Why is the query string a concatenation of two literal strings?

11.   if ((hvz_get_id_from_user($uid)) == '') {
        return FALSE;
      }
      return TRUE;
    
    

      return hvz_get_id_from_user($uid) != ''; 
    

    The original code contains a couple of parentheses that are not needed (the ones around the function call).

12.   switch ($gamestate) {
        case 0:
    
          break;
    
        case 1:
    
          break;
        case 2:
    
          break;
        case 3:
          drupal_set_message(t('ALL OZS REVEALED!'));
          hvz_reveal_ozs();
          break;
        case 4:
    
          break;
    
      }
    
    

    I guess the code is not complete; one of the requirements for CVS applications is that the code is complete.

13.       db_query($query, $uid);
          drupal_set_message(t('UID ' . $uid . ' deleted from HvZ Record.'));
    
    

    The first argument of t() is a literal string, not a concatenation of strings. The script used to create the translation template is not able to handle any dynamic value, even in the case of code similar to t($variable), or t(function()); this means that if the argument of the function is not a literal string, it will not appear in the translation template.

14.   else {
        return t('You are not a zombie.  Stop trying to eat other people\'s brains.');
      }
    
    

    Avoid escaping a string delimiter inside a string, especially if the string is passed to t().

15.         '#description' => t('Volunteer to play as an OZ (you are not guaranteed the role.)'),
    
    

    Description strings should contain sentences ending with a period.

I am closing this application due to lack of replies.

Please read the following links as this is very important information about CVS applications.

• Drupal.org has moved from CVS to Git! This is a very significant change for the Drupal community and for these applications.
• Please read Migrating from CVS Applications to (Git) Full Project Applications and Applying for permission to opt into security advisory coverage on how this affects and benefits you and the application process. In short, every user has now the permissions necessary to create new projects, but they need to apply for opt into security advisory coverage. Without applying, the projects will have a warning on projects that says:
  

  This project is not covered by Drupal’s security advisory policy.