WordPress.org

Make WordPress Core

Opened 19 months ago

Closed 10 months ago

Last modified 10 months ago

#35598 closed defect (bug) (fixed)

Use different error code for PHPMailer exceptions in wp_mail_failed

Reported by: Kau-Boy Owned by: SergeyBiryukov
Milestone: 4.7 Priority: normal
Severity: normal Version: 4.4.1
Component: Mail Keywords: has-patch has-unit-tests needs-testing
Focuses: Cc:

Description

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.

Attachments (5)

32586.diff (634 bytes) - added by Kau-Boy 19 months ago.
35598.diff (674 bytes) - added by Kau-Boy 19 months ago.
35598.2.diff (1.9 KB) - added by Kau-Boy 14 months ago.
added Unit Tests
35598.3.diff (2.0 KB) - added by Kau-Boy 14 months ago.
updating the WP_Error comment
35598.4.diff (2.8 KB) - added by stephenharris 12 months ago.

Download all attachments as: .zip

Change History (32)

@Kau-Boy
19 months ago

@Kau-Boy
19 months ago

#1 @Kau-Boy
19 months ago

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

#2 @Kau-Boy
19 months ago

  • Keywords has-patch added

#3 @flixos90
19 months ago

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.

#4 @Kau-Boy
19 months ago

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.

#5 follow-up: @swissspidy
17 months ago

  • Milestone changed from Awaiting Review to Future Release

#6 @swissspidy
17 months ago

#36219 was marked as a duplicate.

#7 in reply to: ↑ 5 ; follow-up: @Kau-Boy
17 months ago

Was it too late for 4.5? :)

#8 in reply to: ↑ 7 @swissspidy
17 months ago

  • Keywords needs-unit-tests added

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.

#9 @Kau-Boy
17 months ago

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

@Kau-Boy
14 months ago

added Unit Tests

#10 @Kau-Boy
14 months ago

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

#11 @swissspidy
14 months ago

  • Keywords has-unit-tests added; needs-unit-tests removed

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?

#12 @Kau-Boy
14 months ago

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
14 months ago

updating the WP_Error comment

#13 @davidmosterd
14 months ago

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

#14 @stephenharris
14 months ago

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.

#15 @stephenharris
12 months ago

#37894 was marked as a duplicate.

#16 @stephenharris
12 months ago

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.

#17 @SergeyBiryukov
12 months ago

  • Milestone changed from Future Release to 4.7

#18 @SergeyBiryukov
12 months ago

  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#19 @Kau-Boy
12 months ago

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

This ticket was mentioned in Slack in #core by desrosj. View the logs.


11 months ago

#21 @desrosj
11 months ago

  • Keywords needs-testing added

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

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


11 months ago

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


10 months ago

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


10 months ago

#25 @helen
10 months ago

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.

#26 @helen
10 months ago

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

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.

#27 @Kau-Boy
10 months ago

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.

Note: See TracTickets for help on using tickets.