wp_mail() returns true if sending failed

[chmac]

Here: source:/trunk/wp-includes/pluggable.php@23507#L449​

The code is:

	try {
		$phpmailer->Send();
	} catch ( phpmailerException $e ) {
		return false;
	}
	
		return true;

However, $phpmailer->Send() may return false without throwing an exception. A simple change fixes this like so:

	try {
		return $phpmailer->Send();
	} catch ( phpmailerException $e ) {
		return false;
	}

[chmac]

Patch to fix

[SergeyBiryukov]

Duplicate of #23291.

[SergeyBiryukov]

Well, maybe not exactly a duplicate, just related. Introduced in [17753].

[iandunn]

#23291 only returns a WP_Error when PHPMailer throws an exception -- and wp_mail() was called with $wp_error set to true -- so there are still potential cases where Send() returns false without throwing an exception and wp_mail() returns true.

One thing we could easily do here is to add to the $errors array that #23291 creates if $wp_error is passed in as true and Send() returns false. That would basically be a catchall error to let the caller know something went wrong.

I'm not sure what the best thing to do would be if $wp_error is passed in as false, though, which is the default. It seems logical to me that if Send() returns false, then so should wp_mail(), but that would change the current behavior and could break backwards compatibility for plugins, etc. Any thoughts on that?

chmac, do you have any specific use cases you can share where wp_mail() returning true causes a problem?

[chmac]

Just to be clear, this is a bug that was introduced in [17753] in response to #17228.

The documentation on wp_mail reads:

     * A true return value does not automatically mean that the user received the
     * email successfully. It just only means that the method used was able to
     * process the request without any errors.

and:

@return bool Whether the email contents were sent successfully.

The bug was introduced 23 months ago, so has been around for a long while, I noticed it but never bothered looking into it. But I think fixing it restores backwards compatibility, not breaks it. Folks should expect that if wp_mail() returns true, the mail was sent, as the documentation states, and as the convention has been for the 3+ years prior to the introduction of this bug.

@iandunn: I noticed because of the test feature in wp-mail-smtp which started returning true on failure some 23 months ago. Took me a while to figure it out!

[aaroncampbell]

wp_mail_return_false_on_failure.patch​ looks good to me and seems to fix the issue introduced in [17753].

[ocean90]

There is currently no test, which checks the return value of wp_mail(). We should add some.

[iandunn]

I borrowed some of the tests from #23291 and modified them to fit this ticket.

[SergeyBiryukov]

I guess we can combine several assertions in a single test: 23642-unit-tests.2.diff​.

The test currently passes without the patch. There should be an assertion that fails without wp_mail_return_false_on_failure.patch​.

[iandunn]

Thanks for catching that Sergey, I've added another test to cover that. The only way I could see to get PHPMailer to return false was to disable exceptions. chmac, can you confirm that's what you were doing? If not, maybe PHPMailer changed its behavior since this ticket was opened?

Side note: I always thought that tests were supposed to be isolated as much as possible, to make sure changes to the system state from one test didn't affect the other tests. Are you combining them so that the suite runs faster, and then just manually checking to make sure that the combined tests don't have any side-effects on each other? I kept the new test separate from the others, since it does change the system state.

[ocean90]

Related: #24662

[SergeyBiryukov]

Replying to SergeyBiryukov:

The test currently passes without the patch.

With the properly implemented mock PHPMailer from #24662, turned out that 23642-unit-tests.2.diff​ actually fails in the last assertion without the patch, and passes with wp_mail_return_false_on_failure.patch​.

Replying to iandunn:

Are you combining them so that the suite runs faster, and then just manually checking to make sure that the combined tests don't have any side-effects on each other?

The pattern I see in other tests is that we tend to group similar assertions under the same test, if possible (if they don't change the global state).

[SergeyBiryukov]

In 1301/tests:

Test for wp_mail() return value. props iandunn. see #23642.

[SergeyBiryukov]

In 24530:

Make wp_mail() return the actual result of PHPMailer::Send() instead of always returning true. props chmac. fixes #23642.