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

converted columns for emoji support #169

Merged
merged 19 commits into from
May 25, 2018

Conversation

adrum
Copy link

@adrum adrum commented Jun 9, 2017

Fixes #168

This was a little bit involved and touches every text based column, table and schema.

Links to help me along the way are below:

@@ -17,6 +18,9 @@ public function boot()
View::composer(
'partials.components.country-select','App\Http\ViewComposers\CountrySelectViewComposer'
);

Schema::defaultStringLength(191);
Copy link
Member

Choose a reason for hiding this comment

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

Why is that? There are some places in the code where we wanted to limit the number of characters in an input field to 255 characters. If we put this, does that mean that the default VARCHAR is limited to 191?

Copy link

@degan6 degan6 Jun 17, 2017

Choose a reason for hiding this comment

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

Its for backward compatibility for older versions of mysql. Very common problem when deploying.

https://laravel-news.com/laravel-5-4-key-too-long-error

Choose a reason for hiding this comment

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

Just looking through some PRs and noticed this one.

What would happen to the installs of Monica that have a varchar-field that have values of 192 characters or longer? Truncated data is not so fun.

If breaking changes is fine, just forcing users to enable innodb_large_prefix on the older MySQL-installs would be a much more elegant solution and wouldn't cause any data loss.

https://stackoverflow.com/questions/43153719/laravel-5-4-migrate-key-is-too-long-error/43162269#43162269

@dereuromark
Copy link

This looks useful and necessary, I once faced the same issue.
Why didn't you guys proceed here yet? Anything below 4byte risks data loss when saving it.

@CLAassistant
Copy link

CLAassistant commented Apr 2, 2018

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ asbiin
❌ adrum
You have signed the CLA already but the status is still pending? Let us recheck it.

@asbiin
Copy link
Member

asbiin commented May 24, 2018

OK this PR finally works for me.
I removed this (dangerous) Schema::defaultStringLength(191); line, because we don't want to risk on truncate datas.
mysql 5.7.7 or later don't need it. We can simply specify it needs this version of mysql or later.

@monicahq monicahq deleted a comment from codecov bot May 24, 2018
@asbiin asbiin merged commit 643efa1 into monicahq:master May 25, 2018
@github-actions
Copy link

This pull request has been automatically locked since there
has not been any recent activity after it was closed.
Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Emoji Support
7 participants