Framework: [5.3.13] artisan migration issue

[rudkov]

• Laravel Version: 5.3.13
• PHP Version: 7.0.11 (cli) (built: Sep 21 2016 22:56:53) ( NTS )
• Database Driver & Version: Server version: 5.5.42 - Source distribution, Database client version: libmysql - mysqlnd 5.0.12-dev - 20150407
  

  Description:

When I try to run migration (create table and add a composite key) via artisan it fails: it logs an error to the console, creates a table (without this key) and does not create any entry in the migrations table. So it is impossible to rollback.

Error log:

[Illuminate\Database\QueryException]
SQLSTATE[42000]: Syntax error or access violation: 1068 Multiple primary key defined (SQL: alter table car_models add primary key car_models_name_year_from_primar y(name, year_from))

[PDOException]
SQLSTATE[42000]: Syntax error or access violation: 1068 Multiple primary key defined

I believe artisan should rollback the migration on failure by default. Fix me if I'm wrong, please.

Steps To Reproduce:

The migration php file:

    <?php

    use Illuminate\Support\Facades\Schema;
    use Illuminate\Database\Schema\Blueprint;
    use Illuminate\Database\Migrations\Migration;

    class CreateCarModelsTable extends Migration
    {
        public function up()
        {
            Schema::create('car_models', function (Blueprint $table) {
                $table->increments('id');
                $table->string('name');
                $table->smallInteger('year_from');
                $table->smallInteger('year_to');
                $table->softDeletes();
                $table->timestamps();

                $table->primary(array('name', 'year_from', 'year_to'));
            });
        }

        public function down()
        {
            Schema::dropIfExists('car_models');
        }
    }

The console command:
php artisan migrate

[dwightwatson]

You can manually delete the table from your database and then you can migrate again. Laravel inserts the record into the migrations table only after the migration is complete.

The issue here is that you are using increments which creates an incrementing primary integer key, and then you try to create another primary key. You can only have one primary key. I suspect you want to use index instead of primary.

[rudkov]

@dwightwatson I want to use unique instead of primary, I have already fixed it. Thanks nevertheless.

My main point is that we cannot roll back a migration containing errors, so we have to reverse everything manually (in my case it was a table drop). If it is a default and expected behaviour—so be it and let's close this issue. I just thought we could make L5 a little bit friendly.

[dwightwatson]

Yeah, I have wondered if perhaps migrations could be run in a transaction, so that if they fail they can rollback as if nothing ever happened. However, I'm not sure if you can perform schema changes in a transaction or not.

[rudkov]

So am I.
And maybe everything could be run in a try-catch.

[dreferreira]

The whole point of migrate rollback is to be able to manage the database without manual interaction with your DBMS. If a migration has an error, in most cases you should be able to rollback. There have been times, where I've made a migration with a syntax error and have had to do manual interactions too, so in the end the system is not 100% perfect... maybe I'll dive deeper into it later and pose a fix for this.

In this case, without doing too much investigation, I'd recommend the following:
Pull in this/these package(s):
composer require --dev --no-update "xethron/migrations-generator:dev-l5"

composer require --dev --no-update "way/generators:dev-feature/laravel-five-stable"

composer config repositories.repo-name git "[email protected]:jamisonvalenta/Laravel-4-Generators.git"

composer update

The packages will allow you to pull in and generate the migration files directly from your database. After pulling in those packages, manually create the table in your database and apply the composite primary key to your relevant columns, or fields. Then run:

php artisan migrate:generate

This will generate the migration files of the tables and columns that already exist in your database with the syntax that is expected by laravel's migration system for tables with composite primary keys!

See Below Reference for further documentation.
Reference:
https://github.com/Xethron/migrations-generator

[rudkov]

@dreferreira sorry, but have you read the entire post?

[dreferreira]

Yeah was a little late on that, sorry hahaha

[riyuk]

See this Issue about Transactions in Migrations:
https://github.com/laravel/framework/issues/302

[GrahamCampbell]

Right, but we can't have it both ways? Which is least worst?

[dwightwatson]

I suppose one benefit of not having transactions is that you know what part of the migration failed, and you can predict what state the database has been left in to roll it back manually (all of the migration before the failing line of code).

With transactions part of the migration will be reverted for you but some of it might not be (as Taylor mentioned in the other thread) so you may need to dig deeper to fully manually revert a failed migration.

In this way, I would think that no transactions makes a failure a little more predictable and is "less worse".

[petar-dambovaliev]

@dreferreira Laravel provides a layer to manage some of your DB needs, it doesn't replace the DBMS.
On any project you will be required to write some SQL and use DBMS, laravel cannot solve everything for you.
Notice that even on the Laravel website on the homestead guide there are links to DBMS.

[helmut]

Perhaps rather than automatically running the down method on failure (as suggested in the other thread) perhaps we could log the migration as having been run at the start rather than the end of the process. So if it fails halfway through it has technically still run and the developer can rollback which will (for most use cases) restore the database to how it should be. At the moment when the migration fails, we have a database half migrated and no log of it. With this change you end up with a database half migrated and a record in the database of the last migration. That would be a big change no doubt.

Or you could wrap this up into a new console method eg 'migrate:undo' which gets the next available migration that hasn't been run (which would likely be the one that caused an exception) and call the down method. A bit convoluted but would not be a breaking change. And only those that find it necessary can use it.

Or have a way of simply triggering the down method of any migration. That way you could rollback without having to manually go into the db.

Just throwing these ideas out there, i'm sure there is something I'm not considering. And the status quo is probably the safest course of action here...

[ShaunaGordon]

Triggering the down method, regardless of how its done has the weakness of trying to "undo" things that haven't been done to begin with. Calling $table->dropColumn on a column that doesn't exist results in an error.

This part's a straightforward solution, though -- add an IfExists assumption to the drop* methods (or an option to turn on such assumption; at the very least, it'd be nice to have some type of IfExists check that we could add to all of our migration statements).

[antonkomarev]

@ShaunaGordon you can use this package: https://github.com/spatie/laravel-migrate-fresh

It will add php artisan migrate:fresh command.

[ShaunaGordon]

@a-komarev That doesn't address the issue being discussed here. Generally speaking, if you want to roll back a single migration, you don't want to nuke the entire database.

[JoelSanchez]

Apart from making the migrations more granular (not really scalable) and wrapping the migrations in transactions (doesn't work for every change you might make), I proposed changing the way we define the migrations, here #302 .
What do you think? Is it even worse?

[ShaunaGordon]

@JoelSanchez It's an interesting idea, but I don't think it fits within the Laravel ideals. Making them closures makes it unclear that the second half is equivalent to down(). Aside from that, it's essentially serving the purpose of making the migrations more granular, without actually putting them into separate files.

That said, I agree that making migrations more granular isn't very scalable, nor does it make it particularly clear from the code what the final schema looks like (that's one of the big things I don't like about migrations in general, but that's not limited to Laravel). It is possible that rethinking how migrations are laid out, entirely, might be a viable thing to consider.

[jonnylink]

Looking at how many people are confused by failed migrations, it seems this issue isn't resolved. There might not be a magic bullet for automatically fixing the (now partially migrated) database, but it seems like maybe there's an opportunity to provide better error messaging so a dev knows what parts of the migration were successful.