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

dev/core#2146 - Long unicode contact names get truncated badly causing a crash #18862

Merged
merged 1 commit into from
Oct 27, 2020

Conversation

demeritcowboy
Copy link
Contributor

@demeritcowboy demeritcowboy commented Oct 27, 2020

Overview

https://lab.civicrm.org/dev/core/-/issues/2146

  1. Go to create a contact. It's slightly easier to see with Organization.
  2. Enter a pretty long name using unicode characters into the organization name field, around 128 characters.
  3. Click Save.
  4. DB Error: Unknown error.

Before

Error

After

No error. Names that are too long get truncated.

Technical Details

substr() isn't multibyte-aware. But also if you look a couple lines up, it already calls copyValues(), which in turn calls CRM_Utils_String::ellipsify() which is multibyte-aware. So the string is already truncated correctly before it tries to then mangle it.
We can't call ellipsify though because then that breaks the way it currently works for Individual.

Comments

Has test. Without the patch the last 3 org tests fail with something like: [nativecode=1366 ** Incorrect string value: '\xD1' for column civicrm_contact.sort_name at row 1]. Some of the individual tests fail for similar or incorrect string length truncation reasons.

@civibot
Copy link

civibot bot commented Oct 27, 2020

(Standard links)

@civibot civibot bot added the master label Oct 27, 2020
@demeritcowboy
Copy link
Contributor Author

D'oh the call to Individual::format() puts it back again if it's Individual. Will update...

@seamuslee001
Copy link
Contributor

@demeritcowboy test fails relate

@demeritcowboy
Copy link
Contributor Author

Ok yeah I guess that's something that would need concept approval, whether names should be silently truncated (current), or have some indication it's been truncated (the change), or should it flat out fail to create the contact (other). I was keeping it consistent with copyValues but that's obviously not what's currently tested.

Maybe keeping same as current is best for now.

Relatedly, I did notice api v3 and v4 enforce Org name length differently. v3 fails, v4 truncates.

@demeritcowboy
Copy link
Contributor Author

Have updated so it leaves it as silent truncation for individual, same as before patch, but without db failure for both individual and org for long unicode.

@seamuslee001
Copy link
Contributor

This looks fine to me now merging

@seamuslee001 seamuslee001 merged commit 9b91d1b into civicrm:master Oct 27, 2020
@demeritcowboy
Copy link
Contributor Author

Thanks!

@demeritcowboy demeritcowboy deleted the mb-substr branch October 27, 2020 22:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants