backdrop-contrib / d2b_migrate

GNU General Public License v2.0
5 stars 6 forks source link

Database conversion from utf8 to utf8mb4 fails #24

Open indigoxela opened 8 months ago

indigoxela commented 8 months ago

The inital database before migration is utf8mb4, but after the migration steps ran (successfully more or less), the database tables are utf8_general_ci instead of utf8mb4_general_ci.

Note that in settings(.local).php the variable is set to mb4:

$database_charset = 'utf8mb4';

And that's not true after migration, as I ended up with mixed charsets across tables.

indigoxela commented 8 months ago

The trick is to set the state.

state_set('database_utf8mb4_active', FALSE);

Then Backdrop will prompt to update the db tables, and that works smoothly. Not sure, when the best time for that state_set is.

indigoxela commented 7 months ago

Funny, that this issue didn't get any attention, yet. I'd call it a major bug. It's possible to fix that manually, but it requires a database query.

docwilmot commented 7 months ago

The trick is to set the state.

Is that on the Backdrop site after the upgrade completes you mean? Theres no state table in D7 so we'd need maybe to create a hook_update_N() implementation that runs during the upgrade.php run and sets state then. Do hook_update_N() implementations run on disabled modules (d2b will also be disabled during the upgrade run).

indigoxela commented 7 months ago

Is that on the Backdrop site after the upgrade completes you mean?

Yes, after the conversion. Then one would get prompted to run the procedure, which converts the db tables.

docwilmot commented 7 months ago

Still considering this. When D2B completely finishes, it wipes the Backdrop database, loads the D7 database, then it hands off to update.php. At that time the state table does not exist because we then are left with the native D7 database. D2B will also be disabled during and after the upgrade runs (since it doesnt exist in the D7 database). So D2B cannot call any code that sets state. One option is to directly add an entry for D2B in the D7 system table via a database INSERT with status 1 (enabled) before the database wipe, and write an d2b_update_N() implementation that sets database_utf8mb4_active during the upgrade.

Would it be OK to have D2B enabled post install? That might be helpful if we someday think of any post-install checks we'd like to run.

indigoxela commented 7 months ago

So D2B cannot call any code that sets state.

Are you sure? Shouldn't set_state be available after migration stuff ran? Direct db access should also work, of course. The query isn't difficult.

Would it be OK to have D2B enabled post install?

Sure. But note that if the B&M module wasn't installed on the D7 site, this would break, anyway. It's a dependency of this module.

docwilmot commented 6 months ago

May be a strange approach, but seems to work. Created a submodule with an install hook that sets the state then disables itself.

indigoxela commented 6 months ago

May be a strange approach, but seems to work. Created a submodule with an install hook that sets the state then disables itself.

This sounds too complicated to be right. :thinking:

state_get() has a fallback value, and initially, when starting a "traditional" upgrade run, this doesn't even exist in the database.

How about deleting that state at the beginning (or end) of the procedure? Simply db_delete and let Backdrop do the job? Didn't try it, but in theory... And that would be much simpler.

indigoxela commented 6 months ago

I simulated that on a fully installed Backdrop: deleted the row directly in the database. Flushed cashes, ... and Backdrop prompted me to run the conversion, which worked without any problem, although the tables already had the right collation.

So, I'd suggest to delete the row. Not sure, when. Obviously it's possible any time. :wink: And we don't need state_del() for that.

docwilmot commented 6 months ago

It wont work. Here are the D2B steps, to make things clear:

We could just have the hook_update_N code in D2B install file itself, but then if a user leaves D2B enabled and runs updates, could be an issue. So a hidden temp submodule that is just runs its updates and uninstalls itself seemed the best option.

docwilmot commented 6 months ago

Ha, after that long diatribe it seems I am very wrong: BAM doesnt DROP all the existing database tables it seems. It seems it keeps the state table intact. Your idea would work perfectly fine after all. Sorry for the noise, and thanks for making me think harder.