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

Shared token requests #21

Merged
merged 3 commits into from
May 12, 2017
Merged

Conversation

fryiee
Copy link
Contributor

@fryiee fryiee commented Apr 25, 2017

Added Requests for both CreateCard and UpdateCard through RapidShared (roughly mimicking the functionality in RapidDirect). I have not added any response requests as you can utilise the completePurchase() method to finalise both of them all the same. Let me know if you would prefer these as separate responses.

@incarnate
Copy link
Contributor

Thanks for the PR, looks good!

Only change you might want to consider is extending RapidSharedPurchaseRequest for the two requests to cut down on code duplication - just override getData. See RapidDirectCreateCardRequest as an example

@fryiee
Copy link
Contributor Author

fryiee commented Apr 26, 2017

I have just pushed up another commit which makes them extend rather than duplicate code. Thanks!

@incarnate
Copy link
Contributor

Much neater, thanks!
@delatbabel please consider for merging

@fryiee
Copy link
Contributor Author

fryiee commented Apr 30, 2017

Is this likely to be merged? I just need to know if I need to attach my fork to my composer package instead of the mainline gateway as I need to roll out this module this week.

@incarnate
Copy link
Contributor

This should be fine to be merged, however since omnipay is an open source project the maintainers are all volunteers and things don't always happen quickly.

If you need to release this week I'd say roll with your own fork and you can update to official once the merge has happened.

@delatbabel
Copy link
Contributor

Hi, yes I saw the ping and I have looked at the code briefly. It should be OK to merge, but I need to have a deeper read and I have a backlog of paying job tasks at the moment. So I'll try to get this done in the next few days.

@fryiee
Copy link
Contributor Author

fryiee commented May 3, 2017

No problems guys, fully understand and thanks for your responses. I'm running from my fork for now.

@delatbabel delatbabel merged commit 4801534 into thephpleague:master May 12, 2017
@delatbabel
Copy link
Contributor

OK merged, thanks. Sorry for the delay and thanks for your patience. If you can confirm that this works in a production environment then I will tag for release.

@fryiee
Copy link
Contributor Author

fryiee commented Aug 25, 2017

@delatbabel I never responded to this. I've been using my fork in production for a little while now.

@fryiee
Copy link
Contributor Author

fryiee commented Jan 18, 2018

Hi @delatbabel I'm working on another project that is in need of this. Do you know if it's possible to get this tagged for release? At the moment we're going to run our tagged fork but it would be nice to use the mainline package.

@delatbabel
Copy link
Contributor

Released as 2.2.2

@fryiee
Copy link
Contributor Author

fryiee commented Feb 22, 2018

@delatbabel thanks a bunch!

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

Successfully merging this pull request may close these issues.

3 participants