Make WordPress Core

Opened 5 weeks ago

Last modified 4 weeks ago

#65828 new defect (bug)

Replace core's remaining utf8_encode() calls in wxr_cdata() and wp_read_image_metadata()

Reported by: khokansardar Owned by:
Priority: normal Milestone: Future Release
Component: General Version: 6.9
Severity: normal Keywords: has-patch has-unit-tests
Cc: Focuses: coding-standards

Description (last modified by sabernhardt)

Core calls utf8_encode() in three remaining places:

  • src/wp-admin/includes/export.php:249wxr_cdata(), on every WXR export containing non-UTF-8 bytes.
  • src/wp-admin/includes/image.php:1052wp_read_image_metadata(), per EXIF/IPTC meta key.
  • src/wp-admin/includes/image.php:1058wp_read_image_metadata(), per IPTC keyword.

utf8_encode() was deprecated in PHP 8.2 and is removed in PHP 9.0. Core polyfilled it with its own _deprecated_function() notice in [60950], so core is now emitting a deprecation notice from its own code:

  • PHP 8.2–8.4 — the native function still exists, so PHP raises E_DEPRECATED.
  • PHP 9.0 — the native function is gone, core's polyfill runs and raises the _deprecated_function notice.

The deeper problem

All three sites use the same shape:

if ( ! wp_is_valid_utf8( $str ) ) {
      $str = utf8_encode( $str );
}

This assumes that 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 — Windows-1252 text, which is far more common in the wild, has its 0x80–0x9F range silently mapped to C1 control characters — and it is wrong for every multi-byte encoding. The result is silent mojibake written into a WXR backup or into post meta.

This is the concern @dmsnell raised in comment 99 and comment 93 of #55603: the problem is not merely that these functions are deprecated, but that in the absence of concrete knowledge about a text encoding we should avoid assuming that it's ISO-8859-1. IPTC in particular carries no reliable encoding declaration.

Proposed change

Replace the calls with wp_scrub_utf8(), introduced in 6.9 for exactly this purpose — its documentation notes that some contexts (e.g. generating XML) require valid input strings. Invalid byte spans are replaced with the Unicode replacement character (U+FFFD) rather than reinterpreted as an encoding core cannot know.

No new API, and the wp_is_valid_utf8() guards are unchanged.

Behaviour change

This is a deliberate, visible change and should be reviewed as such.

  • Before: invalid bytes are reinterpreted as ISO-8859-1. Correct only when the input genuinely was ISO-8859-1; silent corruption otherwise.
  • After: invalid bytes become U+FFFD. Never guesses an encoding, but the original bytes are not recoverable.

For genuinely-latin1 input this trades a correct conversion for a lossy one. For every other encoding it trades silent corruption for visible, well-defined replacement. Per comment 93, a refactor here has to choose whether to preserve the defects or reject unsupported inputs; this patch rejects the unsound guess.

Verification

Against unpatched trunk the new tests produce 1 error and 5 failures. The error is the defect itself surfacing:

1) Tests_Image_Meta::test_iptc_invalid_utf8_is_scrubbed
Function utf8_encode() is deprecated
/var/www/src/wp-admin/includes/image.php:1052

With the patch applied: 7/7 new tests pass, tests/admin/exportWp.php and tests/image/meta.php pass 17/17, and --group image --group unicode passes 1178/1178. PHPCS reports no new errors or warnings.

Tests added cover lone high bytes, never-valid bytes, truncated sequences, overlong sequences and surrogate halves through the WXR export, plus an IPTC block built at runtime with iptcembed() so no binary fixture is needed.

See #55603 (parent), #62608 (closed as duplicate), #63863 (the 6.9 polyfill). Unblocking these call sites also unblocks #64634, which is waiting on them to update PHPCompatibilityWP.

Attachments (2)

BEFORE.png (1.3 MB ) - added by siliconforks 5 weeks ago.
The image from #35316, uploaded before the change
AFTER.png (1.3 MB ) - added by siliconforks 5 weeks ago.
The image from #35316, uploaded after the change

