Update PHPMailer to 5.2.10

[MattyRob]

PHPMailer 5.2.8 was released in May, we should review the updates and consider updating the PHPMailer libraries bundled with WordPress.
​https://github.com/PHPMailer/PHPMailer/releases/tag/v5.2.8

We also need to bear in mind previous discussions and WordPress amendments to PHPMailer as already discussed: #25560, #25014.

[MattyRob]

Very brief testing done on the attached patch - tried to send one email and it worked.

Fixes for PHPMailer Autoloader are retained and only the 2 currently used files are included. I think the qmail issue has been fixed upstream now but would value a second opinion on that.

[michalzuber]

Changed class-pop3.php

[michalzuber]

Mails came for me (worked) with the following Subjects:

• [WordPress] New User Registration
• [WordPress] Password Reset
• [WordPress] Your username and password
• [WordPress GIT] Please moderate: "Hello world!"
• [WordPress GIT] WordPress 4.0-beta2-20140729-20140729 is available. Please update!
• [WordPress GIT] There were failures during background updates
• [WordPress GIT] Background updates have finished

Also tried (received) WooCommerce:

• [WordPress] New customer order (39) - July 30, 2014
• Your WordPress order receipt from July 30, 2014

[michalzuber]

Have no experience with Multisite, so I didn't try email notifications for it.
But here's the list of email Subjects that I found:

• Activate
• New Site
• New User
• New Admin Email Address
• Delete My Site
• New Site Created

[michalzuber]

With updated class-pop3.php I got the following errors when trying /wp-mail.php

For port 995:
PHP Notice: Undefined property: POP3::$ERROR in /data/o/p/optimalizaciaseo.sk/sub/nevilleweb.sk/WordPress/wp-mail.php on line 45

print_r($pop3) output:

POP3 Object
(
    [Version] => 5.2.8
    [POP3_PORT] => 110
    [POP3_TIMEOUT] => 30
    [CRLF] => 

    [do_debug] => 0
    [host] => 
    [port] => 
    [tval] => 
    [username] => 
    [password] => 
    [pop_conn:POP3:private] => Resource id #235
    [connected:POP3:private] => 
    [error:POP3:private] => Array
        (
            [error] => Server reported an error: 
            [errno] => 0
            [errstr] => 
        )

)

For port 110:
PHP Fatal error: Call to undefined method POP3::user() in /data/o/p/optimalizaciaseo.sk/sub/nevilleweb.sk/WordPress/wp-mail.php on line 44

Update introduced new method login() at ​class.pop3.php#L266

[MattyRob]

Replying to michalzuber:

With updated class-pop3.php I got the following errors when trying /wp-mail.php

The class-pop3.php file is not part of PHPMailer when bundled with WordPress. The only 2 files that are from PHPMailer in WordPress are class-phpmailer.php and class-smtp.php.

[DrewAPicture]

With a serious lack of consensus here on 1) What was changed 2) How those changes will affect us, it might be best to work on this in 4.1.

[MattyRob]

Replying to DrewAPicture:

With a serious lack of consensus here on 1) What was changed 2) How those changes will affect us, it might be best to work on this in 4.1.

I'm not sure I understand what you are saying here, are you looking for a list of changes to PHPMailer? It's not too difficult to review the changes in the diff or side-by-side compare the 2 files with current and new versions.

The page below lists the changes in the most recent release:
​https://github.com/PHPMailer/PHPMailer/releases

[DrewAPicture]

Replying to MattyRob:

Replying to DrewAPicture:

With a serious lack of consensus here on 1) What was changed 2) How those changes will affect us, it might be best to work on this in 4.1.

I'm not sure I understand what you are saying here, are you looking for a list of changes to PHPMailer? It's not too difficult to review the changes in the diff or side-by-side compare the 2 files with current and new versions.

The page below lists the changes in the most recent release:
​https://github.com/PHPMailer/PHPMailer/releases

What's at issue is that anytime a bundled library is updated, we need assess what they changed and how that might affect our integration of their library. So if major changes were made in the latest update, we would need to assess every section of the changed code to make sure that nothing broke in the process on our end. That summary is missing here.

