Use different error code for PHPMailer exceptions in wp_mail_failed

[Kau-Boy]

This is a follow-up to #18926.

I recently had issues with a contact form plugin and while trying to debug it, I found the new wp_mail_failed hook. It would have been quite handy to use, but in it's current implementation, it is quite uselesse.

It is using a WP_Error which is fine. In the WP_Error the constructor checks the $code to be not empty. But as the PHPMailer almost always use a error code of zero, the WP_Error will not provide any useful information (it's empty).

I would recommend to either using wp_mail_failed or something similar.

[Kau-Boy]

You may delete 32586.diff as I accidentally selected the wrong patch.

[flixos90]

What about alternatively checking ! empty( $e->getCode() ), and if so, use the exception code, otherwise use the wp_mail_failed? I guess it would be kind of inconsistent, so I'm not sure about it - but if there actually is an exception code, it will be more helpful than wp_mail_failed.

[Kau-Boy]

The first argument in a call to WP_Error is usually always a static string (see: ​https://codex.wordpress.org/Class_Reference/WP_Error). So in my opinion it is more consistent to also use it for this case.

The PHPMailerException usually only takes the first parameter, which hold the error message. This message would be available as the second argument in WP_Error.

[swissspidy]

#36219 was marked as a duplicate.

[Kau-Boy]

Was it too late for 4.5? :)

[swissspidy]

Replying to Kau-Boy:

Was it too late for 4.5? :)

Yeah we're already in beta with RC1 coming soon. Even though it's clearly a bug, it needs some more testing.

[Kau-Boy]

OK, thanks for the reply. Than I know what to work on at the Contributor Day in London :)

[Kau-Boy]

added Unit Tests

[Kau-Boy]

@ocean90, @swissspidy: Am I again to late for the major release? :)

[swissspidy]

Note that with your patch the hook's docblock isn't correct anymore (“A WP_Error object with the phpmailerException code, message…”).

As you pointed out, PHPMailer doesn't always use zeroes for the exception code. Therefore developers might expect another error code.

What about changing the empty check in the WP_Error constructor instead? Or using WP_Error:add() to add the error code so it's not lost?

[Kau-Boy]

Changing the empty check in the WP_Error might have many side effects, I would guess. As usually the first parameter used for WP_Error is a static string, I found this the optimal solution (I couldn't find a single usage in core not using a static string).

The PhpMailer only uses numeric error codes of 0, 1 and 2 which doesn't really add too much useful information.

[Kau-Boy]

updating the WP_Error comment

[davidmosterd]

@Kau-Boy Your remark on changing the empty check in WP_Error bringing up a lot of potential bad effects might be very true. I think however it would be very useful to be able to map any type of exception from any PHP library to a WP_Error without prefixing it with say 'context_' or something.

Maybe a filter before the empty check would work to catch the 0,1 and 2 cases. Or check more actively on a integer or a string and if that fails assume empty. That way the WP_Error code can work without a context. This would allow you to use WP_Error more easily as the primary interface to errors instead of Exception classes.

I understand that saying the code has to be something semantic is a design choice of WP_Error, which makes sense too.

If this is something we could consider for WordPress, we could make this into it's own ticket. But the comment made sense to be posted here to me.

[stephenharris]

Why not include the original exception code in the the WP_Error in the data argument? There's no precedent for the error code of a WP_Error instance to be unique to the underlying cause - though, clearly being able to distinguish between them would be desirable.

Setting a fixed error code would fix the bug, provide consistency and the data attribute would provide for a more precise diagnosis.

Alternatively, I don't see any major disadvantages to prefixing the error code by 'phpmailer_'.

The only minor issue with both of these suggestions is that it changes the error code (and someone might be checking for '2' or '3') - but that's probably not very likely, and in any case, they should have some generic fallback.

There would be greater risk in changing the behaviour of WP_Error.

[stephenharris]

#37894 was marked as a duplicate.

[stephenharris]

Updated @Kau-Boy's patch to store the original exception code in the WP_Error data. That way we're not loosing any information. I'd be happy to see this in 4.7.

[Kau-Boy]

Saving the original error code in the data array should be OK. I might be able to finalize the unit tests on the Contributor Day at #wcfra tomorrow :)

[desrosj]

This needs some testing and feedback. @SergeyBiryukov do you have any thoughts here?

[helen]

35598.4.diff​ seems like a solid change to me, and a quick look around plugins leads me to believe it's a safe change. Wish I'd had this the last time I was debugging a wp_mail() problem :) It's kind of an enhancement in some ways I suppose, but still a bug fix, so going in. If there's something still lacking with the tests, that can be corrected at any time.

[helen]

In 39086:

Mail: Set a better error code when triggering wp_mail_failed.

This error code is now... wait for it... wp_mail_failed. Previously, this would have been the originating PHPMailer error code, which could be 0, which would then fail (pass?) the empty() check in the WP_Error constructor, thereby rendering the error object fairly useless. The PHPMailer error code is now located within the WP_Error data.

props Kau-Boy, stephenharris.
fixes #35598.

[Kau-Boy]

Thanks for merging it in. The only thing that I couldn't figure out was how to test if the action wp_mail_failed was called AND the exception was thrown. But I think it's safe to asume, that the action was only called if the exception was thrown with the current code. So I think the unit test should be fine.