wp_mail should handle phpmailer exceptions instead of ignoring them

[mark-k]

Right now code in wp_mail that call phpmailer looks like

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

or worse (not failing at all).

Yesterday had to hack core files to find out that an email address did not have a valid email format. Exceptions should either propagate in some way, or be reported to an error log.

[iandunn]

WP doesn't use exceptions at all, so there's nothing for it to bubble up to. That also means there's no precedence in core for how to handle this.

These are the ideas I had:

1. Return a WP_Error instead of false. That would break backwards compatibility, though.
2. Issue an admin_notice. That's probably more visible than desired, though, and it wouldn't always be obvious when it was created or what event it was tied to. It could also be triggered by user A and seen by user B, leading to confusion.
3. Push an entry to PHP's error_log. This seems like the best solution to me. One potential downside is that if display_errors is enabled, the warning will cause the page to crash with "Cannot modify header information - headers already sent". I think that's to be expected, though, and probably consistent with other warnings thrown elsewhere in core.

I've added a patch that implements #3.

[dd32]

I'd go for E_USER_NOTICE so as to prevent it being shown in production environments, but #3 was the first thing that came to my mind.

[iandunn]

Updated patch to use E_USER_NOTICE instead of E_USER_WARNING.

[mark-k]

In my case the error was at front end so option 2 would probably not have helped in diagnosing the problem.

What about having an action/filter to be executed when there is an exception? This way if plugins will want to handle errors they will get a notification about them.

[iandunn]

23291.diff pushes the error through PHP, so you can use ​set_error_handler( 'your-callback-function', E_USER_NOTICE ) to catch it and do whatever you'd like.

Adding an action on top of that would be add a bit of unnecessary overhead. It wouldn't be significant in-and-of-itself, but combined with the hundreds of other hooks already in WP, it can add up. I don't see any added benefit to an extra action over just hooking into PHP's error handling. Does anyone else?

We may want to add some kind of ID to the error message so that it's easier to distinguish from other messages, though. e.g.,

Notice: [wp_mail] Invalid address: first.last@ in /var/www/vhosts/example.com/httpdocs/wp-svn-trunk/wp-includes/pluggable.php on line 359

...instead of just

Notice: Invalid address: first.last@ in /var/www/vhosts/example.com/httpdocs/wp-svn-trunk/wp-includes/pluggable.php on line 359

Thoughts on that?

[mark-k]

set_error_handler is just not very easy to use. It is probably not very known and you have to filter the errors to find the ones you are interested in.

Actually not sure if triggering an error is good enough because many places suppress errors when calling wp_mail, and places which don't, might suddenly display unexpected errors.

Seems to me that returning WP_Error is the only real solution here, anything else will just be an hack around it. The only place in core which cares right now about the return value can be changed to expect either false or WP_Error so it still will be compatible with plugins which override wp_mail, and we are early enough in 3.6 cycle to let authors of plugins that use wp_mail know about the change and prepare for it.

[bpetty]

Replying to mark-k:

Seems to me that returning WP_Error is the only real solution here, anything else will just be an hack around it. The only place in core which cares right now about the return value can be changed to expect either false or WP_Error so it still will be compatible with plugins which override wp_mail, and we are early enough in 3.6 cycle to let authors of plugins that use wp_mail know about the change and prepare for it.

This is not how compatibility is maintained in WordPress. This can't be changed, and that's just the way it is unfortunately. There are other perfectly acceptable solutions that remain backwards compatible anyway. Sure, it would have been nice if wp_mail() was originally designed to return WP_Error, but it wasn't, and it never will now (unless you want to start a discussion about changing the deprecation and API compatibility policies, but that's outside the scope of this issue).

All that aside though, there's plenty of perfectly acceptable ways to validate an email address before calling wp_mail(), and I'm fairly certain most locations that call wp_mail() do that already as any properly written code should when written for an application that doesn't support exceptions.

[iandunn]

I don't really buy that set_error_handler() is hard to use, can you elaborate on that?

I think bringing up suppressed errors is a very good point, though. It looks like 4 out of the 19 calls to wp_mail() in core suppress errors.

To me that begs 2 questions,

