Code cleanup Update Paypal invokeApi to only throw exceptions.

[eileenmcnaughton]

Overview

Standardises error handling within paypal internal function

Before

Currently invokeAPI will return an error for a curl function and throw a processorException for an outcome other than success.

After

invokeAPI always throws an exception if it fails.

Technical Details

Failed transactions result in exceptions being thrown in invokeAPI. However, in the rarer scenario where the error is a curl error CRM_Core_Error objects are returned & (almost always) converted into exceptions & thrown

In just about every instance where it is called the Core_Error is converted into an exception anyway - this
can all be cleaned up as a follow up. The exceptions are

doQuery - called by doPayment
doDirectPayment - this is called by doPayment which converts it into an exception. Calling doDirectPayment directly was deprecated in 4.6
createRecurringPayments - called by doExpressCheckout which in already throws exceptions in other scenarios,
updateSubscriptionBillingInfo - we can update the one place that calls this in core as a follow up like #15676

However, note that an exception was already thrown in all these places in the more common error scenario of the action not succeeding as opposed
to the rare curl error scenario

Follow ons to consider

1. introduce more nuance into PaymentProcessorExceptions - extend the exception class with PaymentProcessorConfigException, PaymentProcessingException or similar to differentiate
2. remove all the handling for invokeApi to have returned an error object.
3. fix the code that calls updateSubscriptionBillingInfo
4. ensure docs reflect that doPayment should throw an exception. AFAIK we have not yet documented other 'do' functions & I'd prefer to stdise them first

Comments

Anything else you would like the reviewer to note

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

test this please

[eileenmcnaughton]

@mattwire @artfulrobot - in reviewing Rich's last change I concluded we should do this - since it makes no sense to return an error for a curl error & throw an exception for a processor error - esp given the former is almost always converted to the latter

[artfulrobot]

Wouldn't we want to include curl_error($ch) output as well as error no?

[eileenmcnaughton]

Yeah I guess it could be

throw new PaymentProcessorException(curl_error($ch), curl_errno($ch));

[eileenmcnaughton]

updated per ^^

[artfulrobot]

I'd be tempted to prefix it with "Network error: " too, as Curl's errors are not very user friendly.

[eileenmcnaughton]

OK - the errno is now duplicated because I have a feeling just putting it as error_code it might get swallowed & more info rather than less is probably helpful if there is a curl error

[artfulrobot]

(CiviCRM Review Template DEL-1.2)

• General standards
  • Explain (r-explain)
    • PASS : The goal/problem/solution have been adequately explained in the PR.
  • User impact (r-user)
    • PASS: The change would be intuitive or unnoticeable for a majority of users who work with * Documentation (r-doc)
    • PASS: The changes do not require documentation.
• Run it (r-run)
  * UNREVIEWED
• Developer standards
  • Technical impact (r-tech)
    • PASS: The change potentially affects compatibility, but the risks have been sufficiently managed.
  • Code quality (r-code)
    • ISSUE: Something was unclear to me. - see comment
  • Maintainability (r-maint)
    • PASS: makes code cleaner
  • Test results (r-test)
    • UNREVIEWED

[artfulrobot]

Code quality (r-code) PASS

Looks good to me, thanks @eileenmcnaughton

[eileenmcnaughton]

Added MOP per @artfulrobot review