Download errors, backup files contain trailing HTML, corrupts archive backups

I applied patch #19 by @jillh68 and created a simple test.

Unfortunately, the test results in Drupal still being in maintenance mode, after the backup has finished. Manually testing this will result in the correct expected behaviour... Maybe the test is missing something, I am unsure.

Setting this to "needs review", maybe someone can fix the test.

Tests were actually correct.

The site will NOT leave maintenance mode, if the backup destination is set to "Download". But it WILL leave maintenance mode, if "Private Files Directory" is set as the backup destination.

This is represented in the added test.

Adding related issue, since it was mentioned in #16 and #17 that downloaded files contain html.

After testing the MR, I can confirm that the applied changes fix the related issue with trailing HTML on downloaded backups!

I will mark that issue a duplicate of this one. Thank you for the fix!

Patch for convenience.

Bumping priority to match that of the related issue.

The last submitted patch, 36: backup_migrate-3228379-m29.patch, failed testing. View results →

Just found a bug, while fixing the last failing test. When downloading the backup, and simultaneously having "Take site offline" checked, the site won't go online after the download has finished.

Taking a further look, the destinations "download" (and "upload" for some reason) are added through line 144-149 in backup_migrate.module:

// @todo Make this better.
if (empty($options['nobrowser'])) {
// Add a download destination.
$user = \Drupal::currentUser();
if ($user->hasPermission('access backup files')) {
$destinations->add('download', new DrupalBrowserDownloadDestination(new Config(['name' => t('Download')])));
}
// Add an upload destination.
$destinations->add('upload', new DrupalBrowserUploadDestination(new Config(['name' => t('Upload')])));
}

I think, that is the root of the problem. "private_files" is generated through an install .yml file ("backup_migrate.backup_migrate_destination.private_files.yml"). Although I don't completely understand the workflow yet. I first thought destinations would be simply plugins, but they are not.
The PluginManager calls all plugins "teardown" method, if they implement one. With private files this is no problem, but instead of destinations being plugins and having their "teardown" function bring the drupal site out of maintenance mode, destination seems to be something else, and instead the "DrupalUtils" plugin will get the site back online, but it only does that for private_files, even though we add the DrupalUtils plugin a few lines below of the before mentioned destination additions.

No idea, we could disallow taking the site offline for both download and upload destinations as a quick fix. Everything else needs to be done by someone with further knowledge.

Should a test also be added to ensure that the related issue doesn't occur again? (Making sure that the downloaded backup is correct)

@Grevil, I think it would be okay to disable taking the site offline for download, for sure, since you can easily download the file afterwards at /admin/config/development/backup_migrate/backups.

I haven't used the upload destination, so I don't know for that one.