Paypal cancel/notify/return URLs

[mattwire]

Overview

This fixes Paypal standard on Drupal9 Webform and will also work for any other offsite payment processor that uses the standard methods.

Paypal: Stop passing component when it exists on the class

Paypal: Use getBaseReturnUrl() function

CRM_Core_Payment: getCancelUrl participant default value of NULL

Paypal (express): Pass participantID to getCancelUrl() if available

Paypal: Use standard functions for cancelUrl / notifyUrl

Before

Paypal using it's own code instead of generic code.

After

Using generic code to generate cancel/notify links.

Technical Details

Historically (a long time ago) these functions didn't exist. Recently (6 years ago) they were updated to allow them to be overridden by eg. webform.

Comments

[civibot]

(Standard links)

• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[eileenmcnaughton]

Does paypal even need the component? I'll take a look at the IPN code to see.... but it would be cleaner just to figure it out from the contribution

[mattwire]

Does paypal even need the component? I'll take a look at the IPN code to see.... but it would be cleaner just to figure it out from the contribution

@eileenmcnaughton I don't know. But don't think that would be a blocker for this PR?

[mattwire]

@KarinG You might be interested?

[mattwire]

By the way I did this as 5 commits so it's easier to review/merge the changes separately if preferred.

[eileenmcnaughton]

Hmm - the way the IPN works it treats the presence or absense of component as meaningful

    if (empty($this->_inputParameters['component'])) {
      $this->_isPaymentExpress = TRUE;
    }

This feels like one of those places where someone looked at what was different but the difference was likely accidental. I suspect the component might be more reliably set on the class than when being passed in so that bad check might break

[mattwire]

@eileenmcnaughton As far as I can tell I'm not changing any passing or not of component. Just cleaning unnecessary function vars - if you look at this commit - component defaults to 'contribute' and is set in the class constructor. Then we pass it around anyway in paypal between functions but it all comes from the same place - the constructor and then the class method.

[eileenmcnaughton]

OK - I agree with you after checking.

I think this PR makes 2 changes then

1. slight change to the signature of getCancelURL - that's fine as long as no classes extend it
2. the actual url for cancel url is slightly changed - to use the same one as other processors. Do you have any idea if the difference has any implications

[mattwire]

1. slight change to the signature of getCancelURL - that's fine as long as no classes extend it

As it's making a required parameter (participantID) optional and almost everywhere passed in NULL anyway I don't think it should matter if a class does extend it as we're still maintaining the 2 params.

2. The actual url for cancel url is slightly changed - to use the same one as other processors. Do you have any idea if the difference has any implications

Well, it means it actually works for webform :-) From grepping code etc. my conclusion was that it was using a very old pattern and most of what it was passing would be ignored - I'm not sure that the cancel URL actually worked before as I didn't test the "cancel" before this PR. But it does work after this PR :-)

[eileenmcnaughton]

@mattwire re "I don't think it should matter if a class does extend it as we're still maintaining the 2 params." - I think it would still bork because the expectation is the signature is the same between parent class & sub-class - but I couldn't find any instances

Re the second - OK - it fixes if for webform & standardises with the other payment processors - I guess if it is fixing something which doesn't work that is a good argument

[mattwire]

Thanks @eileenmcnaughton