D7 upgrade: system_update_1018() produces fatal error if "authorize_filetransfer_connection_settings" exist

[argiepiano]

Description of the bug

This hook runs only for D7 sites that had values in variables that start with authorize_filetransfer_connection_settings_, which is very rare, and perhaps this is why it has not been detected before.

This hook contains the following:

  $settings = db_query("SELECT name, value FROM {variable} WHERE name LIKE 'authorize_filetransfer_connection_settings_%'");
  foreach ($settings as $row)  {
    $row = unserialize($row);
    $config->set('authorize_filetransfer_connection_settings_' . $row[0], $row[1]);
  }

The problem with the above is that looping through the results of this db_query will ALWAYS produce a stdClass object with two properties: name and value. This update hook is trying to unserialize a stdClass, which results in a fatal error.

Instead, this should fix it:

  $settings = db_query("SELECT name, value FROM {variable} WHERE name LIKE 'authorize_filetransfer_connection_settings_%'");
  foreach ($settings as $row)  {
    $value = unserialize($row->value);
    $config->set('authorize_filetransfer_connection_settings_' . $row->name, $value);
  }

Steps To Reproduce

To reproduce the behavior:
Upgrade a D7 site that has a set value for variable authorize_filetransfer_default.

Actual behavior

Fatal error when running hook 1018.

Expected behavior

No error.

Additional information

Reported on Zulip by @olafgrabienski

[argiepiano]

Since I have no way to test my proposed solution, I'll defer to anyone who is upgrading a D7 site with that variable. It's hard to tell exactly which values this new config needs.

[olafgrabienski]

Thanks again for your help, @argiepiano. As reported in Zulip, I was able to upgrade the D7 site to Backdrop without error after changing the lines starting at core/modules/system/system.install#L2057 from:

foreach ($settings as $row)  {
  $row = unserialize($row);
  $config->set('authorize_filetransfer_connection_settings_' . $row[0], $row[1]);
}

to:

  foreach ($settings as $row)  {
    $value = unserialize($row->value);
    $config->set('authorize_filetransfer_connection_settings_' . $row->name, $value);
  }

The site runs generally fine after the upgrade, but I'm not aware how the fixed code part is supposed to work after the upgrade. So, the fix works for me but it's not really tested. And we noticed that the respective config file got a quite long property key:

{
    "_config_name": "system.authorize",
    "filetransfer_default": "ftp",
    "authorize_filetransfer_connection_settings_authorize_filetransfer_connection_settings_ftp": {
        "username": "xxxxxx",
        "advanced": {
            "hostname": "localhost",
            "port": "21"
        }
    }
}

[argiepiano]

I think the remaining question is about the naming of the key for the configuration. Right now you are getting this:
authorize_filetransfer_connection_settings_authorize_filetransfer_connection_settings_ftp which, at first sight, looks wrong, but I can't tell since I have no way to test a PR (having the variable authorize_filetransfer_connection_settings_* is very rare in D7 AFAIK, and I am really not sure what it's supposed to do - I guess one could dig into the code to check this).

So, I could submit a PR, but I wouldn't be able to test if this actually breaks something.