Handle possibility of fee_amount = ''

[eileenmcnaughton]

Overview

This patch responds a report from edgimar on Mattermost (WordPress):

Warning: A non-numeric value encountered in /home/mywebsite.com/wp-content/plugins/civicrm/civicrm/CRM/Contribute. /BAO/Contribution.php on line 356"

Warning: Cannot modify header information - headers already sent by 
(output started at /home/mywebsite.com/wp-content/plugins/civicrm/civicrm/CRM/Contribute/BAO/Contribution.php:356)
in /home/mywebsite.com/wp-content/plugins/civicrm/civicrm/CRM/Utils/System/Base.php on line 948"

These messages just show up on an empty white webpage after pressing submit.

The error appears to involve processing a contribution with fee_amount='' (empty-string). We cannot reproduce this circumstance organically. The fix is based on the assumption that it's happening somehow.

Before

An empty string is invalid for fee amount, leading to a fatal error.

After

The empty string is cast to 0.

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

[kcristiano]

@eileenmcnaughton I tried an r-run on 5.33 without the patch and could not reproduce.

After applying the patch, I was still able to successfully record a backend credit card contribution with fee amount blank.

This PR explicitly allows blank amounts, so it should be OK.

[mattwire]

This isn't actually breaking anything though? And it's not clear if it's actually a regression? On live sites PHP warnings should be off (or at least configured to log and not to display).

As this is happening on submitting a new contribution I'm guessing it's related to a specific payment processor and a specific set of configuration params for the contribution page.

I have no problems with the patch per-se if it helps correct data issues but in my experience this kind of warning is usually pretty useful in pointing to an underlying issue with the configuration/payment processor and is probably hiding other problems.

[eileenmcnaughton]

Yeah my feeling is that because we don't normally distinguish between fee_amount = '' and fee_amount = NULL in various places in code this is the sort of thing we could expect to cause bugs. We know that both are the equivalent of 0 here so casting to 0 seems like a sensible step

[seamuslee001]

So during the Product Maintenance meeting we agreed that this should be merged but also @mattwire we thought that if you felt that if passing a blank string should trigger a deprecation warning or something like that (i.e a Civi Warning not a PHP Warning so won't hopefully trigger the headers issue) we would be ok with merging that in as well.

[mattwire]

👍