From 14dd185717a79705721452ece101901f4058c124 Mon Sep 17 00:00:00 2001 From: Hosh Sadiq Date: Sun, 19 Jul 2020 00:00:19 +0100 Subject: [PATCH] Use php-intl to format currencies Currently the php function `number_format` is used to format currencies. This is problematic as we have to figure out different things for different currencies ourselves. These formats are determined based on the libc's locale functions. The issue arises where an OS doesn't have the proper locales installed, or, in some cases, it's not supported (see below on multiple issues). This addresses this issue by using the php-intl extensions to format the numbers based on the locale. The extension is already a requirement in `composer.json`. The solution does not rely on `LC_MONETARY` from the underlying libc (which in Alpine Linux's case, which uses musl, is not supported as of yet). List of issues that are related and would potentially be fixed using this PR: - #2298 - #2946 - #3070 - #3306 - #3519 --- app/Http/Controllers/JavascriptController.php | 20 ++- app/Support/Amount.php | 151 ++++-------------- app/Support/Facades/Amount.php | 1 - resources/views/v1/javascript/variables.twig | 8 +- .../Controllers/JavascriptControllerTest.php | 3 - tests/Support/AmountTest.php | 79 +++++++++ 6 files changed, 119 insertions(+), 143 deletions(-) create mode 100644 tests/Support/AmountTest.php diff --git a/app/Http/Controllers/JavascriptController.php b/app/Http/Controllers/JavascriptController.php index ba293e76c7..2e9f9f8076 100644 --- a/app/Http/Controllers/JavascriptController.php +++ b/app/Http/Controllers/JavascriptController.php @@ -144,9 +144,8 @@ class JavascriptController extends Controller $currency = app('amount')->getDefaultCurrency(); } - $localeconv = app('amount')->getLocaleInfo(); - $accounting = app('amount')->getJsConfig($localeconv); - $localeconv['frac_digits'] = $currency->decimal_places; + $accountingLocaleInfo = app('amount')->getAccountingLocaleInfo(); + $accountingLocaleInfo['frac_digits'] = $currency->decimal_places; $pref = app('preferences')->get('language', config('firefly.default_language', 'en_US')); /** @noinspection NullPointerExceptionInspection */ $lang = $pref->data; @@ -154,14 +153,13 @@ class JavascriptController extends Controller $uid = substr(hash('sha256', sprintf('%s-%s-%s', (string) config('app.key'), auth()->user()->id, auth()->user()->email)), 0, 12); $data = [ - 'currencyCode' => $currency->code, - 'currencySymbol' => $currency->symbol, - 'accounting' => $accounting, - 'localeconv' => $localeconv, - 'language' => $lang, - 'dateRangeTitle' => $dateRange['title'], - 'dateRangeConfig' => $dateRange['configuration'], - 'uid' => $uid, + 'currencyCode' => $currency->code, + 'currencySymbol' => $currency->symbol, + 'accountingLocaleInfo' => $accountingLocaleInfo, + 'language' => $lang, + 'dateRangeTitle' => $dateRange['title'], + 'dateRangeConfig' => $dateRange['configuration'], + 'uid' => $uid, ]; $request->session()->keep(['two-factor-secret']); diff --git a/app/Support/Amount.php b/app/Support/Amount.php index 27b3856312..eb0f25e863 100644 --- a/app/Support/Amount.php +++ b/app/Support/Amount.php @@ -28,6 +28,7 @@ use FireflyIII\User; use Illuminate\Contracts\Encryption\DecryptException; use Illuminate\Support\Collection; use Log; +use NumberFormatter; /** * Class Amount. @@ -37,80 +38,6 @@ use Log; class Amount { - /** - * bool $sepBySpace is $localeconv['n_sep_by_space'] - * int $signPosn = $localeconv['n_sign_posn'] - * string $sign = $localeconv['negative_sign'] - * bool $csPrecedes = $localeconv['n_cs_precedes']. - * - * @param bool $sepBySpace - * @param int $signPosn - * @param string $sign - * @param bool $csPrecedes - * - * @return string - * - */ - public static function getAmountJsConfig(bool $sepBySpace, int $signPosn, string $sign, bool $csPrecedes): string - { - // negative first: - $space = ' '; - - // require space between symbol and amount? - if (false === $sepBySpace) { - $space = ''; // no - } - - // there are five possible positions for the "+" or "-" sign (if it is even used) - // pos_a and pos_e could be the ( and ) symbol. - $posA = ''; // before everything - $posB = ''; // before currency symbol - $posC = ''; // after currency symbol - $posD = ''; // before amount - $posE = ''; // after everything - - // format would be (currency before amount) - // AB%sC_D%vE - // or: - // AD%v_B%sCE (amount before currency) - // the _ is the optional space - - // switch on how to display amount: - switch ($signPosn) { - default: - case 0: - // ( and ) around the whole thing - $posA = '('; - $posE = ')'; - break; - case 1: - // The sign string precedes the quantity and currency_symbol - $posA = $sign; - break; - case 2: - // The sign string succeeds the quantity and currency_symbol - $posE = $sign; - break; - case 3: - // The sign string immediately precedes the currency_symbol - $posB = $sign; - break; - case 4: - // The sign string immediately succeeds the currency_symbol - $posC = $sign; - } - - // default is amount before currency - $format = $posA . $posD . '%v' . $space . $posB . '%s' . $posC . $posE; - - if ($csPrecedes) { - // alternative is currency before amount - $format = $posA . $posB . '%s' . $posC . $space . $posD . '%v' . $posE; - } - - return $format; - } - /** * This method will properly format the given number, in color or "black and white", * as a currency, given two things: the currency required and the current locale. @@ -142,13 +69,16 @@ class Amount */ public function formatFlat(string $symbol, int $decimalPlaces, string $amount, bool $coloured = null): string { + $locale = app('steam')->getLocale(); + $coloured = $coloured ?? true; - $info = $this->getLocaleInfo(); - $formatted = number_format((float) $amount, $decimalPlaces, $info['mon_decimal_point'], $info['mon_thousands_sep']); - $precedes = $amount < 0 ? $info['n_cs_precedes'] : $info['p_cs_precedes']; - $separated = $amount < 0 ? $info['n_sep_by_space'] : $info['p_sep_by_space']; - $space = true === $separated ? ' ' : ''; - $result = false === $precedes ? $formatted . $space . $symbol : $symbol . $space . $formatted; + $float = round($amount, 12); + + $fmt = new NumberFormatter( $locale, NumberFormatter::CURRENCY ); + $fmt->setSymbol(NumberFormatter::CURRENCY_SYMBOL, $symbol); + $fmt->setAttribute(NumberFormatter::MIN_FRACTION_DIGITS, $decimalPlaces); + $fmt->setAttribute(NumberFormatter::MAX_FRACTION_DIGITS, $decimalPlaces); + $result = $fmt->format($float); if (true === $coloured) { if ($amount > 0) { @@ -300,58 +230,31 @@ class Amount } /** - * This method returns the correct format rules required by accounting.js, - * the library used to format amounts in charts. - * - * @param array $config - * * @return array */ - public function getJsConfig(array $config): array + public function getAccountingLocaleInfo(): array { - $negative = self::getAmountJsConfig($config['n_sep_by_space'], $config['n_sign_posn'], $config['negative_sign'], $config['n_cs_precedes']); - $positive = self::getAmountJsConfig($config['p_sep_by_space'], $config['p_sign_posn'], $config['positive_sign'], $config['p_cs_precedes']); + $locale = app('steam')->getLocale(); + + $fmt = new NumberFormatter( $locale, NumberFormatter::CURRENCY ); + + $positivePrefixed = $fmt->getAttribute(NumberFormatter::POSITIVE_PREFIX) !== ''; + $negativePrefixed = $fmt->getAttribute(NumberFormatter::NEGATIVE_PREFIX) !== ''; + + $positive = ($positivePrefixed) ? '%s %v' : '%v %s'; + $negative = ($negativePrefixed) ? '%s %v' : '%v %s'; return [ - 'pos' => $positive, - 'neg' => $negative, - 'zero' => $positive, + 'mon_decimal_point' => $fmt->getSymbol(NumberFormatter::MONETARY_SEPARATOR_SYMBOL), + 'mon_thousands_sep' => $fmt->getSymbol(NumberFormatter::MONETARY_GROUPING_SEPARATOR_SYMBOL), + 'format' => [ + 'pos' => $positive, + 'neg' => $negative, + 'zero' => $positive, + ] ]; } - /** - * @return array - */ - public function getLocaleInfo(): array - { - // get config from preference, not from translation: - $locale = app('steam')->getLocale(); - $array = app('steam')->getLocaleArray($locale); - - setlocale(LC_MONETARY, $array); - $info = localeconv(); - // correct variables - $info['n_cs_precedes'] = $this->getLocaleField($info, 'n_cs_precedes'); - $info['p_cs_precedes'] = $this->getLocaleField($info, 'p_cs_precedes'); - - $info['n_sep_by_space'] = $this->getLocaleField($info, 'n_sep_by_space'); - $info['p_sep_by_space'] = $this->getLocaleField($info, 'p_sep_by_space'); - - return $info; - } - - /** - * @param array $info - * @param string $field - * - * @return bool - */ - private function getLocaleField(array $info, string $field): bool - { - return (is_bool($info[$field]) && true === $info[$field]) - || (is_int($info[$field]) && 1 === $info[$field]); - } - /** * @param string $value * diff --git a/app/Support/Facades/Amount.php b/app/Support/Facades/Amount.php index 992f6fbfbc..a72db8bb45 100644 --- a/app/Support/Facades/Amount.php +++ b/app/Support/Facades/Amount.php @@ -38,7 +38,6 @@ use Illuminate\Support\Facades\Facade; * @method string getCurrencySymbol() * @method TransactionCurrency getDefaultCurrency() * @method TransactionCurrency getDefaultCurrencyByUser(User $user) - * @method array getJsConfig(array $config) */ class Amount extends Facade { diff --git a/resources/views/v1/javascript/variables.twig b/resources/views/v1/javascript/variables.twig index 5af0c92803..d16978f4d2 100644 --- a/resources/views/v1/javascript/variables.twig +++ b/resources/views/v1/javascript/variables.twig @@ -29,13 +29,13 @@ var uid = "{{ uid }}"; var language = "{{ language|escape }}"; var currencyCode = '{{ currencyCode|escape('js') }}'; var currencySymbol = '{{ currencySymbol|escape('js') }}'; -var mon_decimal_point = "{{ localeconv.mon_decimal_point|escape('js') }}"; -var mon_thousands_sep = "{{ localeconv.mon_thousands_sep|escape('js') }}"; -var frac_digits = {{ localeconv.frac_digits }}; +var mon_decimal_point = "{{ accountingLocaleInfo.mon_decimal_point|escape('js') }}"; +var mon_thousands_sep = "{{ accountingLocaleInfo.mon_thousands_sep|escape('js') }}"; +var frac_digits = {{ accountingLocaleInfo.frac_digits }}; var noDataForChart = '{{ trans('firefly.no_data_for_chart')|escape('js') }}'; var showFullList = '{{ trans('firefly.show_full_list')|escape('js') }}'; var showOnlyTop = '{{ trans('firefly.show_only_top',{number:listLength})|escape('js') }}'; -var accountingConfig = {{ accounting|json_encode|raw }}; +var accountingConfig = {{ accountingLocaleInfo.format|json_encode|raw }}; var token = '{{ csrf_token() }}'; var sessionStart = '{{ session('start').format('Y-m-d') }}'; var sessionEnd = '{{ session('end').format('Y-m-d') }}'; diff --git a/tests/Feature/Controllers/JavascriptControllerTest.php b/tests/Feature/Controllers/JavascriptControllerTest.php index 32625a8643..e35df91c1f 100644 --- a/tests/Feature/Controllers/JavascriptControllerTest.php +++ b/tests/Feature/Controllers/JavascriptControllerTest.php @@ -109,7 +109,6 @@ class JavascriptControllerTest extends TestCase $accountRepos->shouldReceive('findNull')->andReturn($account); $currencyRepos->shouldReceive('findNull')->andReturn($euro); $accountRepos->shouldReceive('getMetaValue')->withArgs([Mockery::any(), 'currency_id'])->andReturn('1'); - Amount::shouldReceive('getJsConfig')->andReturn([])->once(); $this->be($this->user()); $this->changeDateRange($this->user(), $range); @@ -135,7 +134,6 @@ class JavascriptControllerTest extends TestCase $accountRepos->shouldReceive('findNull')->andReturn($account); $currencyRepos->shouldReceive('findNull')->andReturn($euro); $accountRepos->shouldReceive('getMetaValue')->withArgs([Mockery::any(), 'currency_id'])->andReturn('1'); - Amount::shouldReceive('getJsConfig')->andReturn([])->once(); $this->be($this->user()); $this->changeDateRange($this->user(), $range); @@ -157,7 +155,6 @@ class JavascriptControllerTest extends TestCase $account = $this->getRandomAsset(); $euro = $this->getEuro(); //Amount::shouldReceive('getDefaultCurrency')->andReturn($euro)->times(2); - Amount::shouldReceive('getJsConfig')->andReturn([])->once(); $accountRepos = $this->mock(AccountRepositoryInterface::class); $currencyRepos = $this->mock(CurrencyRepositoryInterface::class); diff --git a/tests/Support/AmountTest.php b/tests/Support/AmountTest.php new file mode 100644 index 0000000000..4a24bea25a --- /dev/null +++ b/tests/Support/AmountTest.php @@ -0,0 +1,79 @@ +mockDefaultConfiguration(); + Steam::shouldReceive('getLocale')->andReturn($locale); + Steam::shouldReceive('getLocaleArray')->andReturn([$locale . ".UTF-8"]); + + $amountObj = new Amount(); + $result = $amountObj->formatFlat($symbol, $decimalPlaces, $amount, false); + $this->assertEquals($expectedAmount, $result); + } + + public function getTestLocales() + { + return [ + ['en_US', '£6,000.00', '£', 2, '6000.00000000'], + ['en_US', '-£6,000.00', '£', 2, '-6000.00000000'], + ['en_US', '$6,000.00', '$', 2, '6000.00000000'], + ['en_US', '-$6,000.00', '$', 2, '-6000.00000000'], + ['en_GB', '£6,000.00', '£', 2, '6000.00000000'], + ['en_GB', '-£6,000.00', '£', 2, '-6000.00000000'], + ['en_GB', '$6,000.00', '$', 2, '6000.00000000'], + ['en_GB', '-$6,000.00', '$', 2, '-6000.00000000'], + ['cs_CZ', '6 000,00 Kč', 'Kč', 2, '6000.00000000'], + ['cs_CZ', '-6 000,00 Kč', 'Kč', 2, '-6000.00000000'], + ['el_GR', '6.000,00 €', '€', 2, '6000.00000000'], + ['el_GR', '-6.000,00 €', '€', 2, '-6000.00000000'], + ['es_ES', '6.000,00 €', '€', 2, '6000.00000000'], + ['es_ES', '-6.000,00 €', '€', 2, '-6000.00000000'], + ['de_DE', '6.000,00 €', '€', 2, '6000.00000000'], + ['de_DE', '-6.000,00 €', '€', 2, '-6000.00000000'], + ['fr_FR', '6 000,00 €', '€', 2, '6000.00000000'], + ['fr_FR', '-6 000,00 €', '€', 2, '-6000.00000000'], + ['it_IT', '6.000,00 €', '€', 2, '6000.00000000'], + ['it_IT', '-6.000,00 €', '€', 2, '-6000.00000000'], + ['nb_NO', 'kr 6 000,00', 'kr', 2, '6000.00000000'], + ['nb_NO', '−kr 6 000,00', 'kr', 2, '-6000.00000000'], + ['nl_NL', '€ 6.000,00', '€', 2, '6000.00000000'], + ['nl_NL', '€ -6.000,00', '€', 2, '-6000.00000000'], + ['pl_PL', '6 000,00 zł', 'zł', 2, '6000.00000000'], + ['pl_PL', '-6 000,00 zł', 'zł', 2, '-6000.00000000'], + ['pt_BR', 'R$ 6.000,00', 'R$', 2, '6000.00000000'], + ['pt_BR', '-R$ 6.000,00', 'R$', 2, '-6000.00000000'], + ['ro_RO', '6.000,00 lei', 'lei', 2, '6000.00000000'], + ['ro_RO', '-6.000,00 lei', 'lei', 2, '-6000.00000000'], + ['ru_RU', '6 000,00 ₽', '₽', 2, '6000.00000000'], + ['ru_RU', '-6 000,00 ₽', '₽', 2, '-6000.00000000'], + ['zh_TW', 'NT$6,000.00', 'NT$', 2, '6000.00000000'], + ['zh_TW', '-NT$6,000.00', 'NT$', 2, '-6000.00000000'], + ['zh_CN', '¥6,000.00', '¥', 2, '6000.00000000'], + ['zh_CN', '-¥6,000.00', '¥', 2, '-6000.00000000'], + ['hu_HU', '6 000,00 Ft', 'Ft', 2, '6000.00000000'], + ['hu_HU', '-6 000,00 Ft', 'Ft', 2, '-6000.00000000'], + ['sv_SE', '6 000,00 kr', 'kr', 2, '6000.00000000'], + ['sv_SE', '−6 000,00 kr', 'kr', 2, '-6000.00000000'], + ['fi_FI', '6 000,00 €', '€', 2, '6000.00000000'], + ['fi_FI', '−6 000,00 €', '€', 2, '-6000.00000000'], + ['vi_VN', '6.000,00 đ', 'đ', 2, '6000.00000000'], + ['vi_VN', '-6.000,00 đ', 'đ', 2, '-6000.00000000'], + ]; + } +}