dev/core#3059 Regression fix - be tolerant with smarty money

[eileenmcnaughton]

Overview

To replicate - create a custom field of type money - mine was against entity contribution.
Create a contribution (or whatever) with the field and give it a value greater than 1000.
Attempt to view - fatal error

Before

After

Loads

Technical Details

The issue is the php layer is formatting it to money -and the smarty does so again. By the second time a value > 999 is no longer valid (in US locale) and it fatals. I would expect this to be a problem in European locales for smaller values.

The fix makes seems comparible with the prior laxity. I have also already done an extended reports release which fixes the source issue in reports (ie formats only once...)

Comments

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[eileenmcnaughton]

Attempt to tackle root cause in view pages #22728

[demeritcowboy]

It only seems to come up on contributions, so I'm just trying to figure out if this is the right fix. For non-us, the error does come up for smaller values but again only on contributions. (Sidenote: For non-us it comes up even if you don't fill in a value, because it then tries to format 0,00.)

[eileenmcnaughton]

@demeritcowboy hmm - that's interesting!

I do think formatting at the tpl layer (only) is the right fix probably

[eileenmcnaughton]

@demeritcowboy I think your comment was intended for #22728 & not this PR?

[totten]

Right, so you're saying that there's two patches - #22727 is a broader (more rougher) mitigation for 5.46-5.47, and #22728 is narrower (but more correct) fix for 5.48+.

That seems like a fair split to me...

[eileenmcnaughton]

@totten yep - in addition - I don't think the instance that #22728 fixes will be the last of them - I found 2 in the extended reports extension - hence I added a Civi::log() call but not a noisier notice for now

[totten]

So I did some r-run and confirmed that this mitigates the error - so you can now view the data.

I compared the behavior to 5.45 (default locale=fr_FR, default currency=EUR, thousands='.', decimal=','):

Here's #22727 (this PR; fixes crash; missing currency symbol; fixes separators):

And here's #22728 (fixes crash; has symbol; fixes separators):

So I can confirm that the present (generic) patch does mitigate the biggest problem (the crash), and it makes sense that it would mitigate across more use-cases. But the narrower #22728 does give a better outcome (for this specific screen).

Would it be crazy to put both patches (#22727, #22728) on both branches (5.46,5.47)?

[totten]

Definitely an improvement. Since the r-run is fine, I'll merge. Other fixes can be done as additional PRs.