Make WordPress Core

Opened 14 years ago

Last modified 4 years ago

#15448 reviewing enhancement

wp_mail() sets Content-Type header twice for multipart emails

Reported by: rmccue's profile rmccue Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Mail Keywords: gci has-patch westi-likes needs-testing
Focuses: Cc:

Description

When trying to send emails via wp_mail() with a Content-Type of multipart/alternative, the Content-Type header will be set with $phpmailer->ContentType, and again with $phpmailer->AddCustomHeader(), which causes two Content-Type headers in the email:

Content-Type: multipart/alternative;
	 boundary="example_boundary"
Content-Type: multipart/alternative; charset=""

This appears to cause errors in Outlook, as there is no boundary on the latter.

The cause of this is PHPMailer::GetMailMIME(), as it does not know that the email is a multipart email. The easiest way to achieve this appears to be to simply allow the user to set the AltBody via wp_mail(). In order to achieve backwards compatibility, wp_mail() should work out which part is the text/plain one and which is the text/html one based on the boundary.

I'll be working on a patch for this.

Attachments (11)

15448.diff (4.2 KB) - added by rmccue 14 years ago.
Initial patch. Add support for text/html and text/plain
15448-multi.diff (4.3 KB) - added by rmccue 14 years ago.
Add support for multiple attachments of the same type
15448_March2013.diff (4.5 KB) - added by MattyRob 12 years ago.
15448-unittests.diff (1.3 KB) - added by MikeHansenMe 10 years ago.
see #30284
15448_June2015.diff (5.1 KB) - added by gitlost 9 years ago.
Refresh of @rmccue/@MattyRob patch 15448_March2013.diff for 4.2.2.
15448_Sept2015.diff (5.1 KB) - added by gitlost 9 years ago.
Refresh of @rmccue/@MattyRob patch 15448_March2013.diff for 4.4.
15448_Sept2015-unittests.diff (1.3 KB) - added by gitlost 9 years ago.
Refresh of @MikeHansenMe patch 15448-unittests.diff for 4.4.
15448_Jan2016.diff (5.1 KB) - added by gitlost 9 years ago.
Refresh of @rmccue/@MattyRob patch 15448_March2013.diff for 4.5.
15448_Feb2016.diff (5.1 KB) - added by gitlost 9 years ago.
Updated to use coding standards (changed bits only).
15448_Sep2016.diff (7.9 KB) - added by gitlost 8 years ago.
Refresh for 4.7. Includes unit tests.
15448_Sep2019.11.diff (8.3 KB) - added by bgermann 5 years ago.
Refresh of the patch

Download all attachments as: .zip

Change History (78)

#1 @rmccue
14 years ago

  • Owner set to rmccue
  • Status changed from new to assigned

One thought on how to achieve this was passing the body as an array instead of a string to wp_mail(), with keys being the Content-Type for each. Then, if only text/html and text/plain are set, use AltBody for the text and Body for the HTML. If anything else is set, we'll use PHPMailer's attachment system fully instead.

Thoughts?

#2 @westi
14 years ago

It sounds good to accept an array of alternative versions.

Treat the non-array case either as we do now or as text/plain

#3 @rmccue
14 years ago

If the body is a string, we'll use the current parsing (i.e. get Content-Type from the $headers parameter), otherwise, we'll ignore the Content-Type header from there and use the key for each item.

My only concern is adding custom headers to each type. We could accept something like this, I guess:

$body = array(
	'text/plain' => array(
		'headers' => array(
			'X-Type' => 'text'
		),
		'body' => 'xyz'
	)
);

#4 @westi
14 years ago

Do people need type specific custom headers that often would be my question?

Does supporting multiple content types not remove the need for some of the custom headers?

#5 @rmccue
14 years ago

I agree. Perhaps more elegant would be to simply have a wp_mail_headers_$type filter. Then, the $message parameter we accept could be:

