thephpleague / omnipay-mollie

Mollie driver for the Omnipay PHP payment processing library
MIT License
62 stars 38 forks source link

Mollie Api v2 #48

Closed barryvdh closed 6 years ago

barryvdh commented 6 years ago

Mollie now has a v2 API: https://docs.mollie.com/migrating-v1-to-v2

I haven't check extensively, but it looks like it might have some breaking changes. The current v4 of this library could remain on the V1 API, while 5.x could use the v2 API.

nickurt commented 6 years ago

The biggest breaking change looks like the 'amount' is now passed as a map ...

barryvdh commented 6 years ago

Yeah and different return codes

judgej commented 6 years ago

This may be a good example of where an external library can be pulled in to help. The official Mollie package models all the resources that the REST API supports, and they can be used to construct the request message, and possibly to parse the responses. I've done this for the new Authorize.Net driver - it gets all the XML, JSON, some validation etc, out of the driver, results in more re-usable code, less duplication and a less complex Omnipay driver. On the other hand, there are now two packages to maintain, but I think it is worth it.

willemstuursma commented 6 years ago

Hi, Willem from Mollie here.

I agree with @judgej that the fastest way to move is to use our PHP API client as a dependency. Let me know, how we can help!

barryvdh commented 6 years ago

I'm open for a v5 implementation for the v2 api, using the Mollie package. Perhaps Willem would like to make a PR?

nickurt commented 6 years ago

Ah yeah, great, we can also directly implement this proposal/suggestion I added yesterday.

https://github.com/thephpleague/omnipay-mollie/issues/47

judgej commented 6 years ago

It does have Guzzle as a dependency, which could add complications for some use-cases. Omnipay Common 2.x uses an earlier version of Guzzle, and Omnipay Common 3.x does not have any preference for a HTTP client, and there are many alternatives that could be used.

This is - I think - the reason why Omnipay purposefully stayed clear of any third-party dependencies in the past. Providing models for the API is great and saves time, but then they all tended to pull in their own explicit locked-in transport layer, and that got in the way of Omnipay's (and now the merchant site's) preferences.

However, @willemstuursma the gateway could make a general HTTP client a generic requirement, so the end user could choose, and then pull in Guzzle specifically for dev clones. But I may just be overstating a non-problem anyway :-)

willemstuursma commented 6 years ago

@judgej that's fine, can you provide a PR on mollie/mollie-php-api for that? I can have my team provide a PR, but we're not really experienced with Omnipay. We can probably figure it out though.

I need to discuss planning internally and will report back here.

barryvdh commented 6 years ago

Yeah that is the downside of using those libraries. Not so bad for just 1 gateway, but relying on multiple clients could give some problems.

As the current implementation seems solid, how big would the change be to just use the REST api?

barryvdh commented 6 years ago

As discussed with Mollie, we are open for a change to the Mollie SDK, as long as it doesn't create extra dependencies. For now it's just Guzzle which could be replaced with something HttpClient agnostic, but I do fear that that might be a BC break for Mollie API SDK.

willemstuursma commented 6 years ago

@fred-jan @digibeuk @Feijs any updates on the API v2 implementation in Omnipay will be posted here, follow this and any feedback is welcomed.

Smitsel commented 6 years ago

Good morning all! Martijn from Mollie here.

I've been looking at a way to abstract the http implementation and stumbled upon: http://docs.php-http.org/en/latest/httplug/library-developers.html Please let me know what u guys think.

This uses a virtual package and some discovery and PSR packages to be HttpClient agnostic. I will also provide a link to this PR this afternoon, please feel free to check it out.

barryvdh commented 6 years ago

Hello Martijn,

That is indeed what we also use for Omnipay, see https://github.com/thephpleague/omnipay-common/blob/master/src/Common/Http/Client.php So if the Mollie SDK uses the same virtual implementation requirement, that shouldn't conflict: https://github.com/thephpleague/omnipay-common/blob/8b9f0388cd8cfb94033ee945fa33feb4d89b9a9f/composer.json#L42

You do have to realise that you cannot drop the Guzzle requirement from your existing SDK without a major version release. And if you still require both HttpPlug + the Guzzle adapter, the dependency is still existing.. What we do with Omnipay is to have an omnipay-common package that doesn't rely on any http client, only the virtual implementation. The league/omnipay package requires the common package + a default client: https://github.com/thephpleague/omnipay/blob/master/composer.json

So not sure what is best for Mollie SDK, creating a separate 'core' package or release a new major version which is not tied to any HTTP client.

(for reference, my initial hope was to use PSR-18, but that isn't released yet after years of progress; https://github.com/php-fig/fig-standards/blob/master/proposed/http-client/http-client.md )

Smitsel commented 6 years ago

Hi @barryvdh,

We are choosing to do the necessary changes and release this under a Minor. We agree this is BC breaking, but we prefer this approach since its not really significant. It's just a small addition of having our users require the guzzle adapter.

You can find our updated composer.json here: https://github.com/mollie/mollie-api-php/pull/230/files#diff-b5d0ee8c97c7abd7e3fa29b9a27d1780

Is this acceptable on your end for updating this package with the SDK?

Smitsel commented 6 years ago

Goodmorning!

We've created PR https://github.com/thephpleague/omnipay-mollie/pull/49 for updating to v2. All feedback is welcome.

We decided to not introduce the SDK as dependency. We felt it would not have much added value.

barryvdh commented 6 years ago

Okay nice, I've merged the PR. The branch-alias is updated to 5.0 so you should be able to required 5.x@dev to test this.

Smitsel commented 6 years ago

Hi @barryvdh,

Thanks! We've already been able to do so by adding our fork as repository to test stuff out. All looks good! Thanks for the fast reviewing and merging 👍

barryvdh commented 6 years ago

Okay let me know if you tested the actual version from this repository, then I can tag 5.0.0

Smitsel commented 6 years ago

I was not able to require on 5x@dev. However i was able to require the commit on dev-master on this repository. And went through all requests with a Mollie API test key trough league/omnipay. Need me to verify more?