system_maintenance migrations uses incorrect maintenance mode variable in Drupal 7 migrations

The d7 fixture has a 'site_offline_message' and a 'maintenance_mode_message'. I asked larowlan about this and he thought the site_offline_message is D6 only. And he confirmed this later and posted this in slack

@quietone confirming that site_offline_message is a D6 thing, check update_fix_d7_requirements in D7, this runs at the start of updating from Drupal 6 to Drupal 7 and does this:

// Rename 'site_offline_message' variable to 'maintenance_mode_message'
    // and 'site_offline' variable to 'maintenance_mode'.
    // Old variable is removed in update for system.module, see
    // system_update_7072().
    if ($message = variable_get('site_offline_message', NULL)) {
      variable_set('maintenance_mode_message', $message);

So, ideally we need to remove system_maintenance.yml and create a d6_system_maintenance.yml and a d7_system_maintenance.yml but we don't yet have a way to deprecate migration ymls.

So a d6 and d7 version of the system_maintenance migration is needed. This patch does that but there is still the problem of deprecating system_maintenance.yml.

We need to explicitly test that the message is migrated.

These messages are already tested in d6/MigrateSystemConfigurationTest and d7/MigrateSystemConfigurationTest. The text of the messages didn't not change so no change was needed to the tests.

My mistake. So the migration was wrong, but the fixture was also wrong, and unfortunatly, in this business two wrongs can definitely make a right... :-(

No real way to add a failing test in this case, as we need to assume the fixture is right.'

Thanks for pointing that out. RTBC

Updated the IS to explain why the test didn't change.

At the moment this patch removes core/modules/system/migrations/system_maintenance.yml - can we actually do that and preserve BC? As far as I know most people use migrate plus which copies such templates to config therefore won't be affected.

I looked at #3039240: Create a way to declare a plugin as deprecated and I was wondering where the discussion of the use of deprecating migrations is?

Could we add a test where we run a migration with the template, then remove it and see what happens when we try to rerun the migration again? Could be manual or automated testing.

Would renaming the yml files run afoul of the BC standard and require waiting for #3039240: Create a way to declare a plugin as deprecated?

Technically the files can be renamed, it's the plugin ids that we can't remove. If someone is expecting a migration plugin_id to exist, and it disappears then that's a bc break. #3039240: Create a way to declare a plugin as deprecated is an attempt to let us find a way to remove them during major version updates.

For now, we do need to preserve the system_maintenance migration somehow or postpone on #3039240: Create a way to declare a plugin as deprecated, but I think we can refactor, given that 3039240 would require us to keep it in a deprecated state anyway. I'll put this on my task list.

Refactoring so the deprecation can be avoided. This works if 'site_offline_message' and 'maintenance_mode_message' are mutually exclusive. If they both exist in a database then 'site_offline_message' (from Drupal 6) will be used.

Ready for review.

I like it. Not sure about the need for explicit test coverage or not.

I was hoping that wouldn't be necessary but since you asked I have altered the existing test. What this does is add the Drupal 6 variable name to the Drupal 7 text fixture, thus verifying that if both exist in a Drupal 7 site for some reason then the correct message is migrated. I realize that this patch removes the d6 variable from the d7 fixture and the test adds it back in. That keeps the d7 fixture 'correct' and makes it easier to see what is being tested. I doubt that a d6 site would have a d7 variable name so only the d7 test is modified. I think this is sufficient and avoids creating a new test.

Oh, I changed the order of the variables in the callback so that the d7 one is the default. And then did so in the source plugin so they are in alpha order.

Updating version

All feedback addressed.

Committed/pushed to 9.2.x and cherry-picked to 9.1.x, thanks!