Faustas [faustas]

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.

License files cannot be committed in drupal.org repositories. This is also reported in http://drupal.org/node/539608; I would suggest you to read what else is reported in that page, for your benefit.

Hello,

this week i searched for a module that reminds me to check some of the nodes on my site. I found this module and have to say it's great.
I reviewed the code with the coder module and checked some of the code against my own security standarts. I couldn't find anything alarming.

Would be happy to find this module in the cvs shortly.
Cheers Friik

• 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 version line needs to be removed from the .info file.
   The package name is used when a module is part of a group of modules, not report the name of the module, which is already given with name =.
2. Hook implementation comments should be like the following one:

   /**
    * Implements hook_menu().
    */
   

3. function resubmission_install() {
     drupal_set_message(t('Installing resubmission schema.'));
     switch ($GLOBALS['db_type']) {
       case 'mysqli':
       case 'mysql':
         drupal_install_schema('resubmission');
         break;
       case 'pgsql':
         db_query("CREATE TABLE resubmission_nodes (
           nid integer DEFAULT 0 NOT NULL,
           type varchar(254) DEFAULT NULL,
   	      time integer NOT NULL,
         	once integer NOT NULL,
           ival integer DEFAULT NULL,
           desc varchar(254) DEFAULT NULL,
           PRIMARY KEY (nid)
         )");
         break;
     }
   }// function _install
   
   

   The implementation of hook_install() has not been updated for Drupal 6.
   The comment at the end of the function is not standard in Drupal.

4. See http://drupal.org/coding-standards to understand how a module should be written. In particular, see how Drupal variables, global variables, constants and functions defined from the module should be named; how the code should be formatted.
5. Menu descriptions and titles, as well as schema descriptions, are not passed to t().

6.       $node = node_load($data['nid']);
         $title = $data['nid'];
         if (!empty($node)) {
           if (!empty($node->title)) {
             $title = $node->title;
           }
         }
   
   

   The code doesn't verify if the current logged in user as the permission to see the node data, nor if the node has been unpublished.

7.       if ((int)$data['once'] === 1) {
           $once = '<img src="' . base_path() . $path . '/img/accept.png" />';
         }
   
   

   There is a Drupal theme function for images, as well as a Drupal function that returns a Drupal URL.

8. function resubmission_help($section) {
     switch ($section) {
       case 'admin/help#resubmission':
       // Return a line-break version of the module README
       return filter_filter('process', 1, NULL, file_get_contents( dirname(__FILE__) . "/README.txt") );
     }
   }// _help
   
   

   The implementations of hook_help() can use full HTML, and there is no reason to pass the content to filter_filter().
   There is a function that returns the path of a module, which should be used in such cases.

9.       '#description' => t('Write a description for the resubmission that wil be shown in the overview list. Maximum 254 characters.'),
   
   

   […] that will be shown […].

10.     if ((int)$form['#post']['resubmission_node_interval'] === 0) {
    
          $date = $form['#post']['resubmission_node_date'];
          $target_date = $date['year'] . '-' . $date['month'] . '-' . $date['day'];
          $cur_date = date('Y-n-j');
    
    

    The values to validate are contained in $form_state.

11. function resubmission_form_submit($form_id, $form) {
      if (resubmission_is_node_relevant($form['values']['form_id']) && (int)variable_get('use_resubmission', 0) == 1) {
        $r_active = (int)$form['values']['resubmission_is_activated'];
        $r_interval = (int)$form['values']['resubmission_node_interval'];
        $r_type = (string)$form['values']['type'];
        $r_desc = (string)check_plain($form['values']['resubmission_node_desc']);
        $nid = (int)$form['values']['nid'];
    
        // is the node in the resubmission_nodes table?
        $is_registered = _resubmission_node_registered($nid);
    
        // resubmission activated
        if ($r_active == 1) {
    
          $date_selected = 0;
          // 'date' entry selected?
          if ($r_interval === 0) {
            $date = $form['values']['resubmission_node_date'];
            $target_date = $date['year'] . '-' . $date['month'] . '-' . $date['day'];
            $cur_date = date('Y-n-j');
    
            // to compare
            $cur_date = strtotime($cur_date);
            $target_date = strtotime($target_date);
    
            $date_selected = 1;
            $r_interval = ($target_date - $cur_date) / (60*60*24);
          }
    
          // Store the data
          // already registered?
          if ($is_registered) {
            _resubmission_node_update($nid, time(), $r_interval, $date_selected, $r_desc);
          }
          // has to be registered
          else {
            _resubmission_node_register($nid, $r_type, time(), $r_interval, $date_selected, $r_desc);
          }
        }
        // resubmission deactivated
        else {
          // the node was registered?
          if ($is_registered) {
            _resubmission_node_delete($nid);
          }
        }
      }
    }// _form_submit
    
    

    The function has not been updated for Drupal 6.

12.   $backs = array();
      $results = db_query("SELECT `nid`, `type`, `once`, `desc`, from_unixtime(`time`, '%Y-%m-%e') as time, from_unixtime(`time`, '%Y-%m-%e') + INTERVAL resubmission_nodes.`ival` DAY as target, DATEDIFF((from_unixtime(`time`) + INTERVAL resubmission_nodes.`ival` DAY), NOW()) as cur_diff, `ival` FROM {resubmission_nodes} ORDER BY from_unixtime(`time`) + INTERVAL resubmission_nodes.`ival` DAY ASC LIMIT %d;}", DEFAULT_ROWS_LIST_OVERVIEW);
    
    

    The character ` is not used from all the database engines Drupal is compatible with; the same is true for the SQL function from_unixtime().

13.     watchdog('Resubmission', t('Could not load the data from the resubmission_nodes table.'));
    
    

    The second parameter of watchdog() is a not translated string; watchdog() passed that string to t().

14. function _resubmission_node_delete($nid) {
      if (!empty($nid)) {
        db_query("DELETE FROM {resubmission_nodes} WHERE `nid` = '%nid';", array('%nid' => $nid));
      }
    }// _resubmission_node_delete
    

    Why isn't the code implementing hook_nodeapi() to delete any reference to deleted nodes from its own database table?

15.   foreach ($types as $node ) {
        $selectable_node_types[$node->type] = t('@name (!desc)',
          array('@name' => $node->name, '!desc' => $node->description));
      }
    
    

    $node->description needs to pass through check_plain(); that means its placeholder should be different. See the documentation for t() to understand which placeholder should be used.

16. /**
     *
     * Validate the settings dialog
     */
    function resubmission_admin_settings_validate($form, $form_state) {
      $selected_node_types = array_filter($form_state['values']['resubmission_selectable_node_types']);
      variable_set('resubmission_selected_node_types', $selected_node_types);
    }// _admin_settings_validate
    
    

    The purpose of a form validation handler is only to validate the user input, not to save the user input.

1. /**
    * Implements hook_install()
    */
   function resubmission_install() {
     drupal_set_message(st('Installing resubmission schema.'));
     switch ($GLOBALS['db_type']) {
       case 'mysqli':
       case 'mysql':
         drupal_install_schema('resubmission');
         break;
   
       case 'pgsql':
         break;
     }
   }
   
   

   The code should simply call drupal_install_schema().

2. function resubmission_uninstall() {
     // delete the variables we created.
     variable_del('use_resubmission');
     variable_del('resubmission_default_interval');
     variable_del('resubmission_selected_node_types');
   
     drupal_uninstall_schema('resubmission');
   }
   
   

   // default number of entries in the list overview
   define('DEFAULT_ROWS_LIST_OVERVIEW', 21);
   
   // definitions for the time levels
   define('DAYS_FOR_HOT', 1);
   define('DAYS_FOR_WARM', 3);
   
   

   Drupal variable names should always be prefixed by the module name; the same is true for PHP constants.

3.   $results = db_query("SELECT nid, type, once, descr, from_unixtime(time, '%Y-%m-%e') as time, from_unixtime(time, '%Y-%m-%e') + INTERVAL resubmission_nodes.ival DAY as target, DATEDIFF((from_unixtime(time) + INTERVAL resubmission_nodes.ival DAY), NOW()) as cur_diff, ival FROM {resubmission_nodes} WHERE uid = '%d' ORDER BY from_unixtime(time) + INTERVAL resubmission_nodes.ival DAY ASC LIMIT %d;", $user->uid, DEFAULT_ROWS_LIST_OVERVIEW);
   
   

   Reserved SQL keywords are written in uppercase.

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

Thank you for your contribution!
I am going to update your account so you can opt into security advisory coverage now.
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.