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.

Don't have the full picture, since I commented / added code here a while ago, but we could skip the test using the "take site offline" setting (testAdvancedBackupWithMaintenanceModeEnabledDestinationDownload), if that is manually tested and damien is fine with that.

This is the last issue blocking the D11 compatible 5.1.x release.

If you want to finish the test @solideogloria, the current problem is, that in testAdvancedBackupWithMaintenanceModeEnabledDestinationDownload in line 511, the site is still in maintenance mode ("Current response status code is 503, but 200 expected"), maybe a race condition? Unsure as I haven't touched this in a while, maybe you find a way to fix, (or we remove the test entirely, if that is ok).

@Grevil I'd say we should either revert that feature or finally fix the effects of this issue, to not block the 5.1.0 release. How to proceed?

Ok, we now throw a validation error, when the "site_offline" checkbox is checked AND the "destination_id" is set to "download". I also adjusted the tests accordingly.
I created a minor follow-up issue, where we can resolve this incompatibility in the future: 🐛 Problem with "advanced backup" when checking destination "download" and "take site offline" Active .

@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.

Not really a big fan of disabling the checkbox through drupal states. The "site_offline" checkbox comes before the "destination_id" select, so if the user checks the checkbox, and changes the destination he won't even see, that the checkbox is suddenly disabled and the validation error occurs. Then he has to scroll all the way down to use a different destination just so he can uncheck the checkbox again. Additionally to disabling the checkbox, we could uncheck it automatically, once the destination changes to "download" but this is even more risky IMO, as then the user might accidentally does NOT take the site in maintenance mode.

That's why I implemented it as is. 🙂

Please review!

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.

That's not true. When using Download as the destination, the backup doesn't get saved to /admin/config/development/backup_migrate/backups

@Grevil: I left some comments.

I don't think that this is a proper fix. Instead I agree with #21:

In other words, #20 means that we need to check if the module placed the site in maintenance mode before the backup and removed the maintenance mode before calling the exit() function.

And of course, we need to test it

@damienmckenna should decide, if we should go this way for now, to get 5.1.0 out and this critical one fixed and create a follow-up for the proposed solution in #21. That would be acceptable, but at least comments and @todo's should be added then, linking the follow-up.

@damienmckenna how should we proceed here?

PS: Thanks for adding the important test, that the front page is still functional.

Is there already a test available to check the resulting backup for consistency? (Not being broken like before)? Then we should also add that one for all backup tests, I think!

If not, we should have a follow-up to implement one to save us from dangerous broken / corrupted backups?
BTW I'd indeed vote to fix it like this for now and have two follow-ups to reduce the risky impact of this bug asap. But let's wait for Damien's feedback!

The exit(); is simply readded. It was originally removed through #3180214: Site does not switch out of maintenance mode after backup finished → :
https://git.drupalcode.org/project/backup_migrate/-/commit/790cdc405d646...

There is already a follow-up issue, where this can be solved cleanly:
🐛 Problem with "advanced backup" when checking destination "download" and "take site offline" Active .

I made a comment for this!

Alright, adjusted the comments, please do a final review!

The exit(); is simply readded. It was originally removed through #3180214: Site does not switch out of maintenance mode after backup finished

Yes, that shows the importance of the comment to explain, why it should _not_ be removed.

Thanks for the fixes and the follow-up! I'm setting this RTBC to push this important fix forwards, but as of #51 and #52 let's wait for Damien's final feedback and decision how to proceed.

I definitely vote for fixing this issue right now. We already implemented a temporary workaround, so that enabling "take site offline" and setting destination to "download" will throw a validation error. And we can follow up everything else in 🐛 Problem with "advanced backup" when checking destination "download" and "take site offline" Active .

This looks really good, thank you.

A small nitpick: any URLs should be noted on separate lines with a @see prefix.

Done!

Is there already a test available to check the resulting backup for consistency? (Not being broken like before)?

I don't think so. It would be nice to have something check that the resulting backup actually works. I have occasionally found backup files that contain multiple INSERT statements with the same primary key, causing restore to fail. This could be due to site use during the creation of the backup. No test was added to ensure that the gzipped backup file is valid, either (that it doesn't contain trailing HTML, making it an invalid file).

Added follow-up. ✨ Validate resulting backup file Active

@solideogloria Thanks!

Nitpick from #57 solved! :)

Excellent work, thanks everyone!

Yay! 🎉

Thanks, Damien! 🙂

Automatically closed - issue fixed for 2 weeks with no activity.