-
-
Notifications
You must be signed in to change notification settings - Fork 825
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
Use money formatting for currency in templates #21783
Conversation
(Standard links)
|
I don't have any issues with losing the 'space' |
@petednz being spaced out is not important to you then? |
@KarinG any concerns with losing the space? (I know you are online at the moment :-) |
e58387d
to
0f7ea29
Compare
Says -> in the area of finance -> the proper formatting requires a non-breaking space: “In the area of finance, especially in texts discussing currency values and exchange rates, the universal code CAD is the usual symbol used. CAD is the international currency code established by the International Organization for Standardization (ISO) to represent the Canadian dollar. It is composed of the country code (CA), followed by the letter "D" for "dollar." Write the code first, followed by a non-breaking space and the dollar figure: CAD 350 million |
@KarinG ok - so this change is neutral in that case then? ie this is altering a format that doesn't follow the financial formatting before or after - if we look at the general texts section then this would be closer in it's 'after' form |
d82914f
to
6ac22ec
Compare
We just forked but discussed putting the large token-patches into the 'token-release' 5.43 so people only have to focus on one release |
@eileenmcnaughton You need to drop the "set version" commit from this PR. If we are changing the way money is formatted we should be going directly to the "correct" way of formatting and not another incorrect way. The "space" is important in multiple currencies and as we are not the end-users I don't think we can just say it doesn't matter. What is stopping us moving to formatting correctly for each currency? |
@mattwire so this uses brickmoney - so you would get the space in some locales - eg civicrm-core/tests/phpunit/Civi/Core/FormatTest.php Lines 115 to 126 in 21a7b29
But it is removed from the US locale - since that is not considered standard by the formatting library (@colemanw & @totten indicated it's not something they see a lot of outside of Civi & it's not something that one would normally see in NZ either) |
@eileenmcnaughton Ok thanks - sounds good! |
6ac22ec
to
cf3ca6b
Compare
cf3ca6b
to
f70a513
Compare
return $row->format('text/plain')->tokens($entity, $field, | ||
\CRM_Utils_Money::format($fieldValue, $this->getCurrency($row))); | ||
Money::of($fieldValue, $currency)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just confirming that - to my testing - this change from CRM_Utils_Money::format()
to Civi::format()->money()
produces more adaptive/correct formatting for multinational/multicurrency usage.
Before
For example, if I call the old CRM_Utils_Money::format()
in a mix of languages and locales, it uses the singular $config->moneyformat
for everything, which gives some misleading output:
for curr in USD CAD EUR ; do echo "### Currency: $curr"; for loc in en_CA fr_CA en_US en_GB fr_FR ; do export curr loc; echo -n "(Rendered in $loc) " ; cv ev 'CRM_Core_I18n::singleton()->setLocale(getenv("loc")); echo CRM_Utils_Money::format(1234.56, getenv("curr"));' ; done ; echo; done
### Currency: USD
(Rendered in en_CA) $ 1,234.56
(Rendered in fr_CA) $ 1,234.56
(Rendered in en_US) $ 1,234.56
(Rendered in en_GB) $ 1,234.56
(Rendered in fr_FR) $ 1,234.56
### Currency: CAD
(Rendered in en_CA) $ 1,234.56
(Rendered in fr_CA) $ 1,234.56
(Rendered in en_US) $ 1,234.56
(Rendered in en_GB) $ 1,234.56
(Rendered in fr_FR) $ 1,234.56
### Currency: EUR
(Rendered in en_CA) € 1,234.56
(Rendered in fr_CA) € 1,234.56
(Rendered in en_US) € 1,234.56
(Rendered in en_GB) € 1,234.56
(Rendered in fr_FR) € 1,234.56
After
But if it goes through Civi::format()->money()
, then it gives:
for curr in USD CAD EUR ; do echo "### Currency: $curr"; for loc in en_CA fr_CA en_US en_GB fr_FR ; do export curr loc; echo -n "(Rendered in $loc) " ; cv ev 'echo Civi::format()->money(1234.56, getenv("curr"), getenv("loc"));' ; done ; echo; done
### Currency: USD
(Rendered in en_CA) US$1,234.56
(Rendered in fr_CA) 1 234,56 $ US
(Rendered in en_US) $1,234.56
(Rendered in en_GB) US$1,234.56
(Rendered in fr_FR) 1 234,56 $US
### Currency: CAD
(Rendered in en_CA) $1,234.56
(Rendered in fr_CA) 1 234,56 $
(Rendered in en_US) CA$1,234.56
(Rendered in en_GB) CA$1,234.56
(Rendered in fr_FR) 1 234,56 $CA
### Currency: EUR
(Rendered in en_CA) €1,234.56
(Rendered in fr_CA) 1 234,56 €
(Rendered in en_US) €1,234.56
(Rendered in en_GB) €1,234.56
(Rendered in fr_FR) 1 234,56 €
This looks much more correct to me.
I guess, technically, from some POV, there's a loss of tweakability - it abides the opinions of "Brick Money" instead of abiding the opinion of $config->moneyformat
. That mostly seems like a gain - ie users who conduct transactions in their native currency see their idiomatic formats, and users who transact in a foreign currency see disambiguated format (e.g. en_US
user who pays CAD sees $CA
) -- and neither behavior requires special configuration-work.
IMHO, if anyone does miss the customization of $config->moneyformat
or misses the use of one-size-fits-all formats (like %a %C(%c)
), then that merits an orthogonal issue/discussion/patch about refining Civi::format()
. It wouldn't change what happens in this patch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think things are more complicated than either before or after supports. See accepted answer for EUR for https://ux.stackexchange.com/questions/9105/international-currency-formatting-guidelines-currency-codes. Note that for European institutions, their requirement is that for formal documents use EUR 250 for 4 countries and 250 EUR for the rest, and this is required in legal texts. The euro sign € is primarily used in graphics but is permitted in popular works and promotional publications e.g. sales catalogues. I think that formally our receipts are legal texts, so we might be offside in use the € mark rather that EUR.
We should try to identify standards that we can follows. Currency codes are defined by the listing of currency codes in ISO 4217 but that doesn't specify formatting https://en.wikipedia.org/wiki/ISO_4217#Position_of_ISO_4217_code_in_amounts.
The old Unicode TR35 is a bit dated: https://unicode.org/reports/tr35/tr35-numbers.html#Currencies
There is the Unicode CLDR project which seems to promise a standard for formatting that is better than the listing of currency codes in ISO 4217. This seems relevant: https://cldr.unicode.org/translation/number-currency-formats/number-and-currency-patterns. But I couldn't find anything useful when skimming through the site for our question about whether a space between the amount and currency is allowed or required, and whether the currency should be before or after the amount for a locale.
Overview
Use money formatting for currency. This allows us to render each row with the money formatting appropriate to their locale where known - e.g Norwegian Euro formatting differs from French.
Before
Money formatting does not alter according to the locale context of row being processed
After
The output will vary by row if locale is set (which we do based on contact language in scheduled reminders) ie
Locale = French 1 234,56 €
Locale = German 1.234,56 €
Locale = US €1,234.50
However note that there is a formatting change as a result of using a more standardised money formatting library
My straw poll to date hasn't found much support for the spaces but will try to get some input from 'anyone online'
Technical Details
In order to make this work I've had to set a define
IGNORE_SEPARATOR_CONFIG
- this added as a quasi-placeholder for migrating off configured thousand separators. However, the intent in the money code was that locale-specific formatting would kick in as soon as we were working in a locale that is not the site default. That isn't working because the token processor is changing the variable being checked. The check could be changed to ? or we could just say 'use the define for now if you want this'civicrm-core/Civi/Core/Format.php
Line 175 in 386fe6c
Comments