[MattyRob]

Okay, so what is needed to get that summary? Is that something I can do? Or are you basically saying it needs one of the lead developers to be tasked to do this?

[DrewAPicture]

Replying to MattyRob:

Okay, so what is needed to get that summary? Is that something I can do? Or are you basically saying it needs one of the lead developers to be tasked to do this?

You could absolutely do it :)

The goal would be to asses the ways we leverage PHPMailer and test them to see if anything changed. If it did, how did it change and what recommendations you might have to alleviate any back-compat concerns that might arise.

[MattyRob]

Replying to DrewAPicture:

You could absolutely do it :)

The goal would be to asses the ways we leverage PHPMailer and test them to see if anything changed. If it did, how did it change and what recommendations you might have to alleviate any back-compat concerns that might arise.

I feel I may be missing something here but in attempting to answer the above:

On 'greping' through the current WordPress code (rev 29448), the only places where PHPMailer is called are in the wp-includes/pluggable.php file where the library is used as part of the core wp_mail() function and in some multisite files (wp-includes/ms-default-filters.php and wp-includes/ms-functions.php) where the phpmailer_init hook is called and the outgoing email server name is corrected for multisite emails.

So, essentially both instances above relate to emails sent using the wp_mail() function.

As for changes, looking through the original diff I submitted here:
https://core.trac.wordpress.org/attachment/ticket/28909/28909.diff

The majority of the changes are documentation, string alterations like "FALSE" to "false" changes and a switch to using single instead of double quotes.

Is this the kind of thing you mean? IF so I'll see if I can dig into the SMTP library.

[markjaquith]

Disregard the documentation and string/quotation changes. There are major functional changes (per their own changelog), and I think it's a bit late in the process to be confident about them. Work on this ticket (listing and testing the functional changes) should continue, but for a 4.1 milestone.

[MattyRob]

Having looked at the ​Changelog and removed all of the references to added or changed translation files and document updates this is what remains:

Increase timeout to match RFC2821 section 4.5.3.2 and thus not fail greetdelays, fixes #104
Add timestamps to default debug output
Add connection events and new level 3 to debug output options
Allow custom Mailer types (Thanks to @michield)
Cope with spaces around SMTP host specs
Fix processing of multiple hosts in connect string
Autoloader now prepends
Make autoloader work better on older PHP versions
Avoid double-encoding if mbstring is overloading mail()
Make quoted-printable encoder respect line ending setting
Fix serverHostname on PHP < 5.3
Improve performance of SMTP class
Implement automatic 7bit downgrade
Improve example images, switch to PNG
Remove setting the Return-Path and deprecate the Return-path property - it's just wrong!
Add HTML5 email validation pattern
Check php.ini for path to sendmail/qmail before using default
Don't use quoted-printable encoding for multipart types
Remove useless PHP5 check
Use SVG for build status badges
Store MessageDate on creation
Better default behaviour for validateAddress

I've crossed through the items that would at first glance appear to be irrelevant to WordPress, AutoLoader for example is not bundled or used in WordPress, nor is debug mode or custom Mailer types. The rest of these items need checking.

[zodiac1978]

Related: #27946

[MattyRob]

PHPMailer 5.2.9 is available now and has been around for about 6 months. Might as well jump straight to this version when the library gets updated.

[MattyRob]

It seems there are problems being seen with PHPMailer 5.2.7:
​https://github.com/PHPMailer/PHPMailer/issues/219
​http://en.forums.wordpress.com/topic/rfc-violating-content-transfer-encoding-header
And perhaps:
​https://wordpress.org/support/topic/extra-line-breaks-every-line?replies=9

It also seems that this issue was fixed in PHPMailer 5.2.8

[barry]

5.2.10 has been released and we should update. WordPress.com has been running 5.2.10 for a couple of weeks and there have been no issues - it also fixed a few bugs, one of which was causing emails to be dropped under certain circumstances. I think the only required change from upstream PHPMailer is the (re)addition of require_once 'class-smtp.php'; in class-phpmailer.php. The previous changes regarding the AutoLoader are no longer required.

There is also some code in class-smtp.php to support NTLM which should probably be removed because executing it will result in a Fatal error since the required client is not distributed as part of WordPress:

require_once 'extras/ntlm_sasl_client.php';

Happy to provide a patch if we can find someone to commit it :)

[samuelsidler]

Replying to barry:

Happy to provide a patch if we can find someone to commit it :)

Please do! I think we should get this into 4.3. Are there any PHP 5.2 issues?

[obenland]

We're missing a patch and are entering beta. Let's try again in 4.4.

[MattyRob]

Updated patch to PHPMailer 5.2.10 attached - some minor changes to the file to remove the NTLM section of code in the SMTP class.

Please can this one get checked and committed at some point - 4.4 is fine but I'm getting disillusioned with creating patches that gather dust or go stale.

[helen]

I'm bringing this back into 4.3 for its bugfixing aspects, though we need to get it in ASAP. Can we get some assurances that we're not going to bomb out 5.2.4? I don't mind breaking that in alpha, but since we're about to enter beta, let's be at least slightly more cautious.

@MattyRob Very sorry for the runaround, I agree that's no good. Clearly we need more ownership in this area. What are some things we could do here to ease things - unit/integration tests, documentation about what we have to consider when updating PHPMailer, tickets like #29513, all of the above?

[MattyRob]

@helen

Thank you for taking this up. I'm pleased that you share my feeling that this can be done better. The ticket you link to demonstrates my point also. It does seem that some external libraries (PHPMailer in particular) are almost abandoned and I have no idea why.

There are lots of tickets for fixes and enhancements to wp_mail() and the libraries it uses that are left untouched by those who can commit.

I accept totally that you guys are busy but a little ownership or attention here would be very welcome. Unit test would make patch testing easier for those who can commit the patches (I'd help if I knew what I was doing with patch tests but it's a mystery to me). Beyond that I'm not sure what else to suggest.

I've been running a slightlyodofied PHPMailer at version 5.2.9 for a couple of months on a blog that sends about 5,000 emails per month without issues if that's any reassurance.

[helen]

Honestly, I have no idea who could or wants to own the Mail component and related external library/ies right at this moment. It's not a very glamorous part of core, and yet so critical. Any interest in maintainership here? Being a maintainer doesn't require test writing or even the desire to commit directly to SVN - mostly you need some idea of what the bigger vision for that component is and how various tickets do or don't fit into a roadmap, and a lot of tenacity. Multiple people can and should maintain a component together. It's far easier for a committer (in our centralized version control sense) to get up the willpower to do final review and commit when something's already been reviewed and flagged as potentially ready by somebody who digs deeper into a given area.

If @barry comes back by and says "this patch is what I would have done", I'll commit it. Otherwise, I'll take another couple of days to try to find somebody who's dealt with this before. (Pleeeeease @barry.)

[MattyRob]

@Helen

Perhaps this needs a few of the people who open and contribute tickets to loosely form a group to review PHPMailer tickets and take forward the fixes and enhancements. Is there a WordPress way to arrange this for interested parties? A 'Slack' group or something like that?

I'm happy to be a part of it provided I'm not going to be alone and that the tickets are going to get committed once they've been reviewed and agreed.

[helen]

Coordinating a kick-off Slack meeting would be great! I typically advise that meetings just go ahead and start in #core (when there isn't another meeting going on), and if the discussion becomes overwhelming, then we move to a dedicated channel. I definitely agree we need to find a committer who is up for being a part of this, but in the meantime, you can ping me and/or @samuelsidler in #core on Slack and we can help coordinate and get the message out.

[barry]

Looks good to me. I have no idea if it works with PHP5.2, but can probably do some basic testing if no one else is able to. We have sent ~500 million emails using PHPMailer 5.2.10 and there have been no observed or reported issues.

[obenland]

@MattyRob, do you think you could test it with PHP 5.2?

[MattyRob]

@obenland

Yes, I think I should be able to do that given a little time.

[MattyRob]

Okay, after some hassle installing PHP52 again and also configuring it I've managed to get a testing site on my local Mac.

I had some issue with a stuck mail queue to contend with as well hence the delay.

But, PHPMailer as patched in the most recent patch is delivering emails using WP 4.3beta1-33046-src. I've sent reset password emails and also sent emails from the Subscribe2 plugin using the Send Email page.

Anything else in particular you want me to check?

[MattyRob]

New user registration worked too with email sent to the new user and blog admin.

[samuelsidler]

Let's get this in early this week so it's in betas 2-4.

[ocean90]

Tested with PHP 5.2 as well and works fine there. But I get some unit test failure with 28909v3.diff​.

There were 5 failures:

1) Tests_Mail::test_wp_mail_custom_boundaries
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'
-------=_Part_4892_25692638.1192452070893
-Content-Type: text/plain; charset=ISO-8859-1
-Content-Transfer-Encoding: 7bit
-Content-Disposition: inline
-
-Here is a message with an attachment of a binary file.
-
-------=_Part_4892_25692638.1192452070893
-Content-Type: image/x-icon; name=favicon.ico
-Content-Transfer-Encoding: base64
-Content-Disposition: attachment; filename=favicon.ico
-
-AAABAAEAEBAAAAAAAABoBQAAFgAAACgAAAAQAAAAIAAAAAEACAAAAAAAAAAAAAAAAAAAAAAAAAAA
-AAAAAAAAAAAAAACAAACAAAAAgIAAgAAAAIAAgACAgAAAwMDAAICAgAAAAP8AAP8AAAD//wD/AAAA
-/wD/AP//AAD///8A//3/AP39/wD6/f8A+P3/AP/8/wD9/P8A+vz/AP/7/wD/+v8A/vr/APz6/wD4
-+v8A+/n/APP5/wD/+P8A+vj/AO/4/wDm+P8A2fj/AP/3/wD/9v8A9vb/AP/1/wD69f8A9PT/AO30
-/wD/8/8A//L/APnx/wD28P8A///+APj//gD2//4A9P/+AOP//gD//f4A6f/9AP///AD2//wA8//8
-APf9/AD///sA/v/7AOD/+wD/+vsA9/X7APr/+gDv/voA///5AP/9+QD/+/kA+e35AP//+ADm//gA
-4f/4AP/9+AD0+/gA///3APv/9wDz//cA8f/3AO3/9wD/8fcA//32AP369gDr+vYA8f/1AOv/9QD/
-+/UA///0APP/9ADq//QA///zAP/18wD///IA/fzyAP//8QD///AA9//wAPjw8AD//+8A8//vAP//
-7gD9/+4A9v/uAP/u7gD//+0A9v/tAP7/6wD/+eoA///pAP//6AD2/+gA//nnAP/45wD38eYA/fbl
-AP/25AD29uQA7N/hAPzm4AD/690AEhjdAAAa3AAaJdsA//LXAC8g1gANH9YA+dnTAP/n0gDh5dIA
-DyjSABkk0gAdH9EABxDRAP/l0AAAJs4AGRTOAPPczQAAKs0AIi7MAA4UywD56soA8tPKANTSygD/
-18kA6NLHAAAjxwDj28QA/s7CAP/1wQDw3r8A/9e8APrSrwDCtqoAzamjANmPiQDQj4YA35mBAOme
-fgDHj3wA1qR6AO+sbwDpmm8A2IVlAKmEYgCvaFoAvHNXAEq2VgA5s1UAPbhQAFWtTwBStU0ARbNN
-AEGxTQA7tEwAObZIAEq5RwDKdEYAULhDANtuQgBEtTwA1ls3ALhgMQCxNzEA2FsvAEC3LQB0MCkA
-iyYoANZTJwDLWyYAtjMlALE6JACZNSMAuW4iANlgIgDoWCEAylwgAMUuIAD3Vh8A52gdALRCHQCx
-WhwAsEkcALU4HACMOBwA0V4bAMYyGgCPJRoA218ZAJM7FwC/PxYA0msVAM9jFQD2XBUAqioVAIAf
-FQDhYRQAujMTAMUxEwCgLBMAnxIPAMsqDgCkFgsA6GMHALE2BAC9JQAAliIAAFYTAAAAAAAAAAAA
-AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA
-AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAD/
-//8AsbGxsbGxsbGxsbGxsbGxd7IrMg8PDw8PDw8PUBQeJXjQYE9PcKPM2NfP2sWhcg+BzTE7dLjb
-mG03YWaV4JYye8MPbsLZlEouKRRCg9SXMoW/U53enGRAFzCRtNO7mTiAyliw30gRTg9VbJCKfYs0
-j9VmuscfLTFbIy8SOhA0Inq5Y77GNBMYIxQUJzM2Vxx2wEmfyCYWMRldXCg5MU0aicRUms58SUVe
-RkwjPBRSNIfBMkSgvWkyPxVHFIaMSx1/0S9nkq7WdWo1a43Jt2UqgtJERGJ5m6K8y92znpNWIYS1
-UQ89Mmg5cXNaX0EkGyyI3KSsp6mvpaqosaatq7axsQAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA
-AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=
-------=_Part_4892_25692638.1192452070893--
-
-'
+'=0A------=3D_Part_4892_25692638.1192452070893=0AContent-Type: text/plain; c=
+harset=3DISO-8859-1=0AContent-Transfer-Encoding: 7bit=0AContent-Disposition=
+: inline=0A=0AHere is a message with an attachment of a binary file.=0A=0A-=
+-----=3D_Part_4892_25692638.1192452070893=0AContent-Type: image/x-icon; nam=
+e=3Dfavicon.ico=0AContent-Transfer-Encoding: base64=0AContent-Disposition: =
+attachment; filename=3Dfavicon.ico=0A=0AAAABAAEAEBAAAAAAAABoBQAAFgAAACgAAAA=
+QAAAAIAAAAAEACAAAAAAAAAAAAAAAAAAAAAAAAAAA=0AAAAAAAAAAAAAAACAAACAAAAAgIAAgAA=
+AAIAAgACAgAAAwMDAAICAgAAAAP8AAP8AAAD//wD/AAAA=0A/wD/AP//AAD///8A//3/AP39/wD=
+6/f8A+P3/AP/8/wD9/P8A+vz/AP/7/wD/+v8A/vr/APz6/wD4=0A+v8A+/n/APP5/wD/+P8A+vj=
+/AO/4/wDm+P8A2fj/AP/3/wD/9v8A9vb/AP/1/wD69f8A9PT/AO30=0A/wD/8/8A//L/APnx/wD=
+28P8A///+APj//gD2//4A9P/+AOP//gD//f4A6f/9AP///AD2//wA8//8=0AAPf9/AD///sA/v/=
+7AOD/+wD/+vsA9/X7APr/+gDv/voA///5AP/9+QD/+/kA+e35AP//+ADm//gA=0A4f/4AP/9+AD=
+0+/gA///3APv/9wDz//cA8f/3AO3/9wD/8fcA//32AP369gDr+vYA8f/1AOv/9QD/=0A+/UA///=
+0APP/9ADq//QA///zAP/18wD///IA/fzyAP//8QD///AA9//wAPjw8AD//+8A8//vAP//=0A7gD=
+9/+4A9v/uAP/u7gD//+0A9v/tAP7/6wD/+eoA///pAP//6AD2/+gA//nnAP/45wD38eYA/fbl=
+=0AAP/25AD29uQA7N/hAPzm4AD/690AEhjdAAAa3AAaJdsA//LXAC8g1gANH9YA+dnTAP/n0gDh=
+5dIA=0ADyjSABkk0gAdH9EABxDRAP/l0AAAJs4AGRTOAPPczQAAKs0AIi7MAA4UywD56soA8tPK=
+ANTSygD/=0A18kA6NLHAAAjxwDj28QA/s7CAP/1wQDw3r8A/9e8APrSrwDCtqoAzamjANmPiQDQ=
+j4YA35mBAOme=0AfgDHj3wA1qR6AO+sbwDpmm8A2IVlAKmEYgCvaFoAvHNXAEq2VgA5s1UAPbhQ=
+AFWtTwBStU0ARbNN=0AAEGxTQA7tEwAObZIAEq5RwDKdEYAULhDANtuQgBEtTwA1ls3ALhgMQCx=
+NzEA2FsvAEC3LQB0MCkA=0AiyYoANZTJwDLWyYAtjMlALE6JACZNSMAuW4iANlgIgDoWCEAylwg=
+AMUuIAD3Vh8A52gdALRCHQCx=0AWhwAsEkcALU4HACMOBwA0V4bAMYyGgCPJRoA218ZAJM7FwC/=
+PxYA0msVAM9jFQD2XBUAqioVAIAf=0AFQDhYRQAujMTAMUxEwCgLBMAnxIPAMsqDgCkFgsA6GMH=
+ALE2BAC9JQAAliIAAFYTAAAAAAAAAAAA=0AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=
+AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=0AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=
+AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAD/=0A//8AsbGxsbGxsbGxsbGxsbGxd7IrMg8P=
+Dw8PDw8PUBQeJXjQYE9PcKPM2NfP2sWhcg+BzTE7dLjb=0AmG03YWaV4JYye8MPbsLZlEouKRRC=
+g9SXMoW/U53enGRAFzCRtNO7mTiAyliw30gRTg9VbJCKfYs0=0Aj9VmuscfLTFbIy8SOhA0Inq5=
+Y77GNBMYIxQUJzM2Vxx2wEmfyCYWMRldXCg5MU0aicRUms58SUVe=0ARkwjPBRSNIfBMkSgvWky=
+PxVHFIaMSx1/0S9nkq7WdWo1a43Jt2UqgtJERGJ5m6K8y92znpNWIYS1=0AUQ89Mmg5cXNaX0Ek=
+GyyI3KSsp6mvpaqosaatq7axsQAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=0AAAAAAAAAAAAA=
+AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=3D=0A------=3D_Part_4892_25692638.1=
+192452070893--=0A=0A'

/srv/www/wp-develop/svn/tests/phpunit/tests/mail.php:63

2) Tests_Mail::test_wp_mail_rfc2822_addresses
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'My RFC822 Test Message
-'
+'My RFC822 Test Message'

/srv/www/wp-develop/svn/tests/phpunit/tests/mail.php:92

3) Tests_Mail::test_wp_mail_multiple_rfc2822_to_addresses
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'My RFC822 Test Message
-'
+'My RFC822 Test Message'

/srv/www/wp-develop/svn/tests/phpunit/tests/mail.php:111

4) Tests_Mail::test_wp_mail_multiple_to_addresses
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'My RFC822 Test Message
-'
+'My RFC822 Test Message'

/srv/www/wp-develop/svn/tests/phpunit/tests/mail.php:123

5) Tests_Mail::test_wp_mail_to_address_no_name
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'My RFC822 Test Message
-'
+'My RFC822 Test Message'

/srv/www/wp-develop/svn/tests/phpunit/tests/mail.php:137

FAILURES!
Tests: 3774, Assertions: 14852, Failures: 5, Incomplete: 3, Skipped: 12.

But I don't get these when running $ phpunit --group mail. Any ideas why?

[MattyRob]

It looks like it might be something to do with line breaks. In the unit tests there are line breaks added in the test functions:

$this->assertEquals($message . "\n", $GLOBALS['phpmailer']->mock_sent[0]['body']);

I'm not sure why they are in there but I suspect it was to ensure the previous versions of PHPMailer passed the tests. PHPMailer 5.2.8 and5.2.10 have reported improvements in encoding so perhaps that's why the new version fails these tests.

[MattyRob]

I've done a bit of hunting by trial and error.

Calling phpunit --group=mail works fine, as does calling the file directly with phpunit /tests/phpunit/tests/mail.php

So then I started removing other phpunit test file and I isolated a conflict to comment.php. Next I tried to isolate each function test within that file and found that removing the test_comment_field_lengths() function allows all PHPunit tests to pass. Restoring that function again and the mail tests fail again.

I have no idea why that might be the case but hopefully the pointer will help someone.

[ocean90]

Diff from root

[ocean90]

Replying to MattyRob:

[...] removing the test_comment_field_lengths() function allows all PHPunit tests to pass. Restoring that function again and the mail tests fail again. [...]

Confirmed. Changing 65536 to 998 lets the tests pass too.

[ocean90]

@obenland found the same number as the value for MAX_LINE_LENGTH in both classes.

    /**
     * The maximum line length allowed by RFC 2822 section 2.1.1
     * @type integer
     */
    const MAX_LINE_LENGTH = 998;

