Add utf8 to utfmb8 conversion api command #15969
Conversation
|
(Standard links)
|
|
| } | ||
| $dsn = defined('CIVICRM_LOGGING_DSN') ? DB::parseDSN(CIVICRM_LOGGING_DSN) : DB::parseDSN(CIVICRM_DSN); | ||
| $logging_database = $dsn['database']; | ||
| $dao = CRM_Core_DAO::executeQuery("SHOW TABLE STATUS FROM `$logging_database` WHERE Engine <> 'MyISAM' AND Name LIKE 'log\_civicrm\_%'"); |
There was a problem hiding this comment.
Suggest whitelisting engines we know can handle utf8mb4?
There was a problem hiding this comment.
Hmm in practice people should only have INNODB or archive - anything else & they have done something weird & wonderful
There was a problem hiding this comment.
OK, so I suggest whitelist InnoDB, archive if that's the only thing this is supposed to work with, rather than <> MyISAM - I've heard quite afew people using weird and wonderful stuff!
There was a problem hiding this comment.
OK - we can revisit if people hit issues on trying it
There was a problem hiding this comment.
@eileenmcnaughton @artfulrobot sorry for chiming in late - we use TokuDB for log tables (via the alterLogTables hook), so those would not be caught by the query. Some earlier discussion for reference.
There was a problem hiding this comment.
@pfigel yes, I thought of you when Eileen said "wonderful" :-)
so do you agree we should have a whitelist, and should that include TokuDB?
There was a problem hiding this comment.
@artfulrobot I'd probably lean towards blacklisting known-bad engines. There are lots of weird engines out there, and if anyone wants to use them, I feel like testing compatibility should be up to them. If we add TokuDB here, it feels like that's Civi saying "we support TokuDB" - not sure if that's an accurate assessment.
There's lots of uncertainty there for me though, I'm not sure how common it is for other engines to not support utf8mb4 properly.
There was a problem hiding this comment.
After considering @pfigel's comment I've switched back to the blacklist.
We should note that this is a function we'd expect people with some some knowledge to use - especially in instances where they have done something weird and wonderful.
I agree it's odd that it's exposed in the api explorer. However it's not the only System api in there & we haven't seen support requests on the others. A casual clicker should be put off as the entity is 'System', it has a 'weird name' & I would expect few people would have thee courage to click confirm will-nilly on this
|
@colemanw there is an issue pointed out here that on one hand we create apis to do various tasks. On the other some of them are expected only to be used by somewhat experienced / knowledgeable users. This is not the first / only so I'm not sure it should be blocking but perhaps we need a convention to suppress from the api things like this & a handful of others? |
eileenmcnaughton
force-pushed
the
utfmb8
branch
from
5534e8f to
a8a867e
Compare
This extracts from civicrm#13633 the portion that deals with the conversion and switches it from an upgrade script (which is not agreed at this stage as there are concerns about imposing this change) to an api command (which can be added with relatively little discussion as it only affects those who choose to use it). We don't expect this command to outlive apiv3 so adding there & not creating insta-tech-debt on apiv4. In order to add a unit test I had to alter to support reverting - doesn't seem like a bad thing all round. As an aside - I like the way it changes the DB level charset & collation. I think I've seen these fail to be set in the wild
eileenmcnaughton
force-pushed
the
utfmb8
branch
from
a8a867e to
a0a5d4d
Compare
|
I'm going to merge this as is, APIv3 Explorer can already do some relatively destructive stuff anyway such as disabling logging etc turning on and off multilingual so |
Overview
Adds an api to convert the db from utf8 to utf8mb4 - this is intended to help early adopters of the utf8mb4 standard with CiviCRM. utf8mb4 supports emjois. This is work done by @mfb & all discussion is on #13633 . This change simply brings a subset of that change into what should be an easily mergeable form.
Before
No easy way to convert a DB from utf8 to utf8MB4
After
civicrm_api3('System', 'utf8conversion', []);will do the conversion (supports the is_revert param to undo, mostly for testing purposes.Technical Details
This extracts from #13633 the portion that deals with the conversion
and switches it from an upgrade script (which is not agreed at this stage as there are concerns about
imposing this change) to an api command (which can be added with relatively little discussion as
it only affects those who choose to use it).
We don't expect this command to outlive apiv3 so adding there & not creating insta-tech-debt on
apiv4.
In order to add a unit test I had to alter to support reverting - doesn't seem like a bad thing all round.
As an aside - I like the way it changes the DB level charset & collation. I think I've seen these fail to be set
in the wild
Comments