D7.x: Update PHPMailer to v6.0

I'm debating whether I should try to port this module to PHPMailer 5.2. It might make things better for a 7.x-2.x version. Then again, I've used it just fine for a long time, so I wonder if things are actually buggy enough to fix.

Updating to a newer PHPMailer 5.2 is on my roadmap, but if you want to lead it, I'd glad to help you out!

The "SMTP Authentication Support" module 7.x-1.7 is still using PHPMailer 5.1. Since 9th December 2016, the developer of PHPMailer said, that v5.2.17 will be the last feature release of 5.2 - it will only receive security fixes in future. Instead you should use PHPMailer 6.0.

What are the reasons, that it wasn't updated with the last smtp module version? Do you plan to update it and if yes: When?

Between PHPMailer 5.1 and 5.2.24 were many security/vulnerability fixes, improvements and new features added. Here are just some changes, why I would like to update PHPMailer:

• PHPMailer 5.1 is already very old (October 20, 2009) - Yeah, old software doesn't mean, that it's bad. It also can be better than current software. :)
• DKIM support & signing was improved
• Performance improvements
• Closed several bugs
• New methods/features like GetSentMIMEMessage()
• Emoji in test content
• Minor coding standards cleanup in SMTP class
• More RFC compliant
• Fix bug in CRAM-MD5 AUTH
• Allow custom Mailer types
• Removed html2text converter class by new mechanism for injecting html to text converters
• Store and report SMTP errors more consistently
• More efficient word wrapping
• Amended default values for WordWrap to match RFC
• Add support for S/MIME signing with additional CA certificate
• Minor code cleanup for robustness
• Allow addresses with IDN (Internationalized Domain Name) in PHP 5.3+
• Improve Windows compatiblity
• PHP 7.1 support

this issue is created in 2014, but now still pending.

from the read me
"This module no longer uses the PHPMailer package as an external library"

so maybe close this issue?

Why close? The issue is still valid!
This module includes PHPMailer v5.1 with all versions: 7.x-1.x, 7.x-2.x and 8.x-1.x. The request is to update it.

Considering PHPMailer is already included, only needs updating, it is.not a feature request, changed it into a support request.

EDIT: For those that do understand the issue, have a look at https://www.drupal.org/node/2711559
Note that it is for D8, not D7.

Descriptions of the Priority and Status values can be found in the Issue queue handbook. Further useful informations about issue handling in general and also about which category should be set can be found under Bug Squad: "How to work the issue queue". A quote from the category support: support issues should be general questions about how to perform a specific task.

So if somebody still asks for a library implementation which is not planned yet or has been removed before, it is rather a feature request than a support request. But, TBH, if it has been removed before, the chance is small that it will be implemented again unless the feature request includes good reaons to implement it again.

+1
Request to update to the latest version of PHPMAILER.

Thanks for the module. Very useful.

start point. for me worked, but maybe need some rework and tests.
After applied patch you need install phpmailer by composer. run in folder with module:
composer install

So many thanks for your fast answer and this patch.
I try to apply and test it in few days.

It works for me.
Thanks.

You definitely shouldn't use that patch in a production site. It disables SSL Certificate validation so you have no idea where you're connecting.

Alternatively, you should remove this block from the patch for smtp.mail.inc.

@@ -510,10 +515,24 @@ class SmtpMailSystem implements MailSystemInterface {
     switch (variable_get('smtp_protocol', 'standard')) {
       case 'ssl':
         $mailer->SMTPSecure = 'ssl';
+        $mailer->SMTPOptions = array(
+          'ssl' => array(
+            'verify_peer' => false,
+            'verify_peer_name' => false,
+            'allow_self_signed' => true
+          )
+        );
         break;
 
       case 'tls':
         $mailer->SMTPSecure = 'tls';
+        $mailer->SMTPOptions = array(
+          'ssl' => array(
+            'verify_peer' => false,
+            'verify_peer_name' => false,
+            'allow_self_signed' => true
+          )
+        );
         break;
 
       default:

Re-rolled @cosolum #10 patch, added some legitimate library requirements, warning messages and removed the need for composer (although everyone should be using composer higher up the chain). Included suggestion from #13 @twistednexus.

Don't expect it to work with PHP5.6, but will take a couple stabs at testing ^php7 and see what happens.

Added libraries module requirement.

Test failing on previous patches because it requires PHPMailer. Added catch-all searching logic to load PHPMailer even if it needs to download it using libraries module. Added admin options to control the sent SSL options in #13 (and so that testing can disable them). Added additional notifications when it is using the libraries include_check_path() function, recommending the user install the library "correctly".

Obviously up to the user where they want to put their libraries, but having them hanging out in public files/include/ writable by apache doesn't sound like a good thing, especially since all emails will go through it.

Forgot to add test_dependencies[] = libraries in smtp.info

Testing is failing as though smtp module isn't enabled. I just moved libraries above smtp in setUp() to see if maybe libraries needs to be earlier in the array for setUp(). Other than that, I'm out of ideas. The patch does work well though, and simpletest on my local runs without a hitch. Would appreciate anybody who's familiar with CI to suggest a fix.

  /**
   * {@inheritdoc}
   */
  function setUp(array $modules = array()) {
    // Requirements.
    $modules[] = 'libraries';
    $modules[] = 'smtp';

If a module is a contributed module on drupal.org, has dependencies on other modules, and wishes to test changes to those dependencies using drupalci as part of development, they must have a composer.json that expresses those drupal module dependencies (drupalci can only detect changes to dependencies in patches within composer.json, not in .info/info.yml files)

Any new .info test_dependencies aren't going to be read by testbot unless they are committed, and the alternative of adding require-dev dependencies in a composer.json file makes simpletest exit as ERROR: No valid tests were specified. which looks like how #10 tested as well.

I put more time into fleshing out the library requirements, status reporting, and giving smtp the option to utilize the Include module when available to automatically download PHPMailer. For the DrupalCI testing, I built in functionality specifically for simpletests to download the PHPMailer library from github using ::cough:: file_get_contents().

If we could get the include module added to smtp.info as a test_dependency, or if I can get the composer.json method working, I'll remove that code and rely on Include module to download the library. I'm against adding legitimate dependencies, but doesn't hurt to utilize great utility modules like Libraries and Include when they are present.

 
Since previous patch #21..

Fixed:

- libraries module replacement of phpmailer library search was broken
- some drupal coding standards (in this patch only, not the entire project)

Changed:

- moved the drupalci testbot-related library download code to smtp_tests.module
- now uses drupal_http_request instead of file_get_contents of remote files
- sanity check for returned 'x-github-request-id' header on downloads

Added:

- minimum version setting in smtp.install
- phpmailer version minimum requirement handling, reporting

Note:

- We look for 'PHPMailer' cased library folder, but some other smtp-related modules look for 'phpmailer' (lowercase). I think it's a good idea to continue looking for 'PHPMailer' instead, as it's doubtful other modules are using ^6.0 PHPMailer and this way they might be able to play nice.

Future: @wundo

- Add test_dependencies = include; to smtp.info file so that we don't have to rely on a custom function that hastily does what the include module would do for us in order for DrupalCI testbot (and I assume others) to test successfully. I'll defer to maintainer's judgement.

:: beg ::

This patch (#22) plays nice with a the other patches I'm using:

"2694121: Documentation clean up and PHP 7 compatibility with return values": "https://www.drupal.org/files/issues/2694121-19.patch",
"1813164: Mime Mail -> SMTP with embedded images": "https://www.drupal.org/files/issues/2019-05-24/1813164-auto-embed-local-images-15.patch"

 
Since previous patch #22..

Fixed:

- smtp.admin.inc was missing the new 'Advanced SSL settings' section that allow user setting SSL options (@see #13)

Todo:

- If this gets any traction, I'll remove the chaff smtp.transport.inc and smtp.phpmailer.inc

Up to @wundo what they'd like to do with this, as it addresses a few issues that have RTBC patches, namely https://www.drupal.org/project/smtp/issues/2566561. Worth reviewing though IMO.

 
Since patch #24..

Fixed:

- (#22) libraries module replacement of phpmailer library search was [still] broken.. needed a trailing slash.

Applied patch in #26 and it's working for me, thanks @spelcheck!

Tiny update to #26 that adds an extra module include line in smtp_requirements. This means that if the site's ever in a situation where it's doing an install, but smtp.module hasn't been loaded yet, it won't throw a fatal error due to not being able to find the function smtp_phpmailer_available.

I think this needs another pass, because even with the latest code, I'm not even sure it's including PHPMailer from its sites/all/libraries directory. I suspect it's using the old PHPMailer class file, because it finds the PHPMailer class, but can't use any of the newer methods such as addStringEmbeddedImage. Which is highly unfortunate because I'm trying to use it to fix multipart embedding elsewhere...

Update title to make it much clearer this is a D7 change.

See #2711559: Set phpMailer as a external library using composer (and update it to 6.0) for the D8/9 change, which just reverts to including PHPmailer via Composer.

The install file changes in #29 added a duplicate $t = get_t(); line.

I removed it in this patch.

Applied codesniffer fixes.

You fixed a lot of coding standards problems, but this patch still adds 5 new problems. Those should be corrected - no sense in adding new problems.

It would be nice to see an interdiff so we can tell what you changed from the patch in #29 ...

HI,
i haved phpcs error also in this patch.

The errors from the earlier tests were because an external resource was returning an HTTP error, not because of anything I changed. Literally all I did from 29 was remove a duplicate line. View the patch in #29, scroll down to the install file, and see that it called get_t() twice.

I thought I would review this code as that doesn't seem to have happened yet.

I have serious misgivings about the code that downloads the library to the public files folder and then runs it. If not for that it's mostly OK but in its current state I think this poses a security risk.

1. +++ b/smtp.admin.inc
   @@ -330,6 +366,8 @@ function smtp_admin_settings_form_submit($form, &$form_state) {
   + * Function which handles submit.
   

   Not a good intro line. We can see it's a function...

   I would suggest a short form of the next para, just "Submit handler...form." and of then delete that second para.

2. +++ b/smtp.admin.inc
   @@ -341,5 +379,6 @@ function smtp_admin_settings_submit_post_system_settings($form, &$form_state) {
   +    drupal_set_message(t('A test e-mail has been sent to @email. You may want to <a href="!check">check the logs</a> for any error messages.', array('@email' => $test_address, '!check' => url('admin/reports/dblog'))));
   

   As we're changing the line, could we also trim its length (minimally at "array(").

3. +++ b/smtp.install
   @@ -29,17 +48,84 @@ function smtp_requirements($phase) {
   +    switch ($smtp['status']) {
   +      // PHPMailer was found, but doesn't meet the version requirements.
   +      case -1:
   +        $message = '<a href="!module">%module</a> module requires at least version %minversion of %library but version %version was found.  Please <a href="!library">download</a> %library version %minversion or greater and install it under !path.';
   +        watchdog('smtp', $message, $replacements, WATCHDOG_ERROR);
   +        drupal_set_message($t($message, $replacements), 'error');
   +        smtp_set_system_status($requirements, $t($message, $replacements), $smtp['version'], REQUIREMENT_ERROR);
   +        break;
   +
   +      // No PHPMailer in libraries location or public files/include location.
   +      case 0:
   +        $message = '%library has not been installed. Please <a href="!library">download</a> and install it under !path.';
   +        watchdog('smtp', $message, $replacements, WATCHDOG_ERROR);
   +        drupal_set_message($t($message, $replacements), 'error');
   +        smtp_set_system_status($requirements, $t($message, $replacements), $smtp['version'], REQUIREMENT_ERROR);
   +        break;
   +
   +      // Found PHPMailer in libraries location.
   +      case 1:
   +        $message = 'The <a href="!library">%library</a> class library is installed and meets or exceeds the minimum required version required by <a href="!module">%module</a> module.';
   +        smtp_set_system_status($requirements, $t($message, $replacements), $smtp['version'], REQUIREMENT_OK);
   +        break;
   +
   +      // Found PHPMailer library in files/include crutch location only.
   +      case 2:
   +        $message = 'The <a href="!library">%library</a> library required by the <a href="!module">%module</a> module is located in files/includes folder, but consider installing it under !path as a more secure option.';
   +        drupal_set_message(t($message, $replacements), 'notice');
   +        smtp_set_system_status($requirements, $t($message, $replacements), $smtp['version'], REQUIREMENT_WARNING);
   +        break;
   +
   +      // PHPMailer library class was loaded some other way.
   +      case 3:
   +        $message = 'The <a href="!library">%library</a> class library is installed and is version %version.';
   +        smtp_set_system_status($requirements, $t($message, $replacements), $smtp['version'], REQUIREMENT_OK);
   +        break;
   +
   +      default:
   +        // @todo $smtp['status'] should only return specific codes.
   +        // If it returns something strange we might want to handle it.
   

   Please word-wrap this code.

4. +++ b/smtp.install
   @@ -29,17 +48,84 @@ function smtp_requirements($phase) {
   +    }
   

   It doesn't cost much to put in a "something weird happened" message. Good programming is about handling edge cases.

5. +++ b/smtp.install
   @@ -29,17 +48,84 @@ function smtp_requirements($phase) {
   + *   An array of requirements, referenced from hook_requirements().
   

   I think the doc should state here that (only) the PHPMailer key will be written.

6. +++ b/smtp.install
   @@ -29,17 +48,84 @@ function smtp_requirements($phase) {
   + *   version number, it will usually be set as 'Unknown'.
   

   Why 'usually'?

   This is the definition of this function's API. If you don't know, nobody does...

7. +++ b/smtp.module
   @@ -164,3 +163,190 @@ function smtp_failure_queue_runner($message) {
   +      'path' => DRUPAL_ROOT . '/sites/all/libraries/PHPMailer/',
   

   What about:

   sites/default/libraries

   and possibly:

   ../libraries
   - (making that one up, is it a thing)?

8. +++ b/smtp.module
   @@ -164,3 +163,190 @@ function smtp_failure_queue_runner($message) {
   +      'path' => DRUPAL_ROOT . '/sites/default/files/include/PHPMailer/',
   

   As commented earlier, not happy about this; at the very least, there needs to be protections in place against using this as an attack vector.

   Please reconsider including this.

9. +++ b/smtp.module
   @@ -164,3 +163,190 @@ function smtp_failure_queue_runner($message) {
   +  if (class_exists($search['class'])) {
   +    return smtp_phpmailer_initialize($search, $install_types, FALSE);
   +  }
   +
   +  // PHPMailer is not yet loaded, find it and load it. Attempt to
   +  // download it using include module if available.
   +  return smtp_phpmailer_initialize($search, $install_types, TRUE);
   

   This could be done as:

   $exists = class_exists();
   return smtp_*ize(..., !$exists);

   Is there a good reason to use the long form?

10. +++ b/smtp.module
    @@ -164,3 +163,190 @@ function smtp_failure_queue_runner($message) {
    +      if (module_exists('include')) {
    

    It would be better if this check was done at the strt of the function.

11. +++ b/tests/smtp.unit.test
    @@ -87,14 +95,22 @@ class SmtpUnitTest extends DrupalWebTestCase {
    +    // , 'Email "to" address is correct.');
    

    Can we lose the initial commas please.

    In fact, not really sure these commenta add anything at all. Better remove them.

I really don't like this patch either, for the same reasons stated above by @rivimey

This is D7, and we can require manual installation of external libraries like PHPMailer, or we can use the Libraries module to facilitate this, or we can require composer_manager, or we can bundle our own forked copy of PHPMailer (which is what we currently do in D7), but rolling our own automatic download should not be considered.

Since this issue is about replacing our forked/bundled version of PHPMailer with the current version, I would stick either with bundling the current version as we do now OR modifying the installation instructions to describe how to manually download the current version and where to put it.

But since D7 is mature and nearing its end-of-life, and because there are 100,000 D7 sites using this module, any change here needs a hook_update_N() and a hook_requirements() to ensure that these legacy D7 sites get update properly, regardless of which download method we choose.

And because this is so potentially disruptive, there ought to be a REALLY good reason to change the module, because this change is CERTAIN to cause problems with many existing sites. #4 provides some good reasons, and in general it would be much better and more supportable if we used the current version of PHPMailer rather than our own forked version. But the transition, for existing sites, concerns me, and deserves more attention here.

Perhaps a good argument for keeping it a fork is so that users don't have to worry about which version of the library they are supposed to use.

Also, the issue says update to 6.0, and #4 says to use 5.2. Which are we actually talking about?

@solideogloria I believe we started asking for 5.2 when that was still reasonably current but as it's now obsolete itself, the request changed to 6.0. I would prefer the code was not 'forked' in the sense of setting up a new module name: that would be a retrograde step I feel.

@TR 100% agree with you.

I think the best option re: the upgrade issue would be to bump the major version to 2.x, which has always been a pretty strong hint that something big changed. In this case, the big thing changing would be phpmailer itself, and how to install it: I believe settings can be updated well enough.

What I would suggest re: download would be that a drush command was created that would download phpmailer and put it in (exactly one) directory that the libraries module would see it. Then the smtp_requirements() code could detect its absence and instruct users to put the library there by hand or run the drush command to do it automatically. There are several other contrib modules in D7 which do this already, so I would search them out and copy their mistakes rather than make our own brand new ones.

Given the above is a fair bit of work and how many people are using the module anyway, I'm not sure if this is worth the effort. However if someone else does think it is worthwhile, I think the above would be an acceptable solution.

The big thing I am against is anything that enables the website to write or update its own code without a LOT of security attention being paid to it, and this patch doesn't qualify. One point to note is that when composer downloads code it checks the checksum of the package, rather than just assuming all was well.

I think the best option re: the upgrade issue would be to bump the major version to 2.x, which has always been a pretty strong hint that something big changed.

You're right, that's probably the best option, but it would have to be 7.x-3.x since there's already an abandoned 7.x-2.x branch, unless we wanted to re-purpose that 7.x-2.x branch. But I remember reading something in a closed issue about there were sites still using 7.x-2.x ...

Regardless, if we're taking about branching and upgrading the PHPMailer version, we really need active participation here from a maintainer. I personally wouldn't want to spend any effort to make this happen without the maintainer being 100% behind it.

There are still sites on 2.x

The module is currently bundling an unsupported version of the library, which generates a status report error and may be insecure.

As Drupal 7 is nearing EOL, I'm finding it hard to justify moving to PHPMailer 6.0. However, it should be using the latest version the 5.2 branch.

Marking closed for now. If you want to use PHPMailer 6, feel free to take patches from this issue or roll your own with composer manager. But I don't think we should break existing D7 sites with updating a major version of PHPMailer.

@japerry Please do make this so.... while standard EOL is only a year-ish away, there is also commercial support.

Moving to 5.2 is I think no easier than to 6.0, because the version of the code included in the module is an ancient hacked around version of, probably, PHPMailer 3, with some (but probably not all) security fixes applied. Getting onto 6.0 could well prevent an SMTP module CVE.

If this patch needs simplification for the purposes of risk reduction, then so be it, but I think it would be very preferable, even at this stage, to get onto a supported version of PHPMailer, and do so soon.

To be clear, I am asking for phpmailer to be included verbatim in the module code, if that is permissible, as the safe alternative to getting involved with composer on D7.

I know this will probably never be committed per @japerry's comment above, but here is a new patch that is much simplified. It uses the 'libraries' module to load from a directory named 'phpmailer' (under sites/all/libraries or any of the other locations that 'libraries' will check), and has a hook_requirements() to ensure that PHPMailer 6 is used. The reason the patch is so big is that it's removing the PHPMailer code that was originally included in the module - otherwise, the changes are pretty minimal, as the API for PHPMailer 6.x is very similar to 5.x.

Hopefully, this will be useful to someone!