Make WordPress Core

Opened 11 years ago

Closed 11 years ago

#25560 closed enhancement (fixed)

Update to PHPMailer 5.2.7

Reported by: mattyrob's profile MattyRob Owned by: nacin's profile nacin
Milestone: 3.9 Priority: normal
Severity: normal Version: 3.7
Component: External Libraries Keywords: 3.9-early has-patch
Focuses: Cc:

Description (last modified by SergeyBiryukov)

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.

Attachments (6)

25560.diff (204.1 KB) - added by MattyRob 11 years ago.
25560v2.diff (269.1 KB) - added by MattyRob 11 years ago.
25560v3.diff (500.4 KB) - added by MattyRob 11 years ago.
ticket-25560-phpmailer-full-binary.patch (764.1 KB) - added by bpetty 11 years ago.
This is a binary patch (contains images), and can only be applied with "git apply".
ticket-25560-phpmailer-minimal.patch (450.1 KB) - added by bpetty 11 years ago.
ticket-25560-phpmailer.5.patch (263.0 KB) - added by bpetty 11 years ago.

Change History (29)

@MattyRob
11 years ago

#1 @bpetty
11 years ago

  • Cc bpetty added
  • Milestone changed from Awaiting Review to 3.8

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.

#2 @MattyRob
11 years ago

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
11 years ago

#3 @MattyRob
11 years ago

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

Last edited 11 years ago by SergeyBiryukov (previous) (diff)

#4 @SergeyBiryukov
11 years ago

  • Description modified (diff)

#5 @bpetty
11 years ago

  • Keywords 3.9-early added
  • Milestone changed from 3.8 to Future Release

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.

#6 @MattyRob
11 years ago

  • Keywords dev-feedback 2nd-opinion removed

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.

Last edited 11 years ago by MattyRob (previous) (diff)

@MattyRob
11 years ago

#7 @SergeyBiryukov
11 years ago

  • Version changed from trunk to 3.7

#8 @MattyRob
11 years ago

  • Keywords has-patch added

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
11 years ago

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

#9 @bpetty
11 years ago

  • Milestone changed from Future Release to 3.9

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:

#10 @MattyRob
11 years ago

@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.

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.

Version 0, edited 11 years ago by MattyRob (next)

#11 @bpetty
11 years ago

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.

#12 follow-up: @SergeyBiryukov
11 years ago

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

#13 in reply to: ↑ 12 ; follow-up: @bpetty
11 years ago

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.

#14 in reply to: ↑ 13 @MattyRob
11 years ago

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.

#15 @nacin
11 years ago

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.

#16 @MattyRob
11 years ago

@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)

#17 @bpetty
11 years ago

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 <wordpress@dev-bryanpetty.com>
Message-ID: <63c9cac5c6651a8847fcd142af7b42a3@dev-bryanpetty.com>
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}

#18 @bpetty
11 years ago

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

This ticket was mentioned in IRC in #wordpress-dev by tierra. View the logs.


11 years ago

#20 @nacin
11 years ago

  • Owner set to nacin
  • Resolution set to fixed
  • Status changed from new to closed

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.

#21 @rocksfrow
11 years ago

#27686 was marked as a duplicate.

#22 @rocksfrow
11 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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!!!

#23 @ocean90
11 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed
Note: See TracTickets for help on using tickets.