Make WordPress Core

Opened 13 years ago

Closed 9 years ago

#18926 closed enhancement (fixed)

Provide with the ability to monitor emailing failures

Reported by: npetetin's profile npetetin Owned by: jorbin's profile jorbin
Milestone: 4.4 Priority: normal
Severity: normal Version: 3.2
Component: Mail Keywords: has-patch needs-docs
Focuses: Cc:

Description

Add an action hook in 'wp-mail' function on catching 'phpmailerException' and before returning 'false'.

This way, a plugin may track all email sending failures.

Attachments (8)

18926.diff (338 bytes) - added by jkudish 13 years ago.
pluggable.php.diff (386 bytes) - added by soulseekah 13 years ago.
using action and WP_Error
pluggable.php.2.diff (426 bytes) - added by soulseekah 13 years ago.
using a filter with WP_Error
18926.2.diff (658 bytes) - added by kurtpayne 13 years ago.
Log mail errors when WP_DEBUG is enabled
pluggable.php.3.diff (547 bytes) - added by soulseekah 13 years ago.
action hook, WP_Error contains additional data
pluggable.php.4.diff (705 bytes) - added by soulseekah 13 years ago.
action hook, WP_Error contains additional data, WP_DEBUG
pluggable.php.5.diff (498 bytes) - added by soulseekah 13 years ago.
wp_mail_failed action hook
18926.3.diff (788 bytes) - added by jorbin 9 years ago.

Download all attachments as: .zip

Change History (30)

@jkudish
13 years ago

#1 @jkudish
13 years ago

  • Cc joachim.kudish@… added
  • Keywords has-patch added

Attached patch adds the hook as per the description by @npetetin.

If this patch doesn't get accepted, I should note that wp_mail() is a pluggable function (http://codex.wordpress.org/Pluggable_Functions), meaning you could do this in a plugin by redaclaring the function.

#2 @npetetin
13 years ago

Absolutely, but avoiding redeclaring this huge function for a single hook would be great.

The proposed patch is what was expected, let's just hope it will be get accepted...

#3 @nacin
13 years ago

  • Milestone changed from Awaiting Review to Future Release

No problem here but we should probably pass the exception?

#4 @npetetin
13 years ago

Definitely yes for the exception to be passed as well.

@soulseekah
13 years ago

using action and WP_Error

@soulseekah
13 years ago

using a filter with WP_Error

#5 @soulseekah
13 years ago

I have been thinking a lot about the issue of getting error messages from wp_mail lately and stumbled upon this ticket today, decided to explore the problem-solution space a bit with a small and mostly obvious article.

http://codeseekah.com/2012/02/27/getting-error-feedback-from-wp_mail/

I'm attaching two diffs that that pass a WP_Error object, instead of the exception as the internal implementation may change in the future. One using a filter, another using an action. Both are not too comfortable to use, but we can probably work something out from there + decide on a better hook name, probably specific to wp_mail , again, in case internal implementation changes.

Probably missing something, ready to discuss further.

#6 @scribu
13 years ago

pluggable.php.diff looks like the best approach to me. Most of the time, you can't do anything but log the error anyway.

#7 in reply to: ↑ description ; follow-up: @npetetin
13 years ago

Good concern and good answer. However, passing the phpMailer object allows to give more context and retrieve its attributes (from, to, subject, message, etc.) Could passing a less typed set of these attributes (associative array?) in addition be the best compromise?

@kurtpayne
13 years ago

Log mail errors when WP_DEBUG is enabled

#8 in reply to: ↑ 7 ; follow-ups: @soulseekah
13 years ago

Replying to npetetin:

Good concern and good answer. However, passing the phpMailer object allows to give more context and retrieve its attributes (from, to, subject, message, etc.) Could passing a less typed set of these attributes (associative array?) in addition be the best compromise?

The inner implementation may change in the future, PHPMailer could be switched for something completely different. But we could use the third optional $data parameter of the WP_Error to pass along the relevant data, might be useful indeed.

