Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mollie Api v2 #48

Closed
barryvdh opened this issue May 31, 2018 · 19 comments
Closed

Mollie Api v2 #48

barryvdh opened this issue May 31, 2018 · 19 comments

Comments

@barryvdh
Copy link
Member

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
Copy link

nickurt commented Jun 1, 2018

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

@barryvdh
Copy link
Member Author

barryvdh commented Jun 1, 2018

Yeah and different return codes

@judgej
Copy link
Member

judgej commented Jun 1, 2018

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
Copy link

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
Copy link
Member Author

barryvdh commented Jun 1, 2018

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

@nickurt
Copy link

nickurt commented Jun 1, 2018

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

#47

@judgej
Copy link
Member

judgej commented Jun 1, 2018

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
Copy link

@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
Copy link
Member Author

barryvdh commented Jun 1, 2018

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
Copy link
Member Author

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
Copy link

@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
Copy link
Contributor

Smitsel commented Jul 18, 2018

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
Copy link
Member Author

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
Copy link
Contributor

Smitsel commented Jul 18, 2018

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
Copy link
Contributor

Smitsel commented Jul 23, 2018

Goodmorning!

We've created PR #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
Copy link
Member Author

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
Copy link
Contributor

Smitsel commented Jul 24, 2018

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
Copy link
Member Author

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

@Smitsel
Copy link
Contributor

Smitsel commented Jul 24, 2018

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants