Make WordPress Core

Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#37736 closed defect (bug) (fixed)

Emails fail on certain server setups

Reported by: clorith's profile Clorith Owned by: boonebgorges's profile boonebgorges
Milestone: 4.6.1 Priority: high
Severity: normal Version: 4.6
Component: Mail Keywords: has-patch needs-testing has-unit-tests commit
Focuses: Cc:

Description

In 4.6 (see r38058) we changed how we apply the from email and name, we now use PHPMailers own setFrom() function call.

This function can also autoamtically populates the $Sender variable (which it does by default).

Further down, when mailSend() is called, the behavior differs slightly if $Sender is already set, we set a parameter of -f.

It would appear some hosts do not allow the use of this flag, leading to emails no longer going out.

Attaching a simple patch that tells PHPMailer not to populate this variable, as we previously set the from name and email directly and never saw the $Sender variable, this allows us to keep using PHPMailers internal functions, while avoiding the $Sender flag like we did pre-4.6

Attachments (3)

37736.patch (474 bytes) - added by Clorith 8 years ago.
37736-test.diff (932 bytes) - added by iandunn 8 years ago.
37736.diff (1.4 KB) - added by DrewAPicture 8 years ago.

Download all attachments as: .zip

Change History (32)

@Clorith
8 years ago

#1 @Clorith
8 years ago

  • Milestone changed from Awaiting Review to 4.6.1

#2 @swissspidy
8 years ago

#37725 was marked as a duplicate.

#3 @swissspidy
8 years ago

  • Keywords has-patch needs-testing added

#4 @iandunn
8 years ago

I was bit by this too. Serves me right, since I introduced it in 21659.4.diff :(

I installed Clorth's patch as a hotfix in production and that seems to be working. I'll look out for any problems and drop a note here if I see any.

I imagine this is affecting a lot of people in the wild, and the nature of it will make it fail silently in most cases. I wonder if it should it be marked as a high priority? cc @stephenharris, @boonebgorges, @cbutlerjr

#5 follow-up: @boonebgorges
8 years ago

  • Priority changed from normal to high

Change seems right to me. Could we get a review from @ocean90 or @dd32?

#6 @cbutlerjr
8 years ago

the nature of it will make it fail silently in most cases.

yeowch! That warrants a high priority. Thanks @boonebgorges for changing priority.

#7 @iandunn
8 years ago

I'm working on a unit test, but it looks like it might require refactoring MockPHPMailer so that PHPMailer::postSend() isn't overridden.

#8 @iandunn
8 years ago

Doh, nevermind, it's easy. I'll upload a patch soon.

@iandunn
8 years ago

#9 @iandunn
8 years ago

  • Keywords has-unit-tests added

#10 in reply to: ↑ 5 @dd32
8 years ago

  • Keywords commit added

Replying to boonebgorges:

Change seems right to me. Could we get a review from @ocean90 or @dd32?

Change seems correct to me too, especially with the addition of the unit test (Thanks @iandunn!)

#11 @aaroncampbell
8 years ago

  • Keywords commit removed

We ran into this with a customer whose notification mails started getting bounced. We tracked the fix to exactly what's in the patch here, and then ended up finding this ticket while opening our own.

So +1 on committing

#12 @aaroncampbell
8 years ago

  • Keywords commit added

Looks like in the overlap in comments I accidentally removed the commit keyword.

@DrewAPicture
8 years ago

#13 @DrewAPicture
8 years ago

37736.diff simply combines @Clorith's fix patch with @iandunn's tests patch.

#14 @boonebgorges
8 years ago

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

In 38286:

Mail: Don't set Sender field when setting From.

[38058] changed wp_mail() so that it used PHPMailer's setFrom()
method rather than setting the From and FromName headers directly. See
behavior of setting the Sender field. This causes mail to be
called with the -f flag, which causes outgoing email to fail on some
server environments.

Props Clorith, iandunn, DrewAPicture.
Fixes #37736.

#15 @boonebgorges
8 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for 4.6.1.

#16 @boonebgorges
8 years ago

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

In 38287:

Mail: Don't set Sender field when setting From.

[38058] changed wp_mail() so that it used PHPMailer's setFrom()
method rather than setting the From and FromName headers directly. See
behavior of setting the Sender field. This causes mail to be
called with the -f flag, which causes outgoing email to fail on some
server environments.

Merges [38286] to the 4.6 branch.

Props Clorith, iandunn, DrewAPicture.
Fixes #37736.

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


8 years ago

#18 @swissspidy
8 years ago

#37881 was marked as a duplicate.

#19 @MattyRob
8 years ago

#18792 was marked as a duplicate.

#20 @peterwsterling
8 years ago

Yep - defo fixes the problem. When will this be released?

This ticket was mentioned in Slack in #cli by danielbachhuber. View the logs.


8 years ago

This ticket was mentioned in Slack in #forums by ipstenu. View the logs.


8 years ago

#24 @travisnorthcutt
8 years ago

I haven't been able to fully track this down yet, but FYI I'm seeing problems that look like they may be related to this, when creating new sites on a multisite install, running 4.6.1.

I get:

Fatal error: Uncaught phpmailerException: Invalid address: wordpress@

Which appears to come from the following code around line 324 of /wp-includes/pluggable.php:

if ( !isset( $from_email ) ) {
                // Get the site domain and get rid of www.
                $sitename = strtolower( $_SERVER['SERVER_NAME'] );
                if ( substr( $sitename, 0, 4 ) == 'www.' ) {
                        $sitename = substr( $sitename, 4 );
                }

                $from_email = 'wordpress@' . $sitename;
        }

So it would seem that in this case $_SERVER['SERVER_NAME'] isn't set, and PHPMailer->setFrom() doesn't care for wordpress@ as a from address (as it shouldn't).

Edit to add more info: this is on my local machine, using Laravel Valet, so the webserver is [Caddy](https://caddyserver.com/)

Last edited 8 years ago by travisnorthcutt (previous) (diff)

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


8 years ago

#26 follow-up: @Clorith
8 years ago

It looks like caddy serves PHP over fastcgi, and might not be declaring some env variables (such as $_SERVER['SERVER_NAME']) that we rely on.

Have a look at https://www.nginx.com/resources/wiki/start/topics/examples/fastcgiexample/ which has information on how to declare them, I believe that would be beneficial to you here.

#27 in reply to: ↑ 26 ; follow-up: @joemcgill
8 years ago

Replying to Clorith:

It looks like caddy serves PHP over fastcgi, and might not be declaring some env variables (such as $_SERVER['SERVER_NAME']) that we rely on.

Have a look at https://www.nginx.com/resources/wiki/start/topics/examples/fastcgiexample/ which has information on how to declare them, I believe that would be beneficial to you here.

Possibly related: #25239

#28 in reply to: ↑ 27 @travisnorthcutt
8 years ago

Replying to joemcgill:

Possibly related: #25239

Yep, looks like that's exactly what I've run into. Thanks.

#29 @babakneza
8 years ago

Hi,
I found a simpler solution for this problem. I was go to my host cpanel create an email on my host (for example info@…) and in woocommerce settings> emails > Email Sender Options > "From" Address > I add that new email there. Then the error was fixed and wordpress is now working without Fatal error.
I hope this solution will help some one.

Note: See TracTickets for help on using tickets.