Fix input type for smarty number formatting (more forgiving)

[colemanw]

Overview

Fixes a hard-crash with CiviGrant, regression in 5.46 caused by #22309 .
This is a more forgiving version of #22427

Before

• Enable CiviGrant
• Create a grant
• Hard crash on "Find Grants", "Grants Dashboard" or Grants tab of contact summary screen.

After

Fixed.

Technical Details

There were confusing mismatches between docblocks and the function signatures. smarty_modifier_crmMoney had $amount annotated as type float but passed it into Format::money which requires type string, which passes it onto Money::of which accepts types BigNumber|int|float|string.
I think I've straightened it out.

[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]

what about is (strlen($amount) === 0) ?

that would get FALSE too

[colemanw]

Why would the value ever be FALSE? And if it could be a bool, what on earth would TRUE mean? How much money is TRUE worth?

[eileenmcnaughton]

@colemanw I doubt it would ever be TRUE but we DO sometimes fill our arrays with FALSE where we want to avoid enotices

[colemanw]

I guess I'm wary of type coercion, passing numbers into strlen() sounds like an invitation for weird bugs. I'd rather just add || is_bool($amount).

[demeritcowboy]

The monetary value of TRUE is USD $1 - I think it's in the Geneva Convention. Seriously though I'd argue it should throw an error if TRUE is passed in.

Between this and the other PR, I get the tradeoff between being able to catch bugs/typos early (being strict) and between not having to track down every existing caller (being forgiving), and normally I like the former, but my 2c on this one is to prefer this PR.

Regarding what the if should look like, maybe something like:

// Handle values that Brick Money doesn't like
// We also treat FALSE as '' because sometimes variables are mass-filled with FALSE to avoid being missing, so can't be treated as an error.
if (is_null($amount) || $amount === '' || $amount === FALSE) {
  return '';
}
elseif ($amount === TRUE) {
  throw new CRM_Core_Exception('TRUE is not a valid value for type money');
}

[colemanw]

Ok I've updated this PR to forgive FALSE, NULL and '' but throw an exception for all other non-numeric values.

[eileenmcnaughton]

I'm happy with that!

[colemanw]

retest this please

[demeritcowboy]

@colemanw that looks like it was a real fail. It seems the TokenProcessor assigns objects of type Money to the token values when it thinks it's a money field, and then it always passes a BigDecimal as the value (via getAmount()). But because it has a __toString() method, that previously got autocast by the parameter typehint, but now it stays as a BigDecimal.

not ok 702 - Failure: CRM_Contribute_ActionMapping_ByTypeTest::testTokenRendering
  message: 'Failure in api call for job send_reminder:  Invalid value for type money'

From my local, and if I put the typehint back it passes, confirming the autocasting:

0 \Civi\Token\TokenProcessor.php(489): Civi\Core\Format->money(Object(Brick\Math\BigDecimal), 'EUR')
1 \Civi\Token\TokenProcessor.php(383): Civi\Token\TokenProcessor->filterTokenValue(Object(Brick\Money\Money), Array, Object(Civi\Token\TokenRow))
2 \Civi\Token\TokenProcessor.php(447): Civi\Token\TokenProcessor->Civi\Token\{closure}('{contribution.n...', 'contribution', 'non_deductible_...', NULL)
3 [internal function]: Civi\Token\TokenProcessor->Civi\Token\{closure}(Array)
4 \Civi\Token\TokenProcessor.php(431): preg_replace_callback(';\\{([\\w]+)\\.([\\...', Object(Closure), '\n      first na...')
5 \Civi\Token\TokenProcessor.php(396): Civi\Token\TokenProcessor->visitTokens('\n      first na...', Object(Closure))
6 \Civi\Token\TokenRow.php(320): Civi\Token\TokenProcessor->render('body_text', Object(Civi\Token\TokenRow))

[demeritcowboy]

So what might work as a general catchall is elseif (!is_numeric((string) $amount)) {

Or explicitly only cast BigDecimals.

[demeritcowboy]

Oh no nevermind that would make TRUE be '1'

[colemanw]

What about elseif ($amount === TRUE || !is_numeric((string) $amount)) {

[eileenmcnaughton]

murmurring supportive noises (I don't have much to add since you two seem to be on the right track here)

[demeritcowboy]

Yay! Passing tests.
The code comments might not really explain to future devs why the if is the way it is, but not a blocker.