dev/translation#48 Implement Brick/Money to better handle money values #17608
Conversation
|
(Standard links)
|
|
Looks good to me - when you fix the style can you add "ext-intl": "*" to composer.json |
seamuslee001
force-pushed
the
dev_translation_48
branch
from
6d38345
to
87eeca1
Compare
|
done @eileenmcnaughton |
This is a partial fix for https://lab.civicrm.org/dev/core/-/issues/1603 because we should be doing our formatting at the presentation layer rather than the BAO layer. I suspect the wrong values might still be saved in the database but this gives us a bit more space to fix that without breakin the display. It also includes the brickMoney library per civicrm#17608 as part of the fix. Note that the rounding I chose was RoundingMode::CEILING - this is not set in stone but the sum of the individual amounts come closer to the total than with rounding down. Note there were 2 params passed to the function that I removed - I think they are unnused but I am digging further. Also note that this uses the default locale but we might prefer to pass the user's locale when we know it as the locale can alter formatting e.g for Euro
|
@seamuslee001 I worked on my own mini-implementation of this - #17609 & I hit an issue where the value In the PR I did adding rounding was the answer - but that's because I wanted rounding too |
This is a partial fix for https://lab.civicrm.org/dev/core/-/issues/1603 because we should be doing our formatting at the presentation layer rather than the BAO layer. I suspect the wrong values might still be saved in the database but this gives us a bit more space to fix that without breakin the display. It also includes the brickMoney library per civicrm#17608 as part of the fix. Note that the rounding I chose was RoundingMode::CEILING - this is not set in stone but the sum of the individual amounts come closer to the total than with rounding down. Note there were 2 params passed to the function that I removed - I think they are unnused but I am digging further. Also note that this uses the default locale but we might prefer to pass the user's locale when we know it as the locale can alter formatting e.g for Euro
This is a partial fix for https://lab.civicrm.org/dev/core/-/issues/1603 because we should be doing our formatting at the presentation layer rather than the BAO layer. I suspect the wrong values might still be saved in the database but this gives us a bit more space to fix that without breakin the display. It also includes the brickMoney library per civicrm#17608 as part of the fix. Note that the rounding I chose was RoundingMode::CEILING - this is not set in stone but the sum of the individual amounts come closer to the total than with rounding down. Note there were 2 params passed to the function that I removed - I think they are unnused but I am digging further. Also note that this uses the default locale but we might prefer to pass the user's locale when we know it as the locale can alter formatting e.g for Euro
This is a partial fix for https://lab.civicrm.org/dev/core/-/issues/1603 because we should be doing our formatting at the presentation layer rather than the BAO layer. I suspect the wrong values might still be saved in the database but this gives us a bit more space to fix that without breakin the display. It also includes the brickMoney library per civicrm#17608 as part of the fix. Note that the rounding I chose was RoundingMode::CEILING - this is not set in stone but the sum of the individual amounts come closer to the total than with rounding down. Note there were 2 params passed to the function that I removed - I think they are unnused but I am digging further. Also note that this uses the default locale but we might prefer to pass the user's locale when we know it as the locale can alter formatting e.g for Euro
eileenmcnaughton
changed the title
|
@seamuslee001 can you try adding 269.565217391 to the test to ensure it doesn't kill it |
|
(if good then let's merge) |
…lues Added in test case as per Eileen comment so we know we can handle large floats
seamuslee001
force-pushed
the
dev_translation_48
branch
from
87eeca1
to
ca4cfe7
Compare
|
@eileenmcnaughton had to add in the rounding but this should work now |
|
@seamuslee001 that's what I thought - this does make sense in the context of how this is called - ie. it's serving up presentation layer information, but if it were called earlier in the chain it could cause problems. That's non-blocking here since but I think the learning we should take is that we should pass around the Money object & convert to float at the last minute - so really this function probably should return a money object I think it's OK to merge but we need to do some docs / communicate on this before we are done with it.. @mattwire FYI |
|
@seamuslee001 I'm flinching a bit here -we didn't dig that deep on whether php-intl could be missing. we should think about
|
|
Just a question you've likely already examined but...have we checked about whether our money fields can support all currencies? As I recall there are a number that have a different number of decimals other than 2, and I imagine the ones that require more need to be excluded as unsupported. There were some national currencies like this that are in the standard iirc and others that have had support in CiviCRM at different times like bitcoin. BTW, has anyone spent the money to buy https://www.currency-iso.org/ ? We should charge this to the core team and make it available to developers working on money as needed without inappropriately publishing it. |
|
@JoeMurray they do btc - which has many decimal places.... |
Overview
This implements the PHP library brick/money https://github.com/brick/money to better handle currency amounts
Before
Non standard library used
After
Library used to make calculations
https://lab.civicrm.org/dev/translation/-/issues/48
ping @eileenmcnaughton