WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 6 months ago

#40810 closed defect (bug) (duplicate)

wp_mail fails to send email on WP auto update when wp-cron is called directly by php

Reported by: pessoft Owned by: SergeyBiryukov
Milestone: Priority: normal
Severity: normal Version: 4.7.5
Component: Mail Keywords: has-patch has-unit-tests
Focuses: administration Cc:

Description (last modified by SergeyBiryukov)

Hi!

On some of our WordPress instances wp-cron.php is called by system cron directly as a parameter to php command ( not via HTTP request ). This is partially to make sure that WP cron is executed regularly and partially to make sure that all tasks in cron will not hit a timeout for HTTP communication on servers and reverse proxies.

In a case when WP triggers auto update during such php execution and wants to send email about success, there is no $_SERVER['SERVER_NAME'] available and no From: header passed to wp_mail, therefore sender's email is not fully generated and delivery fails:

PHP Fatal error:  Uncaught exception 'phpmailerException' with message 'Invalid address:  (setFrom) wordpress@'

There are at least three possible solutions which could generate valid email address and maybe help in more scenarios too:

1) WP auto update fix
As WP update knows what is the site URL of WP during update, it could construct sender's email directly from configuration, pass it as From: header to wp_mail and not need to rely on whether SERVER_NAME is set or not.

2) Local hostname fix
When wp_mail is called and From: header is not present, wp_mail tries to construct the sender's email from SERVER_NAME. In case neither SERVER_NAME is available, wp_mail could try to call function like php_uname or gethostname yet as a last resort for construction of sender's email. This could ( at least to some extent ) help with such wp_mail calls where SERVER_NAME is not available.

3) Sender's email configuration option
By providing possibility to have a default sender's email for WP configured, it would be possible to use such address as sender in wp_mail function and rely on SERVER_NAME only if configuration option would be not set ( or set empty ). Based on history of tickets related to sender's email address, this fix would help many users for which current construction of sender's address is not suitable and work also in backward compatible way.

Do you see any of these options acceptable?

Thanks.

Attachments (5)

40810.patch (688 bytes) - added by pessoft 3 years ago.
Use hostname if SERVER_NAME is not available for default sender of wp_mail function
40810.2.patch (806 bytes) - added by pessoft 3 years ago.
Unittest to verify that phpmailer instance From field is updated by wp_cron
40810.diff (1.5 KB) - added by Mte90 3 years ago.
refreshed and merged
40810.2.diff (2.0 KB) - added by Mte90 3 years ago.
fixed coding standards
40810 - getmxrr.diff (2.1 KB) - added by brambo123 2 years ago.
Use more possibilities to find a domain name and use getmxrr function to find valid email domain name

Download all attachments as: .zip

Change History (33)

#1 @chesio
4 years ago

I can confirm the issue, I have the same problem on one webhost that runs WP Cron via PHP CLI. Also, because of this issue, debug.log gets populated with Undefined index: SERVER_NAME notices.

@pessoft
3 years ago

Use hostname if SERVER_NAME is not available for default sender of wp_mail function

#2 @pessoft
3 years ago

  • Keywords has-patch added

@pessoft
3 years ago

Unittest to verify that phpmailer instance From field is updated by wp_cron

#3 @pessoft
3 years ago

  • Keywords has-unit-tests added

I've added unit test to verify that when wp_cron has been called, that also From parameter of PHPMailer instance has been updated from its initial state. This update does not occur in cases when SERVER_NAME is not available and results in WP error messages and emails not being sent. This unit test is depending on #41485 as MockPHPMailer is not correctly reset and From parameter cannot be tested for changes from initial state.

#4 @SergeyBiryukov
3 years ago

  • Description modified (diff)

#6 @Mte90
3 years ago

I confirm this issue, I have plugin that use wp-cron to send emails but in our installation we are using cron by the system.
I am applied that patch but looking at the code will probably fix the issue.
In our case we had a lot of email errors, this can explain our problems on communication.

#7 @Mte90
3 years ago

Applied the patch and no anymore errors on my WordPress!
It is possible to move this on 4.9.5?

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


3 years ago

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


3 years ago

@Mte90
3 years ago

refreshed and merged

#10 @Mte90
3 years ago

Patch refreshed and merged in one diff file.
I hope to will be integrated in the next version because is very boring do a manual patch to wordpress installation.

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


3 years ago

#12 @SergeyBiryukov
3 years ago

  • Milestone changed from Awaiting Review to 5.0

#13 @afercia
3 years ago

40810.diff needs some coding standards.

#14 @afercia
3 years ago

  • Keywords needs-refresh added

@Mte90
3 years ago

fixed coding standards

#15 @Mte90
3 years ago

  • Keywords needs-refresh removed

Refreshed for fix coding standards :-)

#16 @johnbillion
2 years ago

  • Milestone changed from 5.0 to 5.1

#17 @brambo123
2 years ago

There are more options than just $_SERVER['SERVER_NAME'] and php_uname().
The functions get_site_url() and get_home_url() are also very suitable.

In addition, there are more prefixes than www., for example: staging. and dev.
By using getmxrr() it is possible to find out if the domain name is valid to send mail from.

@brambo123
2 years ago

Use more possibilities to find a domain name and use getmxrr function to find valid email domain name

#18 @pento
23 months ago

  • Milestone changed from 5.1 to Future Release

Patches need review and testing.

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


12 months ago

#20 @SergeyBiryukov
12 months ago

  • Milestone changed from Future Release to 5.4
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#21 @audrasjb
10 months ago

  • Milestone changed from 5.4 to Future Release

Hi,

With 5.4 Beta 3 approaching and the Beta period reserved for bugs introduced during the cycle, this is being moved to Future Release. If any maintainer or committer feels this should be included or wishes to assume ownership during a specific cycle, feel free to update the milestone accordingly.

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


6 months ago

#23 @enrico.sorcinelli
6 months ago

It looks like a bug that should at least be fixed in 5.5 (even in a possible 5.4.x maintenance release)

Last edited 6 months ago by enrico.sorcinelli (previous) (diff)

#24 @Mte90
6 months ago

  • Keywords dev-feedback added

This problem is related to this one in multisite that has a patch too #47733

Last edited 6 months ago by SergeyBiryukov (previous) (diff)

#25 @francina
6 months ago

  • Keywords dev-feedback removed

With auto updates from plugins and themes being merged in 5.5 and Core auto-updates in 5.6, this seems like a ticket worth revisiting.

It looks like it needs a few things to be ready for 5.5:

  • Refresh the patch
  • Write the unit test
  • Bring it up to one of the planned bug scrubs

In short, it needs a champion ;-) But it also needs some direction from the Mail components.
In any case, pinging @SergeyBiryukov for evaluation of the ticket.

Thanks!

#26 @SergeyBiryukov
6 months ago

  • Milestone changed from Future Release to 5.5

#27 @johnbillion
6 months ago

  • Keywords needs-refresh added

#28 @ocean90
6 months ago

  • Keywords needs-refresh removed
  • Milestone 5.5 deleted
  • Resolution set to duplicate
  • Status changed from reviewing to closed

Closing as a duplicate of #25239 to keep the discussion in one place and since this doesn't only affect wp-cron.

Note: See TracTickets for help on using tickets.