[11.x] Enhance database migrations #51373

hafezdivandari · 2024-05-10T13:39:08Z

This PR does a lot of improvements and unlocks many possibilities on database migrations, also makes it easier to maintain!

New Features / Enhancements 🚀

Extend SQLite support to 3.26+

Laravel currently requires SQLite 3.35+, this PR extends SQLite support to 3.26+

Add and Drop Foreign Keys on SQLite

Fixes #25475, #23461. SQLite doesn't support adding / dropping foreign keys with alter table command, but as suggested on the docs, you may recreate the table with arbitrary changes, that's what this PR do to implement these commands.

This PR adds support for blueprint's $table->foreign(), $table->dropForeign() methods and ->constrained() modifier on SQLite:

Schema::table('posts', function (Blueprint $table) {
    $table->unsignedBigInteger('user_id');

    $table->foreign('user_id')->references('id')->on('users'); // Now works as expected on SQLite
});

Schema::table('posts', function (Blueprint $table) {
    $table->foreignId('user_id')
        ->constrained(); // Now works as expected on SQLite
});

Schema::table('posts', function (Blueprint $table) {
    $table->dropForeign(['user_id']); // Now works as expected on SQLite, no exception will be thrown anymore!
});

Fixes #51318, SQLite doesn't allow dropping a column if it is used in a foreign key constraint, this PR makes it possible:

Schema::table('sessions', function (Blueprint $table) {
    $table->dropForeign(['user_id']);
    $table->dropColumn('user_id');
});

// or with more magic...
Schema::table('sessions', function (Blueprint $table) {
    $table->dropConstrainedForeignId('user_id');
});

Add and Drop the primary key on SQLite

This PR adds support for blueprint's $table->primary(), $table->dropPrimary() methods, ->primary(), and ->primary(false)->change() modifiers when modifying table on SQLite:

Schema::table('posts', function (Blueprint $table) {
    $table->dropPrimary();              // Drop the current primary key
    $table->string('email');            // Add 'email' column
    $table->primary(['name', 'email']); // Add a composite primary key 
});

Schema::table('posts', function (Blueprint $table) {
    $table->uuid('id')->primary();  // Add 'id' column and make it primary
});

Schema::table('posts', function (Blueprint $table) {
    $table->string('id')->primary(false)->change();  // Change the type of 'id' column to `string` and drop primary key
});

Preserve the order of commands

This is the most important change on this PR; When updating table and compiling blueprint commands to SQL, the order of commands wasn't respected:

First compile all modified columns at one query
Second compile all added columns at once
Then compile other commands

As discussed on #50925, this will causes issue, unexpected results and leads to error most of the time when updating table. For example, the following code raises The 'username' column already exists! SQL error on all databases, because add column command was compiled before rename column:

Schema::table('users', function (Blueprint $table) {
    $table->renameColumn('username', 'email');
    $table->string('username');
});

This PR fixes this by preserving the order of commands as declared on the blueprint. You don't have to call each command in a separate Schema::table anymore that breaks migration transaction!

Blueprint State for SQLite

As explained before, when updating table on SQLite, we have to recreate the table to do arbitrary changes. To do so we need to know the current state of the table on each command. This PR adds BlueprintState class to keep track of changes on the table. After this PR we are able to mix any combination of blueprint commands in a single Schema::table (as demonstrated on the added integration tests) and it works as expected in the same order!

Notes

Unlike other database drivers, foreign keys don't have name on SQLite, so obviously you can't drop a foreign key by its name on SQLite:

Schema::table('posts', function (Blueprint $table) {
    $table->dropForeign('posts_user_id_foreign'); // ❌ doesn't work on SQLite

    $table->dropForeign(['user_id']);             // ✔️ works as expected
}

We probably need to remove withSqlite trait on orchestral/testbench package, it seems we don't need that anymore if it's merged?

github-actions · 2024-05-10T13:39:22Z

Thanks for submitting a PR!

Note that draft PR's are not reviewed. If you would like a review, please mark your pull request as ready for review in the GitHub user interface.

Pull requests that are abandoned in draft may be closed due to inactivity.

shaedrich · 2024-05-13T02:26:29Z

Is there a reason, you mixed the SQLite and migration execution order changes into one PR instead of two separate ones?

ocleroux · 2024-05-13T14:32:19Z

I'm eager to see this merged!

hafezdivandari · 2024-05-13T14:37:25Z

@shaedrich we can't add SQLite features without fixing the migration commands' orders.

shaedrich · 2024-05-13T15:39:39Z

@hafezdivandari You could have created the SQLite PR as draft, added a comment concerning the PR merging order and have backmerged the main branch after the migration command order PR was merged. But in the end, it's Taylor's choice to make

hafezdivandari · 2024-05-15T10:50:55Z

@shaedrich It's fine ;)

taylorotwell · 2024-05-23T18:53:31Z

To be honest this just is too much for us to take on reviewing and maintaining at the moment and I'm not sure the return on investment is there for end users.

Feels like something maybe better reserved for later this year or early next year for Laravel 12.

Gandhi11 · 2024-05-23T19:02:00Z

@taylorotwell How are you guys dropping columns with foreign key in SQLite currently? I do not understand how this is not a major issue for a lot a Laravel applications.

driesvints · 2024-05-24T07:06:26Z

Hi @hafezdivandari. I think we should still give a part of your PR a chance but like Taylor said and like we said on the Passport PR keep in mind to keep PR's small and separate because reviewing and testing such a large PR as this is simply too much for us at this time. Sending in incremental PR's with the different topics from this PR will be much much more easier to review.

I feel like the most important part of this bit is the dropping of foreign keys in SQLite. Could we maybe just focus on that one for now so we can get that back into Laravel v11?

Besides that I went through your code changes and while I feel most of this looks okay I do have a question why you decided to move column modifications to different alter lines instead of packing them into a single query statement with add like it does now. Because with this PR it could lead to a head more query statements. Not sure if that's particularly bad but it did caught my eye and I'd like to get your opinion about it.

hafezdivandari · 2024-05-24T09:04:43Z

Hi @driesvints,

I think we should still give a part of your PR a chance but like Taylor said and like we said on the Passport PR keep in mind to keep PR's small and separate because reviewing and testing such a large PR as this is simply too much for us at this time. Sending in incremental PR's with the different topics from this PR will be much much more easier to review.

First of all, sorry for sending large PRs 😅 but this one is a little different from the PR on the Passport, separating this into more PRs (if technically possible) doesn't make it easier to review. Because although this PR fixes 2 main problems at the same time, it doesn't mean the changes can necessarily be sent on 2 separate PRs.

But I can add comments on each part of the code changes and explain in more details if you want. IMHO that would be really helpful when reviewing.

I feel like the most important part of this bit is the dropping of foreign keys in SQLite. Could we maybe just focus on that one for now so we can get that back into Laravel v11?

As I tried to explain on this comment earlier and this one, It is not possible to fix the drop foreign on SQLite (or any other commands that needs recreating the table on SQLite) without fixing the "order of commands" problem first.

Besides that I went through your code changes and while I feel most of this looks okay I do have a question why you decided to move column modifications to different alter lines instead of packing them into a single query statement with add like it does now. Because with this PR it could lead to a head more query statements. Not sure if that's particularly bad but it did caught my eye and I'd like to get your opinion about it.

Because preserving the order of commands matters. Please read the "Preserve the order of commands" section above. The rule is simple, compile commands one by one, instead of packing them together and losing the order.

driesvints · 2024-05-24T13:23:47Z

We're gonna have another look at this but it could be a few weeks before we get to it. Thanks!

gsxdsm · 2024-05-26T18:31:28Z

FWIW I’m running into the issue of not being able to drop foreign keys in SQLite and would very much appreciate having this fix in. It’s making a mess of things on my end.

Not working with Laravel 11: - laravel/framework#51318 - laravel/framework#51373

src/Illuminate/Database/Schema/Blueprint.php

crynobone · 2024-07-10T01:06:56Z

Hi @hafezdivandari

We probably need to remove withSqlite trait on orchestral/testbench package, it seems we don't need that anymore if it's merged?

WithSqlite is not being used anywhere by default, and requires explicit invoke to trigger.

I started seeing some error with using Schema:::dropIfExists() with in-memory SQLite after the latest upgrade on Testbench.

1) Orchestra\Testbench\Tests\Databases\RefreshDatabaseUsingEventsTest::it_create_database_migrations
Error: Call to a member function getAttribute() on null

