Skip to content

Conversation

@SirLouen
Copy link
Member

Reviewed Patch 30128.2.patch with many improvements

== Explaining the patch

formatting.php

In both pluggable and formatting for the wp_staticize_emoji_for_email function, we have to apply the same headers parsing logic. This is a duplication that has existed for more than 10 years, so we have to bear with it. Maybe in the future a refactor can be done, but there are risks of BC, and since this function is only targeting the Content-Type, we will leave it for now.
Basically it takes the same logic as in 30128.2.patch and applies it here. Very straightforward: after exploding, some short-circuiting, if the array is not associative, and the content is not a simple string, then it's not going to be the Content-Type, so keep iterating.

pluggable.php
Similar logic. First, start iterating, but take into account the possibility that the header content can be an associative array; in that case, convert it into the basic comma-separated string before passing it to the switch
I'm relying a lot on the original algorithm provided by 30128.2.patch
From there, it will keep this as an array. This way it will also serve for the issue presented in #56779 that will become a duplicate after this patch.

wpMail.php
For the tests, I will be testing two things:

  1. The associative array and multiple associative array nature of the headers
  2. The possibility of multiple values with identical keys as part of the same header (more relevant to #56779, but because this patch is actually targeting this problem in parallel, we can hit two birds with one stone).

== Testing Information

  • To test this, you can do multiple scenarios, so you need to setup an email with multiple different headers:
  1. First: Add two custom headers with identical key.
  • Currently: It will only keep one of the two
  • After the patch: It should keep the two
  1. Second: Add associative arrays including the. From, the CC, the BCC and the Reply-To. It can be a combination of one, two, all, and even each of them could have another array within (for example the BCC can have an array of addresses). Be wary, that From cannot have more than one address, only CC, BCC and Reply-To can have multiple addresses
  • Currently: Arrays are not admitted
  • After the patch: You should see all the addresses introduced in the associative array after you send the email and check the result

Trac ticket: https://core.trac.wordpress.org/ticket/30128


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

@github-actions
Copy link

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props sirlouen.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@github-actions
Copy link

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

  • The Plugin and Theme Directories cannot be accessed within Playground.
  • All changes will be lost when closing a tab with a Playground instance.
  • All changes will be lost when refreshing the page.
  • A fresh instance is created each time the link below is clicked.
  • Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
    it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

if ( ! str_contains( $header, ':' ) ) {
foreach ( $headers as $key => $header ) {
if ( is_string( $header ) &&
strpos( $header, ':' ) === false &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why switch to strpos() from str_contains()?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having this option, why using a PHP 8.0 function?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WordPress includes a polyfill for str_contains()

if ( ! function_exists( 'str_contains' ) ) {
/**
* Polyfill for `str_contains()` function added in PHP 8.0.
*
* Performs a case-sensitive check indicating if needle is
* contained in haystack.
*
* @since 5.9.0
*
* @param string $haystack The string to search in.
* @param string $needle The substring to search for in the `$haystack`.
* @return bool True if `$needle` is in `$haystack`, otherwise false.
*/
function str_contains( $haystack, $needle ) {
if ( '' === $needle ) {
return true;
}
return false !== strpos( $haystack, $needle );
}
}

Copy link
Member Author

@SirLouen SirLouen Oct 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I knew that it used a polyfill that triggers that exact function. This is why it makes no sense to me to use str_contains in this context.

Also, now I remember that the original patch using strpos was made like one decade ago, and I felt that most of the logic was very appropriate, so I preferred to preserve it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I knew that it used a polyfill that triggers that exact function. This is why it makes no sense to me to use str_contains in this context.

Why are you reluctant to use it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I knew that it used a polyfill that triggers that exact function. This is why it makes no sense to me to use str_contains in this context.

Why are you reluctant to use it?

Not reluctant. Simply in the original logic in pluggable.php, it was using strpos, so I respected, and wanted to match both scenarios. Since its basically identical, I don't really care one or another.

Comment on lines +6185 to +6186
// Skip because Content-Type must be an string.
if ( ! is_string( $name ) || ! is_string( $content ) ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if someone $content here is array( 'text/html' )?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you show me an example of how you can make this array possible?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per the Trac ticket description: https://core.trac.wordpress.org/ticket/30128

$headers = [
	'From' => 'Me Myself <me@example.net>',
	'Cc'   => [
		'John Q Codex <jqc@wordpress.org>',
		'iluvwp@wordpress.org',
	],
];

What if someone provided:

$headers = [
	'Content-Type' => [
		'text/html',
	],
	'Cc'   => [
		'John Q Codex <jqc@wordpress.org>',
		'iluvwp@wordpress.org',
	],
];

Where arrays are used for every value for consistency.

Relatedly, there's some interesting behaviors when you try to send multiple Content-Type headers, as I found recently: #8412 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, I did not consider adding this to the mix, but technically it is plausible.

I will give this a second thought.

@SirLouen SirLouen requested a review from westonruter October 13, 2025 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants