Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convert AuthorizeNet to doPayment() #20322

Merged
merged 1 commit into from
May 17, 2021

Conversation

mattwire
Copy link
Contributor

@mattwire mattwire commented May 17, 2021

Overview

Ref https://lab.civicrm.org/dev/financial/-/issues/135 Convert from deprecated doDirectPayment to doPayment

Before

After

Technical Details

Comments

@civibot
Copy link

civibot bot commented May 17, 2021

(Standard links)

@eileenmcnaughton
Copy link
Contributor

I've flagged this one has-test as I think the Authorize.net test cover would catch anything this affected. Change makes sense & we are still returning $params so the issue we hit before (where the returned is 'used' for values other than the expected ones) won't come into play

@eileenmcnaughton eileenmcnaughton merged commit bd8662a into civicrm:master May 17, 2021
@mattwire mattwire deleted the authnetdopayment branch May 17, 2021 20:18
@mattwire
Copy link
Contributor Author

Thanks @eileenmcnaughton I've gone for the really simple, play it safe to get us onto doPayment() and "complete" https://lab.civicrm.org/dev/financial/-/issues/135.
We can look at return params etc. as a follow-on. It also gets rid of an is_array() which causes problems when doPayment() returns a propertyBag (though we should be returning an array not a propertyBag anyway).

@eileenmcnaughton
Copy link
Contributor

@mattwire yeah I agree with playing it safe! I think there is quite a bit of work to do to be sure that returning less is safe even though in theory it would be good to do so - I don't see the removed is_array in here?

@mattwire
Copy link
Contributor Author

In the parent doPayment() which is no longer called: https://github.com/civicrm/civicrm-core/blob/master/CRM/Core/Payment.php#L1389

@eileenmcnaughton
Copy link
Contributor

eileenmcnaughton commented May 17, 2021

@mattwire oh right - but that is what is returned - we expect payment processors to continue to return an array - which is what you said above - so that is at best neutral

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants