laravel / cashier-stripe

Laravel Cashier provides an expressive, fluent interface to Stripe's subscription billing services.
https://laravel.com/docs/billing
MIT License
2.37k stars 671 forks source link

10.3.0 introducing "breaking changes" #794

Closed netpok closed 4 years ago

netpok commented 4 years ago

Description:

With Cashier 10.3.0 the underlying Stripe-PHP SDK was updated to v7, while this api is not created by this project, but it is directly exposed via the asStripe... methods which are not marked as internal methods. Following an update we noticed that many of our calls failed because of this.

I think either these kind of changes should be considered as breaking changes and implemented in new major versions or these method should be marked as internal.

driesvints commented 4 years ago

Which things broke for you specifically?

netpok commented 4 years ago

We used the $user->asStripeCustomer()->invoices()->autoPagingIterator() call to iterate through all invoices because currently the $user->invoices() only allows requesting 100 invoices maximum and we searched for the first instance of an invoice, so an iterator made perfect sense.

Slightly unrelated, but this method could be perfectly implemented with a LazyCollection and I plan to make a pull request about this against master (since this requires L6.0 features). Any opinion about this?

driesvints commented 4 years ago

Hmm, I can see how that could be breaking to you yeah. But the thing is what you have to know is that when you directly use features of an SDK outside the public api of a package it's best that you also add the constraint to your composer.json. That way you're locked on that version and won't run into problems with upgrading. You could also downgrade back to v10.2.1. If I have to be frank I don't consider these changes breaking. Nothing of the public api of Cashier was changed with these upgrades.

Btw, the problem you're describing is actually documented under SemVer here: https://semver.org/#what-should-i-do-if-i-update-my-own-dependencies-without-changing-the-public-api

If you depend on specific functionality of a library's dependencies then you need to add that library to the list of your own dependencies.

Slightly unrelated, but this method could be perfectly implemented with a LazyCollection and I plan to make a pull request about this against master (since this requires L6.0 features). Any opinion about this?

Ah yeah that's a good idea actually. You can definitely send in a pr to master if you want. Maybe create a different issue for that.

netpok commented 4 years ago

Yeah, that's why I said it should be marked as internal (with @internal annotation), the docs does not mention the pinning of the stripe library, I will try to make a pull request about that too.

netpok commented 4 years ago

@driesvints I don't know if this counts as a breaking change or not so I'm not opening a new issue but all exceptions were replaced in v7 of stripe, so if anybody was handling those that could be a problem.

Reference: https://github.com/stripe/stripe-php/pull/709

driesvints commented 4 years ago

@netpok well, as I said: if you were relying on exceptions from the stripe sdk then you would need to add the sdk to your own composer.json.

netpok commented 4 years ago

@taylorotwell As you said in #796 you don't consider asStripe... internal methods please have a look at this issue.

In short: With the update 10.3.0 the underlying stripe-php library was updated to v7, which largely changed the interface objects returned by the asStripe... methods. Furthermore it changed all the exceptions thrown by the library and passed through by Cashier, so if one checked for Stripe\Error\Api to detect a network error it fails now.

If this kind of change can happen in the future I'm happy to write a small section to the docs about accessing underlying objects (I don't think exceptions will change in the future).

driesvints commented 4 years ago

@netpok a section in the docs could definitely be helpful. Feel free to send in a pr. Thanks :)