$message = array(
	'text/plain' => 'xyz',
	'text/html' => '<b>xyz</b>'
);

I much prefer this, but it does involve adding an extra filter if people need custom headers.

@rmccue
14 years ago

Initial patch. Add support for text/html and text/plain

#6 follow-up: @rmccue
14 years ago

Turns out you *can't* have custom headers with PHPMailer anyway.

The patch I've included allows you to use an array such as the one I mentioned above. What I'd like to do is have the ability to add inline attachments (e.g. images), but I'm not sure how one would achieve that. With images, you're likely to have more than one, and if they're the same type, you're slightly screwed.

Perhaps allowing the following would be better:

$message = array(
	'text/plain' => 'xyz',
	'text/html' => '<b>xyz</b>',
	'image/png' => array(
		'file1.png',
		'file2.png'
	)
);

But we still then have the issue of working of if it's inline or an attachment. We might be able to do this based on the type, but I'm thinking a filter would work well for it. I'll submit a patch to that effect later.

#7 @nacin
14 years ago

  • Milestone changed from Awaiting Review to Future Release

#8 @westi
14 years ago

  • Keywords gci added

#9 in reply to: ↑ 6 @westi
14 years ago

Replying to rmccue:

Turns out you *can't* have custom headers with PHPMailer anyway.

The patch I've included allows you to use an array such as the one I mentioned above. What I'd like to do is have the ability to add inline attachments (e.g. images), but I'm not sure how one would achieve that. With images, you're likely to have more than one, and if they're the same type, you're slightly screwed.

Perhaps allowing the following would be better:

$message = array(
	'text/plain' => 'xyz',
	'text/html' => '<b>xyz</b>',
	'image/png' => array(
		'file1.png',
		'file2.png'
	)
);

But we still then have the issue of working of if it's inline or an attachment. We might be able to do this based on the type, but I'm thinking a filter would work well for it. I'll submit a patch to that effect later.

This is looking good.

I just checked the "Unit" Tests we have and found that we do have some for the wp_mail function :-)

Would be good to add some more test cases for your new functionality if you get time but you should at least check that the current ones pass :-D

@rmccue
14 years ago

Add support for multiple attachments of the same type

#10 follow-up: @westi
14 years ago

Looks good.

I've added a basic test for this new functionality in [UT322]

#11 in reply to: ↑ 10 ; follow-up: @rmccue
14 years ago

Replying to westi:

I've added a basic test for this new functionality in [UT322]

FYI, it's boundary ;)

Also, it doesn't appear as though the charset matches the blog's character set, but I'm not sure on that. Probably worth a unit test for that too.

#12 in reply to: ↑ 11 @westi
14 years ago

Replying to rmccue:

Replying to westi:

I've added a basic test for this new functionality in [UT322]

FYI, it's boundary ;)

Also, it doesn't appear as though the charset matches the blog's character set, but I'm not sure on that. Probably worth a unit test for that too.

:-) Spilling fail

Unit Tests++

#13 @westi
14 years ago

  • Milestone changed from Future Release to 3.2
  • Owner changed from rmccue to westi
  • Status changed from assigned to reviewing

I would like to roll this small enhancement from GCI into 3.2

#14 @nacin
13 years ago

I think the main check for the $message should be is_array() and else, rather than is_string() elseif is_array(). Patch looks good.

#15 @nacin
13 years ago

  • Keywords has-patch added

#16 @nacin
13 years ago

Per "This appears to cause errors in Outlook," this sounds like an enhancement that fixes a bug? westi, your call on this one. Looked good a few weeks ago for me.

#17 @ramoonus
13 years ago

  • Component changed from General to Mail

#18 follow-up: @Kleor
13 years ago

Another bug with the wp_mail function in WordPress 3.2. It doesn't work if we write this:

wp_mail('Example <me@example.net>', 'The subject', 'The message');

We must write this instead:

wp_mail('me@example.net', 'The subject', 'The message');

