Skip to content

Commit

Permalink
Remove foreign keys
Browse files Browse the repository at this point in the history
  • Loading branch information
driesvints committed Jan 29, 2020
1 parent 80721e3 commit 20e9b66
Show file tree
Hide file tree
Showing 4 changed files with 0 additions and 8 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,6 @@ public function up()
$table->text('scopes')->nullable();
$table->boolean('revoked');
$table->dateTime('expires_at')->nullable();

$table->foreign('client_id')->references('id')->on('oauth_clients')->onDelete('cascade');
});
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,6 @@ public function up()
$table->boolean('revoked');
$table->timestamps();
$table->dateTime('expires_at')->nullable();

$table->foreign('client_id')->references('id')->on('oauth_clients')->onDelete('cascade');
});
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,6 @@ public function up()
$table->string('access_token_id', 100);
$table->boolean('revoked');
$table->dateTime('expires_at')->nullable();

$table->foreign('access_token_id')->references('id')->on('oauth_access_tokens')->onDelete('cascade');
});
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,6 @@ public function up()
$table->bigIncrements('id');
$table->unsignedBigInteger('client_id');
$table->timestamps();

$table->foreign('client_id')->references('id')->on('oauth_clients')->onDelete('cascade');
});
}

Expand Down

8 comments on commit 20e9b66

@robertdeboer
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are foreign key relations going to be added back?
IMHO foreign keys serves as an important data integration enforcement check for relational databases

@driesvints
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, you can do this in your own app with the exported migrations if you want.

@lucacri
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason why they were removed?

@driesvints
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@robertdeboer
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I can see the issue they were causing, I don't think flat out removing them is necessarily the best. It's kinda like throwing out the good with the bad. As was mentioned foreign keys could of been added as a final migration step after all tables were created.

While a developer can modify the migration scripts after installing passport, each time there is an update they have to ensure there is nothing overridden or any table changes do not require alternation of the foreign key implementation. If they are willing to go that route then one could argue that is the risk they take but other would argue they shouldn't have to do this either.

@driesvints
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@robertdeboer we won't be reconsidering this, sorry.

@lucacri
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So instead of reordering the migrations to happen in the correct order, you'd rather remove the foreign keys all together? Specially in a "security related" package like passport, it sounds dangerous to do so for any new install. I strongly suggest to reconsider this

@brunogaspar
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The migrations shouldn't be reordered, it should've been a new migration instead.

As an example, you don't update a migration that has previously run on production, you add a new one, same should've happened here as well.

But either way, it's a bad move by removing and not doing it properly, seems lazy work.

Please sign in to comment.