General: Replace core's remaining utf8_encode() calls. - #12920
General: Replace core's remaining utf8_encode() calls.#12920itzmekhokan wants to merge 3 commits into
utf8_encode() calls.#12920Conversation
…b_utf8()`.
`wxr_cdata()` and `wp_read_image_metadata()` hold the last three calls to
`utf8_encode()` in core. That function was deprecated in PHP 8.2, is removed in
PHP 9.0, and core itself polyfilled it with a deprecation notice in [60950], so
these sites emit a deprecation notice on every affected export and image upload.
All three used the `if ( ! wp_is_valid_utf8( $text ) ) { utf8_encode( $text ); }`
pattern, which assumes any text failing UTF-8 validation is ISO-8859-1 and
re-encodes the raw bytes on that assumption. That guess is wrong for every other
single-byte encoding and silently produces mojibake. Replacing the invalid spans
with the Unicode replacement character neutralizes the corruption without
inventing an encoding, using the UTF-8 pipeline core added in 6.9.
Adds regression tests covering the WXR export and IPTC metadata that carry
invalid UTF-8.
See #65828, #55603.
dmsnell
left a comment
dmsnell
left a comment
There was a problem hiding this comment.
without commenting on the change itself I have left a few code-level notes.
| * Ensures the WXR export neutralizes invalid UTF-8 instead of reinterpreting it as ISO-8859-1. | ||
| * | ||
| * `wxr_cdata()` previously called the deprecated `utf8_encode()`, which assumed any | ||
| * string that failed UTF-8 validation was ISO-8859-1 and re-encoded the raw bytes on |
There was a problem hiding this comment.
see note below, but noting in a comment how code used to work can be interesting, but focuses on code that no longer exists.
| 'Never-valid byte' => array( "a\xC0b", "a\u{FFFD}b" ), | ||
| 'Truncated sequence' => array( "a\xE2\x9Cb", "a\u{FFFD}b" ), | ||
| 'Overlong sequence' => array( "a\xC1\xBFb", "a\u{FFFD}\u{FFFD}b" ), | ||
| 'Surrogate half' => array( "a\xED\xA0\x80b", "a\u{FFFD}\u{FFFD}\u{FFFD}b" ), |
There was a problem hiding this comment.
these tests are asserting invalid UTF-8 as we tend to think about it, but the noted behavioral change is about alternate input encodings, none of which are demonstrated in these tests.
something that can be clearer than leaving a comment about previous bugs is to leave tests demonstrating the current behaviors. i.e. drop the comment “previously” and add test cases demonstrating the behavior in the presence of alternative inputs.
'Non-UTF-8-compatible input' => array(
mb_convert_encoding( 'wyróżnij', 'ISO-8859-2', 'UTF-8' ),
"wyr\u{FFFD}nij"
)| } | ||
|
|
||
| foreach ( $meta['keywords'] as $key => $keyword ) { | ||
| if ( ! wp_is_valid_utf8( $keyword ) ) { |
There was a problem hiding this comment.
there’s no value in calling wp_is_valid_utf8() before calling wp_scrub_utf8() as the latter does the former internally.
- `wp_scrub_utf8()` validates internally, so the preceding `wp_is_valid_utf8()` checks were redundant. The `$meta[ $key ]` truthiness check stays, as `iso` defaults to int `0`. - Test docblocks describe current behaviour rather than narrating the code that was removed. - Adds data-provider cases for text in ISO-8859-1, ISO-8859-2, Windows-1251 and Windows-1252, which exercise the actual behavioural change; the previous cases only covered malformed UTF-8. See #65828.
|
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. |
Per comment:5 and comment:6, replacing the invalid bytes with U+FFFD regresses sites whose text really is ISO-8859-1, which is likely most non-UTF-8 content. The image in #35316 is the concrete case: its Slovak keywords are readable today and become a run of replacement characters under `wp_scrub_utf8()`. Removing the deprecated call does not require changing the output. Adds a private `_wp_iso_8859_1_to_utf8()` alongside the other UTF-8 helpers, following the mbstring/fallback split already used in that file, and points the three call sites at it. `mb_convert_encoding( $text, 'UTF-8', 'ISO-8859-1' )` is the replacement core's own polyfill docblock prescribes and is byte-identical to `utf8_encode()` across all 256 byte values; the no-mbstring branch reuses `_wp_utf8_encode_fallback()`, which is already tested for that equivalence in `Tests_DeprecatedUtf8EncodeDecodeTest`. The `wp_is_valid_utf8()` guards are restored, since `mb_convert_encoding()` does not validate first. The tests now pin the preserved behaviour rather than the scrubbed output, including the #35316 keywords and a case showing that one stray byte sends the valid portions of a string through the conversion too. See #65828, #35316, #55603.
itzmekhokan
changed the title
utf8_encode() calls with wp_scrub_utf8()utf8_encode() calls.
2 participants
wxr_cdata()andwp_read_image_metadata()hold the last three calls toutf8_encode()in core, which PHP deprecated in 8.2 and removes in 9.0, and which core polyfilled with its own_deprecated_function()notice in [60950] — so these call sites emit a deprecation notice from core's own code on every affected export and image upload.This points them at a new private
_wp_iso_8859_1_to_utf8()inwp-includes/utf8.php, following the mbstring/fallback split that file already uses.There is no behaviour change:
mb_convert_encoding( $text, 'UTF-8', 'ISO-8859-1' )is byte-identical toutf8_encode()across all 256 byte values, the no-mbstring branch reuses the existing_wp_utf8_encode_fallback(), and thewp_is_valid_utf8()guards are unchanged.The tests pin that preserved output, including the keywords from the image attached to #35316.
Verified: 36/36 in the two touched test classes, no failures in the
image,unicode,formatting,exportandadmingroups, and PHPCS reports no new errors or warnings.Trac ticket: https://core.trac.wordpress.org/ticket/65828
Use of AI Tools
AI assistance: Yes
Tool(s): Claude
Model(s): Opus 5
Used for: initial exploration, tests, and ticket and PR details. All changes are reviewed and validated by me.
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.