neilotoole [neilotoole]

Hello, and thank you 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.

neilotoole,

Thanks for sharing your code. Your motivation of the module is clear and there is no module duplication indeed. You have done a good job documenting the code and using coder for code style. Well done! I have reviewed your module and have an number of comments to help you improve the module further and you help you learn to write better drupal code.

elastic_email.install
For localization you should for example use $t = get_t() to retrieve the appropriate localisation function name (t() or st()).
See: http://api.drupal.org/api/function/hook_requirements/6

elastic_email.module
Move the elastic_email_send() and _elastic_email_send() functions to the elastic_email.inc file. Including the in .module increases the memory footprint of *each* pageload while the functions are only used when sending out emails. Of course this change requires a module_load_include() in the admin module, but hey that's just admin.

The .module primarily contains hook implementations. Move these functions to the top of the file (after the defines).

elastic_email.inc
The number of code lines is fairly small of a separate module. As a rule of thumb a module should contain at least 50 lines. With the note above this minimum is met.

elastic_email.admin.inc
elastic_email_settings_form() // Introductory text
Use hook_help() for this explanation. This enables experienced users to turn off the help text.

t()
You break your lines of code at 80 chars. Breaking translatable text by inserting line endings is inconvenient for translators and may cause confusion. Don't use line breaks in translatable text if not required for the content (such as email body text).

Please modify your code and I will review it again.

t() is available when hook_install(), or hook_uninstall() are invoked. See the code executed from php_install() as example.

@kiamlaluno, the st() is required when the module is installed by an install profile. See the documentation of hook_requirements() for more info.

In hook_requirements() you need to use get_t(), but that is not true for hook_install(), or hook_uninstall(); in fact, php_install() uses t() directly.

I think these will be my last instructions, your doing fine Neil. I payed extra attention to safety this time.
The number of comments is higher, but maybe just because I had more time to spend ;)

Remove the license.txt file before you commit the package to CVS. The packaging script takes care of this.

elastic_email.info
Also, if the job_queue module is enabled, ...
This is not required to understand what the module does. The purpose of this text is a short general description, not a list of all features nor a sales pitch ;)
Remove this part or shorten it substantionally

elastic_email.install

$t('The variable "smtp_library" is set to !lib. This means another module is using another smtp_library. Be aware that this can cause emails not to be sent properly. Please unset the "smtp_library" manually, or disable the module that uses it, then disable and reenable this module.', array('!lib' => $lib)),

Although $lib (variable_get(DRUPAL_SMTP_LIBRARY, '');) is not likely to be unsafe, better be safe than sorry and use @lib or %lib instead.
smtp_library is usually a text string, so there is no harm in using @ or %
(!lib is used multiple times in this module)

return @drupal_get_path('module', 'elastic_email') . '/elastic_email.inc';

Is the at-sign required here?

function elastic_email_disable() {

  variable_set(DRUPAL_SMTP_LIBRARY, '');
}

If the admin, after reading "This means another module is using another smtp_library ..." decides not to use the Elastic Email module and disables it, the existing smtp_library setting (which may have been set by an other module) will get lost.
Only uset the value if it contains the elastic_email_library_path() value.

elastic_email.module

/**
* Implements hook_help().
*/

Missing spaces before the '*'.

elastic_email.admin.inc

t('To enable queueing, please install and enable the !link module.',
          array('!link' => l(t('job_queue'),
            'http://drupal.org/project/job_queue')))

HTML markup within translation strings is allowed, but should be avoided if possible. The exception are embedded links; link titles add a context for translators, so should be kept in the main string.
(See t() documentation) Use t("... Job Queue module ...", array('@job-queue-url' => 'http://drupal.org/project/job_queue'))

t("Unable to send test email because the 'site_email' variable is not set.");

This message is not realy usefull to an admin. Call it the Site's Email address and point to the relevant settings page (admin/settings/site-information)

$data = '<strong class="fail">'
  . t('Failed.')
  . '</strong> '
  . t('Reason:')
  . ' <tt>' . $result['error'] . '</tt>';

At first the unescaped $result['error'] seemed bad to me. But _elastic_email_send() outputs t() translated text in $result['error'].
Provide a line of comment to explain this.

elastic_email.inc

// If there's an error, log it.
watchdog('elastic_email', $result['error']);

The default watchdog severity is 'notice'. 'Error' is more appropriate here.

$result['error'] = t("Unable to send email because some required email parameters are empty.");

Use single quotes where possible. Single quotes have better performance.

$fp = @fopen('https://api.elasticemail.com/mailer/send', 'rb', FALSE, $ctx);

Although only used once, use a define for the URL "magic string".

$result['success']['msg'] = t(
  'Success [!tx_id]; message sent to: !recipients',
  array('!tx_id' => $response, '!recipients' => $to));

Are you sure $respose is safe? Better safe than sorry and use check_plain()

Code style
Check your code for trailing spaces (use Code module with Review set to "minor").

We're almost there ...

 $result['success']['msg'] = t(
  'Success [!tx_id]; message sent to: !recipients',
  array('!tx_id' => $response, '!recipients' => $to));

You have not convinced me that $response and $to are safe (see #9). Use @tx_id and @recipients instead.

[edit] changed the solution above.

neiltoole, well done!

Approved based on Sutharsan's RTBC.

Michelle