Make WordPress Core

Opened 9 years ago

Closed 4 years ago

Last modified 4 years ago

#35069 closed enhancement (fixed)

Allow short-circuiting wp_mail

Reported by: dvankooten's profile DvanKooten Owned by: johnbillion's profile johnbillion
Milestone: 5.7 Priority: low
Severity: normal Version:
Component: Mail Keywords: has-patch has-unit-tests has-dev-note
Focuses: Cc:

Description

Much like schedule_event and several other functions in the WP codebase, it would be cool if wp_mail could be short circuited by returning a falsey value as the return value of the wp_mail filter.

The function is already pluggable right now, but I think this would make for a neat addition to the default function as this will allow plugins to perform things as queueing emails to be processed in the background (as sending email tends to be slow).

It will also allow for easier disabling of all emails in local / staging / development environments.

Attachments (7)

35069.diff (479 bytes) - added by DvanKooten 9 years ago.
Check if return value of filter is falsey and return early if so.
35069-2.diff (673 bytes) - added by jtsternberg 9 years ago.
35069.2.diff (1.3 KB) - added by swissspidy 9 years ago.
35069.3.diff (1.6 KB) - added by swissspidy 9 years ago.
35069.4.diff (1.7 KB) - added by Mte90 7 years ago.
use null
35069.5.diff (1.7 KB) - added by birgire 7 years ago.
35069.6.patch (1.8 KB) - added by ayeshrajans 5 years ago.
Rerolling the patch against 5.4 branch. Tests: https://travis-ci.org/Ayesh/wordpress-develop/builds/647197094 (passing)

Download all attachments as: .zip

Change History (43)

@DvanKooten
9 years ago

Check if return value of filter is falsey and return early if so.

#1 @swissspidy
9 years ago

  • Component changed from General to Mail

#2 @SergeyBiryukov
9 years ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 4.5

FWIW, it's already possible, this would prevent the email from being sent:

function wp35069_skip_sending_email( $args ) {
	$args['message'] = '';

	return $args;
}
add_filter( 'wp_mail', 'wp35069_skip_sending_email' );

However, 35069.diff would save some processing time and seems like a good idea.

#3 @swissspidy
9 years ago

  • Keywords needs-unit-tests added
  • Version trunk deleted

What does currently happen when you pass an empty array?

#4 @DvanKooten
9 years ago

@swissspidy The current code only uses the returned array values if they are actually set. Otherwise, it will use the passed arguments.

if ( isset( $atts['to'] ) ) {
        $to = $atts['to'];
}

if ( isset( $atts['subject'] ) ) {
        $subject = $atts['subject'];
}

etc..

So emails can be disabled by explicitly returning an empty to or message field but that will still run the entire function, setting up the PHPMailer object etc.

Edit:

One thing I'm in doubt over is the return value when short-circuiting. Right now, I've set it to false but that's not ideal when queueing emails.

Last edited 9 years ago by DvanKooten (previous) (diff)

#5 @swissspidy
9 years ago

Thanks, makes sense.

The return value in the DocBlock would probably need to be reflect that change:

@return bool Whether the email contents were sent successfully.

One thing I'm in doubt over is the return value when short-circuiting. Right now, I've set it to false but that's not ideal when queueing emails.

Perhaps a new filter could be used after the wp_mail filter?

$mail_short_circuit = apply_filters( 'wp_mail_short_circuit', false, … )

If true, it means the mail was successfully sent/scheduled by the function hooking into that. false means proceed with using PHPMailer.

#6 @DvanKooten
9 years ago

What about checking if the returned filter value is an array (which it expects right now) and if it is, proceeding with the function as it stands. If it's not an array, return early using the return value of the filter.

if( ! is_array( $atts ) ) { 
   return $atts;
}

The process could be reversed by checking for a boolean, but as the rest of the function uses the value as if it's an array now anyway I think that would make less sense.

That would be pretty much in line with what the other short-circuited functions do as well.

@jtsternberg
9 years ago

