Fix failing tests on edge based on number formatting by using the Num… #16215
Conversation
…berFormatter class
|
(Standard links)
|
| @@ -62,9 +62,12 @@ public static function format($amount, $currency = NULL, $format = NULL, $onlyNu | |||
| } | |||
|
|
|||
| if ($onlyNumber) { | |||
| if ($valueFormat === '!%i') { | |||
There was a problem hiding this comment.
this is the default value maybe we should add some deprecation notice for other formats?
There was a problem hiding this comment.
if that is the case then yes probably
| // money_format() exists only in certain PHP install (CRM-650) | ||
| if (is_numeric($amount) and function_exists('money_format')) { | ||
| $amount = money_format($valueFormat, $amount); | ||
| $amount = self::formatNumericByFormat($amount, $valueFormat); |
There was a problem hiding this comment.
Just some standardisation / re-use of functions
There was a problem hiding this comment.
Should if [..] function_exists('money_format') also be removed?
|
@mattwire can you review this? |
|
@seamuslee001 I took a pretty thorough look & could not find a single instance where the $valueFormat is passed into this function - I should we should deprecate passing it in & ignore any passed in value. That would mean you could just call formatLocaleNumeric & restrict your weird handling to that function However, I also suspect that in fact we are retaining code that is not really being used & would likely actively break things if it were - I need input from people like @jaapjansma @bjendres @mlutfy @mattwire but my reading is that the money_value config variable could be set to
But because the latter didn't work on all installs (money_format was not always present) we ALSO added all that thousand separator code blah blah & probably that meant that no-one alters the money_value under localisation & that if they do it would potentially actively break things. I'm keen to hear back if people use it but if not I think it will simplify this code & allow us to switch to just the one preferred method (the new number formattter class ) |
|
My understanding is that the money format settings don't impact anything. This setting always sounded technical to me and I haven't seen any effect of changing this. So to me it is a useless setting (but that might be that I dont understand what it does). I have also checked with Betty and see was also not aware of this setting. |
|
By the way have you seen my pr: #16392 |
|
@seamuslee001 I posted in translation channel to solicit more feedback but @jaapjansma's comments support my belief we can kill that setting as an early attempt that didn't get traction |
|
If we can rely on the locale for displaying in the correct format, then yes, let's remove this. The settings names and the code have always been confusing to me. If I understand correctly, more cleanup would be done? Can you ping me before merging? I will apply it on our production sites. |
|
@mlutfy this is how I think the function would look without using that config var Note the json change - but I think NumberFormatter is 'standard'? |
|
@seamuslee001 could you fix the hard coded en_US localization? |
|
Betty and I are reviewing PR's and we came across this one and we see there has been no work on this PR since Jan 29 so we will close this PR. |
…berFormatter class
Overview
This aims to fix the following tests that failed on edge which is running PHP7.3, i can't get them to fail on my local php7.3 instance however money_format is deprecated as of PHP7.4
Before
Tests fail on edge profile
After
Tests pass on edge profile
ping @eileenmcnaughton @monishdeb @mattwire @totten @JoeMurray