[wp-trac] [WordPress Trac] #15448: wp_mail() sets Content-Type header twice for multipart emails

[WordPress Trac]

#15448: wp_mail() sets Content-Type header twice for multipart emails
-------------------------------------------------+-------------------------
 Reporter:  rmccue                               |       Owner:
                                                 |  SergeyBiryukov
     Type:  feature request                      |      Status:  reviewing
 Priority:  normal                               |   Milestone:  Future
                                                 |  Release
Component:  Mail                                 |     Version:
 Severity:  normal                               |  Resolution:
 Keywords:  has-patch has-unit-tests has-test-   |     Focuses:
  info                                           |
-------------------------------------------------+-------------------------

Comment (by SirLouen):

 Replying to [comment:70 rmccue]:
 > Note that due to the `if ( ! empty( $headers ) )` guard around it, this
 will only be triggered if ''another'' header is added, and one which
 doesn't have special handling (from, content-type, cc, bcc, reply-to). The
 example code on the linked SE thread should do that via the MIME-Version
 header, but maybe it's not triggering properly. (You may also need to test
 with a "real" sendmail rather than MailHog.)

 My test report was my initial, very superfluous review.

 I've tested again, with Mailhog and your example, and I can't still see
 the double `multipart`. Maybe some more testing with `sendmail` to some
 other clients (other than basic Mailhog catchall client) is required.

 > Note that this patch goes beyond just fixing that bug to actually
 introduce multipart handling as a core capability.
 >
 > Aside: We're now so far down the track with this function as a pluggable
 one that retrofitting the ecosystem is going to be difficult. At the time
 I wrote the ticket, WP's market share was closer to 10% and there were
 many fewer drop-in replacements for `wp_mail()`. At this point it might be
 worth considering a new `wp_mail_multi()` instead.

 I've been doing further code review both in WP, PHPMailer, and the patch
 submitted, here are my conclusions:

 There are two main functions in PHPMailer that build the
 `multipart/alternative`
 [https://github.com/PHPMailer/PHPMailer/blob/master/src/PHPMailer.php#L3269-L3286
 SetMessageType] and
 [https://github.com/PHPMailer/PHPMailer/blob/mater/src/PHPMailer.php#L2834-L2875
 GetMailMIME]

 The first one decides if the message will be sent as a
 `multipart/alternative` if an `AltBody` exists. This was already covered
 in tests like [35617] and [54529]. So using `phpmailer_init` to set
 `$phpmailer` attributes has not been alien to WordPress in the past 10
 years (although, it's true, that this post hast 15 years in total). So
 [https://core.trac.wordpress.org/ticket/15448?replyto=70#comment:60 for
 some people who have been moaning] because this is not fully supported
 have no full reason for this, it's completely possible to achieve this
 with a simple workaround. Obviously, a full support is always more
 comfortable, but using the workaround is not the end of the world.

 == For anyone coming up new, a quick patch recap

 What the current patch is simply doing, is the following:

 First, is taking advantage of the fact that `wp_mail` already has access
 to `$phpmailer`, and setting `AltBody` accordingly for `multipart/alt` or
 `attachments` for `mixed` or `related` (which is set in PHPMailer)

 Then, it checks if the content of the email is a string or an array. If
 it's a string ([https://core.trac.wordpress.org/ticket/15448#comment:69
 like the test code in my example above]) or the code provided by @rmccue
 in the previous message, it will simply set it as text/plain and will
 probably not work (for now, check in the conclusion, maybe it could get
 fixed, but I believe this should be done in the PHPMailer side)

 But the real interesting thing that this patch does, is adding the
 possibility of passing an array to `wp_mail` `$message` which adds a new
 level of comfortability because building multipart emails this way is very
 convenient.

 == The PHPMailer dilemma

 If something could be sorted in PHPMailer, is the default `switch`
 condition
 [https://github.com/PHPMailer/PHPMailer/blob/0ff2d3c8ead628df2493537d9f91db962aa47718/src/PHPMailer.php#L2855-L2859
 at PHPMailer] seems to be the fallback reason of why a possible "double
 Content-Type" is appearing: The problem is that once set the Header, it
 will be sent as-is but Mailhog will add, again, another broken header,
 with the `default` condition, which is the multipart, without the
 boundary.

 This double-content type, could be sorted in PHPMailer, as @dd32 was
 suggesting for sure, if instead of using that default condition, it
 allowed some logic for headers completely defined by the user (considering
 that we are not passing the `AltBody` by default, in the current code).
 But it seems that PHPMailer, for some reason as a default behaviour, is
 expecting, the AltBody to be present, if we want to send a multipart.

 == Final Conclusion

 PHPMailer works perfectly for anything multipart. Proof of this is that
 the thing is working with `phpmailer_init` action hook. So at this point,
 I won't fully blame PHPMailer considering we have a very valid solution in
 the table.

 I have added a report upstream, to
 [https://github.com/PHPMailer/PHPMailer/issues/3173 support the string
 multipart]. But the current array implementation adds a extra degree of
 correctness. We could obviously, always parse the string content and set
 the AltBody accordingly, but I truly believe that this adds an extra layer
 of unnecesary complexity to the function. As @rmccue this could be content
 for a very specific `wp_mail_multi` function, which I think is an overkill
 (specially if we can happen to find a solution upstream).

 The thing, here, is considering whether this want to be supported natively
 or not with the array alternative. We have the patch, we have the testing
 (further testing will be ridden), we have multiple code reviews, and we
 have good unit tests and everything is passing. I can't really think of a
 reason on why not supporting this at this point for 6.9 with a nice `dev-
 note` as a new feature. I will put this on my reviewing list and let's see
 if we can onboard it again, for the next Milestone.

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/15448#comment:71>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform