dev/core#1959 Brick\Math\Exception\RoundingNecessaryException

[chamilwijesooriya]

Overview

When viewing/editing a contribution, throws Brick\Math\Exception\RoundingNecessaryException: "Rounding is necessary to represent the result of the operation at this scale."

Happens in 5.28.0 as a regression of #17608

Before

Steps to reproduce

1. Install Civi with sample data
2. Go to any contact
3. Create a new General membership with default values (Completed status etc) Amount should be $100.00
4. Go to contributions tab and view the new contribution
5. Throws the following error
   

After

Contribution can be viewed/edited correctly

Technical Details

The issue happens due to BCMath PHP extension. If GMP is installed, it works as intended. The following code in BrickMath chooses the class accordingly https://github.com/brick/math/blob/63dc40f6a2bbebf670b4d4b5f16bef2458a61402/src/Internal/Calculator.php#L76-L94

/**
 * Returns the fastest available Calculator implementation.
 *
 * @codeCoverageIgnore
 *
 * @return Calculator
 */
private static function detect() : Calculator
{
    if (\extension_loaded('gmp')) {
        return new Calculator\GmpCalculator();
    }


    if (\extension_loaded('bcmath')) {
        return new Calculator\BcMathCalculator();
    }


    return new Calculator\NativeCalculator();
}

Upon further investigation showed that while $leftOp amount is taken as BigNumber while $rightOp is taken as BigRational as that amount is not converted in the following code

civicrm-core/CRM/Utils/Money.php

Lines 141 to 144 in c3852f1

	if (is_numeric($leftOp) && is_numeric($rightOp)) {
	$money = Money::of($leftOp, $currency, new DefaultContext(), RoundingMode::CEILING);
	return $money->minus($rightOp)->getAmount()->toFloat();
	}

Therefore added code to get Money::of to $rightOP as well.

Comments

Not sure how rounding might affect this

[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).

[seamuslee001]

@chamilwijesooriya thanks for this are you able to either rebase this PR against 5.29 branch or create a new PR for the 5.29 branch as it is the RC and this is a recent regression

[totten]

Not sure how rounding might affect this

So here's a little throw-away script to see whether/how the behavior changes with inputs of varying precision:

https://gist.github.com/totten/d6894af0580e61d09cac4357952b5353

I ran this locally on PHP 7.1.33 w/both gmp and bcmath installed (so I guess gmp is the active one). Many values are the same with or without the patch. Ex:

1.23456 - 1.23 (USD) = 0.01
1.23456 - 12 (USD) = -10.76

The patch changes the outcome when the right-hand side has unusually high precision:

// Before
'12' - '1.23456' (USD) = Rounding is necessary to represent the result of the operation at this scale.
'12' - '9.8765' (USD) = Rounding is necessary to represent the result of the operation at this scale.

// After
'12' - '1.23456' (USD) = 10.76
'12' - '9.8765' (USD) = 2.12

Basically, the change is on this question: "What do you do when subtracting currencies with unusually high precision?"

Code	If LHS has high precision	If RHS has high precision
Pre-patch	RoundingMode::CEILING	Throw exception
Post-patch	RoundingMode::CEILING	RoundingMode::CEILING

From a general code-aesthetic POV, I think this change makes the function more consistent/intuitive. But I guess the real question is: "what rounding policy makes sense for the use-case?" Thankfully (at least within core), there only seems to be one caller/use-case:

civicrm-core/CRM/Contribute/BAO/Contribution.php

Lines 4181 to 4200 in 11af302

	/**
	* Get the outstanding balance on a contribution.
	*
	* @param int $contributionId
	* @param float $contributionTotal
	* Optional amount to override the saved amount paid (e.g if calculating what it WILL be).
	*
	* @return float
	*/
	public static function getContributionBalance($contributionId, $contributionTotal = NULL) {
	if ($contributionTotal === NULL) {
	$contributionTotal = CRM_Price_BAO_LineItem::getLineTotal($contributionId);
	}
	return (float) CRM_Utils_Money::subtractCurrencies(
	$contributionTotal,
	CRM_Core_BAO_FinancialTrxn::getTotalPayments($contributionId, TRUE),
	CRM_Core_DAO::getFieldValue('CRM_Contribute_DAO_Contribution', $contributionId, 'currency')
	);
	}

The RHS (getTotalPayments(...)) comes from a SQL query SELECT SUM(ft.total_amount) FROM civicrm_financial_trxn ft.... So if we're being thoroughly pedantic, then the reduced question is, "What should be the rounding behavior when SUM(ft.total_amount) has excess precision? Or maybe it doesn't really matter?"

(I've got 🤞 for "doesn't really matter".)

[chamilwijesooriya]

@seamuslee001 thanks for that

[chamilwijesooriya]

@totten good stuff!

I think a decimal point makes a huge difference in cases like bit coins but again it's rare. In the long run you can add precision for each currency and limit front end input accordingly, might solve the issues, as seen in this code

civicrm-core/CRM/Utils/Money.php

Lines 114 to 128 in c3852f1

	/**
	* This is a placeholder function for calculating the number of decimal places for a currency.
	*
	* Currently code assumes 2 decimal places but some currencies (bitcoin, middle eastern) have
	* more. By using this function we can signpost the locations where the number of decimal places is
	* currency specific for future enhancement.
	*
	* @param string $currency
	*
	* @return int
	* Number of decimal places.
	*/
	public static function getCurrencyPrecision($currency = NULL) {
	return 2;
	}

But can't really help with irrationals