cviebrock / sequel-pro-laravel-export

A Sequel Pro / Sequel Ace bundle to generate Laravel migration files from existing tables.
MIT License
921 stars 55 forks source link

Foreign keys output as simple indexes #3

Closed tobz-nz closed 7 years ago

tobz-nz commented 7 years ago

Great work on this bundle, This will be super useful for may people I think.

Given this table:

CREATE TABLE `store_user` (
  `id` int(10) unsigned NOT NULL AUTO_INCREMENT,
  `user_id` int(10) unsigned NOT NULL,
  `store_id` int(10) unsigned NOT NULL,
  `created_at` timestamp NOT NULL DEFAULT '0000-00-00 00:00:00',
  `updated_at` timestamp NOT NULL DEFAULT '0000-00-00 00:00:00',
  PRIMARY KEY (`id`),
  KEY `store_user_user_id_foreign` (`user_id`),
  KEY `store_user_store_id_foreign` (`store_id`),
  CONSTRAINT `store_user_store_id_foreign` FOREIGN KEY (`store_id`) REFERENCES `store` (`id`),
  CONSTRAINT `store_user_user_id_foreign` FOREIGN KEY (`user_id`) REFERENCES `user` (`id`)
) ENGINE=InnoDB AUTO_INCREMENT=104 DEFAULT CHARSET=utf8 COLLATE=utf8_unicode_ci;

This is the migration generated:

<?php

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

/**
 * Migration auto-generated by LaravelMigration.spBundle
 * @see https://github.com/cviebrock/sequel-pro-laravel-export
 */
class CreateStoreUserTable extends Migration
{
    /**
     * Run the migrations.
     *
     * @return void
     */
    public function up()
    {
        Schema::create('store_user', function (Blueprint $table) {
            $table->increments('id');
            $table->integer('user_id');
            $table->integer('store_id');
            $table->timestamps();

            $table->index('user_id', 'store_user_user_id_foreign');
            $table->index('store_id', 'store_user_store_id_foreign');

        });
    }

    /**
     * Reverse the migrations.
     *
     * @return void
     */
    public function down()
    {
        Schema::dropIfExists('store_user');
    }
}

As you can see, the foreign keys are generated as indexes. Not quite right. The user_id & store_id need to be foreign keys, and those keys need to be removed in down().

Great work so far though :) Sorry I have no PR to fix this.

cviebrock commented 7 years ago

I just pushed some changes to master that attempt to rebuild foreign keys. Do you have time to take a look?

Also ... do you have the original Laravel migration that created your table structure, for comparison to what the bundle generates?

tobz-nz commented 7 years ago

Ah, looks like that table was done over 2 migrations.

<?php

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

class CreateStoreUserTable extends Migration {

    /**
     * Run the migrations.
     *
     * @return void
     */
    public function up()
    {
        Schema::create('store_user', function(Blueprint $table)
        {
            $table->increments('id');
            $table->integer('user_id')->unsigned()->index('store_user_user_id_foreign');
            $table->integer('store_id')->unsigned()->index('store_user_store_id_foreign');
            $table->timestamps();
        });
    }

    /**
     * Reverse the migrations.
     *
     * @return void
     */
    public function down()
    {
        Schema::drop('store_user');
    }

}

then:

<?php

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

class AddForeignKeysToStoreUserTable extends Migration {

    /**
     * Run the migrations.
     *
     * @return void
     */
    public function up()
    {
        Schema::table('store_user', function(Blueprint $table)
        {
            $table->foreign('store_id')->references('id')->on('store')->onUpdate('RESTRICT')->onDelete('RESTRICT');
            $table->foreign('user_id')->references('id')->on('user')->onUpdate('RESTRICT')->onDelete('RESTRICT');
        });
    }

    /**
     * Reverse the migrations.
     *
     * @return void
     */
    public function down()
    {
        Schema::table('store_user', function(Blueprint $table)
        {
            $table->dropForeign('store_user_store_id_foreign');
            $table->dropForeign('store_user_user_id_foreign');
        });
    }

}

...Actually, I don't think it was actually created by these migrations - they where generated after the fact (sigh, don't ask) with another composer package - can't remember which one.

tobz-nz commented 7 years ago

The new update looks like its generating foreign keys correctly. note: I haven't actually run it, but looks good to me.

<?php

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

/**
 * Migration auto-generated by LaravelMigration.spBundle
 * @see https://github.com/cviebrock/sequel-pro-laravel-export
 */
class CreateStoreUserTable extends Migration
{
    /**
     * Run the migrations.
     *
     * @return void
     */
    public function up()
    {
        Schema::create('store_user', function (Blueprint $table) {
            $table->unsignedIncrements('id');
            $table->unsignedInteger('user_id');
            $table->unsignedInteger('store_id');
            $table->timestamps();

            $table->foreign('store_id', 'store_user_store_id_foreign')->references('id')->on('store')->onDelete('RESTRICT
')->onUpdate('RESTRICT');
            $table->foreign('user_id', 'store_user_user_id_foreign')->references('id')->on('user')->onDelete('RESTRICT
')->onUpdate('RESTRICT');

        });
    }

    /**
     * Reverse the migrations.
     *
     * @return void
     */
    public function down()
    {
        Schema::dropIfExists('store_user');
    }
}
tobz-nz commented 7 years ago

Also, didn't even know you could prefix unsigned to integer methods. eg: unsignedInteger(). TIL.

cviebrock commented 7 years ago

Cool. Thanks for testing this out!