Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[5.3] Notifications id column type #15719

Merged
merged 1 commit into from
Oct 3, 2016
Merged

[5.3] Notifications id column type #15719

merged 1 commit into from
Oct 3, 2016

Conversation

antonkomarev
Copy link
Contributor

@antonkomarev antonkomarev commented Oct 2, 2016

Because notifications id column using UUID as primary key and it's length is always fixed wouldn't it be better to use CHAR(36) type instead of VARCHAR(255)?

Updated. Using UUID type now.

@@ -13,7 +13,7 @@ class CreateNotificationsTable extends Migration
public function up()
{
Schema::create('notifications', function (Blueprint $table) {
$table->string('id')->primary();
$table->char('id', 36)->primary();
$table->string('type');
Copy link
Contributor

@ryanwinchester ryanwinchester Oct 3, 2016

Choose a reason for hiding this comment

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

Or couldn't you do:

$table->uuid('id')->primary();

Copy link
Contributor Author

@antonkomarev antonkomarev Oct 3, 2016

Choose a reason for hiding this comment

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

@ryanwinchester that's will be even better because it will use Postgres & MsSql native types then.

@antonkomarev
Copy link
Contributor Author

Updated commit with @ryanwinchester proposal. Using UUID type for notification id column.

@taylorotwell taylorotwell merged commit 660849b into laravel:5.3 Oct 3, 2016
@antonkomarev antonkomarev deleted the hotfix/notifications-uuid-column-type branch October 3, 2016 13:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants