The function drupal_get_schema() in the convertTables method does not consider all tables (e.g. from deactivated modules, migrate tables etc.)
Maybe use the show tables query instead?
$schema = db_query("SHOW TABLES")->fetchAllKeyed();
$table_names = array_keys($schema);
Or optional add an option to convert all tables.
| Comment | File | Size | Author |
|---|---|---|---|
| #6 | utf8mb4_convert-2786963-6.patch | 1.1 KB | quicksketch |
| #4 | utf8mb4_convert-2786963.patch | 1 KB | quicksketch |
Comments
Comment #2
stefan.r commentedI'd rather keep the schema thing as a default for now, but I'm willing to look at a patch that converts all tables using a command line option (I guess for the prefix defined in settings.php only?)
Comment #3
quicksketchI also think that getting tables from disabled modules would be a good thing. As the operation is non-destructive, as a developer I'd rather see all my tables converted rather than only the ones that are in use currently. As later when I turn back on whatever module the status report isn't going to notify me that the table hasn't been converted yet.
Here's another version that uses the table prefix:
Comment #4
quicksketchHere's a proof of concept. I found one downside with this approach is that Drupal allows per-table prefixes, which could be a lot trickier to work out than the entire database table prefix.
Comment #5
quicksketchComment #6
quicksketchIt looks like we should skip the tableExists() check because we just queried for the table list, so of course it is going to exist. The tableExists() method also will re-prefix the table, but the table is already the "real" name in the database (with any prefixes). So this check actually may prevent tables from being upgraded if they are prefixed.
Comment #7
stefan.r commentedShouldn't this rather be opt-in?
Comment #8
quicksketchHm, okay more problems. The query to alter the table itself uses the squiggle brackets, which would add the prefix also. If we used this approach we'd need to remove the brackets from all queries in convertTable() and convertTableFields(). This would require more extensive changes yet.
As utf8mb4 encoding is backwards-compatible, I would think that converting unknown tables that happen to be in the same database as Drupal would be better than missing the tables of disabled modules.
But yeah, perhaps making this an option in the Drush command would be preferable. This still needs work anyway.
Comment #9
stefan.r commentedHm, maybe if we check for the tables that are not in the schema being utf8/utf8_general_ci based prior to conversion? Or you reckon that's being too careful?
Comment #10
quicksketchI think that would be a good compromise. Convert the table if:
- It is in the Drupal schema (an enabled module)
- OR it is utf8/utf8_general_ci, and therefore backwards-compatible with the upgrade anyway.
Seems like that would cover just about all situations with almost no downside.
Comment #11
quicksketchWhat about only converting tables if they are utf8/utf8_general_ci in all situations (even those in the Drupal schema)? That would prevent accidental conversion of tables that aren't UTF8 but are in the Drupal database. I'm not sure how frequently this occurs. I've never seen a module or even a site that intentionally used a different encoding for its table. So it seems that a blanket check before any table operation might be acceptable?
Comment #12
stefan.r commentedYeah, I think that should be fine :)
Comment #13
slefevre1 commentedI've run and tested this, and it works as advertised.
Comment #14
mvcThis would have worked for us but we solved this here by adding an option --include-tables in #3088343: Multiple patches: include additional tables, use compressed row format, use utf8mb4_bin only where needed.