WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

#23642 closed defect (bug) (fixed)

wp_mail() returns true if sending failed

Reported by: chmac Owned by: SergeyBiryukov
Milestone: 3.6 Priority: normal
Severity: minor Version: 3.2
Component: Mail Keywords: has-patch commit
Focuses: Cc:

Description

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;
	}

Attachments (4)

wp_mail_return_false_on_failure.patch (378 bytes) - added by chmac 5 years ago.
Patch to fix
23642-unit-tests.diff (982 bytes) - added by iandunn 5 years ago.
23642-unit-tests.2.diff (793 bytes) - added by SergeyBiryukov 5 years ago.
23642-unit-tests.3.diff (1.1 KB) - added by iandunn 5 years ago.

Download all attachments as: .zip

Change History (19)

@chmac
5 years ago

Patch to fix

#1 @SergeyBiryukov
5 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to duplicate
  • Status changed from new to closed
  • Version changed from trunk to 3.2

Duplicate of #23291.

#2 @SergeyBiryukov
5 years ago

  • Milestone set to Awaiting Review
  • Resolution duplicate deleted
  • Status changed from closed to reopened

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

#3 @iandunn
5 years ago

  • Cc ian_dunn@… added

#4 @iandunn
5 years ago

#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?

#5 @chmac
5 years ago

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!

#6 @SergeyBiryukov
5 years ago

  • Milestone changed from Awaiting Review to 3.6

#7 @aaroncampbell
5 years ago

  • Keywords commit added

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

#8 @ocean90
5 years ago

  • Keywords needs-unit-tests added

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

#9 @iandunn
5 years ago

  • Keywords needs-unit-tests removed

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

#10 @SergeyBiryukov
5 years ago

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.

#11 follow-up: @iandunn
5 years ago

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.

#12 @ocean90
5 years ago

Related: #24662

#13 in reply to: ↑ 11 @SergeyBiryukov
5 years ago

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).

#14 @SergeyBiryukov
5 years ago

In 1301/tests:

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

#15 @SergeyBiryukov
5 years ago

  • Owner set to SergeyBiryukov
  • Resolution set to fixed
  • Status changed from reopened to closed

In 24530:

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

Note: See TracTickets for help on using tickets.