-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Allow to use associative arrays beside indexed arrays in wp_mail $headers #9971
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: trunk
Are you sure you want to change the base?
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.
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()?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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()
wordpress-develop/src/wp-includes/compat.php
Lines 402 to 422 in 6b006e9
| 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 ); | |
| } | |
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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_containsin this context.
Why are you reluctant to use it?
There was a problem hiding this comment.
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_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.
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' )?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
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.