Change History (9)

This ticket was mentioned in PR #12920 on WordPress/wordpress-develop by @khokansardar.


5 weeks ago
#1

wxr_cdata() and wp_read_image_metadata() hold the last three calls to utf8_encode() in core, which PHP deprecated in 8.2 and removes in 9.0, and which core itself polyfilled with a deprecation notice in [60950].

All three used the if ( ! wp_is_valid_utf8( $text ) ) { utf8_encode( $text ); } pattern, which assumes anything failing UTF-8 validation is ISO-8859-1 and re-encodes the raw bytes on that guess — wrong for every other single-byte encoding, and silent. This replaces the invalid spans with the Unicode replacement character via wp_scrub_utf8(), per @dmsnell's guidance in #55603 comment:99 that core should not assume an encoding it cannot know.

This is a deliberate behaviour change: invalid bytes become U+FFFD rather than latin1 mojibake. No new API, and the wp_is_valid_utf8() guards are unchanged.

Against unpatched trunk the added tests produce 1 error (Function utf8_encode() is deprecated at image.php:1052) and 5 failures. With the patch: 17/17 pass across the two touched test files, 1178/1178 in --group image --group unicode, 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 andTicket and PR details. All changes are reviewed and validated by me.

#2 @dmsnell
5 weeks ago

thanks for taking this on @khokansardar

one pedantic note here, which was a surprise to me when I learned it

That guess is wrong for every other single-byte encoding — Windows-1252 text, which is far more common in the wild, has its 0x80–0x9F range silently mapped to C1 control characters

This is not accurate today. The WHATWG Encoding standard even explicitly considers ISO-8859-1 as an alias, or alternative label, for Windows-1252.

In effect, due to the inherent subtlety between them, and the frequency of software treating them differently from another, they have all been collapsed into Windows-1252 and on the web, those remapped C1 controls ought to be interpreted as such, rather than as non-characters.

There is nothing material about this for the changes, but I share it because it can help us avoid confusion when discussing text encodings.

This is why, for instance, if you use the fatal flag in a JavaScript TextDecoder that it won’t throw on invalid ASCII or ISO-8859-1, instead decoding as Windows-1252

#3 @sabernhardt
5 weeks ago

  • Description modified (diff)
  • Milestone 7.1Future Release

I moved this to Future Release, and a committer can set a numbered milestone when reviewing.

#4 @khokansardar
5 weeks ago

Replying to dmsnell:

Thanks @dmsnell — you're right, and I've dropped that framing from both the tests and the PR.

The accurate version is narrower than what I wrote. utf8_encode() implements the literal ISO/IEC 8859-1 mapping (byte → U+00XX), which diverges from the WHATWG decoder for that same label at exactly 0x80–0x9F. So the disagreement isn't between "ISO-8859-1 text" and "Windows-1252 text" as two separate populations — as you say, those have been collapsed — it's between utf8_encode() and the way the web decodes the one label it claims to implement.

That is now demonstrated in the tests rather than asserted in prose. “quoted” encoded as Windows-1252 is:

93 71 75 6f 74 65 64 94

Today those become U+0093 and U+0094, so the quotation marks are silently turned into invisible C1 controls. A browser decoding the same bytes labelled iso-8859-1 returns “quoted”. Neither result is what utf8_encode() produces, which I think reinforces the point that this call site should not be guessing at all.

Running the new data provider against unpatched trunk:

Input trunk today with the patch
Café as ISO-8859-1 Café Caf�
wyróżnij as ISO-8859-2 wyró¿nij wyr�nij
Привет as Windows-1251 Ïðèâåò ������
“quoted” as Windows-1252 quoted (C1 controls, invisible) �quoted�

Stating the cost plainly, since it belongs in the ticket rather than only in the PR: where the input genuinely is ISO-8859-1, today's behaviour is correct and this change is lossy — the first row shows Café becoming Caf plus a replacement character. The trade is correctness for one assumed encoding in exchange for predictable neutralisation of all of them. That is the "preserve the defects or reject unsupported inputs" decision you described in #55603 comment:93, and this patch takes the second option.

The PR has been updated for your three code-level notes: the redundant wp_is_valid_utf8() calls are gone from all three sites (the source diff is now a net removal), the docblocks describe current behaviour instead of narrating removed code, and your wyróżnij case is included verbatim. The $meta[ $key ] truthiness check in wp_read_image_metadata() stays, since iso defaults to int 0.

One follow-up this surfaced, which I'd rather not fold into this ticket: IPTC does carry an encoding declaration — IIM DataSet 1:90, Coded Character Set — which core never reads. Honouring it would let wp_read_image_metadata() stop guessing altogether rather than choosing between two guesses. Happy to open a separate ticket if that seems worth pursuing.

@siliconforks
5 weeks ago

The image from #35316, uploaded before the change

@siliconforks
5 weeks ago

The image from #35316, uploaded after the change

#5 @siliconforks
5 weeks ago

Just to clarify the effect of this change - this is a (deliberate?) regression for images like the one in #35316?

If that's the case, I have a couple of objections here:

  1. To be honest, I'd prefer not to make this change at all. I don't really see the purpose of it. Removing utf8_encode() seems unnecessary - there is already a polyfill for it. Even after PHP 9 is released and utf8_encode() is removed from PHP, the polyfill will continue working, presumably forever. Why does this need to be removed?
  1. If it really is necessary to make this change - I would prefer that the description of the change be a bit more clear. I'm reading through the description for PR #12920 and the associated commit messages, and they don't really do a very good job explaining what is actually being changed here. I realize that these are not actually used for the final commit message, and that the final commit message will be in the Subversion repository, but I'm hoping that the final commit message will give a better indication of what is actually going on with this change. (Something like "this is a backward compatibility break" or "ISO-8859-1 is no longer supported in image metadata" or words to that effect. It might also be a good idea to mention #35316 because this change partially reverts the fix for that.)

#6 @dmsnell
5 weeks ago

Happy to open a separate ticket if that seems worth pursuing.

what are your thoughts on the matter, @khokansardar?

---

@siliconforks raises a good point. for sites with ISO-8859-1 data, the former cases worked properly, and those sites make up the majority (probably) of non-UTF-8 sites.

#7 @khokansardar
4 weeks ago

Agreed on both counts, and the patch has been reworked so that there is no behaviour change at all.

@siliconforks is right that the U+FFFD approach regresses #35316, and right that the ticket conflated two separate things. Removing the deprecated call and changing the output are independent, and only the first belongs here.

Why the call still needs to go

Not because utf8_encode() is deprecated in the abstract — @siliconforks is correct that the polyfill would keep working indefinitely. It is that the polyfill added in [60950] calls _deprecated_function(), so on PHP 9.0 every WXR export and every image upload carrying non-UTF-8 bytes emits a deprecation notice from core's own code, aimed at a site owner who has nothing to fix. On PHP 8.2–8.4 the same three lines raise a native E_DEPRECATED. #64634 is also waiting on these call sites before PHPCompatibilityWP can be updated.

None of that requires touching the output.

What the patch does now

Core's own polyfill docblock already names the replacement: Use mb_convert_encoding() instead. mb_convert_encoding( $text, 'UTF-8', 'ISO-8859-1' ) is byte-identical to utf8_encode() across all 256 byte values, so:

  • A private _wp_iso_8859_1_to_utf8() is added to wp-includes/utf8.php, following the mbstring/fallback split that file already uses. The no-mbstring branch calls the existing _wp_utf8_encode_fallback(), whose equivalence to mb_convert_encoding() over every byte value is already asserted by Tests_DeprecatedUtf8EncodeDecodeTest.
  • The three call sites swap the function name and nothing else. The wp_is_valid_utf8() guards are restored, since mb_convert_encoding() does not validate first.

The source diff against trunk is three lines plus the helper.

Note: See TracTickets for help on using tickets.