Issues #16 / #32 / #34 / #36 / #37 / #40 / #63 / #64

judgej commented 8 years ago

Still not complete (needs more tests and docblocks, but functionally complete) and getting up for visibility and automated testing.

This is a rewrite of the notification handler using the newer NotificationInterface. All the listed issues revolve around the notification handling, which has always been a bit "special".

This PR provides a new way to handle the notifications, but does not introduce any BC breaks for existing applications. However, updating the applications should simplify their notification handlers somewhat.

judgej commented 8 years ago

Summary of changes in relation so specific issues are:

16 An invalid signature will no longer raise an exception unless you try to respond to Sage Pay with a error or confirm; you must response with an invalid. A checkSignature() method gives you a true/false indication of whether the signature is valid.
32 This complains of having to call confirm() on a completePurchase() response, which other drivers don't do. This is now a notify response, which is completely different. The responses for the notify handler are not standardised in the project yet, but they can be updated here as they are.
34 No exceptions raised now (see #16). No knowledge needed of the Sage Pay specific POST parameter names - just use getTransactionId() on the notify request to get the merchant transaction ID. This makes it more general.
36 getCode() is now an alias of getStatus().
37 The output is still echoed, which you can catch with ob_* functions. You can turn the auto-exit off though, with $response->setExitOnResponse(false);. It is on by default, as that will likely be the 99% use-case.
40 notifyUrl() should be used now. returnUrl() is still supported as a fall-back to avoid BC breaks.
63 This one spurned on this development, which will form the core to a fix to a third-party application plugin as a result.
64 This is the core of the changes - introducing the AcceptNotification message. We just call it notify here, but can change that to acceptNotification if that is what other gateways use. The documentation is sparse at the moment though.

judgej commented 8 years ago

Check out the documentation in the updated README.md for how this is used:

https://github.com/academe/omnipay-sagepay/blob/6c2a46a069f89c7897751c5092fb3a8df867cc40/README.md

delatbabel commented 8 years ago

Looks good to me. I'm not aware of any other gateways that use AcceptNotification or accept or whatever but I'm considering implementing it in two that do send notifications. Let me know how this goes and I'll likely use it as a model.

judgej commented 8 years ago

I wonder if accept and reject would be better than confirm and error when responding to the notification, keeping invalid in there (or maybe make it error) for failed signatures? They are closer to the language that the gateways use (which may not be a good thing if it causes confusion). For the new Sage Pay notify handler, I'd be happy to change confirm/error/invalid responses to anything else that could apply more generally to all the gateway notification handlers - maybe put them in the interface too?

I've also replaced checkSignature() with isValid() as a more general check that the incoming notification message meets all the required formatting, structural and security requirements to be trusted and usable. I am trying to use that generically now.

judgej commented 8 years ago

I'm also implementing the notify message in the new PAYONE driver, so am trying to get these both consistent where possible:

https://github.com/academe/OmniPay-Payone

lukeholder commented 8 years ago

Hi @judgej, I am the developer of Craft Commerce. I was made aware you are doing some work here to assist us get support for Sagepay Server. Happy to have a skype call, or take your advice as to how we can make our software compatible with the above changes.

skype: lukemholder

lukeholder commented 8 years ago

thumbs up with::acceptNotification() instead of ::notify(). (It is acceptNotification in the omnipay README: https://github.com/thephpleague/omnipay#incoming-notifications)

One example of acceptNotification I found in active use: https://github.com/collizo4sky/omnipay-2checkout/blob/master/src/Gateway.php#L230

judgej commented 8 years ago

Thanks, I'll change that.

judgej commented 8 years ago

@lukeholder It would probably be just as easy to implement the old notification handler for now then change it later when this is merged. 80% of the code will remain the same, with the update mostly involving removing some stuff. Old and new ultimately do the same thing. The old method is just less intuitive to implement, but I can guide you through that.

I still have tests to write for this before it can go live, and there are a few other PRs I desperately need to look at. Could give you a call this afternoon.

judgej commented 8 years ago

Okay, I messed up. That serves me right for trying to juggle half a dozen branches at the same time. So - this can remain or it can be removed and reverted back to a PR - except I am not entirely clear on how to do that, so may need some help.

judgej commented 8 years ago

I really don't know what has happened here, because I can't actually see many of these changes that have apparently been merged in. I'll just create another PR - is says in my source repo that it has not been merged, even though it says here that it has :confused:

judgej commented 8 years ago

Okay - I think it is all sorted now - a new PR #69 to replace this one, and master reverted back to before my mess-up.

thephpleague / omnipay-sagepay

Issues #16 / #32 / #34 / #36 / #37 / #40 / #63 / #64 #65

16 An invalid signature will no longer raise an exception unless you try to respond to Sage Pay with a `error` or `confirm`; you must response with an `invalid`. A `checkSignature()` method gives you a true/false indication of whether the signature is valid.

32 This complains of having to call `confirm()` on a `completePurchase()` response, which other drivers don't do. This is now a `notify` response, which is completely different. The responses for the notify handler are not standardised in the project yet, but they can be updated here as they are.

34 No exceptions raised now (see #16). No knowledge needed of the Sage Pay specific POST parameter names - just use `getTransactionId()` on the notify request to get the merchant transaction ID. This makes it more general.

36 `getCode()` is now an alias of `getStatus()`.

37 The output is still echoed, which you can catch with `ob_*` functions. You can turn the auto-exit off though, with `$response->setExitOnResponse(false);`. It is on by default, as that will likely be the 99% use-case.

40 `notifyUrl()` should be used now. `returnUrl()` is still supported as a fall-back to avoid BC breaks.

63 This one spurned on this development, which will form the core to a fix to a third-party application plugin as a result.

64 This is the core of the changes - introducing the `AcceptNotification` message. We just call it `notify` here, but can change that to `acceptNotification` if that is what other gateways use. The documentation is sparse at the moment though.

thephpleague / omnipay-sagepay

Issues #16 / #32 / #34 / #36 / #37 / #40 / #63 / #64 #65

16 An invalid signature will no longer raise an exception unless you try to respond to Sage Pay with a error or confirm; you must response with an invalid. A checkSignature() method gives you a true/false indication of whether the signature is valid.

32 This complains of having to call confirm() on a completePurchase() response, which other drivers don't do. This is now a notify response, which is completely different. The responses for the notify handler are not standardised in the project yet, but they can be updated here as they are.

34 No exceptions raised now (see #16). No knowledge needed of the Sage Pay specific POST parameter names - just use getTransactionId() on the notify request to get the merchant transaction ID. This makes it more general.

36 getCode() is now an alias of getStatus().

37 The output is still echoed, which you can catch with ob_* functions. You can turn the auto-exit off though, with $response->setExitOnResponse(false);. It is on by default, as that will likely be the 99% use-case.

40 notifyUrl() should be used now. returnUrl() is still supported as a fall-back to avoid BC breaks.

63 This one spurned on this development, which will form the core to a fix to a third-party application plugin as a result.

64 This is the core of the changes - introducing the AcceptNotification message. We just call it notify here, but can change that to acceptNotification if that is what other gateways use. The documentation is sparse at the moment though.

16 An invalid signature will no longer raise an exception unless you try to respond to Sage Pay with a `error` or `confirm`; you must response with an `invalid`. A `checkSignature()` method gives you a true/false indication of whether the signature is valid.

32 This complains of having to call `confirm()` on a `completePurchase()` response, which other drivers don't do. This is now a `notify` response, which is completely different. The responses for the notify handler are not standardised in the project yet, but they can be updated here as they are.

34 No exceptions raised now (see #16). No knowledge needed of the Sage Pay specific POST parameter names - just use `getTransactionId()` on the notify request to get the merchant transaction ID. This makes it more general.

36 `getCode()` is now an alias of `getStatus()`.

37 The output is still echoed, which you can catch with `ob_*` functions. You can turn the auto-exit off though, with `$response->setExitOnResponse(false);`. It is on by default, as that will likely be the 99% use-case.

40 `notifyUrl()` should be used now. `returnUrl()` is still supported as a fall-back to avoid BC breaks.

64 This is the core of the changes - introducing the `AcceptNotification` message. We just call it `notify` here, but can change that to `acceptNotification` if that is what other gateways use. The documentation is sparse at the moment though.