Set phpMailer as a external library using composer (and update it to 6.0)

I'm the maintainer of PHPMailer - let me know if you need any help with this. It's crazy that Drupal 8 is still hard-coded to require a buggy, 8-year-old version with security vulnerabilities! You might like to look at the forthcoming 6.0 release which has a namespace and much improved OAuth2 support.

No need to remove composer.json.

Sorry, added

Patch #17 not worked for me because I already had composer.json file and the changes from file did not apply.
I suggest not to add or edit composer.json file but provide instructions in README.TXT
Review updated patch, please.

@estoyausente Thank you for a suggestion.
I like this way. We can put the same instruction in README and d.org project page. It looks solid and pretty straightforward.
By the way I think we can rename README.txt to README.md as well?

Looking forward hearing from @wundo and other maintainers.

Please add a hard requirement in composer.json of phpmailer library using require statement.

I believe adding external composer dependencies is a rather common way of doing it in D8 now, isn't it?

Just adding a new version of the previous patch, which works with the current code.

I tested this patch with the latest dev and PHPMailer 6.0.2 and had to use PHPMailer\PHPMailer\PHPMailer; in src/Plugin/Mail/SMTPMailSystem.php. Otherwise, I would get "Fatal error: Class 'PHPMailer' not found"

I've created a new patch to use phpmailer ~6.0.

#31 tested and works well!

++ Lets get this committed in..

>++ Lets get this committed in..

Seconded.

I needed to debug this today and methods such as $mail->Debugoutput aren't available in v5.x.

I'm not really keen on moving on this direction, I need sometime to think e decide how to approach this.

@wundo: What other info do you need?

The maintainer of phpMailer, @Synchro, replied here 2 years ago to say:

It's crazy that Drupal 8 is still hard-coded to require a buggy, 8-year-old version with security vulnerabilities!

I second that point. It's irresponsible to be redistributing this library like this. It also violates drupal.org's policies for redistributing 3rd party code. This cannot continue.

You have a few options:

A) Leave a buggy version of a library with known exploits in the d.o git repos in violation of our policies. Nope.

B) Remove the library and add some lines in the README asking users to manually download it and install it in the right place. Not ideal, but a fair number of modules that have external dependencies seem to still do this.

C) Remove the library and let composer worry about it automatically. I'll be the first to admit I have no love of composer. It causes all sorts of pain. No need to re-hash that here. But more and more D8 modules are adopting this as the tool to solve this problem. This is exactly what it's for.

D) Remove the library and let Ludwig install it.

C and D can happily co-exist. Frankly, so could B. You could have a composer.json that does the right thing automatically for the people who have already adopted that solution for their D8 sites. More and more site developers are either choosing or forced to do so, anyway. You could also provide a ludwig.json file for the people who are (rightfully or otherwise) afraid or unwilling to adopt composer. And, as a fallback, the INSTALL.txt could explain the 3 options, and give them clear instructions on how to install this into their site manually, if that's really their preference or need.

But I don't see how this issue can be set to "postponed". If you need help with any of these approaches, let us know. But we can't leave the status quo.

Thanks,
-Derek

p.s. The issue summary could stand a refresh. composer_manager is dead. Times have changed considerably for drupal's adoption of composer.

> The maintainer of phpMailer, @Synchro, replied here 2 years ago to say:
> > It's crazy that Drupal 8 is still hard-coded to require a buggy, 8-year-old version with security vulnerabilities!

What he didn't say is that we are using a stripped down (and simplified) version, that doesn't suffer of any of the known security vulnerabilities.

> But I don't see how this issue can be set to "postponed". If you need help with any of these approaches, let us know. But we can't leave the status quo.

I have other things on my plate and there are thing I want to get settled before I take a decision on this. I updated the issue status just to signal that.

Updated the latest patch to work with beta4 release

The phpmailer library included in the module did not work for me on php 7.1, the mail body was always empty.

I tried to update to phpmailer 6.0.7, and it fixed the issue for me.

Attached a reroll for the latest dev.

This patch also includes the new autotls option, cause in my case i had to send the mail without encryption, and autotls is on by default, so it was impossible without turning it off.

Did a quick review for #40

+    $form['server']['smtp_autotls'] = [
+      '#type' => 'radios',
+      '#title' => $this->t('Enable TLS encryption automatically'),
+      '#default_value' => $config->get('smtp_autotls') ? 'on' : 'off',
+      '#options' => ['on' => $this->t('On'), 'off' => $this->t('Off')],
+      '#description' => $this->t('Whether to enable TLS encryption automatically if a server supports it, even if the protocol is not set to "tls". Be aware that in PHP >= 5.6 this requires that the server\'s certificates are valid.'),
+      '#disabled' => $this->isOverridden('smtp_autotls'),
+    ];
+

I think, you can remove the PHP version part as the upcoming Drupal 8.7.0 no longer supports PHP 5.6.

Setting it to needs review for the testbot.

2 years ago @wundo said,

we are using a stripped down (and simplified) version, that doesn't suffer of any of the known security vulnerabilities (of phpMailer).

While that still may be the case(?), I'm bumping the issue priority to major and adding the 'D8 stable release blocker' tag. Re: #36, I think option C is really the only viable option.

There are some functional differences between the different versions of PHPMailer. For example, it now doesn't set the Return-Path header to more closely align with the relevant RFCs:

• https://github.com/PHPMailer/PHPMailer/issues/144
• https://github.com/PHPMailer/PHPMailer/pull/210

Will this require some changes within the SMTP module? I know Drupal core sets Return-Path but it would be good to at least have the option to follow the RFC as we're encountering some problems with this header being set.

More info: #3055296: Setting the "Return-Path" header doesn't follow RFC 5321

Then again, it might be easier to use the PHPMailer module.

Patch 40 wasn't applying to beta5 of the module, so I did a quick re-roll. I also applied J-Lee's suggestion from comment 41.

Re-rolling for beta6

The #47 applies on beta6, but is it installing PHPMailer properly?

I added the patch to my root drupal composer.json and after composer update it is applied, but when I try to save the smtp form it throws
Error: Class 'PHPMailer\PHPMailer\PHPMailer' not found in Drupal\smtp\Plugin\Mail\SMTPMailSystem->mail() ...
I'm expecting to have a phpmailer directory under vendor, but it is not.

Is it working for you @jacobbell84?

thanks.

Hi espurnes,
I'm not running into that error, but I believe until the patch is applied to the repo you need to install PHPMailer via composer manually (Which is what I've been doing). Even though the patch adds it as a requirement to the composer.json file, composer uses it's own repo cache when computing dependencies, not the composer.json that we're actually patching.

Thanks @jacobbell84,

After adding phpmailer/phpmailer to my root composer file the #48 error has gone.

Now the patch applies and it sends emails but in some circumstances the subject is not properly formatted.
For instance in /user/password?_format=json endpoint the subject is something like =?UTF-8?B?UmVlbXBsYXpvIGRlIGlu[...]iBkZSBpbmljaW8gZGUgc2VzacOzbiA=?= =?UTF-8?B?cGFyYSBhbGVpeC[...]YWlsLmNvbSBlbiBXZSBBcmUgTnV0cml0aW8=?= =?UTF-8?B?bg==?= instead of Reemplazo de información de inicio de sesión para foo@bar.com en Sitename

If we remove Unicode:mimeHeaderEncode from line 350 of smtp/src/Plugin/Mail/SMTPMailSystem.php the subject is properly sent.

It seems the problem is that the subject is two times encoded as header:
- Smtp module does it at line 350 of smtp/src/Plugin/Mail/SMTPMailSystem.php

// Add the message's subject.
    $mailer->Subject = Unicode::mimeHeaderEncode($subject);

- PHPMailer does it again at line 2419 of vendor/phpmailer/phpmailer/src/PHPMailer.php:

// mail() sets the subject itself
        if ('mail' != $this->Mailer) {
            $result .= $this->headerLine('Subject', $this->encodeHeader($this->secureHeader($this->Subject)));
        }

Needs work.

I needed this resolved quickly to meet our needs, which unfortunately resulted in the creation of yet another SMTP module. PHPMailer SMTP is the bare minimum for sending email via SMTP. It is easily installed via composer.

We'll be using it for all our projects so any bugs will be fixed quickly.

I rerolled this for beta7 and took into account espurnes's feedback on the double mime encoding issue.

I have applied this patch and am testing things out. At present I am very happy with it. I did find that I had to explicitly add phpmailer to the main composer.json file (i.e. composer did not auto include it because it was now listed in the smtp/composer.json). I presume this was because it was added as a patch, but if not that would need to be sorted.

As far as I can see there are definite improvements to using PHPMailer 6 (in terms of conformance to RFCs, code quality and documentation) rather than the Drupal hacked version, and I would be much happier using the main project's version.

I won't RTBC yet - it would be best if one or more other people could play around with it first - but in principle I am ready to do so.

Quick update for this to take into account the new ConnectionTester class introduced in beta7. That class was also expecting PHPMailer to throw the Drupal\smtp\Exception\PHPMailerException on errors, so I switched it and the its test over to use PHPMailer\PHPMailer\Exception and removed the custom one since there was no additional logic in it and wasn't being used by anything else.

Small update, passing TRUE to the PHPMailer constructor in the connection tester file so that PHPMailer will throw external exceptions on failures (allowing us to catch and expose that information on the status screen like the Drupal version of PHPMailer did previously).

Been using it both test and live - works fine. Thank you.

diff --git a/composer.json b/composer.json
index 160b4f3..ced2da1 100644

index 160b4f3..ced2da1 100644
--- a/composer.json

--- a/composer.json
+++ b/composer.json

+++ b/composer.json
+++ b/composer.json
@@ -6,5 +6,8 @@

@@ -6,5 +6,8 @@
   "support": {
     "issues": "https://www.drupal.org/project/issues/smtp"
   },
-  "license": "GPL-2.0"
+  "license": "GPL-2.0",
+  "require": {
+    "phpmailer/phpmailer": "~6.0"
+  }
 }
\ No newline at end of file

Nitpicking: EOF newline issue (but possibly already in before, not caused by this patch?)

Not a useful nit to pick, especially without a patch.

Not a useful nit to pick, especially without a patch.

??? ... Is this what you are calling a careful comment? What are you referring to, please? A code review is a code review. And it was helpful and time consuming to check the code. Have you read the code or do you just added the patch to your project? A minor finding is a finding. That's what reviews are for: to make sure that the whole Drupal project and the contributed modules are qualified code and later issues can be tackled down carefully. No matter if you would like to see this RTBC and committed just ASAP.

@diqidoq is a long time contributor to the Drupal project including core and contrib module projects for over a decade and helped to get many things forward including the link module into core 8 and many other initiatives. When he takes his time to review some minor patches here and there to make some comments to leave it to the original patch author to correct this minor issues, it is just a type of kindness regarding the original patch author which you maybe do not understand.

especially without a patch.

Reviews are not for cloning or simply overriding original patches of authors who did the main work. Only if time goes on and the original author steps away from the patch. Reviews are for checking if other users can confirm that the code is OK. And where is your patch then if you complain? I do not think that it would be a big issue for any regular Drupal contributor (including diqidoq) to provide this patch with such one line corrected EOL minor issue (would you?), but usually we leave it up to the initial patch provider to correct such minor issues, unless there is no response over some time.

EOL should be fixed. So: Needs review. If you want RTBC and want to ignore the original author, feel free to provide the patch by yourself.

All good, I've corrected the EOL. To diqidoq's point I think it had been missing to begin with, but for something so minor it seems silly to open a new issue. Since that was the only feedback, I'm thinking we can just mark this as RTBC again to keep things moving along.

@jbitdrop: please calm down. No need to stress things here yet. The times are challenging for all of us in the moment and I am pretty sure @rivimey's intensions where not any offensive. She maybe didn't know about how reviews usually work or just didn't want to wait for another RTBC because of a nitpicking EOL.

But please note: EOL issues are no minor issues in big systems like Drupal since they multiply quickly. That's why we take care of it over all as much as possible since 2 decades. And apart form that I strongly recommend more than one up-to-date and serious code review of a removal before RTBC.

it had been missing to begin with, but for something so minor it seems silly to open a new issue.

@jacobbell84: That's exactly what I thought and just wanted to shear this lefty out quickly since noticed. Thanks for working on it and correcting it. 1++ No new issue needed. But please note: as the patch provider you shouldn't set an issue to RTBC but to "Needs Review". You can thank me later ;-) ...

I recommend to contact somebody on Slack or IRC and ask for another quick review. Otherwise maintainers will maybe hesitate to commit it quickly since you have only a few useful reviews yet. #59 is not really a useful review. But I leave it up to you. All the best and stay safe!

@diqidoq Fair enough, I switched it back to 'Needs Review'. FWIW I normally don't set my own patches to be RTBC, but since this has been the only feedback since rivimey's, I had made an exception.

I recommend to contact somebody on Slack or IRC and ask for another quick review.

I like that idea, this is one of the larger modules I've tried to commit to, so I appreciate the guidance! Looks like there's a patch-review channel on Slack, so I'll try there, unless you had a better place in mind.

@jbitdrop, sorry I offended, and @diqidoq my apologies.

I have been getting a bit tired lately of random reviewers picking up unrelated issues in a patch, complaining of them, but expecting someone else to address them, and thereby delaying fixing the original issue in some cases for years. In this case I was hasty, but it felt like an EOL issue that was not introduced as a result of the changes was not something to delay RTBC for (would have been quite happy for it to have been fixed alongside the rest of the patch).

@jacobbell84: thank you for your efforts.

Needless to say, I remain happy with RTBC in terms of the functionality of the patch. I include a new patch that addresses the point below, and also updates the README significantly. I started off with the thought of word-wrapping the github address line, but realized there were other changes that could be made as well. There is an interdiff as well.

+++ b/config/schema/smtp.schema.yml
@@ -17,6 +17,9 @@ smtp.settings:
+      label: 'Enable TLS encryption automatically'

Could this be changed to:

Enable TLS encryption automatically when supported by the remote host.

... assuming that is the case?

@rivimey: No need to apologize. All good for me. Only a misunderstanding. But as I sad: Reviews are not intended to be made alongside with fixes of the findings immediately included (That's the difference between "Needs review" and "Needs work") since original patch authors maybe still work on it and it would get in the mix then quite easely. Look at most of the core issues we worked on in the last years. If I provided a patch (or somebody else) and others review for me so that I can fix it nobody reviews and instantly puts the changes in. Especially if only one reviewer has been looked at the code. You usually wait for other reviews added to compare opniions and to sum up the changes to be made. Only if I wouldn't come back over a longer time somebody would take over. This is usual and useful practice learned the hard way and has nothing to do with picking random issues to complain about anything. Just bad timing here with that nitpick and your own feelings. Especially why should somebody do that? Others and I have ton of things to do as a CEO of 3 companies and just help others here in their issue queues when they ask me. As I sad: do not get into the mix too quickly. ;-)

So exactly that maybe happens here right now (things get in the mix): From what I understood @jacobbell84 tries to find a second reviewer now for the status of the changes that he made and you bring new things on the table now without discussing them. While you set it RTBC before and actually stated, that it would be ok that way it was. That was your initial review and status change. Now you add things to the patch of an issue which has status "Needs review" before reviewing and discussing it.

I hope you do not get me wrong. As I sad, I am fine with everything. We are lucky people because we can talk and can clarify things. All just maybe a little bit confusing for others who try to follow or maybe work on a review in the moment you did that. But this review would be outdated then. But again, also nothing to worry about. No offense. Just maybe not the best issue practice IMHO and maybe based on your wishes to finish this task as quickly as possible. But that's opinionated and could become an even longer time if things mess up. We shouldn't forget that we all have our own challenges to handle while we meet others. I just would strongly recommend to talk with @jacobbell84 and clarify what the final roadmap of this issue should include. Anything else somewhere else. And maybe somebody should asign to it to stop further confusion now after this. Otherwise it gonna become chaotic and hard to reproduce or tackle down in case of follow up issues. And most maintainers take their time then to follow such issues since there are things to check to make sure not to commit something bad.

Take care and stay safe!

I added the patch because I had just written that I was irritated people were posting reviews and expecting others to do the work, and I do not want to be put in that same position. However, I stand by my earlier comment. It's good enough in #63 to go without my additions, which can if considered good be added in another change.

-- One last OFFTOPIC because of the misunderstanding (sorry) --

@diqidoq: Sorry for my emotional interjection. I just felt that somebody who didn't know catched the wrong person. But you are right. We all should be aware of the situation atm. It is wearing for everybody.

Hi @rivimey: Thanks for your apology. I also want to apologize for my somewhat too emotional "reaction" on you. It just happens too often in the last years, that long-term contributors get attacked for wrong reasons and they start to feel more and more demotivated because of that. But we need them here! And I try to fight against that. So please accept my sorry for being so harsh on it. I hope you understand my reasons. I think I understand yours now.

I was irritated people were posting reviews and expecting others to do the work

No need to be irritated. I think you just took it the wrong way. Actually nobody, at least in the maintainer group of Drupal supporters I know, who reviews a patch is expecting others to "do the work" because reviewing a patch is boring and is work in the same way than writing a patch is work. With the only difference: Writing a patch is far more exiting than reviewing a patch of others. That's why it is so hard to find people to review the own patch. (Serious reviewing, not only testing if "it works" on the own project).

A serious review implements the injection on a fresh default Drupal testing installation, a code review, a check for coding standarts and a check if it interfears with another issue. Sometimes reviewing code takes longer than writing code. Especially if it is not your own code, where you usually remember better why this and that. And since @diqidoq is maintainer and co-maintainer of various projects here on D.O. too, he knows the even little things can badly disturb the flow or could be hard to find again later. Quick nitpicks in core issues are very common since they all have only short time to review others. But after 3 reviewers passing by with nitpicks or with different opinions about changes, the patch author has some useful things collected to do. After a while somebody else will maybe take over.

That being sad, I just want to let you know that I didn't wanted to make you feel bad and that I was just chiming in with the intension to prevent a misunderstanding.

-- OFFTOPIC END --

Thanks for the work on this. Rerolled and formatted it for inclusion. Fixed.

@japerry: +1

Hello I'am new here . I have problem's with installation of the SMTP mail server . I have tried everething already .
But i can't activate the module . Keeps saying :smtp cannot be turned on because the phpmailer library is missing.

where do i have to set the library now . Can anyone give me the correct path ??

I tried

modules/smtp/lib/PHPMailer-PHPMailer/v6.1.5/src/PHPMailer.php

but wont work i also tried to set in in the vendor folder but nu succes.

Stephan, the simplest way to install the library is using composer, which is already covered in the docs. This really is the best way, and you should only try others if it's really impossible to use composer.

If you are unable to use composer, you may have luck putting the library in /libraries but that is not enough, because nothing will look for it there. One option is to build an integration with the libraries API, as this module doesn't include that. The integration is (was, last time I did it) a hook function returning an array that said "this library is there" and is described in the "libraries" contrib module.

If you can run composer but you don't have an overall composer file (i.e. outside docroot), you may be able to composer require the phpmailer library into drupal core's own composer.json. Remember, though, that you will need to redo that each time you update Core, because that composer.json is overwritten each time core is updated.

A final option, still requiring some code but not much. Add some code to settings.php that follows this blog post: https://tutorials.supunkavinda.blog/php/oop-autoloading

... in checking that any class that php asks about autoloading are in a subdirectory of /libraries/PHPMailer. Something like this (but please note this is not directly copy-pastable - I haven't looked to see what will work, but I have improved the comments in the original code for you):

spl_autoload_register(function ($class) {
    // Define a namespace prefix. You want to bail out of this function as fast as
    // possible, because it's called for each and every undefined class anywhere
    // in the code base. Use the class prefix and/or name to do that.
    $prefix = 'PHPMailer\\';

    // check if this is a class from our project
    $len = strlen($prefix);
    if (strncmp($prefix, $class, $len) !== 0)
        return; // nope, it isn't   <<<<<<<<<<<<< IMPORTANT

    // Depending on the relationship between the namespace and the directory
    // structure you may need to modify this area. You are just constructing a
    // filename from the given class name.

    // Get the relative class name: remove the namespace name
    $className = substr($class, $len);

    // base directory of classes. In this case relative to where this code is, but it
    // doesn't have to be. This could be e.g. $baseDir = "libraries/phpmailer/src"
    // for a relative-to-docroot search.
    $baseDir = __DIR__ . '/src/';

    // relative including
    $file = $baseDir . str_replace('\\', DIRECTORY_SEPARATOR, $className) . '.php';

    // if the file exists, require it
    if (file_exists($file)) {
        require $file;
    }
});