How about putting in all the original $to, $subject, $message, $headers = '', $attachments = array() set of data, that is implementation independent, isn't it? Any disadvantages?

#9 in reply to: ↑ 8 ; follow-ups: @kurtpayne
13 years ago

  • Cc kpayne@… added

Replying to soulseekah:

How about putting in all the original $to, $subject, $message, $headers = '', $attachments = array() set of data, that is implementation independent, isn't it? Any disadvantages?

I like this, and the pluggable.php.diff approach.

I think we should always log mail errors under WP_DEBUG, too.

@soulseekah
13 years ago

action hook, WP_Error contains additional data

@soulseekah
13 years ago

action hook, WP_Error contains additional data, WP_DEBUG

#10 in reply to: ↑ 9 @soulseekah
13 years ago

Replying to kurtpayne:

I think we should always log mail errors under WP_DEBUG, too.

Might be useful, although WordPress doesn't seem to log errors too much, only a couple of "third-party" classes (wpdb, simplePIE, POP3, etc.) contain error log writes; can't see much logging going on in the core itself, except deprecated functions firing errors off.

Last edited 13 years ago by soulseekah (previous) (diff)

#11 in reply to: ↑ 8 @npetetin
13 years ago

Replying to soulseekah:


How about putting in all the original $to, $subject, $message, $headers = '', $attachments = array() set of data, that is implementation independent, isn't it? Any disadvantages?

Looks pretty good to me, best implementation with all relevant data.

+1 for pluggable.php.3.diff.

#12 in reply to: ↑ 9 @scribu
13 years ago

Replying to kurtpayne:

I think we should always log mail errors under WP_DEBUG, too.

Plugin territory.

#13 @nacin
13 years ago

error_log() is probably plugin territory, but trigger_error() for E_WARNING might not be.

#14 follow-up: @scribu
13 years ago

First of all, the action should be called 'wp_mail_failed', to be consistent with 'wp_loaded' and others.

Most emails are sent during a POST action, which is usually followed by a redirect, so I don't see the point in calling trigger_error().

Even if we were to call trigger_error(), it should be through a callback hooked into 'wp_mail_failed', so that it's easy for plugins to disable.

#15 in reply to: ↑ 14 @soulseekah
13 years ago

Replying to scribu:

First of all, the action should be called 'wp_mail_failed', to be consistent with 'wp_loaded' and others.

I second this, good thinking.

@soulseekah
13 years ago

wp_mail_failed action hook

#16 @codebykat
11 years ago

  • Cc kat@… added

#17 @chriscct7
9 years ago

  • Severity changed from minor to normal
  • Version changed from 3.2.1 to 3.2

#18 @jorbin
9 years ago

  • Keywords needs-refresh needs-docs added
  • Milestone changed from Future Release to 4.4

We need hook docs

@jorbin
9 years ago

#19 @jorbin
9 years ago

  • Keywords needs-refresh needs-docs removed

Patch adds hook docs.

#20 @jorbin
9 years ago

  • Owner set to jorbin
  • Resolution set to fixed
  • Status changed from new to closed

In 34221:

Fire Action when mail exception is thrown.

new action is wp_mail_failed which contains a WP_Error object with the phpmailerException code, message and an array with the mail information. Plugins can hook in and log when mails fail to send due to a phpmailer issue.

Props soulseekah
Fixes #18926

#21 @DrewAPicture
9 years ago

  • Keywords needs-docs added
  • Resolution fixed deleted
  • Status changed from closed to reopened

@jorbin: Reopening since there are some syntactical issues with these hook docs:

  • The summary needs a period
  • The hook parameter is missing a variable, I would suggest $error
  • The hook parameter description should use the serial or "Oxford comma" style
  • The hook parameter description should end with a period

#22 @SergeyBiryukov
9 years ago

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

In 34239:

Docs: Fix some syntactical issues with the DocBlock for wp_mail_failed action, introduced in [34221].

Fixes #18926.

Note: See TracTickets for help on using tickets.