Framework: SQLlite migration creates incorrect statements

[leewillis77]

• Laravel Version: 5.3.22
• PHP Version: 7.0.11

Description:

Consider the following two up() functions from consecutive migrations:

public function up()
{
    Schema::create('test', function (Blueprint $table) {
        $table->text('column_one');
        $table->text('column_two');
    });
}

public function up()
{
    Schema::table('test', function (Blueprint $table) {
        $table->string('column_three')->unique();
        $table->dropColumn('column_two');
    });
}

When run against MySQL, the second migration creates these (correct) SQL statements:

Blueprint.php: (113) : statements is Array
(
    [0] => alter table `test` add `column_three` varchar(255) null default ''
    [1] => alter table `test` drop `column_two`
    [2] => alter table `test` add unique `test_column_three_unique`(`column_three`)
)

However, when run against SQLite, the following statements are created:

Blueprint.php: (113) : statements is Array
(
    [0] => alter table "test" add column "column_three" varchar null default ''
    [1] => CREATE TEMPORARY TABLE __temp__test AS SELECT column_one FROM test
    [2] => DROP TABLE test
    [3] => CREATE TABLE test (column_one CLOB NOT NULL COLLATE BINARY)
    [4] => INSERT INTO test (column_one) SELECT column_one FROM __temp__test
    [5] => DROP TABLE __temp__test
    [6] => create unique index "test_column_three_unique" on "test" ("column_three")
)

Notice that statements [1], [3], [4] are all missing any reference / usage of the new field created in [0], and the final statement fails with the error:

[PDOException (HY000)]                                                         
  SQLSTATE[HY000]: General error: 1 table test has no column named column_three

Steps To Reproduce:

• Create a new laravel application
• Create two migrations containing the up() definitions above
• Configure Laravel to use the SQLite driver
• Run the migrations

[leewillis77]

As a workaround, splitting the two schema operations in the second migration into two works around the issue, e.g.

Schema::table('test', function (Blueprint $table) {
    $table->string('column_three')->nullable()->default('')->unique();
});
Schema::table('test', function (Blueprint $table) {
    $table->dropColumn('column_two');
});

[leewillis77]

The following operation also fails:

Schema::table('some_table', function ($table) {
    $table->timestamps();
});

Presumably the same underlying issue. The error created is Doctrine\DBAL\Driver\PDOException: SQLSTATE[HY000]: General error: 1 no such column: created_at

[sisve]

Dropping or modifying multiple columns within a single migration while using a SQLite database is not supported.

Source: At the bottom of https://laravel.com/docs/5.3/migrations#columns

[srmklive]

@leewillis77 please review the docs as per @sisve comment. The issues you are mentioning are not officially supported. Also this is not a Eloquent issue, its a Doctrine issue.

[leewillis77]

Hi all;

Thanks for all of the comments. I'm going to reply to each of them in turn.

Dropping or modifying multiple columns within a single migration while using a SQLite database is not supported.

Understood. However, the reason that this is "unsupported" appears to be just because it's broken, and not because it _can't_ be supported. Having a database abstraction layer that works differently depending on which instance you run it with defeats the object of having an abstraction at all, and I believe we should fix this wherever we can.

Also this is not a Eloquent issue, its a Doctrine issue.

That's not correct. This isn't a Doctrine issue at all. It's caused by Laravel assuming that it can ask the for SQL for a set of changes, (e.g. change A and change B) and have the SQL returned be correct even though none of those changes have been applied. Currently Laravel does this:

• Generate SQL required for change A
• Generate SQL required for change B
• Run SQL for change A
• Run SQL for change B

This happens to work for MySQL, but not for SQLite. If Laravel asks for the SQL for change A, and then applies those before asking for the SQL for change B, then everything should work as expected, e.g.

• Generate SQL required for change A
• Run SQL for change A
• Generate SQL required for change B
• Run SQL for change B

For example, there's a proof-of-concept patch here that seems to resolve the issues I originally reported in this ticket:

https://github.com/leewillis77/framework/commit/ea20bc79295fd60be47a94e9365f8c0512d9713e

It's by no means a complete, finished PR. As the comments on #4179 suggest, the unit tests will need updating and verifying that this doesn't actually break any of them. It would also be useful to get some input on whether there's a solid reason that Laravel currently tries to do the changes the way it does; perhaps there's something I've missed?

If there's interest in resolving this (and I feel that there should be), then if someone can re-open this for the discussion, I'm happy to work towards turning the proof of concept into a full PR.

[sisve]

@leewillis77 You're welcome to create a finished PR for this. There's not much to discuss, the current functionality is broken; but it's _documented_ as broken, so we can quickly close related issues with "works as documented".

[rickshawhobo]

@leewillis77 I am interested in a solution. I just spent hours debugging this situation and I imagine other developer hours have been lost. The interesting thing is that I got no error and my migration appears to succeed and inserted a migration row, but did not apply the migration. At this point I'm ready to abandon sqlite altogether.

[leewillis77]

@rickshawhobo I don't think that's the same issue. The issue I reported here does report errors, and there are workarounds documented in this thread (https://github.com/laravel/framework/issues/16273#issuecomment-258555425).

If you have a different issue, I'd open a fresh issue for that providing as much information as you can.

[rickshawhobo]

Thanks @leewillis77 I don't know what it is but I am able to reproduce it.

• install fresh laravel
• change DB_CONNECTION=sqlite
• delete DB_DATABASE=homestead
• touch database/database.qlite

make these two migrations

    public function up()
    {
        Schema::create('test1', function (Blueprint $table) {
            $table->string('col1')->nullable();
            $table->string('col2')->nullable();
        });

    }

    public function up()
    {
        Schema::table('test1', function (Blueprint $table) {
            $table->integer('int1')->nullable();
            $table->dropColumn('col2');
        });
    }

• php artisan migrate -v
• sqlite3 database/database.sqlite
• .schema

...
CREATE TABLE test1 (col1 VARCHAR(255) DEFAULT NULL COLLATE BINARY);

• Observe that the col2 was dropped but int1 was not added
• Observe the migration row was inserted

select * from migrations;
...
3|2019_05_01_013800_test1|1
4|2019_05_01_014041_test2|1

[sisve]

@rickshawhobo This is another one of those "working as documented" cases, because the documentation says ...

Dropping or modifying multiple columns within a single migration while using a SQLite database is not supported.

A workaround is to split your migration into multiple calls to Schema::table, one for every column.

public function up()
{
    Schema::table('test1', function (Blueprint $table) {
        $table->integer('int1')->nullable();
    });

    Schema::table('test1', function (Blueprint $table) {
        $table->dropColumn('col2');
    });
}

[rickshawhobo]

I understand @sisve but I find it disheartening that the laravel community accepts this. It seems like it even used to throw an error and now doesn't. I find that very frustrating for new developers as well as veteran developers who don't memorize every line of the documentation.

[devcircus]

If you know of a guaranteed solution that fixes this and is consistent across implementations, try submitting a PR. This isn't something the community has taken lightly and ignored for years. It's been discussed many times but a fully working consistent solution hasn't been developed yet. That being the case, it's been documented so that when someone happens upon this and starts reading the docs, they'll see what is happening.

[leewillis77]

Indeed - there's a proposed patch further up this thread, so if you have the time to work on it and explore the issues that might be a good place to start. Unfortunately I don't have any time to progress it currently.