#7 @jtsternberg
9 years ago

[35069-2.diff](https://core.trac.wordpress.org/attachment/ticket/35069/35069-2.diff) Checks for an array return from the wp_mail filter and bails if the return is not an array. I think this approach seems the most sane to me.

@swissspidy
9 years ago

#8 @swissspidy
9 years ago

  • Keywords has-unit-tests added; needs-unit-tests removed

#9 @swissspidy
9 years ago

Any objections to this? Would something break?

#10 @johnbillion
9 years ago

  • Focuses performance removed
  • Priority changed from normal to low

I don't feel too strongly either way about this. It introduces another filter that we'll need to maintain back-compat for, but that's not a huge issue.

When the short-circuit kicks in, it should explicitly return boolean false, not $atts. Unless there's a reason not to?

#11 @DvanKooten
9 years ago

@johnbillion Isn't that what WP is all about? ;)

My $.02 is that it's pretty much in line with other core functions like wp_schedule_event and how the WP_HTTP API works using pre_http_request. I think it makes sense for expensive operations to offer such a thing.

Also, it's already possible to do this by passing the $atts array with empty values but that still runs a bunch of code to instantiate the PHPMailer object.

That also gets us to your second question as $atts is treated as if an array after the filter runs, without any checking. This would make it easy to just bail on any non-array value.

#12 @ericlewis
9 years ago

Replying to swissspidy:

Perhaps a new filter could be used after the wp_mail filter?

$mail_short_circuit = apply_filters( 'wp_mail_short_circuit', false, … )

If true, it means the mail was successfully sent/scheduled by the function hooking into that. false means proceed with using PHPMailer.

I like this. It makes the control flow more obvious.

An alternative mailer should be allowed to fail and return its failure as false, as wp_mail() does.

Perhaps we can filter on null. If anything non-null comes back, short-circuit the function and return the filtered value.

We can use the pre_* filter naming convention here, since we'll be returning the filtered value:

<?php
$pre_wp_mail = apply_filters( 'pre_wp_mail', null, $atts );
if ( ! is_null( $pre_wp_mail ) ) {
    return $pre_wp_mail;
}

#13 @swissspidy
9 years ago

  • Keywords needs-patch needs-unit-tests added; has-patch has-unit-tests removed

+1 for the suggested pre_wp_mail filter

@swissspidy
9 years ago

#14 @swissspidy
9 years ago

  • Keywords has-patch has-unit-tests added; needs-patch needs-unit-tests removed

35069.3.diff adds a pre_wp_mail filter.

Passing a non-false value will short-circuit wp_mail(), returning that value instead.

When the short-circuit kicks in, it should explicitly return boolean false, not $atts. Unless there's a reason not to?

If you're building something custom to send emails/notifications, wp_mail() not returning false will still indicate that submission was successful. I'm sure lots of plugins rely on the return value of wp_mail() and do all sorts of stuff if it returns false.

Many other pre_* filters just return the filtered value as well, so this adds some consistency.

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


9 years ago

#16 @jorbin
9 years ago

  • Keywords early added
  • Milestone changed from 4.5 to Future Release

Something like this needs soak time. Punting from 4.5. Let's try and get it early in 4.6

#17 @jtsternberg
8 years ago

Sufficiently soaked? How about in 4.8?

#18 @lukecavanagh
8 years ago

@DvanKooten

Seems like pushing to add pre_wp_mail filter for WP 4.8 would be a solid addition.

#19 @rmccue
8 years ago

  • Keywords early removed

@jtsternberg "Soak" means time in core between commit and release (i.e. commit early in the cycle to allow adequate time for testing).

@swissspidy If you want to own this for 4.8, you seem like the best candidate. :) Removing the early tag, since it's not helping here, and the 4.8 milestone is now open.

#20 @swissspidy
8 years ago

  • Keywords needs-refresh added
  • Milestone changed from Future Release to 4.8
  • Owner set to swissspidy
  • Status changed from new to assigned

