Skip to content

Fix #76093: Format strings w/o loss of precision w/ FORMAT_TYPE_DECIMAL #12432

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix #76093: Format strings w/o loss of precision w/ FORMAT_TYPE_DECIMAL #12432

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
85225a8
Use zval_try_get_long() in NumberFormatter::format()
lucaswerkmeister Nov 26, 2023
18b0136
Fix #76093: Format strings w/o loss of precision w/ FORMAT_TYPE_DECIMAL
cscott Jan 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion ext/intl/formatter/formatter.stub.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,8 @@ class NumberFormatter
public const int TYPE_INT64 = UNKNOWN;
/** @cvalue FORMAT_TYPE_DOUBLE */
public const int TYPE_DOUBLE = UNKNOWN;
/** @cvalue FORMAT_TYPE_DECIMAL */
public const int TYPE_DECIMAL = UNKNOWN;
/**
* @deprecated
* @cvalue FORMAT_TYPE_CURRENCY
Expand All @@ -189,7 +191,7 @@ public static function create(string $locale, int $style, ?string $pattern = nul
* @tentative-return-type
* @alias numfmt_format
*/
public function format(int|float $num, int $type = NumberFormatter::TYPE_DEFAULT): string|false {}
public function format(int|float|string $num, int $type = NumberFormatter::TYPE_DEFAULT): string|false {}

/**
* @param int $offset
Expand Down
10 changes: 8 additions & 2 deletions ext/intl/formatter/formatter_arginfo.h
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

63 changes: 58 additions & 5 deletions ext/intl/formatter/formatter_format.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ PHP_FUNCTION( numfmt_format )
FORMATTER_METHOD_INIT_VARS;

/* Parse parameters. */
if( zend_parse_method_parameters( ZEND_NUM_ARGS(), getThis(), "On|l",
if( zend_parse_method_parameters( ZEND_NUM_ARGS(), getThis(), "Oz|l",
&object, NumberFormatter_ce_ptr, &number, &type ) == FAILURE )
{
RETURN_THROWS();
Expand All @@ -53,30 +53,65 @@ PHP_FUNCTION( numfmt_format )
case IS_DOUBLE:
type = FORMAT_TYPE_DOUBLE;
break;
EMPTY_SWITCH_DEFAULT_CASE();
case IS_STRING:
type = FORMAT_TYPE_DECIMAL;
break;
case IS_OBJECT:
type = FORMAT_TYPE_DECIMAL;
break;
default:
zend_argument_type_error(1, "must be of type int|float|string, %s given", zend_zval_type_name(number));
}
}

// Avoid losing precision on 32-bit platforms where PHP's "long" isn't
// as long as the FORMAT_TYPE_INT64 which is requested.
#if SIZEOF_ZEND_LONG < 8
if (Z_TYPE_P(number) == IS_STRING && type == FORMAT_TYPE_INT64) {
type = FORMAT_TYPE_DECIMAL;
}
#endif

switch(type) {
case FORMAT_TYPE_INT32:
{
bool failed = true;
int64_t value_64 = zval_try_get_long(number, &failed);
if (failed) {
zend_argument_type_error(getThis() ? 1 : 2,
"must be of type int when argument #%d ($format) is NumberFormatter::TYPE_INT32", getThis() ? 2 : 3);
RETURN_THROWS();
}
if (value_64 < -2147483648 || value_64 > 2147483647) {
zend_argument_value_error(getThis() ? 1 : 2,
"must fit in 32 bits when argument #%d ($format) is NumberFormatter::TYPE_INT32", getThis() ? 2 : 3);
RETURN_THROWS();
}
convert_to_long(number);
formatted_len = unum_format(FORMATTER_OBJECT(nfo), (int32_t)Z_LVAL_P(number),
formatted_len = unum_format(FORMATTER_OBJECT(nfo), (int32_t)value_64,
formatted, formatted_len, NULL, &INTL_DATA_ERROR_CODE(nfo));
if (INTL_DATA_ERROR_CODE(nfo) == U_BUFFER_OVERFLOW_ERROR) {
intl_error_reset(INTL_DATA_ERROR_P(nfo));
formatted = eumalloc(formatted_len);
formatted_len = unum_format(FORMATTER_OBJECT(nfo), (int32_t)Z_LVAL_P(number),
formatted_len = unum_format(FORMATTER_OBJECT(nfo), (int32_t)value_64,
formatted, formatted_len, NULL, &INTL_DATA_ERROR_CODE(nfo));
if (U_FAILURE( INTL_DATA_ERROR_CODE(nfo) ) ) {
efree(formatted);
}
}
INTL_METHOD_CHECK_STATUS( nfo, "Number formatting failed" );
}
break;

case FORMAT_TYPE_INT64:
{
int64_t value = (Z_TYPE_P(number) == IS_DOUBLE)?(int64_t)Z_DVAL_P(number):Z_LVAL_P(number);
bool failed = true;
int64_t value = zval_try_get_long(number, &failed);
if (failed) {
zend_argument_type_error(getThis() ? 1 : 2,
"must be of type int when argument #%d ($format) is NumberFormatter::TYPE_INT64", getThis() ? 2 : 3);
RETURN_THROWS();
}
formatted_len = unum_formatInt64(FORMATTER_OBJECT(nfo), value, formatted, formatted_len, NULL, &INTL_DATA_ERROR_CODE(nfo));
if (INTL_DATA_ERROR_CODE(nfo) == U_BUFFER_OVERFLOW_ERROR) {
intl_error_reset(INTL_DATA_ERROR_P(nfo));
Expand All @@ -103,6 +138,24 @@ PHP_FUNCTION( numfmt_format )
}
INTL_METHOD_CHECK_STATUS( nfo, "Number formatting failed" );
break;

case FORMAT_TYPE_DECIMAL:
if (!try_convert_to_string(number)) {
RETURN_THROWS();
}
Comment on lines +143 to +145
Copy link
Member

Choose a reason for hiding this comment

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

This could be simplified as at this point the value should be int|float|string and integers and floats always have string representations.

// Convert string as a DecimalNumber, so we don't lose precision
formatted_len = unum_formatDecimal(FORMATTER_OBJECT(nfo), Z_STRVAL_P(number), Z_STRLEN_P(number), formatted, formatted_len, NULL, &INTL_DATA_ERROR_CODE(nfo));
if (INTL_DATA_ERROR_CODE(nfo) == U_BUFFER_OVERFLOW_ERROR) {
intl_error_reset(INTL_DATA_ERROR_P(nfo));
formatted = eumalloc(formatted_len);
unum_formatDecimal(FORMATTER_OBJECT(nfo), Z_STRVAL_P(number), Z_STRLEN_P(number), formatted, formatted_len, NULL, &INTL_DATA_ERROR_CODE(nfo));
if (U_FAILURE( INTL_DATA_ERROR_CODE(nfo) ) ) {
efree(formatted);
}
}
INTL_METHOD_CHECK_STATUS( nfo, "Number formatting failed" );
break;

case FORMAT_TYPE_CURRENCY:
if (getThis()) {
Copy link
Member

Choose a reason for hiding this comment

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

Aside Nit: this could now be if (object)

Copy link
Author

Choose a reason for hiding this comment

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

I tried it, but it caused several test failures (seemingly object was still null when I wouldn’t have expected it to be).

const char *space;
Expand Down
1 change: 1 addition & 0 deletions ext/intl/formatter/formatter_format.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,5 +22,6 @@
#define FORMAT_TYPE_INT64 2
#define FORMAT_TYPE_DOUBLE 3
#define FORMAT_TYPE_CURRENCY 4
#define FORMAT_TYPE_DECIMAL 5

#endif // FORMATTER_FORMAT_H
2 changes: 1 addition & 1 deletion ext/intl/php_intl.stub.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -390,7 +390,7 @@ function datefmt_get_error_message(IntlDateFormatter $formatter): string {}

function numfmt_create(string $locale, int $style, ?string $pattern = null): ?NumberFormatter {}

function numfmt_format(NumberFormatter $formatter, int|float $num, int $type = NumberFormatter::TYPE_DEFAULT): string|false {}
function numfmt_format(NumberFormatter $formatter, int|float|string $num, int $type = NumberFormatter::TYPE_DEFAULT): string|false {}

/** @param int $offset */
function numfmt_parse(NumberFormatter $formatter, string $string, int $type = NumberFormatter::TYPE_DOUBLE, &$offset = null): int|float|false {}
Expand Down
4 changes: 2 additions & 2 deletions ext/intl/php_intl_arginfo.h
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

38 changes: 34 additions & 4 deletions ext/intl/tests/bug48227.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,20 +5,50 @@ intl
--FILE--
<?php

$testcases = ['', 1, NULL, true, false, [], (object)[]];

echo "OOP\n";
$x = new NumberFormatter('en_US', NumberFormatter::DECIMAL);
foreach (['', 1, NULL, $x] as $value) {
foreach (array_merge($testcases, [$x]) as $value) {
try {
var_dump($x->format($value));
} catch (TypeError $ex) {
echo $ex->getMessage(), PHP_EOL;
}
}

echo "\nProcedural\n";
$x = numfmt_create('en_US', NumberFormatter::DECIMAL);
foreach (array_merge($testcases, [$x]) as $value) {
try {
var_dump(numfmt_format($x, $value));
} catch (TypeError $ex) {
echo $ex->getMessage(), PHP_EOL;
}
}

?>
--EXPECTF--
NumberFormatter::format(): Argument #1 ($num) must be of type int|float, string given
OOP
bool(false)
string(1) "1"

Deprecated: NumberFormatter::format(): Passing null to parameter #1 ($num) of type int|float is deprecated in %s on line %d
Deprecated: NumberFormatter::format(): Passing null to parameter #1 ($num) of type string|int|float is deprecated in %s on line %d
string(1) "0"
string(1) "1"
string(1) "0"
NumberFormatter::format(): Argument #1 ($num) must be of type string|int|float, array given
NumberFormatter::format(): Argument #1 ($num) must be of type string|int|float, stdClass given
NumberFormatter::format(): Argument #1 ($num) must be of type string|int|float, NumberFormatter given

Non-OOP
bool(false)
string(1) "1"

Deprecated: numfmt_format(): Passing null to parameter #2 ($num) of type string|int|float is deprecated in %s on line %d
string(1) "0"
string(1) "1"
string(1) "0"
NumberFormatter::format(): Argument #1 ($num) must be of type int|float, NumberFormatter given
numfmt_format(): Argument #2 ($num) must be of type string|int|float, array given
numfmt_format(): Argument #2 ($num) must be of type string|int|float, stdClass given
numfmt_format(): Argument #2 ($num) must be of type string|int|float, NumberFormatter given
91 changes: 91 additions & 0 deletions ext/intl/tests/bug76093.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
--TEST--
Bug #76093 (NumberFormatter::format loses precision)
--EXTENSIONS--
intl
--FILE--
<?php

class Bug76093Stringable implements Stringable {
public function __construct( private readonly string $string ) {}

public function __toString(): string {
return $this->string;
}
}

# See also https://phabricator.wikimedia.org/T268456
$x = new NumberFormatter('en_US', NumberFormatter::DECIMAL);
foreach ([
'999999999999999999', # Fits in signed 64-bit integer
'9999999999999999999', # Does not fit in signed 64-bit integer
9999999999999999999, # Precision loss seen when passing as number
] as $value) {
try {
var_dump([
'input' => $value,
'default' => $x->format($value),
# Note that TYPE_INT64 isn't actually guaranteed to have an
# 64-bit integer as input, because PHP on 32-bit platforms only
# has 32-bit integers. If you pass the value as a string, PHP
# will use the TYPE_DECIMAL type in order to extend the range.
# Also, casting from double to int64 when the int64 range
# is exceeded results in an implementation-defined value.
'int64' => $x->format($value, NumberFormatter::TYPE_INT64),
'double' => $x->format($value, NumberFormatter::TYPE_DOUBLE),
'decimal' => $x->format($value, NumberFormatter::TYPE_DECIMAL),
]);
} catch (TypeError $ex) {
echo $ex->getMessage(), PHP_EOL;
}
}

# Stringable object also supported
try {
echo $x->format(new Bug76093Stringable('9999999999999999999')), PHP_EOL;
} catch (TypeError $ex) {
echo $ex->getMessage(), PHP_EOL;
}

?>
--EXPECTF--
array(5) {
["input"]=>
string(18) "999999999999999999"
["default"]=>
string(23) "999,999,999,999,999,999"
["int64"]=>
string(23) "999,999,999,999,999,999"
["double"]=>
string(25) "1,000,000,000,000,000,000"
["decimal"]=>
string(23) "999,999,999,999,999,999"
}

Deprecated: Implicit conversion from float-string "9999999999999999999" to int loses precision in %s on line %d
array(5) {
["input"]=>
string(19) "9999999999999999999"
["default"]=>
string(25) "9,999,999,999,999,999,999"
["int64"]=>
string(%d) "%r9,223,372,036,854,775,807|9,999,999,999,999,999,999%r"
["double"]=>
string(26) "10,000,000,000,000,000,000"
["decimal"]=>
string(25) "9,999,999,999,999,999,999"
}

Deprecated: Implicit conversion from float 1.0E+19 to int loses precision in %s on line %d
array(5) {
["input"]=>
float(1.0E+19)
["default"]=>
string(26) "10,000,000,000,000,000,000"
["int64"]=>
string(%d) "%r[+-]?[0-9,]+%r"
["double"]=>
string(26) "10,000,000,000,000,000,000"
["decimal"]=>
string(26) "10,000,000,000,000,000,000"
}
9,999,999,999,999,999,999