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

Allow the base url for the Stripe API to be customised #1273

Merged
merged 2 commits into from
Nov 5, 2021

Conversation

seanmtaylor
Copy link
Contributor

This PR allows the base url of the Stripe API to be changed if needed, such as when using https://github.com/stripe/stripe-mock.

If there's already a way to change the base url without needing this change then please let me know what I'm missing as I couldn't see a way to customise the $options parameter of the stripe() method.

@driesvints
Copy link
Member

I'm not sure what you're trying to achieve. This should already be the default. Why define it explicitly?

Can you show where you need to adjust the url with https://github.com/stripe/stripe-mock ?

@taylorotwell taylorotwell marked this pull request as draft November 4, 2021 13:56
@seanmtaylor
Copy link
Contributor Author

seanmtaylor commented Nov 4, 2021

Sorry @driesvints, I may not have worded that brilliantly.

You're right, my change will simply maintain the default behaviour as it sets the base url to BaseStripeClient::DEFAULT_API_BASE, which is 'https://api.stripe.com'.

However, it will allow the user to change this in their Laravel app if needed.

A scenario when the base url would need to be changed is when you want to test a Laravel app without hitting the Stripe API or having to mock HTTP requests etc.

So stripe-mock can be setup and ran in a docker instance.

Then to make sure that it uses the stripe-mock instance when running the Laravel tests you would need to set the base url to stripemock:12111 or http://localhost:12111 (as seen here).

Which can now be achieved with:

Cashier::$apiBaseUrl = 'stripemock:12111';

// This will now make requests to stripe-mock instead of https://api.stripe.com
$user->createAsStripeCustomer();

Let me know if you need any more info.

@driesvints
Copy link
Member

driesvints commented Nov 5, 2021

Thanks @seanmtaylor. I think this change can be added but I want to warn you about Stripe Mock: since it's stateless I suspect a lot of this library won't be compatible again. Internally, Cashier does a lot of synchronous API calls to Stripe that rely on the data to be persistent for subsequent calls. Therefor something like Stripe Mock isn't useful here. I tried it out in our test suite and the tests failed for the above reasons.

In the meantime though, I don't see a specific reason to not add this change. But I hope I informed you enough about the limitations of Stripe Mock.

@driesvints driesvints marked this pull request as ready for review November 5, 2021 09:32
@seanmtaylor
Copy link
Contributor Author

seanmtaylor commented Nov 5, 2021

Thanks @seanmtaylor. I think this change can be added but I want to warn you about Stripe Mock: since it's stateless I suspect a lot of this library won't be compatible again. Internally, Cashier does a lot of synchronous API calls to Stripe that rely on the data to be persistent for subsequent calls. Therefor something like Stripe Mock isn't useful here. I tried it out in our test suite and the tests failed for the above reasons.

Thanks @driesvints, much appreciated. I'm aware of the limitations, I saw them mentioned in one of your blog posts (https://driesvints.com/blog/testing-cashier/).

My thoughts are at least for the simple calls which don't rely on a persistent state, then I can use stripe-mock, for others that's where I can implement a more advanced solution, like those outlined in your blog post.

Just done a bit of googling to find out more and it seems like to make use of that, it will require changing the url to point at the running instance of stripe-playback. So this PR should hopefully come in handy for that too 🤞

@driesvints
Copy link
Member

@seanmtaylor the repo I linked to is indeed private, sorry.

@taylorotwell taylorotwell merged commit 0e8ed51 into laravel:13.x Nov 5, 2021
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