Sagepay support #42

mattwire · 2018-01-16T04:12:59Z

This is now in use on a production site and working. However, there are a number of changes to core extension files as well as the vendor library so probably can't be merged as is!

@eileenmcnaughton If you get a chance to review any of my changes that would be appreciated!

eileenmcnaughton · 2018-01-16T06:34:26Z

thanks @mattwire sorry I didn't get around to replying to your email!

eileenmcnaughton · 2018-02-05T07:05:48Z

vendor/omnipay/sagepay/src/Message/AbstractRequest.php

@@ -352,6 +352,12 @@ public function sendData($data)
            case 'DeliveryCountry':
              $data[$key] = $data['BillingCountry'];
              break;
+            case 'NotificationURL':


@mattwire I just added this to the extension - 763d01f - which makes this change redundant.

(also fyi @nganivet

@eileenmcnaughton Ok, I agree it looks like case 'NotificationURL' is redundant but I need to pass in qfKey as an URL parameter to generate the redirect URLs correctly. Currently I'm doing that directly in CRM_Core_PaymentExtended::getNotifyURL.

eileenmcnaughton · 2018-02-05T21:21:21Z

CRM/Core/Payment/OmnipayMultiProcessor.php

@@ -639,7 +639,7 @@ public function handlePaymentNotification() {
    if (empty(CRM_Core_Session::singleton()->getLoggedInContactID())) {
      // Don't be too efficient on processing the ipn return.
      // So far the best way of telling the difference is the session.
-      sleep(45);
+      // sleep(45);


So I have 2 thoughts here - fundamental issue is ipn & browser returning at the same time, starting processing the payment at the same time & sending 2 emails.

We could put ipn_delay_time in the metadata . Some processors are more concurrent than others making it more or less of an issue.

we could use a mysql lock - that mostly works prior to mysql 5.7 & can be totally solid in 5.7 with perhaps a small patch - CRM_Core_Lock I believe

I don't fully understand the process... But from the sagepay side they expect a response within 1 second otherwise their IPN request fails. (I spent hours trying to work out why it was timing out on their side, until I found that in their docs somewhere).

Sagepay is a 4-step process as we:

Redirect to sagepay servers to make payment.

Sagepay notifies CiviCRM of payment status and requests a redirect URL (via IPN).

Sagepay receives IPN response (redirectURL) from CiviCRM within 1 second of request (otherwise it retries and times out).

Sagepay redirects to redirectURL.

I think most redirect processors are only 3-steps (ie don't have separate steps 2-3) so maybe this is somewhat unique to sagepay? I don't generally like the idea of arbitrary delays in code (ie. sleep) so if it makes sense to use a mysql lock that might be better? Though I don't really understand the code and reason for the delay :-)

@mattwire I don't suppose you are using mysql 5.7.5+? I feel like using mysql locking is an appropriate way to prevent the 2 processes both processing the ipn at the same time - however, if only works properly with mysql 5.7.5+

Some discussion & links here civicrm/civicrm-core#11720 (but I have added a version of this to extension)

Thanks. I've rebased sagepay branch against master, looking much cleaner now. And I've raised PR #47 to set ipn delay for sagepay

eileenmcnaughton · 2018-02-11T23:27:45Z

CRM/Core/Payment/OmnipayMultiProcessor.php

@@ -657,7 +657,12 @@ public function processPaymentNotification($params) {
    $originalRequest = $_REQUEST;
    $_REQUEST = $params;
    try {
-      $response = $this->gateway->completePurchase($params)->send();
+      if ($this->gateway->supportsAcceptNotification()) {


Hi @mattwire I've just added this commit to the main repo - I'm afraid I'm cherry picking my way through your commits as time permits rather than assessing as a whole, as there are a lot of issues in there to consider

eileenmcnaughton · 2018-02-11T23:38:37Z

Hi @mattwire I've cherry-picked out the things that I'm comfortable with - I'll need to work with you on the remaining items before I feel sure - they probably need splitting out into issues

eileenmcnaughton · 2018-02-25T22:34:54Z

@mattwire do you want to rebase this - quite a bit is merged

eileenmcnaughton · 2018-03-01T23:34:12Z

Ok so for the Sagepay fixes the way to do this is to submit a change to the composer.json to point towards your git repository to retrieve the Sagepay ones - if you do that I'll run composer afterwards & push. I have some reservations - mostly with the buildSignature change but I'm prepared to leave that with you to try to negotiate upstream

mattwire · 2018-03-03T19:10:10Z

@eileenmcnaughton I've added a forked version of the omnipay-sagepay library via this commit: 688f917
Could you merge this and do the composer thing :-)

mattwire · 2018-03-03T19:11:51Z

CRM/Core/Payment/PaymentExtended.php

@@ -201,7 +201,7 @@ protected function getGoBackUrl($qfKey) {
  protected function getNotifyUrl($allowLocalHost = FALSE) {
    $url = CRM_Utils_System::url(
      'civicrm/payment/ipn/' . $this->formatted_transaction_id . '/' . $this->_paymentProcessor['id'],
-      NULL,
+      array('qfKey' => CRM_Utils_Request::retrieve('qfKey', 'String')),


@eileenmcnaughton So the session key is required for sagepay because the IPN handler returns the redirect URL to sagepay when requested, rather than using the URL via the initial transfer offsite. Any thoughts on how this could be handled?

@mattwire I'm trying to remove those functions that have been migrated to core from this extension - that goBackUrl is one of them

@eileenmcnaughton We're talking about getNotifyUrl and not getGoBackUrl. However, I see the same applies. Basically the session key (qfKey) is essential for sagepay because (uniquely) Sagepay does the final user redirect back from their site ONLY after the IPN has completed. It needs the session key to be returned so that it can redirect the user to the CiviCRM payment confirmation page. Any idea how we could gracefully override this in core?

@mattwire normally the QFKey is stored in the user session before leaving the site & retrieved from there on return?

Right, so discussion on mattermost - we should see if we can modify "next url" so that it is set to civicrm/payment/ipn/5 or equivalent and then we would not need the session key.

Yep - although looking at the above - there is an Omnipay-specific thing already in that function

'civicrm/payment/ipn/' . $this->formatted_transaction_id . '/'

Hmm - we shouldn't be sending next url to Sagepay but returnUrl - that will figure out nextUrl

@eileenmcnaughton I don't think there is any way around putting the qfKey in getNotifyUrl:

Done a bit of debugging as I couldn't remember exactly what was going on.

No session key supplied via getNotifyUrl (standard):

CiviCRM payment submitted.

Goes offsite to Sagepay.

Sagepay returns via ipn (eg. civicrm/payment/ipn/146/5).

CiviCRM server responds to IPN via processPaymentNotification and then redirectOrExit. Both of these functions are OUTSIDE of the session so getStoredUrl in redirectOrExit fails.

Sagepay receives redirect URL via $response->confirm($redirectUrl, $userMsg) and redirects the users browser to that URL, but the URL is invalid because we didn't know the session key.

Session key supplied via getNotifyUrl (modified):

CiviCRM payment submitted.

Goes offsite to Sagepay.

Sagepay returns via ipn (eg. civicrm/payment/ipn/146/5?qfKey=abc1234).

CiviCRM server responds to IPN via processPaymentNotification and then redirectOrExit. Both of these functions are OUTSIDE of the session so getStoredUrl in redirectOrExit fails. But we then try again, retrieving the session key from the request URL and now we have access to the session so it works.

Sagepay receives redirect URL via $response->confirm($redirectUrl, $userMsg) and redirects the users browser to that URL which is the correct URL because it came from the stored session.

@mattwire so

$output = $response->confirm($redirectUrl, $userMsg);

only got added for SagePay and it seems like it would work if getReturnUrl were returned instead.

mattwire · 2018-03-03T19:13:29Z

CRM/Core/Payment/OmnipayMultiProcessor.php

+        if (empty($redirectUrl)) {
+          $redirectUrl = $this->getReturnSuccessUrl(CRM_Utils_Request::retrieve('qfKey', 'String'));
+        }
+        if (method_exists($response, 'confirm')) {


@eileenmcnaughton For sagepay we don't seem to have a stored URL (because we are in the IPN handler?) so we need to generate one here with the session key that is passed back to us via the IPN request URL.

mattwire · 2018-03-03T19:15:28Z

CRM/Core/Payment/OmnipayMultiProcessor.php

@@ -839,7 +839,7 @@ protected function redirectOrExit($outcome, $response) {
        if (empty($redirectUrl)) {
          $redirectUrl = $this->getReturnSuccessUrl(CRM_Utils_Request::retrieve('qfKey', 'String'));
        }
-        if (method_exists($response, 'confirm')) {
+        if ($redirectUrl && method_exists($response, 'confirm')) {


@eileenmcnaughton The !$redirectUrl was added by you recently, but I'm not sure I understand why. In sagepay case at least it needs to have a redirectUrl in order to call those functions as that redirect URL is passed back to sagepay via the IPN handler. Uniquely(?) for sagepay the confirm/error/invalid methods do not return as it is up to the sagepay website to initiate the redirect.

mattwire · 2018-03-03T19:16:09Z

CRM/Core/Payment/OmnipayMultiProcessor.php

+      if (empty($value)) {
+        unset($cardFields[$name]);
+      }
+    }


@eileenmcnaughton Is this something that's acceptable for all payment processors?

eileenmcnaughton · 2018-03-04T22:10:41Z

@mattwire did composer run cleanly for you? I got

Loading composer repositories with package infoReading composer.json of omnipay/tests (master) Skipped branch master, Could not parse version constraint 4^: Invalid version string "4^"

Updating dependencies (including require-dev)
Your requirements could not be resolved to an installable set of packages.

Problem 1
- omnipay/omnipay 2.3.x-dev requires omnipay/sagepay ~2.0 -> satisfiable by omnipay/sagepay[2.0.x-dev].
- omnipay/omnipay 2.3.x-dev requires omnipay/sagepay ~2.0 -> satisfiable by omnipay/sagepay[2.0.x-dev].
- omnipay/omnipay 2.3.x-dev requires omnipay/sagepay ~2.0 -> satisfiable by omnipay/sagepay[2.0.x-dev].
- Can only install one of: omnipay/sagepay[2.0.x-dev, dev-2.5.1-mjw].
- Can only install one of: omnipay/sagepay[2.0.x-dev, dev-2.5.1-mjw].
- Installation request for omnipay/sagepay dev-2.5.1-mjw -> satisfiable by omnipay/sagepay[dev-2.5.1-mjw].
- Installation request for omnipay/omnipay (locked at 2.3.x-dev, required as ~2.0) -> satisfiable by omnipay/omnipay[2.3.x-dev].

jamienovick · 2018-12-19T17:48:52Z

@mattwire - ooh wish I'd seen this before - this may solve a few sagepay related issues we are having. How are you getting on with this - anything we could do to help get it over the line? Do you know if it covers all CiviCRM use cases including backend transactions / recurring etc?

mattwire · 2018-12-21T18:44:16Z

@jamienovick It's in use on quite a few sites. It supports everything the omnipay extension supports, as it's a "redirect offsite" type processor you can't do backend stuff. If you'd like to help see this in the main extension (most of it already is) you could get some resource to look at the outstanding changes and the history of comments behind it (most is to do with the redirect back to site after payment).

mattwire · 2020-01-27T13:04:23Z

Closing in favour of #147

eileenmcnaughton reviewed Feb 5, 2018

View reviewed changes

mattwire force-pushed the sagepay branch 2 times, most recently from 4b646da to a7ae3fd Compare February 7, 2018 15:23

eileenmcnaughton reviewed Feb 11, 2018

View reviewed changes

mattwire force-pushed the sagepay branch 2 times, most recently from aff9ef1 to f8fe1e1 Compare March 1, 2018 17:14

mattwire force-pushed the sagepay branch 12 times, most recently from 644af07 to 630e9c0 Compare March 3, 2018 19:07

mattwire commented Mar 3, 2018

View reviewed changes

mattwire force-pushed the sagepay branch from fdf84bd to 60966ad Compare March 3, 2018 20:36

eileenmcnaughton force-pushed the master branch 3 times, most recently from fec31b0 to fe221bc Compare May 28, 2018 06:04

mattwire force-pushed the sagepay branch from 79e8f4b to 46b3fd0 Compare May 28, 2018 21:08

mattwire force-pushed the sagepay branch from 46b3fd0 to e2bc341 Compare June 24, 2018 21:37

mattwire force-pushed the sagepay branch from e2bc341 to 77622bb Compare August 7, 2018 09:46

mattwire force-pushed the sagepay branch 3 times, most recently from 3c66ef3 to cdd9733 Compare September 24, 2018 10:52

mattwire force-pushed the sagepay branch 2 times, most recently from 8072646 to 8bb8101 Compare November 2, 2018 22:48

mattwire and others added 6 commits November 2, 2018 22:53

Add qfKey to notifyUrl

42dd51b

Try harder to get redirect URLs

ac56cbe

call response methods if we have a redirectUrl

c45706f

Fixed redirect issue on Event registration

b27c827

Attempt to handle offsite return URL for webform correctly

08e2604

Set version for sagepay

236d058

mattwire force-pushed the sagepay branch from 8bb8101 to 236d058 Compare November 2, 2018 22:54

mattwire changed the title ~~WIP Sagepay support~~ Sagepay support Nov 2, 2018

Fix version

8c5744b

Add sagepay js to handle optional postcode for Ireland

78ad217

mattwire force-pushed the sagepay branch from 0b03960 to 130f531 Compare March 1, 2019 13:00

Update version to 2.6

44d41e3

mattwire force-pushed the sagepay branch from 130f531 to 44d41e3 Compare June 23, 2019 10:03

mattwire closed this Jan 27, 2020

mattwire deleted the sagepay branch March 10, 2021 14:50

timatidg mentioned this pull request Jun 2, 2022

Database error upgrading to 3.15 #224

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sagepay support #42

Sagepay support #42

mattwire commented Jan 16, 2018

eileenmcnaughton commented Jan 16, 2018

eileenmcnaughton Feb 5, 2018

mattwire Feb 6, 2018

eileenmcnaughton Feb 5, 2018

mattwire Feb 6, 2018

eileenmcnaughton Feb 26, 2018

mattwire Mar 1, 2018

eileenmcnaughton Feb 11, 2018

eileenmcnaughton commented Feb 11, 2018

eileenmcnaughton commented Feb 25, 2018

eileenmcnaughton commented Mar 1, 2018

mattwire commented Mar 3, 2018

mattwire Mar 3, 2018

eileenmcnaughton May 28, 2018

mattwire May 28, 2018

eileenmcnaughton May 28, 2018

mattwire May 28, 2018

eileenmcnaughton May 28, 2018

mattwire May 30, 2018

eileenmcnaughton Jun 3, 2018

mattwire Mar 3, 2018

mattwire Mar 3, 2018

mattwire Mar 3, 2018

eileenmcnaughton commented Mar 4, 2018

jamienovick commented Dec 19, 2018

mattwire commented Dec 21, 2018

mattwire commented Jan 27, 2020

Sagepay support #42

Sagepay support #42

Conversation

mattwire commented Jan 16, 2018

eileenmcnaughton commented Jan 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eileenmcnaughton commented Feb 11, 2018

eileenmcnaughton commented Feb 25, 2018

eileenmcnaughton commented Mar 1, 2018

mattwire commented Mar 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eileenmcnaughton commented Mar 4, 2018

jamienovick commented Dec 19, 2018

mattwire commented Dec 21, 2018

mattwire commented Jan 27, 2020