/Projects/orchestra/testbench-core/vendor/laravel/framework/src/Illuminate/Database/Connection.php:1632
/Projects/orchestra/testbench-core/vendor/laravel/framework/src/Illuminate/Database/Schema/Grammars/SQLiteGrammar.php:39
/Projects/orchestra/testbench-core/vendor/laravel/framework/src/Illuminate/Database/Schema/Blueprint.php:293
/Projects/orchestra/testbench-core/vendor/laravel/framework/src/Illuminate/Database/Schema/Blueprint.php:209
/Projects/orchestra/testbench-core/vendor/laravel/framework/src/Illuminate/Database/Schema/Blueprint.php:130
/Projects/orchestra/testbench-core/vendor/laravel/framework/src/Illuminate/Database/Schema/Blueprint.php:116
/Projects/orchestra/testbench-core/vendor/laravel/framework/src/Illuminate/Database/Schema/Builder.php:564
/Projects/orchestra/testbench-core/vendor/laravel/framework/src/Illuminate/Database/Schema/Builder.php:446
/Projects/orchestra/testbench-core/vendor/laravel/framework/src/Illuminate/Support/Facades/Facade.php:357
/Projects/orchestra/testbench-core/tests/Databases/RefreshDatabaseUsingEventsTest.php:36
/Projects/orchestra/testbench-core/src/Concerns/HandlesDatabases.php:47
/Projects/orchestra/testbench-core/src/Concerns/ApplicationTestingHooks.php:267
/Projects/orchestra/testbench-core/src/Concerns/ApplicationTestingHooks.php:110
/Projects/orchestra/testbench-core/src/Concerns/Testing.php:69
/Projects/orchestra/testbench-core/vendor/laravel/framework/src/Illuminate/Collections/helpers.php:236
/Projects/orchestra/testbench-core/src/functions.php:119
/Projects/orchestra/testbench-core/vendor/laravel/framework/src/Illuminate/Collections/helpers.php:236
/Projects/orchestra/testbench-core/src/Concerns/Testing.php:97
/Projects/orchestra/testbench-core/src/TestCase.php:65

https://github.com/orchestral/testbench-core/actions/runs/9866136206

hafezdivandari · 2024-07-10T02:02:57Z

@crynobone the error is on this line, it seems that connection's getPdo() is returning null which is wierd. I'm still investigating.

hafezdivandari · 2024-07-10T03:26:32Z

@crynobone I may have found the cause of issue:

The RefreshDatabase trait calls $connection->disconnect() so the connection's pdo is null on destroyDatabaseMigrations() method.

It seems that schema builder doesn't reconnect the connection!

crynobone · 2024-07-10T03:28:07Z

Noted. Probably something Testbench specific. I will investigate further. Thanks @hafezdivandari

michaeldyrynda · 2024-07-14T23:18:21Z

Adding a note here that this merge change appears to have affected some tests in my laravel-model-uuid package - specifically where we're making assertions against the shape of the resulting alter statements.