#19 in reply to: ↑ 18 @westi
13 years ago

Replying to Kleor:

Another bug with the wp_mail function in WordPress 3.2. It doesn't work if we write this:

wp_mail('Example <me@example.net>', 'The subject', 'The message');

We must write this instead:

wp_mail('me@example.net', 'The subject', 'The message');

That has already been fixed what build are you testing with?

#20 follow-up: @Kleor
13 years ago

3.2-beta1

#21 in reply to: ↑ 20 @westi
13 years ago

Replying to Kleor:

3.2-beta1

Which was [17903] the $to issues was fixed in [18006] for #17305 please upgrade to a newer nightly build

#22 @westi
13 years ago

  • Keywords 3.3-early westi-likes added
  • Milestone changed from 3.2 to Future Release

I think this still counts as an enhancement rather than a bug.

At the moment we don't really support multipart emails.

Marking for 3.3

#23 @kitchin
13 years ago

The two patches so far both have a major error. If wp_mail() is called twice, AltBody is not cleared. Suggest you add these two lines:

	$phpmailer->ClearCCs();
	$phpmailer->ClearCustomHeaders();
	$phpmailer->ClearReplyTos();

+	$phpmailer->Body= '';
+	$phpmailer->AltBody= '';

	// From email and name
	// If we don't have a name from the input headers
	if ( !isset( $from_name ) )

A test for the error in the patch is as follows: call wp_mail() with an array message to send an HTML email. Then call wp_mail() with a string message to send a different plain email. The second message will be send multi-part, with the previous message in one part and the current message in the other part. Security problem in some uses.

The patch needs to be updated anyway, in light of Bug #17305, so I would also suggest incorporating the improvement I suggested there to allow '<foo@…>' in $to and to clean up the regex:

			// Break $recipient into name and address parts if in the format "Foo <bar@baz.com>"
			$recipient_name = '';
-			if( preg_match( '/(.+)\s?<(.+)>/', $recipient, $matches ) ) {
+			if( preg_match( '/(.*)<(.+)>/', $recipient, $matches ) ) {
				if ( count( $matches ) == 3 ) {
					$recipient_name = $matches[1];
					$recipient = $matches[2];
				}
			}
-			$phpmailer->AddAddress( trim( $recipient ), $recipient_name);
+			$phpmailer->AddAddress( trim( $recipient ), trim( $recipient_name) );
		} catch ( phpmailerException $e ) {
			continue;
..................
				// Break $recipient into name and address parts if in the format "Foo <bar@baz.com>"
				$recipient_name = '';
-				if( preg_match( '/(.+)\s?<(.+)>/', $recipient, $matches ) ) {
+				if( preg_match( '/(.*)<(.+)>/', $recipient, $matches ) ) {
					if ( count( $matches ) == 3 ) {
						$recipient_name = $matches[1];
						$recipient = $matches[2];
					}
				}
-				$phpmailer->AddCc( trim($recipient), $recipient_name );
+				$phpmailer->AddCc( trim($recipient), trim($recipient_name) );
			} catch ( phpmailerException $e ) {
				continue;
..................
				// Break $recipient into name and address parts if in the format "Foo <bar@baz.com>"
				$recipient_name = '';
-				if( preg_match( '/(.+)\s?<(.+)>/', $recipient, $matches ) ) {
+				if( preg_match( '/(.*)<(.+)>/', $recipient, $matches ) ) {
					if ( count( $matches ) == 3 ) {
						$recipient_name = $matches[1];
						$recipient = $matches[2];
					}
				}
-				$phpmailer->AddBcc( trim($recipient), $recipient_name );
+				$phpmailer->AddBcc( trim($recipient), trim($recipient_name) );
			} catch ( phpmailerException $e ) {
				continue;

#24 @kitchin
13 years ago

Security-wise, it would be best to also clear phpmailer at the end of wp_mail(), actually.

#25 @wojtek.szkutnik
13 years ago

This patch has been merged with the "enhanced emails" project code. See #18493 .

#26 @anonymized_7461715
12 years ago

This is still a problem. Constructing one's own multipart/mixed email and sending it through wp_mail() results in attachments not being read properly by Hotmail's e-mail program (Gmail and RoundCube work fine). This is due to wp_mail() adding headers so that e.g. in totality, the following headers and body are produced:

MIME-Version: 1.0
Content-Type: multipart/mixed;
	 boundary="==Multipart_Boundary_x{82b884f3b88a46c15d210eda4b90a8f7}x"
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Content-Type: multipart/mixed; charset=""

--==Multipart_Boundary_x{82b884f3b88a46c15d210eda4b90a8f7}x
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit

Dear Recipient,
...

In this case, the lines

MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Content-Type: multipart/mixed; charset=""

are the ones added by wp_mail(), thus causing Hotmail to ignore any multiparts due to the second Content-Type definition overriding the first.

Could we not just add a flag to wp_mail() that desists from adding any headers other than the ones supplied to it by the function call?

#27 @rmccue
12 years ago

  • Keywords needs-refresh added; 3.3-early removed
  • Milestone changed from Future Release to 3.6
  • Owner changed from westi to nacin

Marking as needs-refresh for now, as it may need parts backported from #18493. I should get to this within a day, if not, yell in my direction. :)

#28 @MattyRob
12 years ago

Attaching updated diff from core code revision 686217.

Having looked at #18493, I feel some of those changes (i.e. default to require a template file) may break plugins currently using the wp_mail() function so I haven't back ported anything from that ticket.

#29 @ryan
11 years ago

  • Milestone changed from 3.6 to Future Release

#30 @SergeyBiryukov
10 years ago

#31653 was marked as a duplicate.

@gitlost
9 years ago

Refresh of @rmccue/@MattyRob patch 15448_March2013.diff for 4.2.2.

#31 @christinecooper
9 years ago

Just to highlight that sending multipart emails (html/text) with wp_mail() (to reduce chance of emails ending up in spam folders) will ironically result with your domain being blocked by Hotmail (and other Microsoft emails).

We are currently discussing this severe issue over at WPSE:

http://wordpress.stackexchange.com/questions/191923/sending-multipart-text-html-emails-via-wp-mail-will-likely-get-your-domain-b

This issue has been pending for 5 years and it hasn't appeared as if there is any interest to implement a fix to core. Maybe it is about that time to invest and resolve this properly.

The patch posted above 15448_June2015.diff​ works.

Consider implementing this solution.

@gitlost
9 years ago

Refresh of @rmccue/@MattyRob patch 15448_March2013.diff for 4.4.

@gitlost
9 years ago

Refresh of @MikeHansenMe patch 15448-unittests.diff for 4.4.

#32 @gitlost
9 years ago

  • Keywords needs-refresh removed

Refreshes for gardening drive.

@gitlost
9 years ago

Refresh of @rmccue/@MattyRob patch 15448_March2013.diff for 4.5.

#33 @gitlost
9 years ago

Refresh for 4.5 Bug Scrub (just removes "Hunk succeeded" messages). 15448_Sept2015-unittests.diff still good.

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


9 years ago

@gitlost
9 years ago

Updated to use coding standards (changed bits only).

#36 @JohnVieth
8 years ago

How close are we to getting the patch added to core? Is there anything I can do to help? We need to get this fixed. Multipart emails is a basic necessity for a lot of WordPress plugin development.

#37 @dawidadach
8 years ago

Gents,

is there any update on that? It is really crucial...

There is already possible solution, why don't include it in a core?
http://wordpress.stackexchange.com/questions/191923/sending-multipart-text-html-emails-via-wp-mail-will-likely-get-your-domain-b

@gitlost
8 years ago

Refresh for 4.7. Includes unit tests.

#38 @gitlost
8 years ago

Refresh for WP 4.7, with unit tests included in the one patch. Added extra test to make sure the workarounds using phpmailer_init mentioned in WPSE post still work, around.

#39 follow-up: @JeffMatson
8 years ago

Is there anything else needed on this? Why hasn't this been merged?

#40 in reply to: ↑ 39 @MattyRob
8 years ago

Replying to JeffMatson:

Is there anything else needed on this? Why hasn't this been merged?

Maybe some testing and for a committer to pick it up.

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


8 years ago

#42 @SergeyBiryukov
8 years ago

  • Milestone changed from Future Release to 4.8

#43 @JeffMatson
8 years ago

#39642 was marked as a duplicate.

#44 in reply to: ↑ description @dd32
8 years ago

When trying to send emails via wp_mail() with a Content-Type of multipart/alternative, the Content-Type header will be set with $phpmailer->ContentType, and again with $phpmailer->AddCustomHeader(), which causes two Content-Type headers in the email:

Content-Type: multipart/alternative;
	 boundary="example_boundary"
Content-Type: multipart/alternative; charset=""

As far as I can tell, the original issue for this ticket, being that two Content-Type headers would be added, was fixed in a PHPMailer update some time ago - I can't find when, although I'll note PHPMailer 5.2.10 added the This is a multi-part message in MIME format. filler line which would've fixed the final Outlook-related parse failures.

I'll note that there's zero code examples posted on the ticket other than a unit test using MockMailer - without example code it's incredibly hard to actually verify if an issue still exists, and if so, if the proposed patches actually fix it.

Using the following code:

$html_message = '<html><head><meta http-equiv="Content-Type" content="text/html; charset=utf-8" /></head><body><p>Hello World!</p></body></html>';
add_action( 'phpmailer_init', $alt_function = function( $mailer ) { $mailer->AltBody = strip_tags( $mailer->Body ); } );
wp_mail( $to, $subject, $html_message );
remove_action( 'phpmailer_init', $alt_function );

will result in the following email:

From: WordPress <wordpress@localhost.localdomain>
Message-ID: <8bd7f4a25da2084a2a803d40a4c823e2@centos>
X-Mailer: PHPMailer 5.2.22 (https://github.com/PHPMailer/PHPMailer)
MIME-Version: 1.0
Content-Type: multipart/alternative;
        boundary="b1_8bd7f4a25da2084a2a803d40a4c823e2"
Content-Transfer-Encoding: 8bit

This is a multi-part message in MIME format.

--b1_8bd7f4a25da2084a2a803d40a4c823e2
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Hello World!

--b1_8bd7f4a25da2084a2a803d40a4c823e2
Content-Type: text/html; charset=UTF-8
Content-Transfer-Encoding: 8bit

<html><head><meta http-equiv="Content-Type" content="text/html; charset=utf-8" /></head><body><p>Hello World!</p></body></html>

--b1_8bd7f4a25da2084a2a803d40a4c823e2--

That being said, I don't mind the approaches taken here - extending the $message parameter of wp_mail() to accept an array of mime typed contents. However, with the above in light that I can't duplicate the original issue, I'm curious if the additional code in 15448_Sep2016.diff is actually needed anymore.

The interactions between this and #18493 could end up weird - however, I expect that this use-case would just override any future HTML email template functionality core added.

#45 follow-up: @MattyRob
8 years ago

Proof of concept:

function mail_check() {
        $to = "recipient@example.com";

        $subject = 'wp_mail testing multipart';
        $message = 'Hello world! This is plain text...';

        $headers = "MIME-Version: 1.0\r\n";
        $headers .= "From: sender@example.com\r\n";
        $headers .= 'Content-Type: multipart/alternative;boundary="----=_Part_18243133_1346573420.1408991447668"';


        // send email
        wp_mail( $to, $subject, $message, $headers );
}
add_action( 'shutdown', 'mail_check' );

Produces the following in WordPress 4.7.2:

From: WordPress <wordpress@localhost.localdomain>
Message-ID: <183f1675f0731e7c2d3b822eb0370c1f>
X-Mailer: PHPMailer 5.2.22 (https://github.com/PHPMailer/PHPMailer)
MIME-Version: 1.0
Content-Type: multipart/alternative;
	 boundary="----=_Part_18243133_1346573420.1408991447668"
MIME-Version: 1.0
Content-Type: multipart/alternative; charset=

Hello world! This is plain text...

#46 in reply to: ↑ 45 @dd32
8 years ago

Replying to MattyRob:

Proof of concept:

Thanks @MattyRob, with that I was able to duplicate the scenario. I don't actually think it's a valid use of PHPMailer myself, but respect that people use all sorts of hacks to send email..

#47 @MattyRob
8 years ago

@dd32

I agree - my PoC is very sub-optimal. Patching the wp_mail() function to handle multipart emails directly should make developers lives easier, account for sub-optimal use of wp_mail() and perhaps increase deliverability too.

Any chance of getting this in the 4.7.x branch at some point?

#48 @dd32
8 years ago

Any chance of getting this in the 4.7.x branch at some point?

A bugfix to remove the duplicate headers - likely, although, I have a feeling that it should be fixed in PHPMailer instead of in WordPress (Based on the code I've read in PHPMailer, it looks like it's detecting the email type wrong - and it is, which is why the duplicate header is added).

Accepting text/html + text/plain as args in wp_mail() will probably be a 4.8 thing, just based on needing to have a much longer testing and bug-finding period being needed.

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


7 years ago

#50 @jbpaul17
7 years ago

  • Milestone changed from 4.8 to 4.8.1

Per yesterday's bug scrub, we're going to punt this to 4.8.1.

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


7 years ago

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


7 years ago

#53 @jbpaul17
7 years ago

  • Milestone changed from 4.8.1 to 4.9

Per today's bug scrub, we'll punt this as the focus for 4.8.1 is regressions only.

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


7 years ago

#55 @jbpaul17
7 years ago

  • Milestone changed from 4.9 to Future Release

Punting to Future Release per today's 4.9 bug scrub.

#56 follow-up: @bgermann
7 years ago

How about including this in WP 5.0?

#57 in reply to: ↑ 56 @munklefish
7 years ago

Replying to bgermann:

How about including this in WP 5.0?

Do you mean WP 500 ?

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


6 years ago

#59 @bgermann
6 years ago

How about including this in WP 5.2?

#60 @tofandel
5 years ago

There is so many bugs with patches that are not released/making it into core...

It's outrageous...

Just keep working on gutenberg and let the rest of the code rot.

Good job the core team. I'm switching to laravel.

@bgermann
5 years ago

Refresh of the patch

#61 @bgermann
5 years ago

How about including this in WP 5.3?

Version 0, edited 5 years ago by bgermann (next)

#62 @SergeyBiryukov
5 years ago

  • Milestone changed from Future Release to 5.3
  • Owner changed from nacin to SergeyBiryukov

#63 @davidbaumwald
5 years ago

@SergeyBiryukov Can this be included in Beta 1 for version 5.3 today? If not, let's move this to 5.4.

#64 @davidbaumwald
5 years ago

  • Milestone changed from 5.3 to Future Release

With version 5.3 Beta 1 releasing shortly, the deadline for enhancements is now passed. This is being moved to Future Release. @SergeyBiryukov if you feel this can be included in 5.4, feel free to move up the milestone.

#65 in reply to: ↑ description @imokweb
4 years ago

What can we do to have this added in a future release?

The patches suggested are working for me and I've spent lots of hours until I found this ticket that actually the content-type: multipart/alternative is broken in wp_mail.

Thanks!
I.

#67 @JeffPaul
4 years ago

  • Keywords needs-testing added
Note: See TracTickets for help on using tickets.