WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 14 months ago

#29513 new enhancement

Move heavy lifting of wp_mail() to child class of PHPMailer

Reported by: stephenharris Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 4.0
Component: Mail Keywords: 2nd-opinion has-patch
Focuses: Cc:

Description

If a plug-in is sending an e-mail, the class PHPMailer has a lot of useful methods (e.g. addStringAttachment()), but these are not available when using wp_mail(), which is a requirement to work with numerous other plug-ins owing to the hooks it triggers.

wp_mail() does a number of things:

  1. Instantiates a PHPMailer instance
  2. Sets default values (e.g. "from" headers, charset, content type etc. )
  3. Parses the passed arguments and feeds it to the PHPMalier instance.
  4. Executes a "pre-send" routine, (e.g. triggering hooks wp_mail_from, phpmailer_init etc)
  5. Sends the e-mail

The attached patch does a number of things:

  1. Defines a WPMailer class ( a child of PHPMailer)
  2. Defines a WPMailerFactory class which creates an instance with appropriate default values
  3. Defines 'helper' methods which do the 'heavy lifting' of (3) above
  4. Overrides the preSend method of PHPMailer to execute the 'pre-send routine' present in wp_mail() (i.e. (4) above)
  5. Refactors wp_mail() to "operate" WPMailer() instance

The result is that developers can either use wp_mail() or $wpmailer = WPMailerFactory::getMailer() to send e-mails, and both will behave identically (in terms of default values, and hooks being triggered), while maintaining backwards compatibility.

This would also effectively close tickets #28407, #28059, #23291, #15539 , #11376 and maybe others.

Remarks

Why just not use phpmailer_init?
This hook is very useful, but offers no context in which wp_mail() is called. As an example, suppose a plug-ins sends an e-mail with an "on-the-fly" purchase receipt attached. At phpmailer_init I don't know the purchase ID from which to generate and attach the receipt.

Class/method naming standards
I've used PHPMailer's naming standards which I understand conflicts slightly with WordPress' naming standards. A future iteration of this patch could well change this if that is deemed best.

Global $phpmailer
The global $phpmailer is redundant, as the factory creates a fresh instance for each request. Or at least it would. The only reason the patch still uses this global, is that all the relevant unit tests pass without any further changes. Subject to this ticket being accepted in principle, these tests should be updated along with the patch.

Backwards compatability
Assuming wp_mail() hasn't been overriden by a plug-in/theme, then the is no change in behaviour. If it has been overridden, it's clear from the original function that the $_GLOBAL['phpmailer'] should not be expected to exist, nor even the required classes to be loaded. As such they can be expected to operate independently of the changes made here, which are non-destructive.

Uni tests
For me, the mail group unit tests pass with 1 skipped. For some reason some tests failed (e.g. Tests_DB::test_bail()), but these failed even without this patch.

Attachments (3)

refactor-wp-mail.diff (20.7 KB) - added by stephenharris 3 years ago.
wpmail.29513-2.diff (21.1 KB) - added by stephenharris 2 years ago.
Patch ported to 4.2
29513-3-wpmail.3.diff (19.6 KB) - added by stephenharris 14 months ago.
Refreshed patch

Download all attachments as: .zip

Change History (15)

#1 @stephenharris
3 years ago

  • Keywords 2nd-opinion has-patch added

#2 @SergeyBiryukov
3 years ago

#29673 was marked as a duplicate.

#3 @wonderboymusic
3 years ago

  • Milestone changed from Awaiting Review to Future Release

This looks interesting - if someone decides to pick this up, could be 4.1

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


2 years ago

#5 @jorbin
2 years ago

  • Keywords needs-refresh added; has-patch removed

Patch needs to be refreshed. Also needs a good review including seeing if there was a good reason for not using $phpmailer more in the begining (I doubt the commit or tickets will tell us much, but at least finding them and linking them here would be helpfull).

@stephenharris
2 years ago

Patch ported to 4.2

#6 @stephenharris
2 years ago

  • Keywords has-patch added; needs-refresh removed

Latest patch passes all unit tests (with 47 skipped). All mail component tests pass.

@jorbin The patch still uses the global $phpmailer. I personally think its a good opportunity to get rid of a global but happy either way. I'll dig a little re. why originally a new instance wasn't created for each e-mail, but I suspect it might be concerns relating to performance. Perhaps constructing a PHPMailer instance is expensive(?). I'll do some benchmark tests.

This ticket was mentioned in Slack in #buddypress by boone. View the logs.


2 years ago

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


21 months ago

#9 @meitar
19 months ago

IIUC, this patch will enable the use of PHPMailer::addStringAttachment() for plugins that make use of the wp_mail filter hook? That's something I need for a plugin I'm working on (see here), so +1 from me.

#10 @stephenharris
17 months ago

@meitar Sorry, seemed to have missed the notification for you're reply. But yes, the original motivation of the ticket was to expose more PHPMailer methods.

#11 @stephenharris
17 months ago

#36751 was marked as a duplicate.

@stephenharris
14 months ago

Refreshed patch

#12 @stephenharris
14 months ago

The global $phpmailer was introduced when PHPMailer was introduced in this change [3862]. I can't determine as to why it was implemented like that, but in the interest of backwards compatibility I'm tempted to say leave it.

What I think we should probably consider though, is destroying and recreating a new PHPMailer instance each time. This would fix #33972.

In addition to the tickets mentioned in the description, this would also provide a work-around for #31775

Note: See TracTickets for help on using tickets.