Fix collision and performance issue #213
Conversation
|
@ash-jc-allen This one needs your review 😊. All tests are passing. I don't think I need to add any extra tests. |
|
Hey @hari! Apologies for the delay in getting this merged (it's been a busy few weeks!). Huge thank you 😄 |
|
Hey @ash-jc-allen, did you see my comment above? Came across this same issue with another package a while ago and their migrations threw an error for me because this particular collation doesn't exist on Postgres, which is case-sensitive by default. |
|
Hey @boris-glumpler! I can't see any comments from you on this page, sorry. This is all I see:
I wonder if the comment might be part of a "review" that's not been submitted yet, maybe? Thanks for pointing this out to me. I've not got any experience with using Postgres, so I wasn't aware of that. I'll make sure to update the readme to mention that you might want to remove this part of the migration if you're using Postgres 🙂 |
| @@ -16,7 +16,7 @@ public function up() | |||
| Schema::create('short_urls', function (Blueprint $table) { | |||
| $table->bigIncrements('id'); | |||
| $table->text('destination_url'); | |||
| $table->string('url_key')->unique(); | |||
| $table->string('url_key')->unique()->collation('utf8mb4_bin'); | |||
There was a problem hiding this comment.
I think utf8mb4_bin is specific to MySQL and will fail on Postgres.
There was a problem hiding this comment.
This should work:
$table->string('url_key')->unique()->when(
Schema::getConnection()->getConfig('driver') === 'mysql',
function (Blueprint $column) {
$column->collation('utf8mb4_bin');
}
);
There was a problem hiding this comment.
Nice one, thanks for this suggestion! I can't quite make my mind up between whether we should automatically handle this in the migration, or just mention it in the docs so people can make the change themselves. I guess there are pros and cons to either side 🤔
What's your opinion on this? 🙂
There was a problem hiding this comment.
@boris-glumpler thank you for the information. I think we should do both - update the doc and add the conditional logic @ash-jc-allen
There was a problem hiding this comment.
Not sure there's a downside to adding the conditional. If it bothers people, they can change it. If not, then no harm done.
|
Duh... You should be able to see it now ;) |
Closes #210