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: | 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:
- Instantiates a
PHPMailerinstance - Sets default values (e.g. "from" headers, charset, content type etc. )
- Parses the passed arguments and feeds it to the
PHPMalierinstance. - Executes a "pre-send" routine, (e.g. triggering hooks
wp_mail_from,phpmailer_initetc) - Sends the e-mail
The attached patch does a number of things:
- Defines a
WPMailerclass ( a child ofPHPMailer) - Defines a
WPMailerFactoryclass which creates an instance with appropriate default values - Defines 'helper' methods which do the 'heavy lifting' of (3) above
- Overrides the preSend method of PHPMailer to execute the 'pre-send routine' present in
wp_mail()(i.e. (4) above) - 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)
Change History (19)
#3
@
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
@
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).
#6
@
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
@
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
@
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.
#12
@
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
@
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
@
6 years ago
- Keywords needs-refresh added
- Milestone → Future Release
The patch here needs to be refreshed.
#15
@
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:
- Adds
WPMailer, which extendsWP_PHPMailerand owns the setup work moved out ofwp_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. - Adds
WPMailerFactory::getMailer(), which returns a configuredWPMailerapplying the same defaultswp_mail()applies, including theis_email()based validator. - Moves the
phpmailer_initaction intoWPMailer::preSend(). This is what makes a mailer behave identically when driven outside ofwp_mail(), and it means the test double has to extendWPMailerfor the action to fire during tests. - 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. WPMailerextendsWP_PHPMailerrather thanPHPMailer\PHPMailer\PHPMailerso 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
![(please configure the [header_logo] section in trac.ini)](/chrome/site/your_project_logo.png)
#29673 was marked as a duplicate.