search

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] Change "name" column to "type" #1620

Closed driesvints closed 8 months ago

driesvints commented 8 months ago

I know this PR is opinionated but hope it can be considered. Right now, we regularly get issues about people being confused about this column's actual usage. People seem to be using this column as user-readable representation of the subscription product ("Gym Subscription") rather than what it actual is: a single internal subscription type (gym).

I've already patched Cashier Paddle v2 with this change and would like to make the same one in Cashier Stripe. This will require a single DB migration on the user's end to rename the column.

I know we try to avoid breaking changes but I think this one will be beneficial as it more closely indicates this column's purpose.

oprypkhantc commented 7 months ago

Hello @driesvints, has the effect on zero downtime applications been considered at all with this change?

Up to until recently MySQL didn't even support instant column renames, and many applications still use MySQL version <8.0.28. There are also forks of MySQL, like AWS Aurora, which only added support for instant column a couple of months ago.

Slightly older applications simply would not be able to upgrade to Cashier 15 without a significant downtime. Please consider this with future migrations. Breaking changes in code are one thing, breaking changes in a live database are completely different.

driesvints commented 7 months ago

Sorry you feel that way @oprypkhantc but we thought this change was worth doing.

oprypkhantc commented 7 months ago

@driesvints I agree it should have been done; just in a different way. This could been done in a two step process: upgrade to cashier 14.x+1 version, which adds a new column and fills it in, but doesn't yet use it; then upgrade to Cashier 15.x which drops the old column and starts using the new one. This would avoid downtime :)

driesvints commented 7 months ago

I don't think it's reasonable that it's expected we take in account for all of that for every change we make. Most modern databases support instant column renaming. I'm sorry this is a bit disruptive to you.

natacado commented 7 months ago

@oprypkhantc given that this change also makes a breaking migration the responsibility of the developer upgrading by writing their own migration, there's nothing stopping those needing a seamless migration from writing a more complex two step migration themselves, I think? The name/type column is nullable so pre creating it on 14.x, then stopping writes on 15.x before dropping seems like it'd work...