As I'm specifically testing that the generated queries are correctly formatted, I don't necessarily think this is a breaking change to end users as the resulting query is ultimately the same, just that now there are multiple queries rather than a single one.

Not sure this has tangible impact to framework consumers, though.

The queries themselves were not changed, however, the statements are now generated as separate array items, rather than as a single query. Pinned minimum framework version to `^11.15` and lifted package to PHP `^8.2`, seeing as we need these as minimums to satisfy these changes here.

hafezdivandari · 2024-07-15T08:13:47Z

Thanks @michaeldyrynda, these changes were expected, as I explained on this comment before merge and the changes on framework's tests also show that.

Nice package by the way. You may also want to check PR #50355 for use in your code.

j-mastr · 2024-07-15T10:32:46Z

Looks like I'm a bit late to the party, though ...

I feel like it's a 99.9% chance this PR will break existing applications

– Yep just happened. Logically, @hafezdivandari is right to have the migration commands in consecutive order like they depend on each other, which helps reducing confusion. However, imagine the following case:

$table->renameColumn('oldName', 'newName');
$table->string('addedColumn')->after('oldName');

Because we figured out that migrations were not running in the right order, we used this schema for rename/adding in a single step. Even though I agree that it is supposed to be a bug, doesn't mean people are not depending on it's behaviour.

Luckily, this doesn't affect our production systems because the past migrations don't have to run again. Instead, it rather pulled the plug on our integration pipeline because all out of a sudden, all tests were failing. Fortunately, these are only a few affected migrations and quickly fixed. However, finding the root cause took quite some time, because this change wasn't appropriately documented (due to the size of the PR?) and it's breaking character wasn't obvious at first.

Just as a short heads-up that even bug fixes can introduce breaking changes and to confirm @taylorotwell's feeling ;-)

hafezdivandari · 2024-07-15T11:05:25Z

@j-mastr off course Taylor's feeling is right. It was a hard decision to make and the PR was open for 2 months.

Besides that, it was much easier for me to send this to 12.x instead. Since the patch release on Tuesday, I'm checking the issue board every hour and try to find related discussions on other packages too!

About your example, you are adding a column after a non-existent (renamed) column. The orders of commands are obviously wrong. This PR is doing what you asked and compiling the commands in the order you have declared!

However, finding the root cause took quite some time, because this change wasn't appropriately documented (due to the size of the PR?) and it's breaking character wasn't obvious at first.

The PR description couldn't be more long I think, tried to cover every aspect + other comments on this thread. But I believe this maybe could be covered better on weekly Laravel newsletters after release to better inform the community.

Thanks for the feedback and sorry for the trouble, everyone.

fulopattila122 · 2024-07-26T08:31:54Z

I think this might be a breaking change. Disclaimer: the feature set is nice, so I'm not complaining, just thought to share the information.

One of our migrations started to fail on SQLite, beginning with Laravel 11.15:
https://github.com/vanilophp/framework/blob/4.1.0/src/Links/resources/database/migrations/2024_05_31_074502_add_root_item_to_link_group_items_table.php

Having Laravel 11.15 it fails:

Switching back to Laravel 11.14 solves the problem:

The linked example is from a library that supports Laravel 10.43+ and Laravel 11.0+

I believe we can find a workaround, like checking what Laravel version we have, and acting accordingly. But maybe you have a better suggestion.

hafezdivandari · 2024-07-26T08:43:58Z

@fulopattila122 you are trying to drop a column with foreign key on SQLite, that causes error, you may either don't add foreign key on sqlite or drop the foreign before dropping the column:

Solution 1:

if ('sqlite' !== DB::getDriverName()) {
    $table->foreign('root_item_id')->references('id')->on('link_group_items');
}

Solution 2:
Remove this line, and drop foreign by column:

$table->dropForeign(['root_item_id']);

ekandreas · 2024-08-05T13:58:47Z

Ohh, this is a large one... Breaking the tests for us.
Are there any more simple workarounds?

hafezdivandari · 2024-08-05T14:05:24Z

@ekandreas would you please share more info? Code? Exception?

ekandreas · 2024-08-06T08:11:18Z

Sorry, some kind of integrity violation in our seeder that caused an error.
We had to exclude some foreign keys in the schemas.

            if (env('DB_CONNECTION') == 'sqlite') {
                return;
            }

src/Illuminate/Database/Schema/Grammars/SQLiteGrammar.php

enhance migrations

d2f441e

formatting

b77e691

hafezdivandari marked this pull request as ready for review May 10, 2024 16:23

hafezdivandari mentioned this pull request May 10, 2024

Laravel 11 SQLite Migration Failure: after drop column: unknown column "XXXXXX" in foreign key definition #51318

Closed

hafezdivandari added 5 commits May 12, 2024 21:09

support drop column on legacy sqlite

1a43f5f

formatting

fa6211a

fix tests

9a38044

force re-run tests

f2c6235

formatting

35c97a5

taylorotwell closed this May 23, 2024

hafezdivandari deleted the 11.x-enhance-schema-migrations branch May 23, 2024 18:58

hafezdivandari restored the 11.x-enhance-schema-migrations branch May 23, 2024 18:59

taylorotwell reopened this May 24, 2024

Simounet added a commit to Simounet/flox that referenced this pull request May 28, 2024

Excluding primary key deletion for tests using SQLite

cb06098

Not working with Laravel 11: - laravel/framework#51318 - laravel/framework#51373

mchev mentioned this pull request May 30, 2024

Laravel 11 InvoiceShelf/InvoiceShelf#84

Merged

9 tasks

imanghafoori1 reviewed Jun 2, 2024

View reviewed changes

src/Illuminate/Database/Schema/Blueprint.php Outdated Show resolved Hide resolved

imanghafoori1 reviewed Jun 2, 2024

View reviewed changes

src/Illuminate/Database/Schema/Blueprint.php Outdated Show resolved Hide resolved

imanghafoori1 reviewed Jun 2, 2024

View reviewed changes

src/Illuminate/Database/Schema/Blueprint.php Show resolved Hide resolved

hafezdivandari mentioned this pull request Jul 10, 2024

Upgrading laravel/framework v11.15.0 fails migration containing table update (ORA-01430) yajra/laravel-oci8#864

Closed

timoteo7 mentioned this pull request Jul 10, 2024

fix: compileAdd with getColumn (laravel_framework_11.15.0) yajra/laravel-oci8#865

Closed

This was referenced Jul 12, 2024

Problem dropping foreign keys when run tests using in memory sqlite. #23461

Closed

[5.7] Sqlite & dropForeign #25475

Closed

damiencriado mentioned this pull request Jul 13, 2024

Some errors + CI fails on Laravel 11.15 tpetry/laravel-postgresql-enhanced#85

Closed

hafezdivandari mentioned this pull request Jul 17, 2024

[11.x] Extend SQLite support to 3.26+ laravel/docs#9768

Merged

midnite81 mentioned this pull request Jul 17, 2024

Failure to create composite primary key on upgrade to laravel framework 11.15.0 #52166

Closed

georgecoca mentioned this pull request Jul 24, 2024

Drop indexes before dropping the column statamic/eloquent-driver#328

Merged

iejfd930r2jfo32 mentioned this pull request Jul 31, 2024

dropForeign method removes all my database tables with SQLite #52348

Closed

hafezdivandari mentioned this pull request Aug 1, 2024

[11.x] Support SQLite foreign keys laravel/docs#9803

Merged

taka-oyama mentioned this pull request Aug 5, 2024

fix: migration broke in laravel v11.15 colopl/laravel-spanner#224

Merged

incrize mentioned this pull request Aug 26, 2024

[11.x] SQLite Error: "General error: 1 no such table" after adding a foreign key when using a table prefix. #52578

