WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 7 weeks ago

#41750 accepted enhancement

Update PHPMailer to 6.0

Reported by: Synchro Owned by: SergeyBiryukov
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: External Libraries Keywords: has-patch needs-testing needs-dev-note early
Focuses: 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 (6)

41750.diff (243.6 KB) - added by SergeyBiryukov 7 months ago.
41750.2.diff (242.1 KB) - added by desrosj 7 months ago.
41750.3.diff (362.3 KB) - added by donmhico 2 months ago.
PHPMailer 6.1.4
41750.4.patch (439.2 KB) - added by ayeshrajans 8 weeks ago.
41750-phpcs-fixes.patch (1.1 KB) - added by ayeshrajans 8 weeks ago.
41750.6.patch (406.7 KB) - added by ayeshrajans 7 weeks ago.
https://travis-ci.org/Ayesh/wordpress-develop/builds/647882845

Download all attachments as: .zip

Change History (41)

#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
17 months ago

#45276 was marked as a duplicate.

#6 @SergeyBiryukov
7 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
7 months ago

#45822 was marked as a duplicate.

#8 @SergeyBiryukov
7 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
7 months ago

#9 @desrosj
7 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
7 months ago

FWIW, I'm, very happy to see this update going ahead, thanks for getting it into WP! Is there anything you need help with?

Last edited 7 months ago by Synchro (previous) (diff)

#11 @SergeyBiryukov
6 months ago

#48072 was marked as a duplicate.

#12 @Hareesh Pillai
6 months ago

  • Component changed from Mail to External Libraries

#13 @davidbaumwald
6 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
6 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
6 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
6 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
5 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
2 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.


2 months ago

#20 @SergeyBiryukov
2 months ago

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

#21 @SergeyBiryukov
2 months ago

#49298 was marked as a duplicate.

@donmhico
2 months ago

PHPMailer 6.1.4

#22 @donmhico
2 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.


2 months ago

#24 @ayeshrajans
8 weeks 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

@ayeshrajans
8 weeks ago

#25 @ayeshrajans
8 weeks 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
8 weeks 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
8 weeks 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
8 weeks 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
8 weeks 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
8 weeks 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
7 weeks 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
7 weeks 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
7 weeks 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
7 weeks 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
7 weeks 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.

Note: See TracTickets for help on using tickets.