1. Why is it not used consistently? Are there reasons for suppressing it in some cases, or is it just the lack of an official policy that leads to different developer's preferences being implemented in different patches?
2. Should error suppression be used at all? What's the case for it? I think it's reasonable to assume that production servers will have display_errors disabled, and that anyone with it enabled on a dev server would want to be aware of the errors.

I'd propose getting rid of error suppression for all wp_mail() calls, so that the new trigger_error() calls in this patch can be relied upon.

[iandunn]

Also, ​set_error_handler() callbacks are still fired even when error suppression is used.

The errors wouldn't show up in the error_log, though, so that's still a problem for troubleshooting.

[mark-k]

Replying to bpetty:

All that aside though, there's plenty of perfectly acceptable ways to validate an email address before calling wp_mail(), and I'm fairly certain most locations that call wp_mail() do that already as any properly written code should when written for an application that doesn't support exceptions.

Sure, but what happens if your shared host upgraded php and forgot to compile mail into it, or if you use a plugin to make wp_mail send mail via SMTP and the SMTP server is down or your password was changed? Right now you don't get any chance to even know about that.
And BTW the most popular contact form plugin WCF7 doesn't validate addresses otherwise we wouldn't be discussing this thing here.

[mark-k]

Ok, different suggestion, what about holding the exceptions in a global (or accessed via function) array, something like $wp_mail_lasterrors, and in addition maybe call trigger_error if WP_DEBUG is defined.

IMO set_error_handler is harder to use because theoretically anything that is called by wp_mail can also trigger an error and therefor your handler should be smart enough to filter out those errors. Not impossibly hard, just harder.

[rmccue]

What about introducing an extra parameter to wp_mail() to enable WP_Error? We already do this in wp_insert_post(), and this would maintain full compatibility with existing WP_Error replacement functions (we just need to check for WP_Error and false in the consuming code, which is no worse than the current situation).

[iandunn]

@mark-k, I don't see the point in only calling trigger_error() if WP_DEBUG is enabled. PHP's error_reporting already allows people to filter what kinds of messages they want. I think the standards practice is to push all errors/warnings/notices, and let the user pick which ones they want to display/log in a given context.

@rmccue, Adding a @wp_error param sounds like a good idea to me, but we'd still need to trigger_error(), unless you're suggesting we change all the core calls to wp_mail() to use the new param. Otherwise exceptions triggered by core calls to wp_mail() would still be ignored.

[mark-k]

Adding the parameter will fail because if you change the core to use it you will get errors on sites which override wp_mail and didn't update the function's signature

[rmccue]

Replying to iandunn:

@rmccue, Adding a @wp_error param sounds like a good idea to me, but we'd still need to trigger_error(), unless you're suggesting we change all the core calls to wp_mail() to use the new param. Otherwise exceptions triggered by core calls to wp_mail() would still be ignored.

I am indeed suggesting that we change core to do this.

Replying to mark-k:

Adding the parameter will fail because if you change the core to use it you will get errors on sites which override wp_mail and didn't update the function's signature

Providing extra parameters to functions doesn't cause an error (with the exception of a few built-in PHP functions), so this isn't an issue. The only issue is if people are using an extra parameter for internal parameter passing, in which case they shouldn't be overriding wp_mail() with that.

[mark-k]

Replying to rmccue:

Replying to mark-k:

Adding the parameter will fail because if you change the core to use it you will get errors on sites which override wp_mail and didn't update the function's signature

Providing extra parameters to functions doesn't cause an error (with the exception of a few built-in PHP functions), so this isn't an issue. The only issue is if people are using an extra parameter for internal parameter passing, in which case they shouldn't be overriding wp_mail() with that.

Sure, me bad, haven't had enough coffee yet.

[iandunn]

I updated the patch to do these things:

1. Added an optional $return param to wp_mail().
   1. Because of the structure of the function, though, the trigger_error() calls are still necessary. The function only returns false | WP_Error if sending fails. There are cases where errors occur and sending is still successful. For example, if $to is valid, but there's an error in the CC header. If we remove the trigger_error() calls, those errors will continue to be ignored and undetectable.
   2. $return = 'bool' | 'wp_error' isn't consistent with wp_insert_post()'s $wp_error = false | true signature, but it conforms to the coding guidelines.
