Skip to content
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.

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

ValueError: NumberFormatter::format(): Argument #3 must be a NumberFormatter::TYPE_* constant #9421

Closed
bobmagicii opened this issue Aug 24, 2022 · 3 comments

Comments

@bobmagicii
Copy link

bobmagicii commented Aug 24, 2022

Description

from the php manual

public NumberFormatter::format(int|float $num, int $type = NumberFormatter::TYPE_DEFAULT): string|false

not quite sure about this mystical argument # 3.

$Fmt = new NumberFormatter('en_US', NumberFormatter::CURRENCY);
var_dump($Fmt->format(1234.45, NumberFormatter::TYPE_CURRENCY));

Resulted in this output:

Output for 8.0.1 - 8.0.22, 8.1.0 - 8.1.9

    Fatal error: Uncaught ValueError: NumberFormatter::format(): Argument #3 must be a NumberFormatter::TYPE_* constant in /in/mc4XU:4
    Stack trace:
    #0 /in/mc4XU(4): NumberFormatter->format(1234.45, 4)
    #1 {main}
      thrown in /in/mc4XU on line 4

    Process exited with code 255.

But I expected this output instead:

pretty much anything except saying it needs a third argument that isn't documented.

https://3v4l.org/mc4XU

PHP Version

PHP 8+

Operating System

Linux 5.4.0-88-generic # 99-Ubuntu SMP Thu Sep 23 17:29:00 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux

[edit] idk it says i added those tags (bugs, triage) but they showed up on their own i just report the news

@damianwadley
Copy link
Member

So there's two bugs here:

  1. The wrong argument number.

Notice the other syntax:

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

Internally, class methods look a lot like regular functions that happen to take the "this" object as the first argument, and the procedural syntax for numfmt_format intentionally mirrors that fact. So when it says argument 3 it does mean the $type and it's not noticing that you used the OOP syntax.

Given this procedural/OOP stuff is a common setup, I assume this "is it argument 2 or 3?" problem has a standard solution.

  1. TYPE_CURRENCY isn't supported.

This is likely deliberate because there's a formatCurrency method available. It takes a special $currency argument so that functionality can't simply be added to format.

IMO this needs two fixes: one that PHP detects TYPE_CURRENCY and then tells you that you need to use formatCurrency instead, then another for the docs.

@bobmagicii
Copy link
Author

bobmagicii commented Aug 25, 2022

glad someone agrees that something is amiss. both of the proposed fixes sound like the correct direction.

just for full vision the reason i did what i did the way i did, was it seemed like numberformatter could be told to init with a locale and desired formatting. and indeed i noticed the formatCurrency method, and more specifically, that the format method took a type.

this suggested to me that i should initialise one numberformatter global to my app instance, using the user's preferences, and for consistency sake across all my various calls, i elected to only use format with type currency so that it looked like all the other calls to format this and that and those. the final assumption i made was that numberformatter as en_US would default to USD, which would then make format(value, CURRENCY), seem reasonable.

@cmb69
Copy link
Member

cmb69 commented Aug 25, 2022

Given this procedural/OOP stuff is a common setup, I assume this "is it argument 2 or 3?" problem has a standard solution.

It seems so far it has not. In theory it could be done by modifying the zend_argument*error() functions, but that would be a BC break, since external extensions may already cater to that, like for example, transliterator_transliterate():

zend_argument_value_error(object ? 3 : 4, "must be greater than or equal to -1");

So we likely need to handle the individual calls.

IMO this needs two fixes: one that PHP detects TYPE_CURRENCY and then tells you that you need to use formatCurrency instead, then another for the docs.

I would go with a doc fix only, but given that it is documented to support any TYPE_*, an explicit error message may be reasonable.

cmb69 added a commit to php/doc-en that referenced this issue Aug 25, 2022
@Girgias Girgias self-assigned this Aug 30, 2022
Girgias added a commit to Girgias/php-src that referenced this issue Aug 30, 2022
Girgias added a commit to Girgias/php-src that referenced this issue Sep 6, 2022
tiffany-taylor pushed a commit to tiffany-taylor/doc-en that referenced this issue Jan 16, 2023
claudepache pushed a commit to claudepache/php-doc-en that referenced this issue Jun 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants
@bobmagicii @cmb69 @Girgias @damianwadley and others