Make WordPress Core

Opened 12 years ago

Last modified 38 hours ago

#29513 new enhancement

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

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

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 12 years ago.
wpmail.29513-2.diff​ (21.1 KB ) - added by stephenharris 11 years ago.
Patch ported to 4.2
29513-3-wpmail.3.diff​ (19.6 KB ) - added by stephenharris 10 years ago.
Refreshed patch

Download all attachments as: .zip

Change History (19)

#1 @stephenharris
12 years ago

  • Keywords 2nd-opinion has-patch added

#2 @SergeyBiryukov
12 years ago

#29673 was marked as a duplicate.

#3 @wonderboymusic
12 years ago

  • Milestone Awaiting Review → 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.


11 years ago

#5 @jorbin
11 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
11 years ago

Patch ported to 4.2

#6 @stephenharris
11 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.


11 years ago

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


11 years ago

#9 @meitar
11 years 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
11 years 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
10 years ago

#36751 was marked as a duplicate.

@stephenharris
10 years ago

Refreshed patch

#12 @stephenharris
10 years 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

#13 @drzraf
6 years ago

Still an acute problem and the only way to process outgoing email attachments which is a major issue.

Patch does not apply anymore.

#14 @desrosj
6 years ago

  • Keywords needs-refresh added
  • Milestone → Future Release

The patch here needs to be refreshed.

#15 @SirLouen
14 months ago

  • Keywords needs-refresh removed

I think that a subclass will be a bad idea for the future, this will generate a major dependency with that library, and I see no plans to internally maintain it anytime soon.

Although, wp_mail is currently a pluggable, so technically it would never matter much, but still keeping such will provoke a painful deprecation cycle. We will be sticking to the lifecycle of PHPMailer, which is relatively different in other libraries.

Instead, I would suggest creating an Interface such as WP_Mailer_Interface, which could be easily displaced if needed. But still provide an object that can be easily manipulated before sending as this report reviews.

Before further working on this patch, more should be discussed around this topic, and we might explore all options including the one proposed in #49661

This ticket was mentioned in ​PR #14246 on ​WordPress/wordpress-develop by ​skikken.


38 hours ago
#16

  • Keywords has-unit-tests added

This ticket asks for the heavy lifting of wp_mail() to be moved out of the function and into a mailer class, so that plugins can obtain a mailer directly and reach PHPMailer methods that wp_mail() does not expose (for example addStringAttachment()), while still triggering the same hooks and defaults.

wp_mail() currently does five things inline: it instantiates PHPMailer, applies the default values, parses the arguments and feeds them to the instance, runs the pre-send routine, and sends. Only the pre-send routine and the send are reachable from outside, and phpmailer_init fires from inside the function, so a plugin that sends through its own mailer does not get the same behaviour.

Current state on trunk: the class the ticket proposes does not exist. Core does already ship a PHPMailer subclass, WP_PHPMailer (@since 6.8.0), but it only overrides the internationalization method, so the setup work this ticket is about is still entirely inside wp_mail().

This PR:

  1. Adds WPMailer, which extends WP_PHPMailer and owns the setup work moved out of wp_mail(): resetting stale state, parsing the headers, resolving the From address and name, and populating the recipients, subject, body, content type, charset, custom headers, attachments, and embeds.
  2. Adds WPMailerFactory::getMailer(), which returns a configured WPMailer applying the same defaults wp_mail() applies, including the is_email() based validator.
  3. Moves the phpmailer_init action into WPMailer::preSend(). This is what makes a mailer behave identically when driven outside of wp_mail(), and it means the test double has to extend WPMailer for the action to fire during tests.
  4. Refactors wp_mail() to delegate the setup and the send to the mailer, keeping every filter and action in place with the same arguments and the same return values.

Back-compatibility notes for review:

  • wp_mail() still uses $GLOBALS['phpmailer'] when it already holds a PHPMailer instance, and only creates one through the factory when it does not.
  • The setup routine is deliberately static and takes the mailer as its first argument, so a mailer supplied by a plugin through the global is configured exactly as before rather than requiring it to be a WPMailer. Without this, replacing the global with a plain PHPMailer or a plugin's own subclass, which is how many SMTP plugins work, would fail.
  • WPMailer extends WP_PHPMailer rather than PHPMailer\PHPMailer\PHPMailer so that the translated error strings and the locale switching behaviour are preserved.

To confirm the refactor is behaviour preserving, the generated output was compared against trunk across a matrix of cases (plain, multiple and RFC 2822 recipients, HTML content type, From headers with and without a space, cc/bcc/reply-to, custom headers, attachments including named ones, embeds, a long quoted-printable line, a charset header, and state bleed between two consecutive sends). The assembled message, the mailer properties, the order of the hooks that fire, and the arguments passed to wp_mail_succeeded and wp_mail_failed are identical.

Use of AI Tools

AI assistance: Yes
Tool(s): Claude
Model(s): Sonnet
Used for: test cases, code review

Note: See TracTickets for help on using tickets.