Fix GH-9421 Incorrect argument number for ValueError in NumberFormatter #9453
Conversation
|
|
||
| case FORMAT_TYPE_CURRENCY: | ||
| if (getThis()) { | ||
| zend_argument_value_error(2, "cannot use NumberFormatter::TYPE_CURRENCY constant, use formatCurrency() method instead"); |
There was a problem hiding this comment.
Not sure about the formatCurrency(); shouldn't that better be ::formatCurrency()? cc @kocsismate
There was a problem hiding this comment.
My preference would be to prepend the class name before the method name, just like in case of the constant name.
There was a problem hiding this comment.
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...?
There was a problem hiding this comment.
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.
| if (getThis()) { | ||
| zend_argument_value_error(2, "cannot use NumberFormatter::TYPE_CURRENCY constant, use formatCurrency() method instead"); | ||
| } else { | ||
| zend_argument_value_error(3, "cannot use NumberFormatter::TYPE_CURRENCY constant, use numfmt_format_currency() function instead"); | ||
| } |
There was a problem hiding this comment.
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.
|
Note sure if this is relevant for this PR when merging manually, but I'd like to point out that the target branch likely is incorrectly set to |
|
Closing in favour of #9489 because I forgot to use a feature branch |
No description provided.