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.5] Add dropAllTables() to DB Schema Builder #18484

Merged
merged 8 commits into from
Apr 5, 2017
Merged

[5.5] Add dropAllTables() to DB Schema Builder #18484

merged 8 commits into from
Apr 5, 2017

Conversation

laurencei
Copy link
Contributor

@laurencei laurencei commented Mar 25, 2017

This is a another attempt to implement a dropAllTables() type function into the database connection. Based upon advice in the original PR was here #18051 - this now has the dropTable functionality on the connection itself (see changes below).

This PR is also only for the dropTables functionality. If/when this functionality is merged into the framework, I'll do a separate PR for an artisan command to actually drop the tables.

What this PR needs help with:

  1. Is this PR generally acceptable (i.e. on the right track), or does it need conceptual re-working? Specifically based upon advice in the original PR, this includes a change to the ConnectionInterface. Are we happy to go with that? (see changes below)

  2. Can someone who has SQL Server please test the SQL dropTables functionality? done

  3. Needs tests - I'm a bit lost how to test this. If someone can point me in the right direction with 1 test for one of the connections, I can probably do the others myself. done

ping @tillkruss @freekmurze

edit: all done - see below.

@taylorotwell
Copy link
Member

As far as testing goes we would just want to integration test it. That is easiest with SQLite.

@taylorotwell
Copy link
Member

I think it's generally on the right track. I do wonder if the methods belong on the Schema\Builder class though which has the drop method already.

@laurencei
Copy link
Contributor Author

@taylorotwell - you are right as usual :)

I've refactored the PR completely. It now just has changes to the Builder, and no longer has changes to Connection or ConnectionInterface - making this a non-breaking PR for 5.5 I think.

The only 'thing' is the base Builder class is not abstract. And I cant have a "default" dropAllTables() implementation there (because they are all different). So I've got the base Builder throwing a LogicException there if someone tries to use it (which would only be for a custom connection anyway I think?).

$dbPath = $this->connection->getConfig('database');

if (file_exists($dbPath)) {
unlink($dbPath);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this will give problems when someone tries to drop all tables in an in-memory SQLite database (which is of course not a very common use case, but still). An approach similar to the Postgres solution might be better.

You can get a list of tables with:

SELECT name FROM sqlite_master WHERE type='table'

Copy link
Contributor Author

@laurencei laurencei Mar 26, 2017

Choose a reason for hiding this comment

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

@casperboone - thanks. I'm not worried about in-memory calls - I dont think that will ever happen.

But I didnt like that the command was doing a file manipulation - that didnt feel very right.

Using your code as a starting point - I've come up with a pure sql solution that should clear a sqlite database without actually deleting the file.

Should work for both in-memory and file sqlite databases.

Thanks.

@laurencei laurencei changed the title [5.5] [WIP] Add dropAllTables() to Connection Interface [5.5] [WIP] Add dropAllTables() to Database Builder Mar 26, 2017
@laurencei laurencei changed the title [5.5] [WIP] Add dropAllTables() to Database Builder [5.5] Add dropAllTables() to DB Schema Builder Mar 26, 2017
@laurencei
Copy link
Contributor Author

Ok - I've added an integration test.

I think this is ready.

Please let me know if any issues preventing from merging and I'll get onto it.

*/
public function compileDropAllTables($tables)
{
return 'drop table '.implode(',', $tables).' cascade';
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't some sort of escaping be used here? It's rarely, but schema and tables may have really weird names

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bohdan-shulha - thanks, good catch. I've updated the PR

@taylorotwell
Copy link
Member

Will try to look into this soon. This does not handle triggers, views, etc. that may exist right? Wasn't there some contention about that on the PR before this one?

@laurencei
Copy link
Contributor Author

@taylorotwell - the database will automatically drop the views when the tables no longer exist (confirmed by @Garbee on the original PR).

This PR does not drop functions.

My thinking is the name of this function is dropAllTables() - so the function is doing as the name implies and not overreaching its responsibilities.

There is the possibility of adding another function in the future on another PR called dropAllFunctions() or dropAllStoredProcedures(). This could be an enhancement option to the upcoming artisan command for db:nuke (or whatever it will be called).

@taylorotwell taylorotwell merged commit f55331d into laravel:master Apr 5, 2017
@taylorotwell
Copy link
Member

Thanks!

@Garbee
Copy link
Contributor

Garbee commented Apr 5, 2017

FTR the view dump I tested was only on Postgres DB's. MySQL/MsSQL may work differently in that regard.

@laurencei laurencei deleted the dropAllTables branch April 5, 2017 16:37
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

5 participants