Swift Mailer module

Consider running your entire module through Coder Review.

README.txt

Needs to be properly word-wrapped at space characters, rather than blindly inserting a linefeed in the middle of words.

swift_mailer.info

It might be better to name your module swiftmailer rather than swift_mailer so as to avoid overlapping namespaces with a future module named swift.

Change package = E-mail to package = Mail to be consistent with other mail-sending modules.

Class include files should be listed in a files[] section. See http://drupal.org/node/542202

Module configuration path should be listed in a configuration line.

Needs a trailing linefeed

swift_mailer.install

If you want to create a new variable name, it should be prefixed by your module name. So mail_system_old should be swift_mailer_mail_system_old.

For proper handling of the mail_system variable, see http://drupal.org/node/900794#comment-4160574

See http://api.drupal.org/api/drupal/modules--system--system.api.php/function/hook_uninstall/7#comment-15344

swift_mailer.module

Class include files should be autoloaded. See http://drupal.org/node/542202

Please explain the reasoning behind the plethora of constant declarations.

includes/classes/SWIFTMailSystem.inc

The format() function incorrectly assumes that $message['body'] always exists and always contains an array.

All message formatting should be performed by the format() function. Your code appears to add attachments, etc. in the mail() function, which should only be responsible for delivery of the formatted message.

The "\r\n" string should be replaced by variable_get('mail_line_endings', MAIL_LINE_ENDINGS)

Consider integrating with Libraries API rather than reinventing the wheel.

Consider spelling $supressable as $suppressable.

Consider correcting All attachments needs to to All attachments need to.

Consider combining separate but closely-related configuration variables into a single array variable.

Dynamic values in watchdog() errors should be added via the $variables parameter rather than being concatenated into the $message parameter.

includes/helpers/conversion.inc

Using a constant for your date pattern prevents the possibility of translation.

Consider spelling parametrized as parameterized in function names, as you already do in their respective comment blocks.

Remove blank lines between subsequent @param declarations. See http://drupal.org/node/1354#functions

The @param declaration should match the function signature. (e.g., @param Swift_Message $message should be followed by function swift_mailer_foo(Swift_Message $message,.)

Consider spelling excact as exact.

See also (@see) sections should generally be at the documentation header.

Documentation headers for swift_mailer_is_id_header() and swift_mailer_is_path_header() are confusingly similar, and don't seem to match the code.

Consider wrapping all the is_foo_header() and add_foo_header() functions by a single add_header() function.

includes/helpers/utilities.inc

Consider spelling prodived as provided.

Again, consider integrating with Libraries API rather than reinventing the wheel.

includes/pages/swift_mailer_page_admin_config_swift_mailer_default.inc

The usual practice would be to name this file swift_mailer.admin.inc.

Missing @file header. See http://drupal.org/node/1354

Missing or incomplete function documentation headers.

Consider using shorter function names, for readability. It is enough to prefix the function name with your module name; you don't have to include the path as well.

I think that's enough for now.

Subscribing.

Subbing. @crell discussed the need for a standard mail library at #dctc 2011. This might be just the ticket!

subscribing.

Subbing. @sbrattla: you saved my day, keep up the good work. Hope to see a stable version soon.

* git release branch is missing, see http://drupal.org/node/1015226
* CHANGELOG.TXT should be CHANGELOG.txt
* same for README.txt
* info file contains wrong line endings ('\r'), you must use unix line endings, see http://drupal.org/node/318#indenting
* info file: only files should be listed there that contain classes or interfaces
* install file: hook doc blocks on functions are missing
* install file, module file: indentation should be 2 spaces only.
* module file misses @file doc block, see http://drupal.org/node/1354#files
* swiftmailer_menu(): doc block format is not correct, see http://drupal.org/node/1354#hookimpl
* you have committed your netbeans project settings folder. Remove it from the repository.

No, you are not doing anything wrong. We are currently running short on reviewers, so it can take up to 4-5 weeks until you get another review (I'm working the issue queue backwards from the oldest updated date).

You can speed up the process by reviewing other project applications, so we can get back to yours sooner. You can also get in touch with other community members that might be interested in your project and ask them for a review. http://drupal.org/community

--------------
MANUAL REVIEW
--------------

1) When I first installed the module, and have the swiftmailer in sites/all/libraries/ ....it still requires a path to the library to be specified under /admin/config/swiftmailer/ . When I enter "sites/all/libraries/swiftmailer" into the field, the error goes away. Can you default this in detecting this path automatically and if it it doesn't find it there, then display the error?

2) Should we make the password field under the Transport tab a "password" type text field? I know in some cases clients may not want their password displayed in plain text to see for those who have access to that admin area.

