WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 6 weeks ago

Last modified 3 weeks ago

#41750 closed enhancement (fixed)

Update PHPMailer to 6.x

Reported by: Synchro Owned by: desrosj
Milestone: 5.5 Priority: normal
Severity: normal Version:
Component: External Libraries Keywords: has-patch early has-dev-note
Focuses: administration, performance Cc:

Description

PHPMailer 5.2 has now been deprecated and PHPMailer 6.0 has been released. Minimum requirements have been increased to PHP 5.5 and it now has a namespace, which will require changes in WP's use of it.

Attachments (11)

41750.diff (243.6 KB) - added by SergeyBiryukov 11 months ago.
41750.2.diff (242.1 KB) - added by desrosj 11 months ago.
41750.3.diff (362.3 KB) - added by donmhico 6 months ago.
PHPMailer 6.1.4
41750.4.patch (439.2 KB) - added by ayeshrajans 6 months ago.
41750-phpcs-fixes.patch (1.1 KB) - added by ayeshrajans 6 months ago.
41750.6.patch (406.7 KB) - added by ayeshrajans 6 months ago.
https://travis-ci.org/Ayesh/wordpress-develop/builds/647882845
41750.4.diff (409.2 KB) - added by desrosj 3 months ago.
41750.5.diff (409.6 KB) - added by desrosj 3 months ago.
41750-7-obsolete.patch (410.0 KB) - added by ayeshrajans 3 months ago.
Updated full patch, after applying the new-line character fixes: https://github.com/Ayesh/wordpress-develop/commit/9af56987a639a6c1d6c7f648d292379be54b6aca
41750.8.patch (410.5 KB) - added by ayeshrajans 3 months ago.
41750.6.diff (410.7 KB) - added by desrosj 2 months ago.

Change History (73)

#1 @netweb
3 years ago

Related: #40472 Update PHPMailer to 5.2.25

cc @MattyRob

#3 @johnbillion
3 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to maybelater
  • Status changed from new to closed

As the WordPress project is a long way from requiring PHP 5.5 as a minimum, updating to PHPMailer 6 isn't on the cards, unless somebody wants to maintain a fork that's compatible with PHP 5.2.

Closing as maybelater. We'll get there eventually.

#4 @johnbillion
3 years ago

  • Version trunk deleted

#5 @ocean90
21 months ago

#45276 was marked as a duplicate.

#6 @SergeyBiryukov
11 months ago

  • Keywords needs-patch added
  • Milestone set to Future Release
  • Resolution maybelater deleted
  • Status changed from closed to reopened

Reopening. As of WordPress 5.2, PHP 5.6.20+ is required, so if that was the remaining limitation in upgrading, it shouldn't be a blocker anymore.

#7 @SergeyBiryukov
11 months ago

#45822 was marked as a duplicate.

#8 @SergeyBiryukov
11 months ago

  • Keywords has-patch needs-testing needs-dev-note added; needs-patch removed
  • Milestone changed from Future Release to 5.3

41750.diff seems to work in my testing, bumping PHPMailer to the latest release, 6.0.7 at the moment.

Per the Stack Overflow thread and upgrading notes, the biggest change for us is probably the introduction of the PHPMailer\PHPMailer\PHPMailer namespace:

The reason this class ends up with this "triple name" is because it's the PHPMailer class, in the PHPMailer project, owned by the PHPMailer organisation. This allows it to be differentiated from other forks of PHPMailer, other projects by the PHPMailer organisation, and other classes within the project.

With enough testing and a dev note, I think this can still make it into 5.3.

@desrosj
11 months ago

#9 @desrosj
11 months ago

41750.2.diff updates the MockPHPMailer class to extend the PHPMailer class at the correct new namespace.

Also, it looks like PHPMailer now includes its own Exception class that is used by default. The only thing that class does is wrap exception messages in HTML. 41750.2.diff adds a preceding \ to all Exception occurrences to use the globally namespaced Exception instead.

There are still some unit test failures that need to be investigated.

#10 @Synchro
11 months ago

FWIW, I'm, very happy to see this update going ahead, thanks for getting it into WP!

Version 0, edited 11 months ago by Synchro (next)

#11 @SergeyBiryukov
11 months ago

#48072 was marked as a duplicate.

#12 @Hareesh Pillai
11 months ago

  • Component changed from Mail to External Libraries

#13 @davidbaumwald
11 months ago

@desrosj Is this something that make it in for Beta 1 of version 5.3 tomorrow, or do the test failures still need investigating?

#14 @desrosj
11 months ago

  • Milestone changed from 5.3 to 5.4

I think we should play this one safe and punt. @SergeyBiryukov if you feel otherwise, feel free to move it back.

#15 follow-up: @Synchro
11 months ago

I'm not sure when your deadline is for 5.3, but there are some reasonably important bug fixes in the pipeline for PHPMailer.

#16 in reply to: ↑ 15 @jrf
10 months ago

  • Keywords early added

Replying to Synchro:

I'm not sure when your deadline is for 5.3, but there are some reasonably important bug fixes in the pipeline for PHPMailer.

@Synchro Hi Marcus, sorry that nobody replied to you before, but WP 5.3 has already gone into feature freeze.

An upgrade like this will need plenty of testing as the impact could be large if not done 100% correctly, not just for WP itself, but also for plugins/themes using the functionality provided via WP, so should preferably be made early in a release cycle.

#17 @nasiralamreeki
9 months ago

WordPress really needs to take more seriously updating external libraries. Years between updating libraries from upstream is unacceptable. There’s tremendous benefits to getting these libraries updated faster.

We can’t wait for theme and plugin developers forever to test. Sometimes we have to push the web forward by pushing ahead with changes even if they break themes and plugins as this in turn will push developers to update their code.

We pushed Gutenberg ahead even though most major themes and plugins weren’t ready for it.

#18 @davidbaumwald
7 months ago

  • Keywords needs-refresh added

The latest patch no longer applies cleanly, so it needs-refresh. @desrosj given that, is this doable for 5.4?

This ticket was mentioned in Slack in #core by david.baumwald. View the logs.


7 months ago

#20 @SergeyBiryukov
7 months ago

  • Owner set to SergeyBiryukov
  • Status changed from reopened to accepted

#21 @SergeyBiryukov
7 months ago

#49298 was marked as a duplicate.

@donmhico
6 months ago

PHPMailer 6.1.4

#22 @donmhico
6 months ago

  • Keywords needs-refresh removed

41750.3.diff integrates PHPMailer 6.1.4 . https://github.com/PHPMailer/PHPMailer/tree/v6.1.4. As @davidbaumwald mentioned, there are failing unit tests that needs more investigating. I'll try to look more into it, but anyone please feel free to jump in.

This ticket was mentioned in Slack in #core by david.baumwald. View the logs.


6 months ago

#24 @ayeshrajans
6 months ago

The tests fail because the PHPMailer/Exception class is not loaded. This is new from the current version, so we need to manually require this class in addition to the class-phpmailer.php file.

This patch takes a slightly new approach:

  1. Instead of updating src/wp-includes/class-smtp.php and class-phpmailer.php files, this patch creates a new directory src/wp-includes/PHPMailer/ and adds verbatim PHPMailer.php, SMTP.php, and POP3.php classes. There is a new exception class at Exception.php, which is included as well.
  1. To maintain BC, existing src/wp-includes/class-smtp.php, class-phpmailer.php, and class-pop3.php files require the new files.
  1. PHPMailer uses the PHPMailer\PHPMailer\Exception as the exception class. This is updated in pluggable.php file. We use Fully-Qualified Class Names (FQCN) here because using FQCN doesn't pollute the header of pluggable.php class.
  1. In this patch, all tests related to PHPMailer and SMTP are passing. However, the new POP3.php class is not exactly an easy migrate, nor I have personally done it before. Help welcome.
  1. It also updated the mail mockup file to use the new file paths.

So far, this patch fixes all but one test related to mail-fetching from ./wp-mail.php file.

This will break the phpcs.xml ignore list because PHPMailer is in a new path. This can be fixed with a patch similar to that updates the phpcs.xml file. It is also attached to this ticket.

Future plans:

  1. We will need more test coverage because right now, all PHPMailer-thrown exceptions are silently ignored. This is a bad practice, and ideally should be handled in a layer above.
  1. The POP3 functionality doesn't appear to have enough tests.

CI results: https://travis-ci.org/Ayesh/wordpress-develop/jobs/647470540

#25 @ayeshrajans
6 months ago

@Synchro you are the author of the library :) Please provide any input you have, specially might be helpful for the POP3 usage. Thank you.

#26 @desrosj
6 months ago

If we are going to move the library into its own directory, then we should consider requiring it with Composer and using the grunt build process to copy the files into the correct location.

The old files should also include a _deprecated_file call to alert developers the files have moved.

#27 @desrosj
6 months ago

Hit submit too soon.

This would be the first library we include this way. We would need to confirm that the build server has Composer installed and configured correctly.

It would make updating the library going forward much easier, though.

#28 @ayeshrajans
6 months ago

It would be lovely if we can get the build server involved to fetch library. We might as well be able to incorporate composer autoloader (with composer.json config.vendor-dir set to src/wp-includes or another way.

To be honest, I think this would derail this issue a bit outside because the majority of the work would be composer/grunt changes as opposed to modifying WordPress core to adapt to PHPMailer 6.

Composer integration is something I personally would love to see in WP, and I would gladly dedicate a few hours every week to volunteer on this ♥️.

#29 @Synchro
6 months ago

@ayeshrajans your approach sounds pretty sensible to me.

You only need to use PHPMailer\PHPMailer\Exception if you're explicitly using exceptions (they are turned off by default). You do still need to have the Exception class loaded as even with exceptions disabled it is used internally. So long as it's disabled, you shouldn't have any uncaught exceptions. Do you have examples where this is a problem?

Frankly I feel a little unwell at the thought that some might still be using the POP3 library! Nobody should be using POP-before-SMTP for auth any more - the last time I used it was about 30 years ago. Indeed it doesn't have any test coverage - it was originally a third party library that was donated to the project, and it's a difficult thing to test as it requires coordination across protocols. Do you have any visibility on whether it's being used at all?

Composer, at least in a build system, would seem essential, but WP obviously has a lot of BC issues to consider, and I know very little about the WP codebase.

#30 @ayeshrajans
6 months ago

Thanks for you reply @Synchro .

You only need to use PHPMailer\PHPMailer\Exception if you're explicitly using exceptions (they are turned off by default). You do still need to have the Exception class loaded as even with exceptions disabled it is used internally. So long as it's disabled, you shouldn't have any uncaught exceptions. Do you have examples where this is a problem?

The way WordPress deals with the exceptions is by, well, ignoring them. For example, some tests fail where it tries to set an invalid email address, and the expected exception is not the same as PHPMailer 6 throws. As of now, all PHPMailer-specific exceptions are abstracted. Fatal errors are thrown with WP_Error which throws a WPDieException. As of now, the exceptions are closed within wp_mail function.

Frankly I feel a little unwell at the thought that some might still be using the POP3 library! Nobody should be using POP-before-SMTP for auth any more - the last time I used it was about 30 years ago. Indeed it doesn't have any test coverage - it was originally a third party library that was donated to the project, and it's a difficult thing to test as it requires coordination across protocols. Do you have any visibility on whether it's being used at all?

Thank you - I honestly doubt if there is a meaningful real-life usage of this mail-fetch feature. I'd be overjoyed if this feature was moved to a contributed plugin and kept WP core clean.

Composer, at least in a build system, would seem essential, but WP obviously has a lot of BC issues to consider, and I know very little about the WP codebase.

Every file in wp-includes is included on-demand with hardcoded file names, and WP does not necessarily follow PSR-4 or PSR-0 naming conventions either. Realistically, we have a long way to go to support composer fully throughout WordPress.

#31 @Synchro
6 months ago

If you don't enable exceptions in PHPMailer, they shouldn't leak out because they are either not thrown (for example here, where the function returns false instead) or are caught internally (like here).

Are you saying that because you have not loaded PHPMailer's own exception class, it actually dies with an unknown class error, as opposed to throwing an exception that's uncaught?

#32 @ayeshrajans
6 months ago

Hi @Synchro, yes the test failures were from the undeclared PHPMailer\PHPMailer\Exception class. CI log.

Even before this patch, WordPress Opts-in to exceptions (with new PHPMailer( true )).

So this patch preserves this behavior, and catches the new PHPMailer\PHPMailer\Exception. I think this is safer because because we get the strict validation from PHPMailer, and any unhandled exceptions from WordPress side will rightfully bubble up.

#33 @ayeshrajans
6 months ago

I opened #49386 to receive feedback from core maintainers about deprecating Post via email feature, which WordPress uses the POP3 class for.

#34 @ayeshrajans
6 months ago

The tests pass now!

  • It does not try to change anything with POP3. The class-pop3.php file does not appear to be related to the PHPMailer library we have (not 5.2, not 6.0, it might even be unrelated). So the class-pop3.php file remains unchanged.
  • Further, wp-mail.php file is not changed either. It will continue to use the class-pop3.php file which we don't change.
  • Composer is not incorporate, because I believe we should have discuss it further as to how to implement it properly. As of now, we copy the current PHPMailer 6.1 release verbatime on files SMTP.php, PHPMailer.php, and Exception.php to src/wp-includes/PHPMailer directory.

The failing test from the last patch was not due to POP3 class. It does not look like "Post via email" feature even has tests.

The failing test was because a strpos call on a sent email (mocked) does not contain proper headers. It did, but it was mime encoded which resulted in the strpos call from failing. This patch fixes that test too, but manually decoding the mime-encoded headers.

#35 @davidbaumwald
6 months ago

  • Milestone changed from 5.4 to Future Release

This ticket is marked early for soak time, and we're past that point for 5.4. with Beta 1 landing tomorrow. This is being moved to Future Release. If any maintainer or committer feels this can be included in 5.4 or wishes to assume ownership during a specific cycle, feel free to update the milestone accordingly.

This ticket was mentioned in PR #242 on WordPress/wordpress-develop by desrosj.


3 months ago

Attempting to update PHPMailer to the latest version (currently 6.1.5).

Trac ticket:

@desrosj
3 months ago

#37 @desrosj
3 months ago

Thanks for working on this, everyone!

I looked at the latest patch, and I think that this approach should work fine. However, I uploaded 41750.4.diff with a few small changes.

Some minor adjustments:

  • Changed the version notes to 5.5.0.
  • Updated the PHPMailer library to which was released on March 14, 2020.
  • Changed the wording a bit on the deprecated file calls and copied the same sentence into the inline documentation to be more consistent.

Updating the library did introduce a test error to Tests_Mail::test_wp_mail_custom_boundaries, PHPMailer\PHPMailer\Exception: Invalid header name or value. I have not yet gotten to investigate that, but it should be a result of one of the changes in 6.1.5.

Lastly and most importantly, I am concerned that 41750.6.patch is not quite there from a backwards compatibility standpoint and would break too many sites. Even if we get the word out to plugin developers and they update their plugins, there is no guarantee that site owners actually update their plugins.

I did some exploring of the plugin directory to try and validate this and found 612 plugins that call new PHPMailer(). Only 11 had 50k+ installs, and there were a small handful of plugins that were bundling their own, updated versions of PHPMailer.

41750.4.diff re-adds a PHPMailer class in the original class-phpmailer.php file and only contains a __construct() method that will return an instance of the new, correctly namespaced class and trigger a deprecated function notice. The idea here is to prevent fatal errors when calling new PHPMailer() the "old" way (even though that way was discouraged and technically incorrect). I'm trying to think of any "gotchas" with this approach.

@desrosj
3 months ago

#38 @desrosj
3 months ago

Sorry, that last approach in 41750.4.diff doesn't actually work.

41750.5.diff uses class_alias() instead, which does work as intended. 41750.5.diff also merges the changes from 41750-phpcs-fixes.patch into a single patch, and makes some PHPCS changes to the suggested changes.

The failing test still remains: https://github.com/WordPress/wordpress-develop/pull/242/checks?check_run_id=627685741

Props to @wpscholar for testing and refining the latest patch with me.

#39 @ayeshrajans
3 months ago

Thanks for the patches, and I think we are almost ready for this 🎉.

I took @desrosj's [41750.5.diff] applied on clean master. The exception that caused the test to fail is because PHPMailer rightfully rejected the header with a new line character (\n) in the custom header.

$phpmailer->addCustomHeader( sprintf( "Content-Type: %s;\n\t boundary=\"%s\"", $content_type, $boundary ) );

The content-type header does not need this new line or the tab, so I removed these characters and . tests now pass.

(more comments to follow)

#40 @ayeshrajans
3 months ago

Please see 41750-8.patch for updated patch including few more improvements (patch list compared to 41750-7-obsolete.patch):

  1. Simplify $phpmailer->addCustomHeader to use single quotes to the double quotes don't need to be escaped:

$phpmailer->addCustomHeader( sprintf( 'Content-Type: %s; boundary="%s"', $content_type, $boundary ) );

as opposed to:

$phpmailer->addCustomHeader( sprintf( "Content-Type: %s; boundary=\"%s\"", $content_type, $boundary ) );

  1. Add exception handling for other $phpmailer->addCustomHeader calls. This brings back the old behavior of silently handling invalid values. $phpmailer->addAttachment already has similar exception handling.
  1. Use ::class constant in the deprecated class-phpmailer.php file. PHP 5.5 supports this, and can make the class names clickable in IDEs and helps static analyzers as well.

Tests: passing

Last edited 3 months ago by ayeshrajans (previous) (diff)

#41 @Synchro
3 months ago

Nice to see this is progressing! While I agree with the quoting, that's a very strange example to use: if you're setting content-type headers and boundaries via addCustomHeader, you're using PHPMailer wrong!

#42 @ayeshrajans
3 months ago

if you're setting content-type headers and boundaries via addCustomHeader, you're using PHPMailer wrong!

This put me off too, seeing WordPress meticulously going through content-type header. I suppose this is an old artifact because even if I go as far as 10000 commits, that code is still there.

This ticket was mentioned in Slack in #core by desrosj. View the logs.


3 months ago

#45 @desrosj
2 months ago

In 47902:

General: Continuing to work towards a passing PHP Compatibility scan.

This is a final pass to fix PHP compatibiilty issues in the codebase with code changes or adding phpcs:ignore comments.

With this change, all PHP compatibility warnings and errors without specific tickets have been addressed (see #49810 and #41750).

Props desrosj, johnbillion, jrf.
See #49922.

@desrosj
2 months ago

#46 @desrosj
2 months ago

  • Focuses administration performance added
  • Owner changed from SergeyBiryukov to desrosj
  • Status changed from accepted to assigned
  • Summary changed from Update PHPMailer to 6.0 to Update PHPMailer to 6.x

Thanks @ayeshrajans!

41750.6.diff is an updated patch that includes PHPMailer 6.1.6, which was a security update.

I think this is in a really good spot. I plan on committing this at some point this week (hopefully with another review from an additional seasoned contributor), and drafting a call for testing for Make Core/Make Plugins.

@ayeshrajans Let's open new tickets for all of the "using PHPMailer wrong" type issues that are found.

#47 @ayeshrajans
2 months ago

w00t w00t, thank you!
I started to watch upstream releases of PHPMailer, and will try to submit patches to WordPress to keep WordPress's copy up to date, until we eventually get Composer support. 41750.6.diff looks good to me. Thank you.

I will also open a ticket for PHPMailer usage patterns too. Thank you, I'm excited to see this finally seeing the light 🎉.

#48 @Synchro
2 months ago

It's great to see this happening! No major changes are expected in PHPMailer in the near future, 6.1.6 should be good to go.

#49 @desrosj
2 months ago

  • Milestone changed from Future Release to 5.5

Moving into the 5.5 milestone.

This ticket was mentioned in Slack in #core by david.baumwald. View the logs.


2 months ago

This ticket was mentioned in Slack in #core by desrosj. View the logs.


2 months ago

#52 @desrosj
2 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 48033:

External Libraries: Upgrade PHPMailer to version 6.1.6.

Now that WordPress Core supports PHP >= 5.6, the PHPMailer library can be updated to the latest version.

The PHPMailer files now reside in a new directory, wp-includes/PHPMailer. These files are copied verbatim from the library upstream and will make updating in the future easier. For backwards compatibility, the old files will remain and trigger deprecated file warnings.

The PHPMailer class is also now under the PHPMailer\PHPMailer\PHPMailer namespace. The PHPMailer class in the global namespace has been aliased for a seamless transition.

This upgrade also clears up a handful of PHP compatibility issues detailed in #49922.

For a full list of changes, see the PHPMailer GitHub: https://github.com/PHPMailer/PHPMailer/compare/v5.2.27...v6.1.6.

Props Synchro, SergeyBiryukov, desrosj, donmhico, ayeshrajans.
Fixes #41750.

#53 @desrosj
2 months ago

  • Keywords needs-testing removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening this for a developer note. I plan to tackle next week, but if someone else would like to create a draft, please feel free! It should focus on the high level changes in the library that plugins and site owners may need to be aware of.

#54 @Synchro
2 months ago

That's great news!

You may find the upgrading guide a useful starting point: https://github.com/PHPMailer/PHPMailer/blob/master/UPGRADING.md

and also this question & answer I wrote:
https://stackoverflow.com/questions/45940509/how-should-i-upgrade-from-phpmailer-5-2-to-6-0

#55 @ayeshrajans
2 months ago

Thanks for committing this. We finally have PHPMailer 6!
I opened #50374 to make further improvements to how we are using PHPMailer.

Last edited 2 months ago by SergeyBiryukov (previous) (diff)

#56 @SergeyBiryukov
2 months ago

In 48035:

External Libraries: Add class aliases for phpmailerException and SMTP to deprecated files to account for the new namespace.

This ensures backward compatibility with plugins using the old class-phpmailer.php or class-smtp.php files.

Follow-up to [48033].

Props Otto42.
Fixes #50379. See #41750.

#58 @SergeyBiryukov
8 weeks ago

In 48036:

Coding Standards: Exclude the whole PHPMailer directory from WPCS checks, for consistency with other external libraries in their own directory.

Follow-up to [48033].

See #41750.

#59 @desrosj
8 weeks ago

In 48058:

Build/Test Tools: Prevent double /s when including PHPMailer files.

ABSPATH includes a trailing slash already.

Follow up of [48033].
Props desrosj.
See #50377, #41750.

#60 @desrosj
6 weeks ago

  • Keywords has-dev-note added; needs-dev-note removed
  • Resolution set to fixed
  • Status changed from reopened to closed

Dev note was just published for this: https://make.wordpress.org/core/2020/07/01/external-library-updates-in-wordpress-5-5-call-for-testing/

I also compiled a list of plugins all plugins with active installs that appear to be utilizing the library in their code: https://docs.google.com/spreadsheets/d/1bYVY3mea2lpv-DA8Ic71v2QJtrRyMKY1GW2JpPGCOC4/edit?usp=sharing

I created this list based off the following three searches:

I sent a draft of an email to the plugin team for them to relay to those plugin authors as they feel appropriate.

#61 @SergeyBiryukov
4 weeks ago

In 48443:

Tests: Ignore EOL differences in email tests using multiline string assertions.

Unix vs. Windows EOL style mismatches can cause misleading failures in tests using the heredoc syntax (<<<) or multiline strings as the expected result.

Follow-up to [46612], [48033].

Props davidbaumwald.
See #31432, #41750.

#62 @SergeyBiryukov
3 weeks ago

In 48530:

Mail: Make sure the PHPMailer class is only required once if a plugin requires wp-includes/class-phpmailer.php directly.

Follow-up to [48033].

Props david.binda.
Fixes #50716. See #41750.

Note: See TracTickets for help on using tickets.