Cakephp: Cake\I18n\Number::currency ignores "places" option

[danielzzz]

This is a (multiple allowed):

• [x] bug
• [ ] enhancement
• [ ] feature-discussion (RFC)
• CakePHP Version: 3.0.10
• Platform and Target: linux

What you did

$this->Number->currency('120000, 'USD', ['places'=>0, 'locale'=>'en_GB']);

Expected Behavior

I was hoping to get: $120,000

Actual Behavior

but I am getting actually $120,000.00
not sure if it's intended or a bug, but it seems that the locale takes precedence over places.
(for some other locale strings like es_ES the fraction part is gone)
I don't want to use a specific format, as I'd like the amount to be formatted according to locale, but without the fraction part.

[dereuromark]

It is possible that https://github.com/cakephp/cakephp/blob/master/src/I18n/Number.php#L230 overwrites the previously correct formatting returned from the internal Number class formatting.

[chinpei215]

@danielzzz You need to specify the same locale as the currency. It is a feature of Intl.

echo $this->Number->currency('120000', 'USD', ['places'=>0, 'locale'=>'en_US']);
// $120,000

[dereuromark]

@chinpei215 But then don't we need to document that the old 2.x places option bears no value anymore then? Or at least in most cases it does not.

[chinpei215]

@dereuromark You have a point. So should we workaround for keeping backward compatibility?

[chinpei215]

Related with #8508. It would also work in 2.x.

[JustDoItSascha]

@danielzzz I had the bug myself and first of all places is the wrong option I think for what you want. Places sets the MIN_FRACTION_DIGITS option of NumberFormatter. So you get two digits and this is correct, because you set a minimum of 0.

You have to use precision because you want to set the MAX_FRACTION_DIGITS. But even this is not working for me at least when I try to use another currency than EUR. With EUR it works fine, with everything else I get two fraction digits.

Maybe someone can confirm this behavior, that it works with EUR and does not work with USD. It seems to be a bug in PHP.

[chinpei215]

@JustDoItSascha Perhaps, no. Intl would reject to change format if the locale and the currency are mismatched.

[JustDoItSascha]

@chinpei215 Indeed you are right. Setting the locale to en_US lets me set the MAX_FRACTION_DIGITS correctly for USD, but not for EUR. de_DE works with EUR but not with USD.

Why is this? Is there a possibility to circumvent this? Because I have a table with different amounts in different currencies and the currencies are stored in the database like EUR or USD. But I don't know all locales for alle currencies.

[dereuromark]

Sounds like we do need an internal matching "currency to locale" stored per supported currency as well, this is required as a good default value here when not manually providing one.

[JustDoItSascha]

@dereuromark But think of difficulties with currencies which doesn't exist anymore, like DEM. The matching locale would be de_DE, but this is also not working. Is this behavior from NumberFormatter new? Can we override the option, so that the NumberFormatter is ignoring a mismatch between locale and currency?

[chinpei215]

Why is this?

@JustDoItSascha I am guessing that intl only load the database of the specified locale. For the reasons, without specifing locale, it wouldn't change the default format witch belongs to other locale. If you are interested, you can download the source code from ICU. But I don't know whether the answer is in the source code or not. It might be an issue of PHP intl-extension.

[JustDoItSascha]

@chinpei215 Ok, but in my opinion it makes no sense to ignore overrides of option attributes. When I set MAX_FRACTION_DIGIT this has nothing to do with a specific locale. It is a option for the appearance of a number, a mathemical thing. Why should intl protect me from changing such an option.

I know that you are not responsible for this, just want to discuss this. Maybe I miss something...

[dereuromark]

I agree, we should probably do the fraction (number_format()) part after INTL is done and before we return the value.

[chinpei215]

@JustDoItSascha Don't worry. Your thoughts are reasonable. I am guessing that they would avoid partial supporting. For example, I think , is used for the decimal marker in de-DE. It would be difficult to format/parse the value without using the database of the locale, if users can specify arbitrary formats.

[danielzzz]

It still does not resolve my problem which is to get rid of decimals when showing prices (event though I still expect to have a dot or a coma as a thousand separator in countries they use it)

@dereuromark what you propose might work for me. I mean first set the proper locale format, then check for the max decimals option. But I am not sure if it's (easily) doable, because you would have efectivelly double format the price. once for the locale format and then for decimals.

[chinpei215]

@danielzzz Sorry for the inconvenience. Then, it might be no way to resolve your problem at the moment. We need to fix it.

Although I couldn't come up with any good workarounds, if I were you, I would do like the following:

echo explode('.', $this->Number->currency(120000, 'USD'))[0];
echo explode('.', $this->Number->currency(120000, 'GBP'))[0];

or

echo $this->Number->format(120000, ['pattern' => '$ #,###']);
echo $this->Number->format(120000, ['pattern' => '£ #,###']);

[danielzzz]

@chinpei215 I like the first option more, as it's the same for all locales. thanks a lot!

[chinpei215]

@danielzzz You are welcome. Thank you for reporting this issue :)

[markstory]

This looks like an issue in intl itself. I'm able to reproduce the same issue just using NumberFormatter

$formatter = new \NumberFormatter('en_GB', \NumberFormatter::CURRENCY);
$formatter->setAttribute(\NumberFormatter::MIN_FRACTION_DIGITS, 0);
$formatter->setAttribute(\NumberFormatter::MAX_FRACTION_DIGITS, 0);
var_dump($formatter->formatCurrency(120000, 'USD'));

[chinpei215]

@markstory Yes, this is an issue in intl itself. But compared to 2.x behavior, it would be nice if we can fix it. The following code might be a workaround. I confirmed it with ICU version 54.1.

$formatter = new \NumberFormatter('en_GB', \NumberFormatter::DECIMAL);
$formatter->setPattern('¤ ' . $formatter->getPattern());

$formatter->setTextAttribute(\NumberFormatter::CURRENCY_CODE, 'USD');
$formatter->setAttribute(\NumberFormatter::MIN_FRACTION_DIGITS, 0);
$formatter->setAttribute(\NumberFormatter::MAX_FRACTION_DIGITS, 0);
var_dump( $formatter->format(120000) );
// US$ 120,000

$formatter->setTextAttribute(\NumberFormatter::CURRENCY_CODE, 'JPY');
$formatter->setAttribute(\NumberFormatter::MIN_FRACTION_DIGITS, 4);
$formatter->setAttribute(\NumberFormatter::MAX_FRACTION_DIGITS, 4);
var_dump( $formatter->format(120000) );
// ¥ 120,000.0000

US$ seems strange to me though.

[markstory]

@chinpei215 Those changes cause other tests to fail. I had this diff

diff --git a/src/I18n/Number.php b/src/I18n/Number.php
index b4442f4..187757a 100644
--- a/src/I18n/Number.php
+++ b/src/I18n/Number.php
@@ -217,7 +217,13 @@ class Number
             return $options['zero'];
         }

-        $formatter = static::formatter(['type' => static::FORMAT_CURRENCY] + $options);
+        $formatter = static::formatter(['type' => NumberFormatter::DECIMAL] + $options);
+
+        $formatter->setPattern('¤' . $formatter->getPattern());
+        if ($currency) {
+            $formatter->setTextAttribute(NumberFormatter::CURRENCY_CODE, $currency);
+        }
+
         $abs = abs($value);
         if (!empty($options['fractionSymbol']) && $abs > 0 && $abs < 1) {
             $value = $value * 100;
diff --git a/src/View/Helper/NumberHelper.php b/src/View/Helper/NumberHelper.php
index 3c0902b..545c834 100644
--- a/src/View/Helper/NumberHelper.php
+++ b/src/View/Helper/NumberHelper.php
@@ -40,7 +40,7 @@ class NumberHelper extends Helper
     ];

     /**
-     * Cake\I18n\LocalizedNumber instance
+     * Cake\I18n\Number instance
      *
      * @var \Cake\I18n\Number
      */
diff --git a/tests/TestCase/I18n/NumberTest.php b/tests/TestCase/I18n/NumberTest.php
index 2f8e535..6b6ebf6 100644
--- a/tests/TestCase/I18n/NumberTest.php
+++ b/tests/TestCase/I18n/NumberTest.php
@@ -271,6 +271,14 @@ class NumberTest extends TestCase
         $result = $this->Number->currency('22.389', 'CAD');
         $expected = 'CA$22.39';
         $this->assertEquals($expected, $result);
