ositoblanco [ositoblanco]

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.

I've just looked at the privatemsg related sub-module...

- Note that the database schema has changed in privatemsg 6.x-2.x. This is not a problem in your code, just an information :)
- You're doing a pretty huge query with a ton of left joins there. Given that the pm_ tables could contain a very large number of entries, this might be quite slow. But I'm not sure if there is a better way.
- You should use the db_placeholders (http://api.drupal.org/api/function/db_placeholders/6) function to insert the role ids...

I am changing the status as per previous comment; db_query()-placeholders should be used instead of injecting a value into the query, especially if the value is not static.

• This is a partial review.
• 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. The version line needs to be removed from the .info file.

2. function digest_uninstall() {
     db_query("DELETE FROM {variable} WHERE name LIKE 'digest%%'");
     drupal_uninstall_schema('digest');
   }
   

   Deleting Drupal variables using a query that matches any Drupal variable with a name that starts with the module name would remove also the Drupal variables of other modules.

3.     if (variable_get('digest_verbosity', 1)) {
         watchdog('digest', 'There where no digest mails to be send.', array(), WATCHDOG_DEBUG);
       }
   
   

   It should be there were no digest mails.

4.     if (variable_get('digest_verbosity', 1)) {
         watchdog('digest', 'Digest mail sent to !uid (!username, !mail).', array('!uid' => $uid, '!username' => $digest_user_info['#user']['name'], '!mail' => $digest_user_info['#user']['mail']), WATCHDOG_DEBUG);
       }
   
   

   The username could contain characters that have particular meaning for HTML.

5.   $options = array(
       0 => 0,
       1 => 1,
       2 => 2,
       3 => 3,
       4 => 4,
       5 => 5,
       6 => 6,
       7 => 7,
       8 => 8,
       9 => 9,
       10 => 10,
       11 => 11,
       12 => 12,
       13 => 13,
       14 => 14,
       15 => 15,
       16 => 16,
       17 => 17,
       18 => 18,
       19 => 19,
       20 => 20,
       21 => 21,
       22 => 22,
       23 => 23,
     );
   
     $form['digest_cron']['digest_notify_start_time'] = array(
       '#type' => 'select',
       '#title' => t('Start time'),
       '#description' => t('At that time the cron job starts a new digest run. The time the digest mail actually will be send depends on the amount of digest messages to be send and how many of them could be send per cron run. This could result in various cron runs for one digest. In this case notifications for activities which are taking place after the digest has started will be delivered by the next digest run (at the next day) &emdash; not by the next cron run at the same day.'),
       '#options' => $options,
       '#default_value' => variable_get('digest_notify_start_time', 18),
     );
   
   

   The array $options should simply be array(0, 1, 2, 3, 4, /*... */), or range(0, 23).

6.   $form['digest_mail'] = array(
       '#type' => 'fieldset',
       '#title' => t('Digest mail'),
       '#collapsible' => FALSE,
       '#collapsed' => FALSE,
     );
   
   

   By default, a fieldset is not collapsible, and not collapsed; the default values should not be declared.

7. function _digest_notify_default_subject() {
     $default_subject = variable_get('site_name', '')? '[' . variable_get('site_name', '') . '] - ' : '';
     $default_subject .= 'Notification';
     
     return $default_subject;
   }
   
   

   Strings used in the user interface should be translated.

If the module name is digest.module, then every Drupal variable name should be prefixed by digest_.

Looks good to me.

A couple of small things:
1) README.txt I think doesn't need $Id$

2) LICENCE.txt is not needed as CVS will add this automatically

Hi. Please read all the following and the links provided as this is very important information about your CVS Application:

Drupal.org has moved from CVS to Git! This is a very significant change for the Drupal community and for your application. Please read the following documentation on how this affects and benefits you and the application process:
Migrating from CVS Applications to (Git) Full Project Applications

• The status of this application will be put to "postponed" and by following the instructions in the above link, you will be able to reopen it.
• Or if your application has been "needs work" for more than 5 weeks, your application will be marked as "closed (won't fix)". You can still reopen it, by reading the instructions above.

As per previous comment, I am setting this issue to won't fix.

Since new users can now create full projects, applications have a different purpose and they are handled on a different issue queue. See Apply for permission to opt into security advisory coverage for more information.