Implemented new MultiSafepay REST API

nielstholenaar commented 8 years ago

See #4 for more information about this pull request.

Pawnerd commented 8 years ago

Thanks! Came just in time

barryvdh commented 8 years ago

Good work. I'm not really sure what best to do and how necessary these changes are, but there is Omnipay v3 coming soon(ish). Otherwise we'd just release this version, which is breaking for existing users, and release a new one shortly after?

barryvdh commented 8 years ago

This could probably have been a good use-case to try the v3 API / http interface, but guess I'm a bit late to say that :/

gwigz commented 8 years ago

Think the current version (of this wrapper) has been out of commission for quite a long time, I had to avoid using Omnipay because of this issue.

barryvdh commented 8 years ago

Do you mean the current 2.2 version is completely broken and doesn't work at all?

nielstholenaar commented 8 years ago

@barryvdh The current implementation of their API doesn't support all features. Such as recurring payments. It also uses their old XML based API, which is deprecated and not supported anymore.

I agree we may should wait, and apply the required changes to be compatible with Omnipay V3. But I don't know how long it is gone take before Omnipay V3 will be released?

kevindierkx commented 8 years ago

@barryvdh Well I guess a working implementation is beter than a broken one. When the work is already done, what would be the harm of releasing 2 version after each other? A properly tagged version shouldn't cause any BC breaks. Thats the whole point of versioning. Just my 2 cents.

delatbabel commented 8 years ago

OK perhaps because this breaks BC we should actually release it as a new gateway (MultiSafePayNew or similar). In my past experience, gateways don't actually destroy old versions of their API, they just deprecate them and don't support them any more for new customers (this is the case for First Data for example). So there may be existing users who depend on the features of the now deprecated gateway rather than this new one.

delatbabel commented 8 years ago

In particular I note that:

The existing gateway endpoint https://api.multisafepay.com/ewx/ still works, but appears to be XML based. In particular I can point my browser at that endpoint and get a response.
The new gateway endpoint is https://api.multisafepay.com/v1/json and they should be able to exist side by side.
The new gateway is JSON based (yay) not XML based (boo) but existing users may very well have done things that rely on the XML structure emitted by the old gateway endpoint.
The initialisation parameters are completely different. It's not possible to initialise the new gateway with the parameters of the old gateway.

Therefore I suggest that the code be restructured as follows:

Change the existing gateway name from Gateway to XmlGateway -- for clarity. Include a note in the docblock that it's deprecated. You should be able to still use it by calling Omnipay::create('Multisafepay_Xml');
Leave in a backwards compatibility shim class called Gateway (extends XmlGateway). Have a look at the omnipay-firstdata classes to see another case where this has been done.
Create this new gateway as RestGateway or JsonGateway with all of the message classes being prefixed by Rest. Again omnipay-firstdata or omnipay-paypal should give you examples of how this is done.
Update docblocks to give examples of initialising the new gateway. e.g. the new gateway will be initialised by Omnipay::create('Multisafepay_Rest');

There are missing docblocks. Please attend to this. Phpleague standards require "docblock all of the things". Although this has not been the case in all pre-phpleague gateway plugins for omnipay it is now a requirement. Files also require docblocks (which means you need 2 docblocks per class file, one for the file and one for the class). Again see omnipay-paypal (REST) or omnipay-firstdata for some examples.

Other than that I see no issues with the code submitted. The docblocks that are present are complete and provide the correct parameter and return definitions. I can check test coverage once the refactoring discussed above is complete.

nielstholenaar commented 8 years ago

@delatbabel Thanks for the response :+1:

I'have a look at the changes you proposed.

barryvdh commented 8 years ago

That would indeed be better. We can deprecate the old gateway and only use the new one in v3

nielstholenaar commented 8 years ago

I finished the implementation of the proposed changes.

barryvdh commented 8 years ago

LGTM

@delatbabel Comments? Otherwise I'll merge.

delatbabel commented 8 years ago

OK let's merge then run some tests.

thephpleague / omnipay-multisafepay

Implemented new MultiSafepay REST API #5