[wp-trac] [WordPress Trac] #56779: wp_mail header ignores multiple headers with same name

[WordPress Trac]

#56779: wp_mail header ignores multiple headers with same name
-------------------------------------------------+-------------------------
 Reporter:  pentatonicfunk                       |       Owner:  (none)
     Type:  defect (bug)                         |      Status:  new
 Priority:  normal                               |   Milestone:  Awaiting
                                                 |  Review
Component:  Mail                                 |     Version:
 Severity:  major                                |  Resolution:
 Keywords:  has-test-info good-first-bug 2nd-    |     Focuses:
  opinion has-patch                              |
-------------------------------------------------+-------------------------
Changes (by SirLouen):

 * keywords:  has-test-info good-first-bug has-patch needs-testing => has-
     test-info good-first-bug 2nd-opinion has-patch


Comment:

 Hey @rishabhwp sorry for the late reply
 Thanks for submitting a patch.

 It looks good, but it feels a little redundant despite I can clearly see
 how this could be potentially causing some backwards compatibility issues.

 For example here

 {{{
 // Handle multipart boundary if present.
                 if ( ! empty( $headers ) ) {
                         if ( false !== stripos( $content_type, 'multipart'
 ) && ! empty( $boundary ) ) {
                                 $phpmailer->addCustomHeader( sprintf(
 'Content-Type: %s; boundary="%s"', $content_type, $boundary ) );
                         }
                 }
 }}}

 `$headers` is not being used at all, its just a check to see if we have to
 build manually the boundary or not (which in fact is completely broken
 right now, because PHPMailer doesn't support this format, so basically
 those 3 lines are completely useless.

 == Decision Required

 The problem appears here:
 https://github.com/SirLouen/wordpress-
 develop/blob/965ee6152caa8a3e2bea2bf18e405220e45c53df/src/wp-
 includes/pluggable.php#L296-L297

 The original headers should be correctly formatted to accept pretty much
 any scenario. But there, they are flattened to form a simple 2 dimensional
 associative array.

 So basically the key here is to create some Unit Tests, and check if we
 can simply upgrade `$headers` to a array of associative array structure
 and use it the way you are using it, without breaking the system, or keep
 the header structure flat, but in the original form.

 The only risky point I see for BC issues is in `wp_mail_succeeded` action,
 since most users extending this hook, might be taking the simple string
 array, instead of a new improved version with an associative array.

 The `pre_wp_mail`, `wp_mail` filters hooks are taking the headers as they
 are originally. But the `wp_mail_succeeded` is taking the wrong watered
 down version

 But this version is intrinsically buggy (as the bug here itself). We could
 flatten the associative array (as its being done right now) but with the
 original format, or we could simply break the backwards compatibility in
 this action with the array of associative arrays. In both cases, if
 extenders are consuming this `$headers` like a simple associative array,
 they are going to find their functions broken, so I would prefer the later
 option which makes all the sense to me because users are consuming wrongly
 designed headers.

 Once we sort out this, we can continue with the patch.

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