[dd32]

The reason for the unit test failures is that PHPMailer::createBody() will set $this->encoding = 'quoted-printable' (away from it's default of 8bit) when it encounters a line longer than 999 characters.
It doesn't look like PHPMailer cleans up after itself / presets all variables, that one included, which is causing the problem (so it doesn't start with a clean slate each time). It does however override many variables on each mail being sent, which is why it probably doesn't have a reset-everything-to-default routine that I can see.

I thought Globals were backed up / restored during each test iteration, which would've avoided this, but it doesn't seem like it does.

28909.unit-test-vars.diff​ works around the issue in PHPMailer by setting the Encoding variable in preSend(), which appears to fix it.. but I don't know what kind of side effects it has.
It also shows re-creating the $phpmailer global on each test iteration, a unit test that does nothing other than trigger this bug, and rewrites MockMailer to override postSend() instead of send() for no other reason than I did it while diagnosing what was going on.

[ocean90]

@dd32 Thanks for your investigations. I want to add that this happens because wp_new_comment() triggers wp_mail() which contains a single line of 65535 characters long.

I did a look at 28909.unit-test-vars.diff​ and it does fix the unit tests for me. It has one bug in MockPHPMailer::preSend(): parent:preSend() needs to be returned.
I'll vote for the change to MockPHPMailer instead of re-creating the $phpmailer global but I'm not quite sure why we need to reset $this->encoding.
I hadn't the time to test what happens when we send two mails via wp_mail() with PHPMailer::postSend() since core creates only one instance of $phpmailer too, so we wouldn't reset $this->encoding in this case.

[dd32]

To be clear; It's a "bug" in PHPMailer that the Encoding variable (and potentially others) isn't reset between mail calls.
It's not a serious bug however, as it doesn't affect the ability to send emails, it only trips up our unit testing.

Based on that, I think I'd be behind the angle of working around it in our MockMailer for now, and submitting a issue upstream.

[samuelsidler]

Replying to dd32:

Based on that, I think I'd be behind the angle of working around it in our MockMailer for now, and submitting a issue upstream.

Sounds like a good way forward. @MattyRob: Do you want to work up a patch to our MockMailer that works around this issue? I think that's all that's needed for this to go in and would like to see it land before beta 2.

[MattyRob]

@samuelsidler I not sure I'm the best person for that - I know virtually nothing about the unit tests and I'm not at all clear on what I'm testing that's different from the patch above.

[ocean90]

We already have the tests in 28909.unit-test-vars.diff​. I'm going to commit these and the update in a few.

[jorbin]

@ocean90 your patch 28909v4.diff includes a change to src/wp-includes/css/dashicons.css that doesn't seem to belong here.

[ocean90]

In 33124:

Update PHPMailer to 5.2.10 from 5.2.7.

Includes two modifications for WordPress:

• Removes support for NTLM in class-smtp.php since the required client (extras/ntlm_sasl_client.php) is not distributed as part of WordPress.
• Requires class-smtp.php for backwards compatibility with direct (non-wp_mail()) uses of PHPMailer, as the autoloader isn't used. See [27385].

This also includes a change to our MockMailer for unit tests. It now overrides postSend() instead of send(), and preSend()`.
preSend() resets $this->Encoding because PHPMailer doesn't clean up after itself / presets all variables. This becomes an issue when PHPMailer::createBody() sets $this->Encoding = 'quoted-printable' (away from it's default of 8bit) when it encounters a line longer than 998 characters. Tests_Comment::test_comment_field_lengths is such a case.

props MattyRob, dd32.
fixes #28909.

[ocean90]

In 33142:

Remove debug cruft from [33124].

see #28909.