Skip to content

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

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

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
Contributor

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
ContributorAuthor

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 threadext/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, 😭😭

@LamentXU123LamentXU123 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.

LamentXU123 added a commit that referenced this pull request Aug 18, 2026
* PHP-8.5:
FixGH-23094: Use byte offsets in NumberFormatter parsing (#23318)
lacatoire pushed a commit to lacatoire/php-src that referenced this pull request Aug 18, 2026
* PHP-8.4:
FixphpGH-23094: Use byte offsets in NumberFormatter parsing (php#23318)
Sign up for freeto 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

@ColumbusLabs@LamentXU123@NickSdot