Foreign keys commented out in migration

Just noticed that foreign keys are commented out in migration. Can we uncomment after the install migration has finished???

class PermissionRolesTable extends Migration
{
    /**
     * {@inheritDoc}
     */
    public function up()
    {
        if (!$this->schema->hasTable('permission_roles')) {
            $this->schema->create('permission_roles', function (Blueprint $table) {
                $table->integer('permission_id')->unsigned();
                $table->integer('role_id')->unsigned();
                $table->timestamps();

                $table->engine = 'InnoDB';
                $table->collation = 'utf8_unicode_ci';
                $table->charset = 'utf8';
                $table->primary(['permission_id', 'role_id']);
                //$table->foreign('permission_id')->references('id')->on('permissions');
                //$table->foreign('role_id')->references('id')->on('roles');
                $table->index('permission_id');
                $table->index('role_id');
            });
        }
    }

Thanks in advance,

Mike

If the migration is ran, uncommenting this won’t change anything. The db Model (not the same thing as migrations!) will use the relation nonetheless.

This was commented because… I don’t remember. @alexweissman Some issue with a particular db driver?

You can actually use the git blame feature to investigate specific changes like this.

In this case, it looks like this was done in 4.0; see https://github.com/userfrosting/UserFrosting/blame/4.0/app/sprinkles/account/migrations/4.0.0-alpha.php. This leads us to this commit, where it looks like I added them as commented out lines :facepalm:

So, in this case blame doesn’t shed too much light on the situation. I believe this was because of the risk of foreign key constraint errors happening before we had migration dependencies implemented (4.1). But, now we do, so my guess is that it would probably be safe to uncomment these and fix this in the next hotfix.

Can you open an issue about this?

Actually, I’ve gone ahead and done this: https://github.com/userfrosting/UserFrosting/issues/833