Skip to content

Fix GH-9421 Incorrect argument number for ValueError in NumberFormatter #9453

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

Closed
wants to merge 1 commit into from
Closed

Fix GH-9421 Incorrect argument number for ValueError in NumberFormatter #9453

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 8 additions & 2 deletions ext/intl/formatter/formatter_format.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,9 +103,15 @@ PHP_FUNCTION( numfmt_format )
}
INTL_METHOD_CHECK_STATUS( nfo, "Number formatting failed" );
break;

case FORMAT_TYPE_CURRENCY:
if (getThis()) {
zend_argument_value_error(2, "cannot use NumberFormatter::TYPE_CURRENCY constant, use formatCurrency() method instead");
Copy link
Member

Choose a reason for hiding this comment

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

Not sure about the formatCurrency(); shouldn't that better be ::formatCurrency()? cc @kocsismate

Copy link
Member

Choose a reason for hiding this comment

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

My preference would be to prepend the class name before the method name, just like in case of the constant name.

Copy link
Member

Choose a reason for hiding this comment

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

Otherwise Argument #2 ($type) cannot use NumberFormatter::TYPE_CURRENCY... sounds a bit unnatural to me. What about Argument #2 ($type) cannot be NumberFormatter::TYPE_CURRENCY...?

Copy link
Member

Choose a reason for hiding this comment

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

What about Argument #2 ($type) cannot be NumberFormatter::TYPE_CURRENCY...?

Yeah, that's better, in my opinion.

And maybe we should print the actual class name, since NumberFormatter is not final, nor are its class constants.

Copy link
Member Author

Choose a reason for hiding this comment

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

ACK

} else {
zend_argument_value_error(3, "cannot use NumberFormatter::TYPE_CURRENCY constant, use numfmt_format_currency() function instead");
}
Comment on lines +107 to +111
Copy link
Member

Choose a reason for hiding this comment

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

IMO, referring to NumberFormatter::formatCurrency() in both cases should be sufficient. After all, they can be used interchangeably, and if it was for me, we would deprecate the procedural API anyway.

RETURN_THROWS();
default:
zend_argument_value_error(3, "must be a NumberFormatter::TYPE_* constant");
zend_argument_value_error(getThis() ? 2 : 3, "must be a NumberFormatter::TYPE_* constant");
RETURN_THROWS();
}

Expand Down
9 changes: 8 additions & 1 deletion ext/intl/formatter/formatter_parse.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,8 +85,15 @@ PHP_FUNCTION( numfmt_parse )
val_double = unum_parseDouble(FORMATTER_OBJECT(nfo), sstr, sstr_len, position_p, &INTL_DATA_ERROR_CODE(nfo));
RETVAL_DOUBLE(val_double);
break;
case FORMAT_TYPE_CURRENCY:
if (getThis()) {
zend_argument_value_error(2, "cannot use NumberFormatter::TYPE_CURRENCY constant, use parseCurrency() method instead");
} else {
zend_argument_value_error(3, "cannot use NumberFormatter::TYPE_CURRENCY constant, use numfmt_parse_currency() function instead");
}
goto cleanup;
default:
zend_argument_value_error(3, "must be a NumberFormatter::TYPE_* constant");
zend_argument_value_error(getThis() ? 2 : 3, "must be a NumberFormatter::TYPE_* constant");
goto cleanup;
}

Expand Down
65 changes: 65 additions & 0 deletions ext/intl/tests/formatter_format_and_parse_errors.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
--TEST--
ValueErrors for format/parse methods and procedural functions
--SKIPIF--
<?php if( !extension_loaded( 'intl' ) ) print 'skip'; ?>
--FILE--
<?php

$o = new NumberFormatter('en_US', NumberFormatter::PATTERN_DECIMAL);
$num = 5;
$str = "string";

/* Unknown type constant */
try {
numfmt_format($o, $num, -20);
} catch (\ValueError $e) {
echo $e->getMessage(), \PHP_EOL;
}
try {
$o->format($num, -20);
} catch (\ValueError $e) {
echo $e->getMessage(), \PHP_EOL;
}
try {
numfmt_parse($o, $str, -20);
} catch (\ValueError $e) {
echo $e->getMessage(), \PHP_EOL;
}
try {
$o->parse($str, -20);
} catch (\ValueError $e) {
echo $e->getMessage(), \PHP_EOL;
}

/* With NumberFormatter::TYPE_CURRENCY */
try {
numfmt_format($o, $num, NumberFormatter::TYPE_CURRENCY);
} catch (\ValueError $e) {
echo $e->getMessage(), \PHP_EOL;
}
try {
$o->format($num, NumberFormatter::TYPE_CURRENCY);
} catch (\ValueError $e) {
echo $e->getMessage(), \PHP_EOL;
}
try {
numfmt_parse($o, $str, NumberFormatter::TYPE_CURRENCY);
} catch (\ValueError $e) {
echo $e->getMessage(), \PHP_EOL;
}
try {
$o->parse($str, NumberFormatter::TYPE_CURRENCY);
} catch (\ValueError $e) {
echo $e->getMessage(), \PHP_EOL;
}

?>
--EXPECT--
numfmt_format(): Argument #3 ($type) must be a NumberFormatter::TYPE_* constant
NumberFormatter::format(): Argument #2 ($type) must be a NumberFormatter::TYPE_* constant
numfmt_parse(): Argument #3 ($type) must be a NumberFormatter::TYPE_* constant
NumberFormatter::parse(): Argument #2 ($type) must be a NumberFormatter::TYPE_* constant
numfmt_format(): Argument #3 ($type) cannot use NumberFormatter::TYPE_CURRENCY constant, use numfmt_format_currency() function instead
NumberFormatter::format(): Argument #2 ($type) cannot use NumberFormatter::TYPE_CURRENCY constant, use formatCurrency() method instead
numfmt_parse(): Argument #3 ($type) cannot use NumberFormatter::TYPE_CURRENCY constant, use numfmt_parse_currency() function instead
NumberFormatter::parse(): Argument #2 ($type) cannot use NumberFormatter::TYPE_CURRENCY constant, use parseCurrency() method instead