Allow to use associative arrays beside indexed arrays in wp_mail $headers#9971
Allow to use associative arrays beside indexed arrays in wp_mail $headers#9971SirLouen wants to merge 6 commits intoWordPress:trunkfrom
Conversation
|
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 Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Test using WordPress PlaygroundThe 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
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
| if ( ! str_contains( $header, ':' ) ) { | ||
| foreach ( $headers as $key => $header ) { | ||
| if ( is_string( $header ) && | ||
| strpos( $header, ':' ) === false && |
There was a problem hiding this comment.
Why switch to strpos() from str_contains()?
There was a problem hiding this comment.
Having this option, why using a PHP 8.0 function?
There was a problem hiding this comment.
WordPress includes a polyfill for str_contains()
wordpress-develop/src/wp-includes/compat.php
Lines 402 to 422 in 6b006e9
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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_containsin this context.
Why are you reluctant to use it?
There was a problem hiding this comment.
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_containsin 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.
| // Skip because Content-Type must be an string. | ||
| if ( ! is_string( $name ) || ! is_string( $content ) ) { |
There was a problem hiding this comment.
What if someone $content here is array( 'text/html' )?
There was a problem hiding this comment.
Can you show me an example of how you can make this array possible?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
True, I did not consider adding this to the mix, but technically it is plausible.
I will give this a second thought.
Reviewed Patch 30128.2.patch with many improvements
== Explaining the patch
formatting.phpIn 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.phpSimilar 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.patchFrom 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.phpFor the tests, I will be testing two things:
== Testing Information
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.