We should give user feedback in case of a Payment Processor Exception

[artfulrobot]

Payment processors can throw exceptions when trying to do things like update or cancel a recurring payment. But the system expects them to return a CRM_Core_Error.

Before

no errors, modal edit forms just hang if error.

After

user should get error

Technical Details

I added a catch block for this particular exception and wrapped it up in an CRM_Core_Error.

I've not been able to test this though, hence the WIP.

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

[mattwire]

@artfulrobot +1 for concept - however, can't we do away with the if CRM_Core_Error check entirely and just report the error in the catch? Rather than wrapping in a CRM_Core_Error? Also, looking at the existing code if it gets a CRM_Core_Error won't it report the error and then continue and report success? That doesn't seem quite right! I'd probably consider a CRM_Core_Error::statusBounce() here.

[eileenmcnaughton]

@artfulrobot I like where you've put the try-catch block. I think we are trying to get away from the CRM_Core_Error & can just call

CRM_Core_Session::singleton()->setStatus($e->getMessage());

in the next line and the remove the CRM_Core_Error handling that follows

[eileenmcnaughton]

oh snap - @mattwire said more or less the same thing. In general statusBounce is setMessage + return to the previous page

[artfulrobot]

@mattwire @eileenmcnaughton yes I agree - I had to look up PEAR_ErrorStack online to find out how to even use CRM_Core_Error!

Much cleaner to do away with it, but I don't know which other code uses it so didn't want to break anything there.

[artfulrobot]

How does that ↑ look?

[mattwire]

@artfulrobot I think we should get rid of the if (is_a($cancelSubscription, 'CRM_Core_Error')) { blocks entirely and trigger the same statusBounce that you are adding here. It's at a form level that is only called in a couple of places so easy to test and both those forms are notorious for the spinning wheel of death.
By removing those is_a we end up with a much cleaner function as we don't need some of the subsequent if statements.

[mattwire]

@artfulrobot Having asked you to remove if (is_a($cancelSubscription, 'CRM_Core_Error')) { lines I think they need to go back in following your comments! Sorry!

[eileenmcnaughton]

I have thoughts about this but have been wanting to get the @artfulrobot getters & setters resolved before getting into this one

[artfulrobot]

ok that's back in for now.

[mattwire]

@artfulrobot - needs rebase, then I'm happy for it to be merged. @eileenmcnaughton comments make sense but do not hold up this PR.

[artfulrobot]

@mattwire I've rebased.

[eileenmcnaughton]

I've been dithering on this - an external function could be calling cancelSubscription & handling an error. However, looking into this there are already scenarios where exceptions are thrown - notable Paypal - so it makes sense to be consistent. In addition I would note that this is much less heavily used that the doPayment function & possibly not called at all in extensions

However, I'd at least like to get thoughts on this alternate approach which starts to deprecate cancelSubscription & handles the in between @mattwire @artfulrobot
https://github.com/civicrm/civicrm-core/compare/master...eileenmcnaughton:cancel?expand=1

[artfulrobot]

@eileenmcnaughton So have I got this right? The point of this PR is:

• Make core code call doCancelRecurring() with cleaner signature, result and throws spec.
• Add an implementation on CRM_Core_Payment that wraps existing procesors' cancelSubscription() and converts old CRM_Core_Error objects to exceptions.
• Encourage processors to move over to implementing doCancelRecurring instead.

It's a bit tricky because processors might need to do this in reverse - i.e. they write a nice new doCancelRecurring method, but then they have to provide cancelRecurring which wraps the other method for the sake of people on older CiviCRM versions. Chicken and egg and puts quite a lot of expectation on myriad processors to dance with us through the deprecation journeys.

It's a nice thing to do though - I'm in favour of cleaner signatures, clearer results.

With that in mind:

• we're currently only passing in the processor_id field value, wrapped in an array.
  • if this is to be the only and correct way to do things then: let's stop wrapping it in an array and just use string $processorID; documenting in docblock that here processor id refers to civicrm_contribution_recur.processor_id and not payment processor ID.
  • We could instead pass in a int $contributionRecurID and let processors fetch whatever they need themselves. This is less prescriptive on how processors store their data which might be more flexible.

Basically I'm not sure whether all processors can and all processors have and all processors do store a unique value in that field. I recently updated GoCardless to use that field instead of trxn_id, just to work with this requirement.

Also, I think we need to document the result.

I like functions which are clear input → processing (possible Exception) → clear output rather than passing in references, e.g. $message with the old cancelSubscription - although I note thta is lost in the code you shared.

We don't need to return a bool, because we should throw an exception if it fails. I 'm unclear which processors return FALSE (and not an error) but I'd suggest looking for that and throwing an exception.

e.g.

  /**
   * Do cancellation.
   *
   * This should be overriden in the processor and we will move towards
   * deprecating cancelSubscription.
   *
   * A PaymentProcessorException should be thrown if the method does not
   * succeed in cancelling the recurring payment.
   *
   * @param string $processorID  Value to match civicrm_contribution_recur.processor_id
   * @return string              Optional message from processor.
   *
   * @throws \Civi\Payment\Exception\PaymentProcessorException
   */
  public function doCancelRecurring(string $processorID) {

    $message = '';
    $result = $this->cancelSubscription($message, ['subscriptionId' => $processorID]);
    if (is_a($result, 'CRM_Core_Error')) {
      throw new PaymentProcessorException(CRM_Core_Error::getMessages($result));
    }
    if (!$result) {
      throw new PaymentProcessorException(
        "Problem requesting cancellation from payment processor"
        . ($message ? ": $message" : '')
      );
    }
    return $message;
  }

[mattwire]

we're currently only passing in the processor_id field value, wrapped in an array.
if this is to be the only and correct way to do things then: let's stop wrapping it in an array and just use string $processorID; documenting in docblock that here processor id refers to civicrm_contribution_recur.processor_id and not payment processor ID.
We could instead pass in a int $contributionRecurID and let processors fetch whatever they need themselves. This is less prescriptive on how processors store their data which might be more flexible.

@artfulrobot I'm completely in favour of adding the recur ID into the params array - it's really stupid that it doesn't get passed through - I just wanted it in it's own PR, and it should include a unit test to make sure we don't lose it again.

[eileenmcnaughton]

@artfulrobot right - there is always migration involved if you add new methods & that isn't instant. On the other hand I'm just a little worried about changing the behaviour of an existing function AND that function is problematic because of the function_exists nastiness around it so it would be nice to migrate off it.

I agree with having something sensible passed in. Unless we are serious about renaming the processor_id field (hint I think that could be painful) I think we might as well pass in

['processor_id' => x, 'contribution_recur_id' => y];

& the default doCancelRecurring can ensure those are set

[artfulrobot]

I don't think the function_exists is bad - processors can (and should) easily override supportsCancelSubscription() and the default implementation is a shortcut for legacy code.

OK, why don't we use a propertyBag for this?

$bag = new PB();
$bag->setContributionRecurID($recur_id);
$bag->setRecurProcessorID($thatWeirdDetailsArray->subscriptionId);
try {
  $message = $processor->doCancelPayment($bag);
}
catch (PayProcException $e) {...}

...
function doCancel(PropertyBag $bag) {
  $bag->require(['contributionRecurID', 'recurProcessorID']);
  ...
}

That way we give yet more examples of use, and we get a mild bit of validation added for good measure. Strictly speaking, having the two is redundancy though. I'd prefer to stick with just one (recur ID seems more sensible to me as we know it is guaranteed to eb unique).

[eileenmcnaughton]

@artfulrobot the problem is that the isSupportsCancelRecurring is tied to the existence of a function that isn't needed. ie. you don't need to override doCancelRecurring for a good chunk of processors because they support it simply by recognising that the the contribution_recur record has been updated - so ideally they would only need to implement

isSupportsCancelRecurring & return TRUE & nothing else.

Yes, I think a property bag makes sense here.

Just recur ID makes sense but if it was a batch process then not discarding the value we have in order to refetch it would have a performance impact.

[artfulrobot]

@eileenmcnaughton I don't understand the cancellation (or the change process) support feature.

As I understand there are two features

1.UI can offer a Cancel link when viewing a contact's recurring contribs.

2. templates can offer a Cancel URL that would allow the supporter to choose to cancel their payments.

These both require a payment processor service to offer an API that allows recurring subscriptions to be cancelled, and for the payment class to implements that.

I'm unclear what logic determines which of these are offered.

I understood that the UI checks the payment class's supportsCancelRecurring() output to determine whether this is supported.

I understand that the default implementation of that (on CRM_Core_Payment) just checks for the cancelSubscription method's existence and then assumes it's OK if it exists.

Payment classes can override supportsCancelRecurring() however they want. We could change core to also check for the existence of doCancelRecurring? Or we could just gut that function and return FALSE - forcing processors that support it to override it.

Sorry if I'm being thick.

As a side note: I think it's also possible that whether a particular payment can be cancelled/changed depends on other factors - currently impossible since no ContribRecurs are passed into these supports functions.

[eileenmcnaughton]

@artfulrobot so from the calling code's point of view both Eway & Paypal support cancelRecurring - that's all the calling code needs to know.

However, Eway code doesn't need to do anything to support recurring because it just has a cron that charges all valid recurring profiles so it should be able to declare that if you are using Eway you can cancel recurrings but it doesn't need a 'cancelRecurring' function as it doesn't take any actual action. But we can't add a default empty cancelRecurring action to CRM_Core_Payment to handle that because then 'supportsCancelRecurring' would always exist.

We CAN have a default empty doCancelRecurring though - which would get us to the point where they just need to implement the one thing - return TRUE when supportsCancelRecurring is called

[artfulrobot]

@eileenmcnaughton thanks for the explanation.

So at this point, eway has to do this to enable cancellations through the UI

public function cancelSubscription($whatever) {
  // We don't need to do any processing here because a cron job
  // is what takes the money, so we'll simply stop taking the money
  return TRUE;
}

because then the default supportsCancelRecurring works, but the doing function does nothing.

The problem is that we can't include a stub signature of cancelSubscription in CRM_Core_Payment though, so it's not very discoverable to programmers working on new payment classes.

Soon we will require payment classes that support cancellations to override doCancelRecurring (with a NOOP function if no action is needed) and supportsCancelRecurring because the default implementation will return FALSE instead of checking for the existence of cancelSubscription. However they will still need to have cancelSubscription function too, to support older CiviCRMs.

So for a processor like eway, it will have to:

/** Required for CiviCRM v5.[?]+ when this function stopped returning the existence of cancelRecurring */
public function supportsCancelRecurring() { return TRUE; }
/** Required for CiviCRM v5.[?]+ when this function stopped wrapping cancelRecurring */
public function doCancelRecurring() { }
/** Required for CiviCRM <v5.[?] */
public function cancelReccurring() { return TRUE; }

And for a processor like GoCardless

/** Required for CiviCRM v5.[?]+ when this function stopped returning the existence of cancelRecurring */
public function supportsCancelRecurring() { return TRUE; }
/** Required for CiviCRM v5.[?]+ when this function stopped wrapping cancelRecurring */
public function doCancelRecurring($recur_id) {
  // All the work with the external API
}
/** Required for CiviCRM <v5.[?] */
public function cancelReccurring(&$m, $p) { 
  $recur_id = api3('ContribRecur', 'getvalue', ['return'=>'id', 'processor_id'=>$p['subscriptionId']]);
  $m = $this->doCancelRecurring($recur_id);
  return TRUE;
}

cancelSubscription and doCancelRecurring will both need to do the same things but take different inputs. So likely one would become a wrapper for the other.

In the far future, once nobody's running pre 5.2[?] then we can drop the cancelSubscription from processors.

So...

• Now: define one function, but it's a bit unintuitive.
• With this change: maintain three functions (do..., cancel... and supports...), bit confusing because processor code has to support old and new.
• In the future: define one function, more sensible, easier to document, structure

[eileenmcnaughton]

No Eway would need to have

public function supportsCancelRecurring() { return TRUE; }

It shouldn't need to implement doCancelRecurring - we should alter the proposal above so cancelRecurring is only called if exists (& add a deprecation if it is called via doCancelRecurring to encourage people to stop calling doCancelRecurring).

In core we would switch to calling doCancelRecurring in 5.22 so Eway would only need to implement that from it's 5.22 release onwards - I rely on the extension system to have people run compatible versions in general, but perhaps I'd add some transitional support in a deprecated function for a couple of months

[eileenmcnaughton]

Opps - comment above on wrong PR - now deleted - leaving this in case of any confusion

[eileenmcnaughton]

Regardless of the discussion above I have just looked again into the Paypal code & am feeling better about switching the expectation to be that cancelRecurring throws exceptions.

Currently the paypal code will already throw an exception under some circumstances in cancelRecurring - so switching to always throwing an exception is not changing the expectation but standardising it.

I'd like to keep the conversation going but I think I can merge this as 'better in than out' based on the above .