Fix propertyBag regression on amount

[eileenmcnaughton]

Overview

Fixes a regression by payment processors from this line

https://github.com/civicrm/civicrm-core/pull/21583/files#diff-c8d9bcea3e41ecc2ae13f5b654fefbbe3e95a73c9066b83017af82204533f9d2R29

The expectation is that functions calling doPayment pass the parameters in documented here

https://docs.civicrm.org/dev/en/latest/extensions/payment-processors/create/#available-parameters

In some cases the parameter passing has been historically inconsistent and the PropertyBag has been a technical solution to help with that inconsistency - e.g a form might not have passed billing address fields and the syntax when they do is a bit nasty - e.g billing_street_address-{$billingLocationID} so a payment processor may choose to implement the property bag to get easier access to the properties passed in.

However, the documented parameters are the ones that should be prioritised for use (where the calling code sloppily passes in more than one similar property) - and in the case of amount the amount field has been used by many processors for many years and we now know of at least one scenario where total_amount is actually incorrect to use.

I don't think we should have a 'sloppy alternative'
to being called with the correct documented
amount property

Before

When both the contracted property amount and the non-required property total_amount are passed in the cast function treats total_amount as the amount

After

As per before #21583 - amount is used, total_amount is ignored

Technical details

@mattwire - this reverses just the line of the PR you merged that appears to be causing problems - the more I think about it the more I think the line should go - we definitely want functions that call doPayment to do so with the documented parameters. The point of the propertyBag (as I understand it) was to help out where they didn't - but not to make it so the calling code could be more loose about what it passes

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

[artfulrobot]

I agree this makes good sense, since total_amount is not documented it should not be used. (supplying both and casting will, as others have found, mean the last one in the supplied array wins, and since total_amount may be locale-formatted, cause the wrong amount to be stored.)

[eileenmcnaughton]

thanks for taking a look @artfulrobot

[mattwire]

Yes, makes sense. I think this was added because something somewhere was passing total_amount. But if we find out what that was we should fix it there instead.

[magnolia61]

Thank you for working together on this and figuring out the issue. Appreciate that!

[eileenmcnaughton]

@magnolia61 thanks for pinpointing the issue

[magnolia61]

Is it too late for this to make its way to the RC as it is a regression?

[eileenmcnaughton]

@magnolia61 it IS in the rc - which is due for release next Wed - I was on the fence about asking @totten to drop a release of 5.48 prior to then

[magnolia61]

Oh, nice. Thats fine. High impact but kind of niche still.

[eileenmcnaughton]

@magnolia61 yeah - with a week to go to 5.49 it is a bit borderline - if #23295 gets turned around quickly-ish then I think I will push for it to go out before next Wed