2. Updated the call to wp_mail() in retrieve_password() to use the new param. It was the only function in core that was actually checking the return value.
3. Removed the error suppression operator from 4 calls to wp_mail(). It doesn't seem like there's a valid case for using it, and it could potentially hide errors and make debugging/troubleshooting unnecessarily difficult. See ​1 and ​2.

I don't think there's a way to translate the error messages thrown by PHPMailer, since they're in a variable rather than a string. Is that correct?

[mark-k]

Replying to iandunn:

I updated the patch to do these things:

1. Added an optional $return param to wp_mail().
   1. Because of the structure of the function, though, the trigger_error() calls are still necessary. The function only returns false | WP_Error if sending fails. There are cases where errors occur and sending is still successful. For example, if $to is valid, but there's an error in the CC header. If we remove the trigger_error() calls, those errors will continue to be ignored and undetectable.

I can't form a strong opinion whether failing to send to any of the recipients should be considered as failure to send. Is there any facility in WP_Error to report "Worked, but ...."? OTOH failing to send an attachment should probably be reported as a failure.

In other words, maybe if there was an error, the error object should be returned and let the caller decide about the severity of the error.

I don't think there's a way to translate the error messages thrown by PHPMailer, since they're in a variable rather than a string. Is that correct?

My PHP inheritance knowledge isn't great but can't you extend phpmailer and override its setLanguage function or just setup its language array by using the translation function?

Other remarks:

1. I don't think the error codes should be prefixed with "phpmailer_". If some day phpmailer will be replaced it will make no sense.

2. In wp_login.php, might as well display the error message from the WP_Error object if one was returned instead of the generic "'Possible reason: your host may have disabled the mail() function..."

[rmccue]

Replying to iandunn:

1. Because of the structure of the function, though, the trigger_error() calls are still necessary. The function only returns false | WP_Error if sending fails. There are cases where errors occur and sending is still successful. For example, if $to is valid, but there's an error in the CC header. If we remove the trigger_error() calls, those errors will continue to be ignored and undetectable.

Can we return the error anyway? The calling code can always check the error code and decide what to do with it.

2. $return = 'bool' | 'wp_error' isn't consistent with wp_insert_post()'s $wp_error = false | true signature, but it conforms to the coding guidelines.

I personally think it should be a boolean. Which specific guideline are you referencing here?

2. Updated the call to wp_mail() in retrieve_password() to use the new param. It was the only function in core that was actually checking the return value.

I feel like that should be fixed up where functions are already returning WP_Errors, but that's probably out-of-scope for this ticket. (I'll defer to your judgement here.)

3. Removed the error suppression operator from 4 calls to wp_mail(). It doesn't seem like there's a valid case for using it, and it could potentially hide errors and make debugging/troubleshooting unnecessarily difficult. See ​1 and ​2.

We do use the error suppression in a few places, but where possible it makes sense to avoid it. This is one of those places, so that looks good. :)

I don't think there's a way to translate the error messages thrown by PHPMailer, since they're in a variable rather than a string. Is that correct?

It looks like PHPMailer reads the strings from a $language property (see PHPMailer::SetLanguage) that we could use (although there are a couple of missed strings there that could use an upstream patch). Again, I'd say that's out-of-scope for this ticket.

[SergeyBiryukov]

Replying to rmccue:

Replying to iandunn:

2. $return = 'bool' | 'wp_error' isn't consistent with wp_insert_post()'s $wp_error = false | true signature, but it conforms to the coding guidelines.

I personally think it should be a boolean. Which specific guideline are you referencing here?

​http://codex.wordpress.org/WordPress_Coding_Standards#Self-Explanatory_Flag_Values_for_Function_Arguments

[rmccue]

Replying to SergeyBiryukov:

​http://codex.wordpress.org/WordPress_Coding_Standards#Self-Explanatory_Flag_Values_for_Function_Arguments

Aha, thanks Sergey. Another one to add to my list of silly coding standards. :)

In that case, bool | wp_error seems fine to me, but I'd still go with consistency personally. I'll defer to your choice though.

[iandunn]

I'll update the patch to do these things:

1. Return WP_Error even for non-fatal errors, like with the CC or attachment.
   1. If $return == 'wp_error', then it will return a WP_Error if any errors occur, and the calling function can decide what to do with them.
   2. If $return == 'bool', though, it will still return true for non-fatal errors and false only when $phpmailer->Send() fails.
2. Change the WP_Error name prefix from "phpmailer" to just "mailer" so that it's more generic.
3. Update retrieve_password() to display the detailed error message, rather than the generic one that's currently shown.

I should have some time for that later this week.

I started #23311 for the PHPMailer translation issue.

[iandunn]

Adds optional $return param to return a WP_Error

[iandunn]

Replying to iandunn:

I'll update the patch to do these things:

1. Return WP_Error even for non-fatal errors, like with the CC or attachment.
   1. If $return == 'wp_error', then it will return a WP_Error if any errors occur, and the calling function can decide what to do with them.
   2. If $return == 'bool', though, it will still return true for non-fatal errors and false only when $phpmailer->Send() fails.
2. Change the WP_Error name prefix from "phpmailer" to just "mailer" so that it's more generic.
3. Update retrieve_password() to display the detailed error message, rather than the generic one that's currently shown.

I should have some time for that later this week.

23291.2.diff is updated with those changes.

[nacin]

I do generally like explanatory arguments, though $wp_error = false is already a pattern used in wp_insert_post(), wp_update_post(), and some lesser functions. I would definitely be okay with that here.

[iandunn]

Changed $return = 'bool' | 'wp_error' to $wp_error = true | false

[iandunn]

Changed $return = 'bool' | 'wp_error' to $wp_error = true | false

[iandunn]

I updated the patch and unit tests to use the same $wp_error signature as wp_insert_post(), etc.

[SergeyBiryukov]

#23642 was marked as a duplicate.

[bpetty]

@iandunn - Could you review your changes here against the recently updated PHPMailer library? It's error handling may have changed a bit since you wrote this patch.

[iandunn]

No problem. Should have some time for that next week.

[iandunn]

I didn't see anything in PHPMailer's changelog about error handling, so I ran a diff of 5.2.4 against 5.2.1, and didn't notice anything of consequence. They added phpDoc tags for which exceptions are thrown, and a few exception messages changed, but it looks like that's it as far as exceptions are concerned.

The new unit tests still pass, too.

Were there any specific issues that you had in mind? If so, could you point me in the right direction? If not, I think 23291.3.diff is still good.

[iandunn]

If #23642 is committed, then the patch here should probably be updated to add an item to $errors if $phpmailer->Send() returns false.

[bpetty]

Thanks Ian, I didn't have anything specific in mind.

[iandunn]

Some of the tests in 23291-unit-tests.2.diff​ were modified for use in #23642. If those versions are committed, then the ones here may no longer be needed here, or may need to be modified/refreshed.

[iandunn]

I refreshed the patch, and then updated the patch and tests in light of #23642 and #24662.

[wonderboymusic]

these are all marked 3.7-early

[nacin]

I'm not sure I understand why wp-login.php underwent some logic changes. When $message is empty, nothing happened previously; now, an error is shown. Also, the text "Possible reason: your host may have disabled the mail() function." vanished. I am going to take a wild guess that the string for PHPMailer's exception is crappier, written "by developers for developers", and untranslated. I don't actually know if WP_Error is useful here — if wp_mail() failed, for any reason, we should just tell them.

Rather than if ( $wp_error == true ), if ( $wp_error ). Rather than count( $errors->get_error_codes() ) > 0, just do $errors->get_error_code().

We should consider passing the failed address/attachment/etc. as data to WP_Error:add() (third argument).

[nacin]

Still waiting for feedback.

[desrosj]

It looks like this was solved in #18926. [34221] adds the wp_mail_failed action that gets fired when a PHPMailer related exception is encountered. Plugins and custom code can hook into this action to read or log the WP_Error object that is passed.

If there are any exceptions where the action hook is not fired but is desired, please open a new ticket.