Opened 11 years ago
Closed 11 years ago
#25560 closed enhancement (fixed)
Update to PHPMailer 5.2.7
Reported by: | MattyRob | Owned by: | 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 )
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)
Change History (29)
#2
@
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.
#3
@
11 years ago
This patch also contains some of the code from an associated ticket:
http://core.trac.wordpress.org/ticket/18493
#5
@
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
@
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.
#8
@
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?
#9
@
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:
- 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
, andtest
. Doesn't requiregit apply
since there's no images included.
#10
@
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. [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.
#11
@
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:
↓ 13
@
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:
↓ 14
@
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
@
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
@
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
@
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
@
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
@
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
@
11 years ago
- Owner set to nacin
- Resolution set to fixed
- Status changed from new to closed
In 27385:
#22
@
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!!!
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.