Update to PHPMailer 5.2.7

[MattyRob]

As suggested in ticket #25014 I am opening a new ticket to where we can review the inclusion of PHPMailer libraries in WordPress (possibly from 3.8).

Things I think we need to consider:
1/ PHPMailer uses an AutoLoader that needs including now, should we move to a sub-folder like other external libraries to aid file management as we now have 3 or more files to manage
2/ Should we keep with the '.' file notation of PHPMailer now or continue renaming and editing files using WordPress '-' notation.
3/ What files from the PHPMailer library should we include? Languages? The Extras folder?

A starter patch is attached.

[bpetty]

I'll be back to this for a deeper review later here, likely adding some back compat API to go with this, but just some quick notes for now:

(1) Yes, I think it's time to move this to it's own folder since we're being pushed to add new file(s) for the first time in years.

(2) The only reason we keep renaming these files was for back compat purposes, and if this is moved to a new folder, we'll just end up either deciding to break this, or adding a bare placeholder include file in it's place. Either way, we should definitely stick with PHPMailer official filenames.

(3) I think @nacin has expressed that there's not any issues with including the entire library as intended, and really it's not that big. I'd like more input from others here as well. I personally think we should just include all of it as intended.

[MattyRob]

I've noticed an error in the attached diff in the file names - still using WordPress convention in pluggable but files not renamed in the PHPMailer AutoLoader. I've amended the pluggable name and the new attached patch works for me.

[MattyRob]

This patch also contains some of the code from an associated ticket: #18493

[bpetty]

I'm sorry I didn't help get this pushed for 3.8, but since we've missed the feature freeze, this needs to be pushed back to 3.9 now. This can't miss a beta release.

By the way, I have briefly reviewed your patch, and just want to quickly point out that we can't apply patches that merge changes from two entirely unrelated issues. Your changes with multi-part messages in wp_mail() (allowing HTML) can not be included in this patch.

[MattyRob]

Shame it's being bumped but with the faster cycles it hopefully won't be long until 3.9 ;)

I've attached an updated patch that's cleaner in patching the wp_mail() function and doesn't overlap with other tickets.

I have included things like the PHPMailer license, readme and changelog also - not sure if they should be there or not.

[MattyRob]

I have tested my most recent patch and can't see any issues - wp_mail still works fine.

Can we have this ticket moved to the 3.9 milestone?

[bpetty]

This is a binary patch (contains images), and can only be applied with "git apply".

[bpetty]

I'm guessing you generated that last patch from Windows since all your files are set as executable (or you just have really wrong autoprops configured). Also, assuming the intention was to include the entire release, you're still missing a bunch of files from the official package (and not just binary image files):

 src/wp-includes/PHPMailer/.gitignore                         |    4 +
 src/wp-includes/PHPMailer/.travis.yml                        |   20 +
 src/wp-includes/PHPMailer/composer.json                      |   33 +
 src/wp-includes/PHPMailer/docs/Callback_function_notes.txt   |   17 +
 src/wp-includes/PHPMailer/docs/DomainKeys_notes.txt          |   55 ++
 src/wp-includes/PHPMailer/docs/Note_for_SMTP_debugging.txt   |   17 +
 src/wp-includes/PHPMailer/docs/extending.html                |  145 +++
 src/wp-includes/PHPMailer/docs/faq.html                      |   67 ++
 src/wp-includes/PHPMailer/docs/generatedocs.sh               |    5 +
 src/wp-includes/PHPMailer/docs/pop3_article.txt              |   50 ++
 src/wp-includes/PHPMailer/examples/images/phpmailer.png      |  Bin 0 -> 8670 bytes
 src/wp-includes/PHPMailer/examples/images/phpmailer_mini.gif |  Bin 0 -> 1042 bytes
 src/wp-includes/PHPMailer/examples/scripts/XRegExp.js        |    0
 src/wp-includes/PHPMailer/examples/styles/wrapping.png       |  Bin 0 -> 631 bytes
 src/wp-includes/PHPMailer/extras/EasyPeasyICS.php            |   90 ++
 src/wp-includes/PHPMailer/extras/class.html2text.php         |  696 +++++++++++++++
 src/wp-includes/PHPMailer/extras/htmlfilter.php              |  861 ++++++++++++++++++
 src/wp-includes/PHPMailer/extras/ntlm_sasl_client.php        |  185 ++++
 src/wp-includes/PHPMailer/test/fakepopserver.sh              |  125 +++
 src/wp-includes/PHPMailer/test/fakesendmail.sh               |   22 +
 src/wp-includes/PHPMailer/test/phpmailerLangTest.php         |  340 ++++++++
 src/wp-includes/PHPMailer/test/phpmailerTest.php             | 1402 ++++++++++++++++++++++++++++++
 src/wp-includes/PHPMailer/test/runfakepopserver.sh           |   10 +
 src/wp-includes/PHPMailer/test/test_callback.php             |   82 ++
 src/wp-includes/PHPMailer/test/testbootstrap-dist.php        |    7 +
 25 files changed, 4233 insertions(+)

I'm starting to wonder if it's still a good idea to distribute the entire release now though. It's 836K in total, but if we exclude the docs, tests, and especially the examples, it drops 416K of that. Additionally, it's not a direct security vulnerability, but I feel a little safer not including all of the utility shell scripts found in all 3 of those folders.

I've taken this opportunity to also actually clear out the old PHPMailer code in class-phpmailer.php and class-smtp.php, and replace them with backwards compatible shim includes pulling in the code in the new location.

I've also updated the MockPHPMailer class in the unit tests with the really minor changes upstream in PHPMailer (they apparently decided methods shouldn't be capitalized).

I'm including two versions of this patch for review:

• ticket-25560-phpmailer-full-binary.patch​: This is the full PHPMailer release including images, in the form of a binary patch (can only be applied with git apply).
• ticket-25560-phpmailer-minimal.patch​: Same as above, but with the following directories not included: docs, examples, and test. Doesn't require git apply since there's no images included.

[MattyRob]

@bpetty,

Thanks for the follow up, I'm actually on OSX and have never messed with the subversion config so I have no idea why that happened or how to change it - I'm updating via MacPorts now to see if that helps. [EDIT] The issue was that the files were all set with 755 permissions instead of 644 from the original archive I copied! And it applies to all the PHP files.

As for the missing files - I actually never intended to include the full package and asked for some guidance from a core dev in point 3 made in the original trac ticket about what needed including and what can be missed.

I'm not sure WordPress uses the language files currently so could they be skipped? The docs, examples and tests are also unnecessary IMO as anyone wanting those should be using PHPMailer directly - I don't see the need in having them within WordPress - but I don't know what the core guidelines are for third party libraries. Include what is needed or the entire library irrespective of size or make a case by case decision for the best result for WordPress.

[bpetty]

This ticket was discussed in ​today's IRC meeting.

Lots of debate about even including the extras folder (mostly concerns over htmlfilter security) or even the autoloader, so definitely minimal patch approach in mind, but this also degenerated from there into "let's just update the files in place again".

It was also already pointed out though that this will require file modifications to PHPMailer core files since they now require the autoloader in PHPMailer and SMTP classes.

So, if anyone has the time to see about attempting to create a patch with just that with the autoloader dependency cut out, it wouldn't be too hard of a patch to create, otherwise I'll get to this myself in a couple days. Also since we'll be cutting files again, it will need another thorough review for places that might break with missing library files.

[SergeyBiryukov]

FWIW, autoloader appears to be introduced as a part of a bigger commit (I didn't blame it properly in IRC):
​https://github.com/Synchro/PHPMailer/commit/d992ae6dc4a14ccce7019d8a71d52b7f70d73403

[bpetty]

Replying to SergeyBiryukov:

FWIW, autoloader appears to be introduced as a part of a bigger commit (I didn't blame it properly in IRC):
​https://github.com/Synchro/PHPMailer/commit/d992ae6dc4a14ccce7019d8a71d52b7f70d73403

Yeah, took me a minute to track down too since it was rolled up with the PSR-2 changes. Mixing code format changes with actual functional changes... *sigh*

@nacin, how about pushing to a subfolder, and still just sticking with the following files: LICENSE (since it is LGPL, not GPLv2 - probably should be included), class.phpmailer.php, class.smtp.php, and PHPMailerAutoload.php ? This would avoid any file changes from upstream, and even though we also strip extras, languages, POP3, and all other package files, it still benefits from being moved. I know the autoloader implementation is not ideal, but the only reason it's actually harmful is because we were already renaming files to begin with. This is WP's fault, not PHPMailer. It might be time to just fess up and fix it.

[MattyRob]

Replying to bpetty:

@nacin, how about pushing to a subfolder, and still just sticking with the following files: LICENSE (since it is LGPL, not GPLv2 - probably should be included), class.phpmailer.php, class.smtp.php, and PHPMailerAutoload.php ?

+1 to that from me. If I can draw every bodies attention to the 3 questions I raised when opening the ticket - I believe this proposed solution provides a reasonable answer to 1 and 2 and answers question 3 with a reasonable response. We'd be including the files we need for WordPress, not any extra files that are probably unnecessary and might pose a security risk.

[nacin]

I don't mind moving files around if it makes sense. And, really, fine with LICENSE if we move it to a directory.

The problem with autoloading is that SPL can still be disabled in PHP 5.2, and considering we've seen installs with hash disabled, I'm not liking our chances of this blowing up some sites. This would be the first usage of SPL in core — we don't use any classes, interfaces, or functions currently. The closest we get is ArrayAccess, which is a non-SPL interface that ArrayObject implements.

I doubt they will accept a PR to remove the autoloader check because it would be trivial to subclass PHPMailer and just not call the parent constructor. It seems forced to me, what they're doing, but I guess it is for backwards compatibility reasons, so I can only complain so much. So we may just need to remove those lines. Considering it is basically the entire constructor, this is not (yet) difficult to maintain. It'd be our only hack, and because if it accidentally slips back in during a future update to the library, it'd fatal error in testing.

So if we don't ship the autoloader, we're back down to the same two files we already have. And since they're no longer requiring class.smtp.php, we no longer need to hack it up for the purposes of hyphens. Though I guess we'll need to add a require line to the constructor.

[MattyRob]

@nacin,

Having just spent the last few minutes reading what SPL actually is I submit to your superior knowledge and understand your concerns. The AutoLoader calls and SPL function as does the PHPMailer class file.

I think we should avoid branching the PHPMailer code if possible. Maybe we should consider delaying incorporating this ticket until PHP 5.3 is a requirement for WordPress especially when the current mail system is working well. (Presuming PHP 5.3 resolves the concerns with SPL)

[bpetty]

Latest patch takes all discussion up to this point into consideration, specifically, it *only* updates class-phpmailer.php and class-smtp.php in place while post-patching files to remove the autoloader and manually include class-smtp.php as necessary.

It passes ​unit tests. I've also manually tested this by hand with Bluehost hosting with the given headers delivered ('To' and delivery path headers stripped):

Subject: [Test Site] Password Reset
X-PHP-Originating-Script: 2648:class-phpmailer.php
Date: Tue, 18 Feb 2014 21:02:03 +0000
From:  WordPress <[email protected]>
Message-ID: <[email protected]>
X-Priority: 3
X-Mailer: PHPMailer 5.2.7 (https://github.com/PHPMailer/PHPMailer/)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
X-Identified-User: {:box573.bluehost.com:devbryan:dev-bryanpetty.com} {sentby:program running on server}

[bpetty]

Post-patch changes to official PHPMailer are isolated here:
​https://github.com/tierra/wordpress/commit/44d21c14b

[nacin]

In 27385:

Update PHPMailer to 5.2.7 from 5.2.4.

Includes two trivial modifications for WordPress:

• Doesn't use the autoloader, so the check to enforce the autoloader from the constructor is removed.
• Requires class-smtp.php for backwards compatibility with direct (non-wp_mail()) uses of PHPMailer, as the autoloader isn't used.

props bpetty.
fixes #25560.

[rocksfrow]

#27686 was marked as a duplicate.

[rocksfrow]

What is the status with this? I am experiencing dkim failures because WordPress is still using PHPMailer 5.2.4. I accidentally created a duplicate ticket but have now closed it referencing this one. https://core.trac.wordpress.org/ticket/27686#comment:1

Can you please advise when I can expect this to make it to my production WordPress?

I am currently manually replacing class-phpmailer.php and adding PHPMailerAutoload.php.

It is that easy of a fix, why is it taking 6 months to make it to production?

Please advise ASAP. Thank you!!!