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 670 forks source link

[15.x] Refactor subscription type handling #1621

Closed driesvints closed 8 months ago

driesvints commented 8 months ago

Alternative to https://github.com/laravel/cashier-stripe/pull/1619

This PR contains a few scary changes and may seem like a big breaking change but it should not be. I'll try to explain below so bare with me.

What this PR mainly tries to achieve is better support for apps which offer multiple subscriptions. Imagine the following scenario: we have a gym type subscription but also a tennis subscription for a customer. Right now, there's no way to do a single check if the customer is subscribed at all to either of them. You'd have to do:

if ($user->subscribed('gym') || $user->subscribed('tennis')) {
    // ...
}

Each subscription type need to be checked individually. With the changes of this PR you could simply do:

if ($user->subscribed()) {
    // ...
}

And this would return true if the customer is subscribed to either a gym subscription or a tennis subscription.

So you might think this does implies a breaking change because the default fallback is now changed to null but in reality there shouldn't be a realistic scenario where this breaks any app. I'll try to explain in the scenarios below:

Single type subscription apps which use default as their type should be the majority of use cases and aren't affected because the changes made accommodate to fallback to the single subscription type. So basically ->subscribed() is still the same as doing ->subscribed('default'). The same can be said for subscriptions which use a different type name than default. They are already doing ->subscribed('gym') in their apps so they shouldn't need to do any changes to their codebase.

Multi type subscriptions probably are largely unaffected as well. Since they mostly work with dedicated types they are already doing $user->subscribed('gym') and $user->subscribed('tennis') checks by providing the correct type. So these apps also don't have to change anything to their codebase.

The only scenario I foresee a breaking change is when a multi type subscription app uses the default type in combination with other "types" of subscriptions. For example, default and gym. In this scenario, the changes will return the first matching result for either of the subscription types when using ->subscribed() while before it would always return the result of default. In this situation, we should encourage apps who work with multiple subscription types to always provide the type when calling any of the changed methods.

Lastly I'd like to demonstrate how the result set is determined for these changes. Imagine the following database table (subscriptions) and its values:

ID type status created_at
1 default canceled 2023-12-01
2 gym trialing 2023-12-02
3 tennis canceled 2023-12-03
4 default active 2023-12-04
5 tennis incomplete 2023-12-05

Now we'll do $user->subscribed() on this set of values. Here's how Cashier checked the results before this PR:

ID type status created_at checked
1 default canceled 2023-12-01
2 gym trialing 2023-12-02
3 tennis canceled 2023-12-03
4 default active 2023-12-04
5 tennis incomplete 2023-12-05

And here's how Cashier will check the results after this PR:

ID type status created_at checked
1 default canceled 2023-12-01
2 gym trialing 2023-12-02
3 tennis canceled 2023-12-03
4 default active 2023-12-04
5 tennis incomplete 2023-12-05

We now check every subscription type but we still only check the lastly added result of a certain subscription type. This is how Cashier handled things before as well so nothing changes here.

I think the best example of how little this breaks is that I didn't have to make any changes to the existing test suite. Everything still works as before.

taylorotwell commented 8 months ago

I don't think we should make this breaking change without significantly more community feedback.

clementmas commented 4 months ago

I'm in favor of this breaking change. I'm currently overriding the subscribed() and subscription() methods to ignore the $type parameter.

I think it should only check for a $type parameter if we provide one.