dev/core#2493 Stop attempting to format money in the processor class #20040
Conversation
We've said this is wrong before but it has survived until now because it seemed scarier to change it. However, now the reverse seems true. The value in amount should always be machine friendly and there are no known processors that expect locale specific formatting. On the other hand the format() function is intended to prepare money for DISPLAY which is not what is going on here
|
(Standard links)
|
|
Agree with removing CRM_Utils_Money::format but not sure about returning the array as-is. PropertyBag, mjwshared, omnipay all do: |
|
hmm - I'd rather consider something more complicated in master (with a test to demonstrate the difference) |
|
@eileenmcnaughton I've been keeping away from the money formatting stuff for a little bit so happy for a change to be made if you/others are confident it won't cause other regressions. But I think (any) change here carries a high-risk of causing further regressions and we need to be very careful. The advantage of the pattern I pointed out is that it forces the value to be numeric and to use the decimal separator so values such as "$10.00" won't pass - obviously if we could be confident that every "amount" will be passed in correctly we wouldn't need to worry but I don't think we're there yet. |
|
Hmm - I think it's pretty hard to get to this line without having been passed through formatting now. We've got an active regression so it's currently broken because it is trying to do double formatting |
… to return appropriate amount in that case otherwise just return amount
|
@eileenmcnaughton @mattwire as discussed in today's prod-mtc meeting I have added in a deprecation notice for when $params['amount'] is not numeric and used the filter_var there instead otherwise we just return $params['amount'] which I think resolves the concern you were expressing Matt |
|
Looks good @seamuslee001 |
|
I see this is in 5.37 and backported to 5.36 - what about 5.35? And is there test coverage for amounts > $1k now? |
|
We won't be backporting this patch nor anything other than the security patch to 5.35 |
Overview
Addresses a regression whereby values are being mis-formatted before being sent to the payment processor
Before
Per https://lab.civicrm.org/dev/core/-/issues/2493#note_57664 paypal flow (& maybe others) is not handling $1000 + values
After
Gitlab comments advise this works
Technical Details
We've said this is wrong before but it has survived until now because it seemed scarier to change it. However,
now the reverse seems true.
The value in amount should always be machine friendly and there are no known processors
that expect locale specific formatting.
On the other hand the format() function is intended to prepare money for DISPLAY which
is not what is going on here.
Comments
@mattwire @seamuslee001