+
+        $result = $this->Number->currency('120000', 'USD', ['places' => 0]);
+        $expected = '$120,000';
+        $this->assertEquals($expected, $result);
+
+        $result = $this->Number->currency('120000', 'USD', ['places' => 0, 'locale' => 'en_GB']);
+        $expected = 'US$120,000';
+        $this->assertEquals($expected, $result);
     }

     /**

The last two tests are the new ones from this issue.

[chinpei215]

@markstory I encountered another issue in intl itself :cry:
Here is an example code:

$formatter = new \NumberFormatter('en_GB', \NumberFormatter::CURRENCY);
var_dump($formatter->formatCurrency(120000, 'USD'));

When I tested it on ICU version 54.1, it dumped US$120,000.00.
But when I tested it on ICU version 51.2, it dumped $120,000.00 not US$120,000.00.
(I tested both versions on my local computer.)

Therefore my commit couldn't pass the test on travis.
https://travis-ci.org/chinpei215/cakephp/jobs/123295855#L623

Regardless of my change, if there is a following test, the build would fail on travis.

$result = $this->Number->currency('120000', 'USD', ['locale' => 'en_GB']);
$expected = 'US$120,000.00';
$this->assertEquals($expected, $result);

What should I do? Should I hack this behavior too? Or should I test the other currency (e.g. CAD) for passing test?

[ADmad]

TLDR; People should make their own helper function which uses Number::format() and prefix the currency code/symbol to get desired format consistently. That's what I did :smile:

[chinpei215]

Parhaps, he want to display any arbitrary currencies with any arbitrary format. It would be boring to implement.

$10,000
€10,000
£10,000
¥1,000,000
...

[markstory]

@chinpei215 I think there will be many of those kinds of issues with different intl versions. Given how far we are away from the intl implementation, and how variable the intl output is, I'd rather not try and hack around output that we don't know.

[chinpei215]

@markstory Would it be better to discard this commit?

[elboletaire]

I guess, this is the issue because I'm trying to truncate a number and it won't truncate?

$this->Number->config(['places' => 1, 'precision' => 2]);
echo $this->Number->currency(23.59);
// expected output: 23.5
// real output: 23.59

TLDR; People should make their own helper function which uses Number::format() and prefix the currency code/symbol to get desired format consistently. That's what I did :smile:

I was doing that, because I didn't know there was a currency method in the NumberHelper. When I discovered the method I realized about the issue, so I'll continue using my own CurrencyHelper (which extends NumberHelper) until this is fixed.

[chinpei215]

@elboletaire Probably you can use the 3rd argument of the currency() method if you don't mind rounding up.

ini_set('intl.default_locale', 'en_US');

echo $this->Number->currency(23.59, null, ['precision' => 1]);
// $23.6

[elboletaire]

Thanks @chinpei215 but I need to truncate (client specs...). I thought that using 'precision' => 2 with 'places' => 1 would do the trick, but, actually, it doesn't.

[chinpei215]

@elboletaire The places option is used for NumberFormatter::MIN_FRACTION_DIGITS, that is not suitable for your purpose. Probably, we would need to add a new option into Cake\I18n\Number::_setAttributes() to allow to change NumberFormatter::ROUNDING_MODE.

$nf = new NumberFormatter('en-US', NumberFormatter::CURRENCY);
$nf->setAttribute(NumberFormatter::MAX_FRACTION_DIGITS, 1);

// This option would be suitable for your purpose but not implemented yet in Cake\I18n\Number
$nf->setAttribute(NumberFormatter::ROUNDING_MODE, NumberFormatter::ROUND_FLOOR);

echo $nf->formatCurrency(23.59, 'USD');
// $23.5

[lorenzo]

I'll mark this as an enhancement. The possibility is there, by configuring the formatter using the rounding options, but it could be made easier as a future enhancement

[dereuromark]

Is anyone able to provide a PR here?

[github-actions[bot]]

This issue is stale because it has been open for 120 days with no activity. Remove the stale label or comment or this will be closed in 15 days

[github-actions[bot]]

This issue is stale because it has been open for 120 days with no activity. Remove the stale label or comment or this will be closed in 15 days

[othercorey]

Did anyone submit a php bug report for this?

[othercorey]

I found the bug for this at https://bugs.php.net/bug.php?id=65572

There is an upstream bug attached to it at https://unicode-org.atlassian.net/browse/ICU-10997

[othercorey]

This was a bug in libICU that is fixed. The issue is still open targetting 69.1, but was fixed in ICU 61 at least. I provided a test case so they can close the bug and it appears to work fine.

https://github.com/unicode-org/icu/pull/1444