-
Notifications
You must be signed in to change notification settings - Fork 11.1k
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
[11.x] Improve readability of SQLite schema dumps #52172
Conversation
if (getenv('DB_CONNECTION') !== 'testing') { | ||
parent::defineEnvironment($app); | ||
|
||
if ($this->driver !== 'sqlite') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This changes allow the TestCase to be executed on persistent and in-memory SQLite
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
$migrations = collect(preg_split("/\r\n|\n|\r/", $process->getOutput()))->reject(function ($line) { | ||
return str_starts_with($line, 'CREATE TABLE sqlite_'); | ||
})->all(); | ||
$migrations = preg_replace('/CREATE TABLE sqlite_.+\);[\r\n]+/is', '', $process->getOutput()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change here seems to have affected an application I have in which it used to return all the tables I had in my sqlite database file, but now only returns the migrations
one. I'm not sure how to fix it to make it work with all of them though. I think this test is only testing the sqlite in memory and not the sqlite database file and so it got missed.
@bakerkretzmar Have you tried this with a sqlite database file? I updated today to the latest Laravel framework and my previous commands that worked now don't and I was able to trace it to this and the variable Here is how I've been generating these dump files which worked prior to this merge: Create a new sqlite database
Run migrations
Dump the schema file
Previously after the 3rd step, the schema dump had all of my tables ( |
This PR adds the
--indent
option to the SQLite schema dump command to generate an indented dump file that is much easier to read than the default (which puts each table on a single line). This also now matches the format of dumps from other databases.Before
After
MySQL for comparison
This PR also fixes the
SqliteSchemaState
test, which was changed in #52139 and generated an empty dump file, so was no longer testing anything. It requires a real database file - @crynobone if there's a cleaner way to do this I'm happy to change anything you like, but for the test to be useful there has to be an internalsqlite_
database table created and the generated dump file has to not include it. I added assertions to make that intent clearer.