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

[5.4] [WIP] Add db:empty command #18051

Closed
wants to merge 4 commits into from
Closed

[5.4] [WIP] Add db:empty command #18051

wants to merge 4 commits into from

Conversation

laurencei
Copy link
Contributor

@laurencei laurencei commented Feb 21, 2017

Background and discussion here: laravel/ideas#421

This PR adds the db:empty command. This allows you to empty an entire database, which can help people who dont want to use down migrations (especially during development). You can use it in a couple of ways:

Option 1: Just empty the database:
php artisan db:empty

Option 2: Empty the database and then re-run all migrations:
php artisan db:empty --migrate

Option 3: Empty the database, re-run all migrations, then seed:
php artisan db:empty --migrate --seed

I've added some tests that should cover most situations.

Note: it would be nice if someone with an actual SQL Server could test the command there (ideally with a table with foreign constraints). The SQL code I've used to drop all tables is from StackOverflow - and I dont have a SQL Server to test against.

Credit to @spatie @freekmurze for inspiration for this PR and some of the code is from the package here https://github.com/spatie/laravel-migrate-fresh (used with permission from @freekmurze) .

@devcircus
Copy link
Contributor

how does this differ from

php artisan:migrate reset
php artisan:migrate refresh 
php artisan:migrate refresh --seed

@devcircus
Copy link
Contributor

Ok. I see. For those who don't write down migrations.

@laurencei
Copy link
Contributor Author

laurencei commented Feb 22, 2017

Not just for lack of down migrations though; I've had situations where I've written failed up migrations that create a few tables, but then throw an exception half way through. Running migrate:refresh doesnt always work in those situations - so this is an easier way to reset and try again (during development).

And also - this will empty all tables from the database - regardless if you created them via a migration or not. That is why I've called it db:empty and not migrate:empty.

@sword-jin
Copy link

@lioannou Maybe your application have many db connections, so you should empty all

@laurencei
Copy link
Contributor Author

@RryLee - it can actually already handle that and specify different connections other than the default

php artisan db:empty --database=mysql
php artisan db:empty --database=other

@taylorotwell
Copy link
Member

I don't mind this particular feature but not sure about this implementation. Be simpler to just have the migration skip calling down if it doesn't exist?

@laurencei
Copy link
Contributor Author

laurencei commented Feb 22, 2017

Hi @taylorotwell - but the migrator skipping down doesnt actually solve the problem.

The problem is you are still left with tables in your database - with no way to remove them using artisan without manually writing something to remove those tables.

This provides a way to do it as an artisan call (and then some flags to re-migrate and re-seed if required). Then people who dont want down commands can still re-migrate their databases during development.

@tillkruss
Copy link
Collaborator

tillkruss commented Feb 23, 2017

@lioannou: We talked about this a little bit today and agreed, that the TableDropper stuff should be directly on the connection. Something like $connection->dropAllTables(). Can you please update the PR?

Another suggestion was to use a more aggressive name, like db:nuke or db:reset that implies that it's a destructive action and tables will be deleted.

@tillkruss tillkruss reopened this Feb 23, 2017
@laurencei laurencei changed the title [5.4] Add db:empty command [5.4] [WIP] Add db:empty command Feb 23, 2017
@laurencei
Copy link
Contributor Author

Ok @tillkruss - I'll make some changes and update the PR.

@taylorotwell
Copy link
Member

I agree with @tillkruss

@roberto-aguilar
Copy link
Contributor

@lioannou @tillkruss db:nuke will be cool as hell 🤘

@barryvdh
Copy link
Contributor

Why not just db:drop or db:drop-tables?

@driesvints
Copy link
Member

db:reset makes the most sense imo.

@laurencei
Copy link
Contributor Author

@barryvdh - cant be db:drop because that sounds like it is dropping the database itself.

I'm thinking db:drop-all-tables - as that is the most expressive verbose command and there would be no confusion what it means.

Would give something like php artisan db:drop-all-tables --migrate --seed

@driesvints
Copy link
Member

driesvints commented Feb 23, 2017 via email

@Garbee
Copy link
Contributor

Garbee commented Feb 23, 2017 via email

@driesvints
Copy link
Member

@Garbee seems out of scope for this issue I think? Perhaps we can add an extra flag for that in another PR?

@Garbee
Copy link
Contributor

Garbee commented Feb 23, 2017

I don't really think it is out of scope. If the idea is to nuke/clear/empty the database for re-migration, then that could be a key part to make sure your migrations run right again. Especially if you do raw SQL (which is supported) to add more advanced features in. Otherwise, if your developing your stuff and you nuke to reload, your stuff won't work. In fact, with views in Postgres your tables will fail to drop with this. Since the view depends upon a table and the table won't drop until the view gets removed.

I'd need to test to see if drop cascade works in the view scenario, but iirc from the other day working with them it doesn't. Postgres requires the view to go out first in a separate transaction.

@laurencei
Copy link
Contributor Author

An alternative; - drop the actual database - and then create a new database. That ensures a clean slate.

The only issue is possible database permissions for users - but given this would a development only command - would it really matter?

@tillkruss
Copy link
Collaborator

db:reset makes the most sense to me as well. I wouldn't make this command dev-only, can come in really handy in staging or testing environments as well

@Garbee
Copy link
Contributor

Garbee commented Feb 23, 2017

Just tested and when you cascade drop tables the views relating to them do drop as well. So that's clear as long as the drop is a cascade.

That still leaves functions being a problem though, as those are external objects not bound to the tables (the triggers should drop like views.) So for postgres functions should be queried up and dropped as well after the tables are dumped.

@laurencei
Copy link
Contributor Author

laurencei commented Feb 23, 2017

@tillkruss - question; if I add dropAllTables() to the Illuminate\Database\Connection - then do we want that method added to the ConectionInterface - forcing anyone who creates their own custom DB connection to implement their own version?

If I do that though - it would make this a 5.5 feature, since we couldnt implement an interface change in 5.4?

The alternative is just leave it as an empty method on Illuminate\Database\Connection - and have each current database connection extend it with their own implementation. Anyone who has a custom version of the database connection would just get an empty command.

Please let me know which you prefer and I'll finish the PR.

@tillkruss
Copy link
Collaborator

I personally would go with an interface change and target 5.5. @freekmurze, thoughts?

@driesvints
Copy link
Member

Yeah 5.5 might be a better target.

@freekmurze
Copy link
Contributor

Yea, interface change and target 5.5 sounds great.

@deleugpn
Copy link
Contributor

db:nuke is the best name this could ever have.

@joshmanders
Copy link
Contributor

I'd do db:reset also.

@laurencei
Copy link
Contributor Author

laurencei commented Feb 25, 2017

Does anyone here have an actual SQL Server? Could you test the current PR to confirm if actually drops the tables (ideally with some tables with foreign constraints)? The SQL Server code I've used to drop all tables is from StackOverflow - and I dont have a SQL Server to test against.

Dont worry about the rest of the PR - I'm redoing it - but would be good to confirm the current drop code actually works for SQL Server for the new PR.

@laurencei laurencei closed this Feb 25, 2017
@Garbee
Copy link
Contributor

Garbee commented Feb 25, 2017

Anyone can get a copy of SQL Server vnext (even for Linux) and test things out. This is a pre-release version of MS's next SQL Server offering which works across platforms. It should be good enough for testing basic table drop functionality.

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.

None yet