I do not have that much time in the next 1-2 months but sure :)

We'll probably end up with something along the lines of 35069.3.diff. See also this comment by @ericlewis:

An alternative mailer should be allowed to fail and return its failure as false, as wp_mail() does.

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


8 years ago

#22 @jbpaul17
8 years ago

  • Milestone changed from 4.8 to 4.8.1

Since this appears to still need a refresh and given the short timeline to land patches for 4.8 by Friday, May 12th I'm going to punt this to 4.8.1. If the patch is ready by then, we can bring it back into 4.8.

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


7 years ago

#24 @jbpaul17
7 years ago

  • Milestone changed from 4.8.1 to 4.9

Punting to 4.9 per today's bug scrub.

#25 @swissspidy
7 years ago

  • Milestone changed from 4.9 to Future Release
  • Owner swissspidy deleted

@Mte90
7 years ago

use null

#26 @Mte90
7 years ago

I am trying to understand what are the next steps to improve that patch but I didn't got it.
Actually if the filter return false is fine, wp_mail works without issue but if there are other values wp_mail stop and return that value.
So if an alternative mailer wants to add a different behaviour can hook in that filter and can return everything. Only false is not allowed but the alternative mailer needs to sent also that value.

In the docs of the filter we are talking of a Trusthy value so it can be a boolean or not. Using null as suggested can help the dev to output the status that he prefer like true or false and we don't have any issue because the alternative mailer can handle as it prefer.

@birgire
7 years ago

#27 @birgire
7 years ago

@Mte90 your null suggestion sounds ideal, to have the ability to return both false or true.

A minor refresh to keep the ticket going forward.

In 35069.5.diff there are some minor docblock changes, like:

  • Changed @args to $atts.
  • Changed @param bool $return to @param null|bool $return.
  • Changed Passing a truthy value to Passing a non-null value.

Also it removes white spaces according to the Coding Standard.

It uses a null !== check instead of ! is_null() as it seems to be more often used in the core for short-circuits. I guess the former is more readable?

Adjusts the test method:

  • Avoided the cleanup after the assertion.
  • Added a test method description, according to the Coding Standard.
Version 0, edited 7 years ago by birgire (next)

#28 @SergeyBiryukov
5 years ago

  • Milestone changed from Future Release to 5.4
  • Owner set to SergeyBiryukov
  • Status changed from assigned to reviewing

@ayeshrajans
5 years ago

Rerolling the patch against 5.4 branch. Tests: https://travis-ci.org/Ayesh/wordpress-develop/builds/647197094 (passing)

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


5 years ago

#30 @davidbaumwald
5 years ago

  • Keywords needs-refresh removed

Latest patch applies cleanly to trunk, but given it's low priority and our proximity to 5.4 Beta 1, this is being moved to Future Release. If any maintainer or committer feels this can be refreshed and included in time, or can assume ownership during a specific cycle, feel free to update the milestone accordingly.

Also, re-marking early for maximum soak time, and needs-refresh for the @since in the doc block.

Last edited 5 years ago by davidbaumwald (previous) (diff)

#31 @davidbaumwald
5 years ago

  • Keywords needs-refresh early added

#32 @davidbaumwald
5 years ago

  • Milestone changed from 5.4 to Future Release

This was marked early, and with 5.4 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.

#33 @johnbillion
4 years ago

  • Keywords early removed
  • Milestone changed from Future Release to 5.7
  • Owner changed from SergeyBiryukov to johnbillion

#34 @johnbillion
4 years ago

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

In 49844:

Mail: Introduce a pre_wp_mail filter to allow short-circuiting the wp_mail() function without having to override the pluggable function.

Props DvanKooten, swissspidy, SergeyBiryukov, jtsternberg, ericlewis, Mte90, birgire, ayeshrajans

Fixes #35069

#35 @audrasjb
4 years ago

  • Keywords needs-dev-note added; needs-refresh removed
Note: See TracTickets for help on using tickets.