Skip to content

Fix GH-23094: Use byte offsets in NumberFormatter parsing - #23318

Open
ColumbusLabs wants to merge 3 commits into
php:PHP-8.4from
ColumbusLabs:fix/numberformatter-byte-offset
Open

Fix GH-23094: Use byte offsets in NumberFormatter parsing#23318
ColumbusLabs wants to merge 3 commits into
php:PHP-8.4from
ColumbusLabs:fix/numberformatter-byte-offset

Conversation

@ColumbusLabs

Copy link
Copy Markdown

Fixes #23094.

PHP exposes NumberFormatter parsing offsets as UTF-8 byte offsets, while ICU expects UTF-16 code-unit positions. Convert the input offset before parsing and the returned offset afterward for both parse() and parseCurrency().

This targets PHP-8.4 as the lowest actively supported branch; the equivalent C++ change can be merged upward.

Tests:

  • debug + ZTS build with ICU 78.3
  • sapi/cli/php run-tests.php ext/intl/tests/gh23094.phpt ext/intl/tests/formatter_parse.phpt ext/intl/tests/formatter_parse_currency.phpt
  • sapi/cli/php run-tests.php ext/intl/tests (437 passed, 111 skipped, 1 expected failure, 0 failures)

@LamentXU123

Copy link
Copy Markdown
Member

The issue is: NumberFormatter rejects offsets inside UTF-8 sequences
For example:

<?php

$formatter = new NumberFormatter('en_US', NumberFormatter::DECIMAL);

$offset = 1; // Inside the two-byte UTF-8 encoding of "é".
var_dump($formatter->parse(
    "\u{00E9}123",
    NumberFormatter::TYPE_INT32,
    $offset
));
var_dump($offset);

?>

should return

bool(false)
int(1)

But in this PR, it is

int(123)
int(5)

Otherwise, this is correct!

@ColumbusLabs

Copy link
Copy Markdown
Author

Thanks for catching that. I changed the byte-to-UTF-16 conversion to reject offsets that split a UTF-8 sequence while leaving the referenced offset unchanged. I also added regression coverage for both parse() and parseCurrency(); the focused tests and full Intl suite pass.

@LamentXU123

Copy link
Copy Markdown
Member

Could you please set the error state after failure?
That is, add this line in your tests failure path:

var_dump(intl_is_failure($formatter->getErrorCode()));

expected

bool(true)

Comment thread ext/intl/formatter/formatter_parse.c Outdated

if(zposition) {
position = (int32_t) zval_get_long(zposition);
if (!numfmt_utf8_offset_to_utf16(str, str_len, &position)) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@LamentXU123 perhaps $position needs your master changes to be always correct.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That's painful, but yes. #22572 is rather a feature or a BC break than a bug fix and must not be backported. So yes, 😭😭

@LamentXU123 LamentXU123 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM!

@LamentXU123

LamentXU123 commented Aug 16, 2026

Copy link
Copy Markdown
Member

This fix is a little different in the master branch since we have the UnicodeString refactors. I will take care.
But just in case, I will wait for David's review before merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants