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

[15.x] Change "name" column to "type" #1620

Merged
merged 1 commit into from
Dec 19, 2023
Merged

Conversation

driesvints
Copy link
Member

@driesvints driesvints commented Dec 18, 2023

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.

@driesvints driesvints changed the title Change "name" column to "type" [15.x] Change "name" column to "type" Dec 18, 2023
@driesvints driesvints marked this pull request as ready for review December 18, 2023 14:42
@taylorotwell taylorotwell merged commit 2205495 into master Dec 19, 2023
7 checks passed
@taylorotwell taylorotwell deleted the change-name-to-type branch December 19, 2023 15:12

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is PR is described as a breaking change, but changing existing migrations is a really breaking change; anyone upgrading is going to find that their site is broken, and without another migration they'll have to manually rename the column. Is this intended?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@natacado yes. Please check the upgrade guide.

@oprypkhantc
Copy link

oprypkhantc commented Jan 18, 2024

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
Copy link
Member Author

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

@oprypkhantc
Copy link

@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
Copy link
Member Author

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
Copy link

@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...

QuentinGab pushed a commit to finller/laravel-cashier-connect that referenced this pull request Mar 16, 2024
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.

4 participants