dev/core#2425 - php 7.4 - E_NOTICE every time you save a contribution

[demeritcowboy]

Overview

https://lab.civicrm.org/dev/core/-/issues/2425

Before

1. Use php 7.4
2. Make a contribution in the backend.
3. Save it.

Notice: Trying to access array offset on value of type null in CRM_Contribute_Form_Contribution::formRule() (line 915 of CRM\Contribute\Form\Contribution.php).

After

No notice.

If you want to check that the financial account validation is still working you can:

1. Turn on deferred revenue in civicontribute settings.
2. Make a contribution with a price set that uses a financial type that doesn't have a corresponding deferred revenue account, like Donation by default.
3. Enter a recognition date on the form.
4. You should get an alert box validation error.

Technical Details

See lab ticket. The gist is to not calculate the line items from the _priceSet variable which is missing and instead get the line items from the order and pass it in.

In terms of the affected scope:

• If deferred revenue isn't enabled nothing changes and it just returns early.
• If you don't fill in a recognition date, nothing changes it just returns early.
• If it's a new contribution and you don't use a price set, the line items are empty and both before and after it skips trying to get them (line 401), so again nothing changes.
• When you're editing a contribution, I'm not sure what 'prevContribution' is or when it would be present, but when it isn't then again it returns early.
• checkFinancialTypeHasDeferred() is only called from the one place in the contribution formRule, in core at least.
• In the call to processAmount() before, $params was passed by reference and updated, however it's never used again in checkFinancialTypeHasDeferred() and was passed into there by value, so removing the call to processAmount() doesn't change that.
• The purpose of calling processAmount() was to get the line items, but we already have it now.
• The only element of the line item it actually uses is the financial_type_id.

So the scope is pretty limited. It might miss the validation warning if for some reason the order'ized version of the line items is somehow different than before. I don't know enough about it so I'm just assuming it always matches up.

I'm sure there is more here to clean up but I really just want the problem to go away since it comes up every single time you save a contribution.

Comments

Has test. Note it passes on php 7.3 or lower even before the patch, but fails on php 7.4.

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

Do we need the if (!empty ) here - I feel like it has carried over from the original but I suspect it is no longer needed

[demeritcowboy]

If I change the function signature to default [] instead of null then I think that would be ok, since then the loop won't choke iterating over null.

[eileenmcnaughton]

yeah - it looks like the only 'live' call to this would always pass an array but the test calls don't

[eileenmcnaughton]

I feel so happy seeing the demise of this line

[monishdeb]

Tested on test build http://core-19978-75go.test-1.civicrm.org:8001/ (cred - admin / aPshklfG1cPf), on contribution back office form, encountered no regression or breakage. Reviewed the patch looks good to me. Added unit-test passed on php 7.4 on my local m/c. Hence merging the PR

[demeritcowboy]

Thanks! @monishdeb