jerome.megel [jeromemegel]

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. See http://drupal.org/coding-standards to understand how a module should be written. In particular, see hoe Drupal variables, global variables, constants, and functions defined from the module should be named; how the code should be formatted.

2. function email_tester_init() {
     jquery_ui_add(array('ui.core', 'ui.dialog', 'ui.draggable', 'ui.resizable', 'ui.accordion'));
     drupal_add_js(drupal_get_path('module', 'email_tester') .'/js/email_tester.js', 'module', 'header', TRUE);
     drupal_add_css(drupal_get_path('module', 'email_tester') .'/css/ui.all.css');
     
     // Only define our mail wrapper if the email_tester module is the current mail wrapper.
     if (variable_get('smtp_library', '') == drupal_get_filename('module', 'email_tester')) {
       /**
        * Save the mails sent out in $_SESSION instead of mailing.
        * $_SESSION['emails_to_disp'] will be displayed in hook_footer()
        */
       function drupal_mail_wrapper($message) {
         $reroute_email = variable_get('reroute_email', NULL);
         if($reroute_email) {
           $to = $reroute_email;
         }
         else {
           $to = $_SESSION['emails_to_disp'][$message['id']]['to'] ?
             $_SESSION['emails_to_disp'][$message['id']]['to'] .= ' ,  '.$message['to'] :
             $message['to'];
         }
         
         $_SESSION['emails_to_disp'][$message['id']] = array(
             'to' => $to,
             'from' => $message['from'],
             'language' => $message['language']->name,
             'subject' => $message['subject'],
             'body' => $message['body'],
         );
         return TRUE;
       }
     }
   }
   
   

   PHP doesn't have functions that are local to another function; every function is global.

3. Hook implementation comments should be like the following one:

   /**
    * Implements hook_menu().
    */
   

4. /**
    * Implementation of hook_enable().
    */
   function email_tester_enable() {
     // We save initial configuration to recover it when the module is disabled
     variable_set('backup_smtp_library', variable_get('smtp_library', ''));
     variable_set('smtp_library', drupal_get_filename('module', 'email_tester'));
   }
   
   /**
    * Implementation of hook_disable().
    */
   function email_tester_disable() {
     variable_set('smtp_library', variable_get('backup_smtp_library', ''));
     if(variable_get('devel_old_smtp_library', '') == drupal_get_filename('module', 'email_tester')) {
       variable_set('devel_old_smtp_library', '');
     }
   }
   
   

   Those hooks should be in the installation file.

5. The module doesn't implement hook_uninstall().

6.     case 'devel_admin_settings':
         $form['smtp_library']['#options'][drupal_get_filename('module', 'email_tester')] =
                 t('Display in modal frame').' (email_tester module)';
   
   

   Strings used in the user interface should be translated.

Point #1, and #5 are still valid.

1.       $file_name = drupal_get_filename('module', 'email_tester');
         $text      = t('Display in modal frame') .' (email_tester module)';
   

   Strings used in the user interface should be translated (part of the message is not passed to t()).

2.           <p><strong>'. t('From') .'</strong>:&nbsp;'. $email['from'] .'</p>
             <p><strong>'. t('To') .'</strong>:&nbsp;'. $email['to'] .'</p>
             <p><strong>'. t('Language') .'</strong>:&nbsp;'. $email['language'] .'</p>
             <p><strong>'. t('Subject') .'</strong>:&nbsp;'. $email['subject'] .'</p>
             <p><strong>'. t('Body') .'</strong>:&nbsp;'. $email['body'] .'</p>
   

   When possible, it is better to give more context to the translators; it's normally better to not include HTML tags in the string to translate, but not if that means to split a phrase. In these strings, it's better to use placeholders, like in:

             <p> . t('<strong>From</strong>:&nbsp;@from', array('@from' => $email['from'])  . '</p>
   

   Words can be translated differently, basing on the context; if translators see the context in which a word is placed, they can understand better how to translate it (in example, in Esperanto of is translated in different ways).

3. Drupal variables set by the module should be prefixed by the module name; that doesn't happen with the variable backup_smtp_library, and reroute_email. That is also true for session variables (like $_SESSION['emails_to_disp']).

Hello,

Please add a README.txt. See:
http://drupal.org/node/161085

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.