[5.3] Allow wrapping schema changes in a transaction if the database supports it

[ameliaikeda]

This is mostly to deal with a pet peeve of mine, and a source of endless frustration for newcomers: making an error in a migration file that results in changes being half applied.

This could be an error after a create table, in which the table is created, but the migration entry is never added. So you then get a "table exists" error, or a data migration that was only half-applied and was never idempotent. You then need to either call the query builder from tinker and reverse the migration manually, or dive into the database and fix stuff up (again, manually).

Certain databases (SQL Server, PostgreSQL) support wrapping schema changes in a transaction so they can be fully rolled back in the event of an error. I've only included PostgreSQL in here as I don't have a copy of SQL Server so I'm unable to actually test it myself.

I'm also not entirely sure if SQLite fully supports alter table within a transaction. Documentation seems to suggest it, and it works when I try it on another app of mine, but SQLite's somewhat lacking support for alter table means DBAL is needed anyway so I'm not sure if simply wrapping the migration would actually effectively roll back errors.

I'd love feedback/criticism and maybe pointers on how to actually test this, or how to do this in a more elegant way.

[yajra]

@ameliaikeda nice PR. I suggest we set the protected $transactions = true; by default since I think most of the DB driver have DB::transaction() handler?

For migrations that have multiple task, I usually enclose them within the DB::transaction() closure and this PR would definitely save us some lines of code. Thanks!

[ameliaikeda]

@yajra the difference between transaction support and schema transaction support is that databases like mysql dont support wrapping table-altering statements in a transaction.

MySQL automatically commits after an alter table statement, so if you do:

start transaction;

alter table foo add column bar integer; -- this has an implicit commit after it

You then try to migrate data to populate the bar column based on something else, or try to rollback on an exception, and so on, and mysql will completely fail to roll back the alter table.

You can do this in other RDBMSes by doing this (and we wrap every migration in my workplace):

public function up()
{
    DB::transaction(function () {
        Schema::table('accounts', function (Blueprint $table) {
            ...
        });
    });
}

However, automatically wrapping schema changes via the Migrator in a transaction seems like a way, way cleaner solution.

[JoostK]

Taylor approves of this cascade 😂

[ellej16]

Hi guys, not sure if this should be the thread I should comment on, artisan migrate:rollback doesn't work with this change. Here's the issue for it: #15892 .

I fixed it by adding a check if the getSchemaGrammar() returns a null and subsequently do a useDefaultSchemaGrammar() if so, as answered here #15892 (comment) .

I'm just wondering if it's the right way, or is there a cleaner way to approach this?

[ameliaikeda]

Yikes, good catch!

I didn't catch that one on my end, because I thought that getSchemaGrammar would always return an object. The fix is kinda hacky, but I'll put up a quick PR for it and work out a better way of going about this.

[ellej16]

Thanks @ameliaikeda ! ping me up when you do so. (Till then the quick fix will stay for a bit)

Cheers for this feature though 👍

[ameliaikeda]

@ellej16: fix is up in a PR now

[djeux]

It would've been great if someone tested it with postgres before merging it to master. Because there's no way proper way to disable this "feature" in 5.3. And this "feature" breaks "CREATE INDEX CONCURRENTLY" migrations in postgres.

[ameliaikeda]

@djeux make an issue for this. It was tested at the time, and you're reporting a bug fairly badly 😉

(also, add a failing test in a PR if you want, and it'll greatly speed up a fix)

[djeux]

#27334