Follow-up to #1314214: MySQL driver does not support full UTF-8 (emojis, asian symbols, mathematical symbols)
Problem/Motivation
LanguageItem
fields are hardcoded to be regular varchar
columns in LanguageItem::schema()
and also have an is_ascii
flag set that does nothing. This flag was supposed to (and did in an earlier revision of the patch) set langcode fields to be ascii.
So currently some langcode fields are ASCII-only (those defined through hook_schema()
, ie node_access.langcode), and some aren't (those created through LanguageItem
, ie. node.langcode)
Proposed resolution
Convert all langcode fields to ASCII for consistency.
We want to take out is_ascii
in the schema definition, as it actually does nothing, and set the column type to varchar_ascii instead.
Remaining tasks
Review patch
User interface changes
N/A
API changes
N/A
Data model changes
N/A
Beta phase evaluation
Issue category | Bug because we changed is_ascii to varchar_ascii in #1923406: Use ASCII character set on alphanumeric fields so we can index all 255 characters and forgot to apply this change to langcode fields |
---|---|
Issue priority | Normal |
Prioritized changes | The main goal of this issue is documentation/DX improvement, as developers may look at core files for documentation on how to implement another field type. is_ascii is a field setting, and varchar_ascii is a schema thing. This is confusing enough as it is, and the current code only confuses things even more. |
Disruption | No, just an update hook |
Comment | File | Size | Author |
---|---|---|---|
#71 | 2549821-59_0.patch | 7.23 KB | stefan.r |
#67 | interdiff-58-59.txt | 385 bytes | stefan.r |
#66 | 2549821-59.patch | 7.23 KB | stefan.r |
#60 | 2549821-58.patch | 7.23 KB | stefan.r |
#60 | interdiff-57-58.txt | 2.56 KB | stefan.r |
Comments
Comment #2
stefan.r CreditAttribution: stefan.r commentedComment #3
stefan.r CreditAttribution: stefan.r commentedComment #4
stefan.r CreditAttribution: stefan.r commentedComment #5
jhedstromWon't this result in triggered entity schema changes for columns with data since the stored schema will contain
is_ascii => TRUE
?Comment #6
stefan.r CreditAttribution: stefan.r commentedif it does, it shouldn't, as is_ascii is not a valid schema property
Comment #7
jhedstromIt may not be valid, but it is stored:
I'm not sure why this doesn't break our existing upgrade path tests since the starting database contains data in
users_field_data
.Comment #8
jhedstromI'm going to dig into #7 a bit.
Comment #9
BerdirWhat i suggested in IRC is to just go through the stored schema and clean it up by hand.
Comment #10
jhedstromThis quick change to an upgrade test does show that this change requires entity updates to be applied. The second runUpdates call was necessary to see the actual exception that gets thrown for tables with data in them. Cleaning the schema by hand may work as suggested in #9.
Comment #11
jhedstromComment #13
stefan.r CreditAttribution: stefan.r commentedHere's an update hook that removes the is_ascii key from the stored schemas.
Comment #14
stefan.r CreditAttribution: stefan.r commentedWhile this patch would still be an improvement, I'm thinking maybe we may as well fix this completely (by changing the mysql charset on LanguageItem fields) instead of making it go from completely broken to half-broken?
Comment #16
jhedstromComplete fix makes sense to me since. This should be possible using an approach similar to the update hook in #13.
Comment #17
stefan.r CreditAttribution: stefan.r commentedComment #19
jhedstromThe update hook is looking good. I'm going to dig into why the test is failing, and move it to a dedicated test class.
Comment #20
jhedstromThis test actually looks for the change we're making, and moves it to a dedicated test case.
Comment #21
jhedstromOops, this removes a lingering debug.
Comment #22
stefan.r CreditAttribution: stefan.r commentedTest looks great!
This is fine, varchar_ascii only works on MySQL :)
Comment #23
jhedstromStill good to document though so folks know where to go if they are looking at the code and wondering why it's mysql-only.
Comment #24
stefan.r CreditAttribution: stefan.r commentedComment #25
stefan.r CreditAttribution: stefan.r commentedComment #26
plachOverall, this looks great. Only one (minor) complaint:
Can't we just hard-code the schema definition here? It should be more reliable.
FYI #2542748: Automatic entity updates can fail when there is existing content, leaving the site's schema in an unpredictable state is adding a check so that only relevant field schema properties actually trigger a mismatch between code definitions and state ones.
Comment #27
stefan.r CreditAttribution: stefan.r commented@plach like this?
Nice that #2542748: Automatic entity updates can fail when there is existing content, leaving the site's schema in an unpredictable state would simplify this, do we postpone this issue on that or at this point it doesn't matter?
Comment #28
stefan.r CreditAttribution: stefan.r commentedTechnically if we hardcode the schema like this #27 should check for the length of 12 as well -- in the extreme edge case same mistake in a contrib/custom module implementing their own field and is upgrading from an older beta, we might accidently reduce the length to 12 otherwise.
Comment #29
plachYeah, good point, let's go back to #21.
Comment #31
stefan.r CreditAttribution: stefan.r commentedBack to #21
Comment #32
stefan.r CreditAttribution: stefan.r commentedComment #33
plachThanks!
Comment #35
stefan.r CreditAttribution: stefan.r commentedreroll and back to RTBC
Comment #37
stefan.r CreditAttribution: stefan.r commentedDumps are now set in setDatabaseDumpFiles()
Comment #38
stefan.r CreditAttribution: stefan.r commentedTest went from red to green, back to RTBC considering the interdiff
Comment #40
stefan.r CreditAttribution: stefan.r commentedreroll
Comment #42
stefan.r CreditAttribution: stefan.r commentedThat included a file from another patch :/
Comment #43
plachComment #44
alexpottNeeds tests. All updates should have tests and be run against all DBs.
Comment #45
alexpottAnd obviously we need to check with and without data.
Ah I see an mysql only update... hmmm. Still needs testing.
Comment #46
stefan.r CreditAttribution: stefan.r commentedThis actually already had tests :/
Comment #47
stefan.r CreditAttribution: stefan.r commentedCompared to patch previously rtbced by @plach, this has the test again and is green now so can probably go back
Comment #48
stefan.r CreditAttribution: stefan.r commentedMissed this bit. This has no filled test here yet, I'll add one
Comment #49
stefan.r CreditAttribution: stefan.r commentedComment #53
stefan.r CreditAttribution: stefan.r commentedComment #54
stefan.r CreditAttribution: stefan.r commentedComment #57
stefan.r CreditAttribution: stefan.r commentedComment #58
plachLooks good, thanks
Comment #59
alexpottoops
Comment #60
stefan.r CreditAttribution: stefan.r commentedComment #61
plachThanks
Comment #65
stefan.r CreditAttribution: stefan.r commentedis taken...
Comment #66
stefan.r CreditAttribution: stefan.r commentedComment #67
stefan.r CreditAttribution: stefan.r commentedComment #69
stefan.r CreditAttribution: stefan.r commentedseemingly unrelated fails on postgres/sqlite
Comment #70
plachTry to reupload the patch
Comment #71
stefan.r CreditAttribution: stefan.r commentedReuploading the patch. Why though, it was green?
Comment #72
plachLooks good now
Comment #73
alexpottCommitted e8dfde4 and pushed to 8.0.x. Thanks!
Thanks for adding the beta evaluation to the issue summary.