Merged

lorenzolosa mentioned this pull request Sep 5, 2024

Attempting to alter a table with a column with default value (JSON_ARRAY()) by adding a foreign constraint fails with an error on SQLite #52655

Closed

mjauvin reviewed Oct 23, 2024

View reviewed changes

src/Illuminate/Database/Schema/Grammars/SQLiteGrammar.php Show resolved Hide resolved

ismaail mentioned this pull request Nov 27, 2024

Migrations fixes oitcode/samarium#117

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[11.x] Enhance database migrations #51373

[11.x] Enhance database migrations #51373

hafezdivandari commented May 10, 2024 •

edited

Loading

github-actions bot commented May 10, 2024

shaedrich commented May 13, 2024

ocleroux commented May 13, 2024

hafezdivandari commented May 13, 2024

shaedrich commented May 13, 2024 •

edited

Loading

hafezdivandari commented May 15, 2024

taylorotwell commented May 23, 2024 •

edited

Loading

Gandhi11 commented May 23, 2024

driesvints commented May 24, 2024

hafezdivandari commented May 24, 2024 •

edited

Loading

driesvints commented May 24, 2024

gsxdsm commented May 26, 2024

crynobone commented Jul 10, 2024

hafezdivandari commented Jul 10, 2024

hafezdivandari commented Jul 10, 2024

crynobone commented Jul 10, 2024

michaeldyrynda commented Jul 14, 2024

hafezdivandari commented Jul 15, 2024

j-mastr commented Jul 15, 2024 •

edited

Loading

hafezdivandari commented Jul 15, 2024 •

edited

Loading

fulopattila122 commented Jul 26, 2024

hafezdivandari commented Jul 26, 2024 •

edited

Loading

ekandreas commented Aug 5, 2024

hafezdivandari commented Aug 5, 2024

ekandreas commented Aug 6, 2024

[11.x] Enhance database migrations #51373

[11.x] Enhance database migrations #51373

Conversation

hafezdivandari commented May 10, 2024 • edited Loading

New Features / Enhancements 🚀

Extend SQLite support to 3.26+

Add and Drop Foreign Keys on SQLite

Add and Drop the primary key on SQLite

Preserve the order of commands

Blueprint State for SQLite

Notes

github-actions bot commented May 10, 2024

shaedrich commented May 13, 2024

ocleroux commented May 13, 2024

hafezdivandari commented May 13, 2024

shaedrich commented May 13, 2024 • edited Loading

hafezdivandari commented May 15, 2024

taylorotwell commented May 23, 2024 • edited Loading

Gandhi11 commented May 23, 2024

driesvints commented May 24, 2024

hafezdivandari commented May 24, 2024 • edited Loading

driesvints commented May 24, 2024

gsxdsm commented May 26, 2024

crynobone commented Jul 10, 2024

hafezdivandari commented Jul 10, 2024

hafezdivandari commented Jul 10, 2024

crynobone commented Jul 10, 2024

michaeldyrynda commented Jul 14, 2024

hafezdivandari commented Jul 15, 2024

j-mastr commented Jul 15, 2024 • edited Loading

hafezdivandari commented Jul 15, 2024 • edited Loading

fulopattila122 commented Jul 26, 2024

hafezdivandari commented Jul 26, 2024 • edited Loading

ekandreas commented Aug 5, 2024

hafezdivandari commented Aug 5, 2024

ekandreas commented Aug 6, 2024

hafezdivandari commented May 10, 2024 •

edited

Loading

shaedrich commented May 13, 2024 •

edited

Loading

taylorotwell commented May 23, 2024 •

edited

Loading

hafezdivandari commented May 24, 2024 •

edited

Loading

j-mastr commented Jul 15, 2024 •

edited

Loading

hafezdivandari commented Jul 15, 2024 •

edited

Loading

hafezdivandari commented Jul 26, 2024 •

edited

Loading