3) On the Transport tab, when the settings are saved, can you have the system validate the information to make sure it is correct? Maybe send a test email? This would help tremendously in debugging why things aren't working, or when the settings have changed.

4) I tried to send out a test email, but I received this error: Parse error: syntax error, unexpected T_STATIC, expecting T_STRING or T_VARIABLE or '$' in /Users/chrisroane/Sites/acquia-drupal/sites/default/modules/swiftmailer/includes/classes/SWIFTMailSystem.inc on line 200

------------
AUTO REVIEW
------------
It appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.
Review of the master branch:

• Remove "version" from the info file, it will be added by drupal.org packaging automatically.
• ./includes/helpers/conversion.inc: all functions should be prefixed with your module/theme name to avoid name clashes. See http://drupal.org/node/318#naming
  

  function swift_mailer_add_date_header(Swift_Message $message, $key, $value) {
  

• Bad line endings were found, always use unix style terminators. See http://drupal.org/coding-standards#indenting
  

  ./swiftmailer.info:                               ASCII English text, with CRLF line terminators
  swiftmailer.info
  

• Run coder to check your style, some issues were found (please check the Drupal coding standards). See attachment.
• Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards). See attachment.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. Get a review bonus and we will come back to your application sooner.

There are quite a few other issues with your code, that doesn't meet the drupal standards. Here is a complete list: http://ventral.org/pareview/httpgitdrupalorgsandboxsbrattla1163884git

I suggest using the automatic review tool to resolve all of the issues.

Forgot to change the status to needs work.

1. You really need to fix the git organization issues before this module is published.

I'm new to using git as well. But I think you need to create a new branch called: 7.x-1.x. Merge the latest changes into that branch and delete all of the files in the master branch. Then delete the 7.x-1.0-alpha branch (my understanding is that the system will automatically handle the version based on what is in the .info file).

2. When I have the swiftmailer plugin in /sites/all/libraries, it still displays a form error when no value is in the field on /admin/config/swiftmailer . But maybe this is because the master branch doesn't have the latest files? I grabbed the latest files from the master branch and the password field is still a plain text field.

3. That's probably right. Maybe having a "Test SMTP" area that allows you to enter a To email address and send one email when clicked? This isn't an absolute need at this point, though. So I created a new issue for this feature: #1432972: Test SMTP Settings.

4. I took a look at the README.txt file and it has a lot of useful information. However, it seems to be overwhelming with how much is in that file. However, I don't think this is something that has to happen right away. Here is the new feature request I created: #1432986: Simplify README.txt.

As soon as you can get the git repository organized and setup properly, I will go ahead and test the code and take a closer look at the module. I don't think it is worth working on anything else until you can get this ironed out. I know you have been working on this module for a while (on and off), but I know that this a priority issue that needs to be resolved as soon as possible.

I'm very interested in helping you maintain this project. But for the time being I'm also trying to get a module through the approval process: #1425392: Splashify. Once I get that through, I would love to become a co-maintainer and help you manage the module. I could see myself using this module all the time.

Just to clarify here, you can't actually delete the master branch. You should be able to delete the files that are in it....and that is what I would do.

I'll take a closer look at the code either tomorrow or Friday.

I grabbed the correct branch.

1. I verified the Test tab works.

2. You should be able to delete the files under the Master branch. From my understanding, you can't delete that branch in git. You will want to do this, because it makes it confusing which branch contains the latest code.

3. I just learned this with my module, but you should use the Libraries module (version 2.0: http://drupal.org/project/libraries ) to include the swiftmailer library. This should make it easier for to get working.

4. It is my understanding, that you should not include html when sending strings to the t() function. The Drupal 6 documentation describes this much better than the Drupal 7 version: http://api.drupal.org/api/drupal/includes%21common.inc/function/t/6 . I found many of these cases. Example: Line 87 in swiftmailer_admin_transport.inc ... Also lines: 91, 142, 146, 176, etc...This is in pretty much every other PHP file I looked at.

5. I did test sending out the default contact form, and I received the email.

They requested I use the Libraries module for the module I am trying to get approved, which is why I mentioned it. The good thing about it is that it automatically will detect the library if it is in any of the "site" directories, without having to enter any path in an admin. It also looks like a lot of other modules use it. But I don't know if it is something they will require or not in order to get approved.

EDIT: never mind: I thought you weren't setting the Return Path, but I tested with my Bounce module and it's clear that Swift Mailer is setting the return path. I'll go on to review while I'm here.

1) Trivial bug in swiftmailer_admin_default.inc

  $form['description'] = array(
    '#markup' => '<p>' . t('The Swift Mailer module replaces the default mail system that is shipped
      with Drupal and installs itself as the primary mail system. This allows
      you to choose how e-mails should be sent. Furthermore, with the Swift
      Mailer module you can easily add attachments to e-mails. To read more
      about how this module works, please have a look at the !documentation.') . '</p>',
  );

Missing a definition for !documention.

2) Another trivial bug in swiftmailer_admin_transport.inc:

    switch ($form_state['values']['transport']['type']) {
      case SWIFTMAILER_TRANSPORT_SMTP:
        variable_set(SWIFTMAILER_VARIABLE_SMTP_HOST, $form_state['values']['transport']['configuration'][SWIFTMAILER_TRANSPORT_SMTP]['server']);
        variable_set(SWIFTMAILER_VARIABLE_SMTP_PORT, $form_state['values']['transport']['configuration'][SWIFTMAILER_TRANSPORT_SMTP]['port']);
        variable_set(SWIFTMAILER_VARIABLE_SMTP_ENCRYPTION, $form_state['values']['transport']['configuration'][SWIFTMAILER_TRANSPORT_SMTP]['encryption']);
        variable_set(SWIFTMAILER_VARIABLE_SMTP_USERNAME, $form_state['values']['transport']['configuration'][SWIFTMAILER_TRANSPORT_SMTP]['username']);
        variable_set(SWIFTMAILER_VARIABLE_SMTP_PASSWORD, $form_state['values']['transport']['configuration'][SWIFTMAILER_TRANSPORT_SMTP]['password']);
        drupal_set_message(t('Drupal have been configured to send all e-mails using the SMTP transport type.'), 'status');
        break;
      case SWIFTMAILER_TRANSPORT_SENDMAIL:
        variable_set(SWIFTMAILER_VARIABLE_SENDMAIL_PATH, $form_state['values']['transport']['configuration'][SWIFTMAILER_TRANSPORT_SENDMAIL]['path']);
        variable_set(SWIFTMAILER_VARIABLE_SENDMAIL_MODE, $form_state['values']['transport']['configuration'][SWIFTMAILER_TRANSPORT_SENDMAIL]['mode']);
        drupal_set_message(t('Drupal have been configured to send all e-mails using the Sendmail transport type.'), 'status');
        break;
      case SWIFTMAILER_TRANSPORT_NATIVE:
        drupal_set_message(t('Drupal have been configured to send all e-mails using the PHP transport type.'), 'status');
        break;
    }

Replace 'Drupal have been' with 'Drupal has been'.

