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

[10.0] Ship migrations with package #663

Merged
merged 1 commit into from
May 3, 2019
Merged

[10.0] Ship migrations with package #663

merged 1 commit into from
May 3, 2019

Conversation

driesvints
Copy link
Member

@driesvints driesvints commented May 3, 2019

This adds Cashier's migrations to the package itself so they're shipped with Cashier and can be published just like with Passport and Telescope. This also directly integrates the migrations into the test suite so they're tested. Orchestra testbench helps us to implement this.

I needed to update migrations for a future PR so I thought I'd implement this first.

Closes #639

This adds Cashier's migrations to the package itself so they're shipped with Cashier and can be published just like with Passport and Telescope. This also directly integrates the migrations into the test suite so they're tested. Orchestra testbench helps up implement this.

Closes #639
@driesvints driesvints changed the title [10.0] Migrations [10.0] Ship migrations with package May 3, 2019
@taylorotwell taylorotwell merged commit aaf9fbe into master May 3, 2019
@driesvints driesvints deleted the migrations branch May 3, 2019 13:44
@SlyDave
Copy link

SlyDave commented Aug 20, 2019

Am I missing something here...?

If we've already run the migrations as per the documentation for Laravel <5.8 the migrations will fail, if we use the disable migrations function; future migrations for the package wont run...

Shouldn't there be command to populate the migrations table with details of the migration to let the system know the migrations don't need running without preventing future migrations running?

@SlyDave
Copy link

SlyDave commented Aug 20, 2019

Also the migration assumes the users table is the model that is holding the payee entity, which isn't always the case... (such as in ours)

@driesvints
Copy link
Member Author

@SlyDave any breaking changes to the table structure will be documented in the upgrade guide so you can always reference that when upgrading.

Also the migration assumes the users table is the model that is holding the payee entity, which isn't always the case... (such as in ours)

You can publish the migrations and alter the table name.

@SlyDave
Copy link

SlyDave commented Aug 20, 2019

Thanks @driesvints

For anyone else:
publish the migrations: php artisan vendor:publish --tag="cashier-migrations"
and remove the up and down content, leaving them as empty stubs.

This will allow the migrations to run and allow future package migrations to work.
(Note however, if you're changing the CASHIER_MODEL you'll always need to check future migrations anyway...)

@driesvints
Copy link
Member Author

Glad you figured it out 👍

This is also documented here btw: https://laravel.com/docs/5.8/billing#installation

@SlyDave
Copy link

SlyDave commented Aug 20, 2019

There is one unfortunate side effect of this that isn't as easily overcome.

The original migration that was used before the migration was bundled has the same class name...

If I change the published migration, the package will use it's own version again.
If I change my migration, it'll be seen as a new migration.

In my case, because I'm changing the CASHIER_MODEL I'm going to disable migrations from Cashier, and I can simply not use the published migrations. But for people not changing the model, this will present a difficult to overcome problem.

They will have to update their migration class name and filename to be different, then manually change the migrations table setting the 'migration' of their implementation to the new name.

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