3) You don't have to do this:

drupal_set_message(t('An e-mail has been sent to !email.', array('!email' =>  check_plain($form_state['values']['test']['recipient']))));

when you could do this:

drupal_set_message(t('An e-mail has been sent to @email.', array('@email' => $form_state['values']['test']['recipient'])));

4) In general, you should probably check all the places you have used !token in strings and make sure that, yes, you really want to pass strings through unchecked. It's generally better security practice, I think, to default to using @token - which will check_plain() - and only then go back and see what has to be let through.

5) Earlier commenters are right in noting that you'll have to be working in a branch called 7.x-1.x - see how I have things set up in the Bounce module you were reviewing. Actual versioning (e.g. 7.x-1.0-alpha1) is done by using tags, not branches.

1) In the install file: shouldn't the line below be variable_del(SWIFTMAILER_VARIABLE_MAIL_SYSTEM_OLD)?

  // Delete the old mail configuration.
  variable_del('mail_system_old');

2) And in swiftmailer_uninstall(), why not use the variable names you've defined rather than strings? Not a big deal; it isn't broken.

3) swiftmailer_admin_test.inc, line 69:

 drupal_set_message(t('An e-mail has been sent to @email.', array('!email' =>  $form_state['values']['test']['recipient'])));

That !email should be @email.

----

Aside from that, I'm using Swift Mailer in testing some of my mail-related code that needs an SMTP server involved in the process - this module works and does what it says on the label. It is a favorable alternative to SMTP Authentication Support. Given the rest of this thread, I think it should be set to Reviewed & Tested by the Community.

Setting to BRTC based on my review and earlier reviewers' work.

Review of the 7.x-1.x branch:

• Bad line endings were found, always use unix style terminators. See http://drupal.org/coding-standards#indenting
  

  ./swiftmailer.info:                               ASCII English text, with CRLF line terminators
  swiftmailer.info
  

• Run coder to check your style, some issues were found (please check the Drupal coding standards). See attachment.
• Drupal Code Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. Get a review bonus and we will come back to your application sooner.

manual review:

• "7.x-1.0-alpha1" follows the git tag name pattern and should not be a branch. Remove that branch.
• "'#default_value' => check_plain($path),": no need to run check_plain() on #default_value, the Form API will sanitize that for you. Double escaping is bad, please read http://drupal.org/node/28984 again.
• swiftmailer_menu(): I think the description of the last menu item is wrong.
• "DEFINE('SWIFTMAILER_ADMINISTER', 'swiftmailer_administer');": "DEFINE" should be "define" (all lower case).
• "module_load_include('inc', 'swiftmailer', '/includes/helpers/utilities');": Are you sure you need to globally load that file on every single page request? Include it when you actually need it. Also, no need to use module_load_inlcude as you are including files from your own module which you already know where they are. Use something like require_once dirname(__FILE__) . '/mymodule.inc';.
• "watchdog('swiftmailer', 'An attempt to send an e-mail message failed: !exception_message'": the exception message needs to be properly sanitized with an "@" placeholder.
• swiftmailer_get_random(): why do you need that function? for $length = 10 you could just call rand(0, 9999999999), right?
• swiftmailer_admin_default_page(): if you just want to display a form you can use drupal_get_form() as page callback directly in hook_menu() and remove that function.
• swiftmailer_admin_default_form_submit(): you don't need that function if you use system_settings_form() in swiftmailer_admin_default_form() and rename the form key for the library to the variable name.

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Everything passes Coder review. Manual review:

• swiftmailer.info:4 - Remove "mail" package per http://groups.drupal.org/node/97054
• swiftmailer.install:13 - This module appears to clobber all other mail systems in hook_install(). http://drupal.org/node/900794#comment-4160574 explains that this may not be such a great idea if that array is already populated. http://drupal.org/project/mailsystem provides an API that you might consider using for doing this more cleanly.
• includes/classes/SWITFMailSystem.inc:267 - New control structures should be on a new line.
• includes/helpers/conversion.inc:7 - define() should be lowercase.

Additional suggestions:

• swiftmailer.module:66 - The constant here is non-standard. I'd stick to the string literals for permissions.
• swiftmailer.module:27 - Same thing here - I'd stick with the string literals for variable_g|set keys. The pattern isn't used elsewhere, and not much is gained by making them constants.
• includes/pages/swiftmailer_admin_default.inc:48 - The constant SWIFTMAILER_VARIABLE_PATH_DEFAULT forces the dev to look in another file for the default path. Should probably be a string literal.
• includes/helpers/conversion.inc:81 - Could be shortened to return preg_match(…);
• includes/helpers/conversion.inc:328 - Could be shortened to return preg_match(…);

EDIT: Split my review into definite issues versus stylistic/functional suggestions. Hope this helps!

Hi sbrattla!

1. No need to define it explicitly; it's the default package where none is defined. :]
2. I'm curious to see the results!
3. Apologies for the ambiguity; see http://drupal.org/coding-standards#controlstruct. Rather than:

if (condition1 || condition2) {
  // foo
} elseif (condition3 && condition4) {
  // bar
} else {
  // baz
}

The standard would be:

if (condition1 || condition2) {
  // foo
} 
elseif (condition3 && condition4) {
  // bar
} 
else {
  // baz
}

with each component of the control structure (if, elseif, else) starting on a new line. The same applies for try-catch and the like.

4. Fantastic.

I should clarify again that the remainder were merely suggestions, and would not necessarily block your application from passing review, but I'd happily weigh in below:

5. Regarding the use of constants, many of them I completely understand and agree with, though I've never seen them used this way in variable_s|get and the like. It is a minor matter of style, though if you prefer to keep them, I for one don't see why it should block the application.
6. I see your point regarding return types, and agree that

if preg_match(...) ? TRUE : FALSE;

may be stronger. Generally speaking, it is best to use the minimal amount of code to achieve a task, provided it is still readable, performant and self-documenting, which is why I recommended it. Again, however, leaving it as is would not block your application from moving on to the next step in the process.

EDIT: I would say, however, the configuration setting being used as the value in that variable_set should still be a string, as it is a configuration value versus a magic constant. While I'm still not sure what the value is of having constants that are identical names to keys in the variable table, perhaps someone else can speak to best practice here.

Happy to help!

if preg_match(...) ? TRUE : FALSE;

As the above is not valid PHP code, presumably you mean one of the following:

• if (preg_match(...) ? TRUE : FALSE) {
   ...
  

• return (preg_match(...) ? TRUE : FALSE);
  

• if (preg_match(...)) {
    return TRUE;
  }
  return FALSE;
  

Thanks for your reviews, just make sure that you pick applications that did not get a review in a long time. When finishing your review comment also set the issue status either to "needs work" (you found some problems with the project) or "reviewed & tested by the community" (you found no flaws).

Review of the 7.x-1.x branch:

• Drupal Code Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. Get a review bonus and we will come back to your application sooner.

manual review:

1. swiftmailer_admin_default_form(): no need to use module_load_inlcude as you are including files from your own module which you already know where they are. Use something like require_once dirname(__FILE__) . '/mymodule.inc';.
2. Also elsewhere.
3. "'#default_value' => check_plain($user->mail),": do not use check_plain() on #default_value form API keys, the form API will automatically sanitize that for you. See http://drupal.org/node/28984 Also elsewhere in your code.

Although you should definitively fix those issues I think this is RTBC. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

EDIT: Miscommunication (re: #54, thanks!); please disregard.

@davidwatson: comment #52 was intended for sbrattla.

Hi sbrattla,

Thanks for your contribution and welcome to the community of project contributors on drupal.org.

I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects, at your discretion.

Now that you've experienced the full review process, please consider reviewing other projects that are still awaiting review. Anyone can help with reviews, following the guidelines.