Backup files being overwritten [#2941981]

About a week after updating to 7.x.3.4 I noticed that my daily backups were overwriting the previous day's files, because the backup files had no timestamps.

On checking the settings I found the file-save option was "overwrite" instead of "append timestamp", which is what it was previously. Changing it back to "append timestamp" restored my previous functionality, but I was alarmed that there had been a silent change.

I've looked at the recent commits, and Commit b05a282 added the option to overwrite. The problem is that the enumeration of the options has been changed. Rather than overwrite being added as enum 2, it's been inserted as enum 1, moving timestamp to enum 2. This means all existing settings that stored the value 1 have changed their meaning.

It's too late to revert this, as the damage has been done, and doing so would cause even more confusion. So I'm posting this mainly as a warning that if you have been using time-stamped backups, please check your settings as you may well be inadvertently overwriting the same file rather than preserving a history.

I don't know if there's any way to get this message out to users who don't follow the issues list. The impacts could be quite widespread.

Comment	File	Size	Author
#12	backup_migrate-n2941981-12.interdiff.txt	772 bytes	DamienMcKenna
#12	backup_migrate-n2941981-12.patch	2.79 KB	DamienMcKenna
#12	7.x-3.x: PHP 5.3 & MySQL 5.5, D7 2 pass
#10	backup_migrate-n2941981-10.interdiff.txt	413 bytes	RickJ
#10	backup_migrate-n2941981-10.patch	2.72 KB	RickJ
#10	7.x-3.x: PHP 5.3 & MySQL 5.5, D7 2 pass
#9	backup_migrate-n2941981-9.patch	2.55 KB	DamienMcKenna
#9	7.x-3.x: PHP 5.3 & MySQL 5.5, D7 2 pass
#9	backup_migrate-n2941981-9.interdiff.txt	840 bytes	DamienMcKenna
#7	backup_migrate-n2941981-5.interdiff.txt	470 bytes	RickJ
#5	backup_migrate-n2941981-5.patch	2.32 KB	RickJ
#5	7.x-3.x: PHP 5.3 & MySQL 5.5, D7 2 pass
#4	backup_migrate-n2941981-4.interdiff.txt	589 bytes	DamienMcKenna
#4	backup_migrate-n2941981-4.patch	2.32 KB	DamienMcKenna
#4	7.x-3.x: PHP 5.3 & MySQL 5.5, D7 2 pass
#3	backup_migrate-n2941981-3.patch	1.74 KB	DamienMcKenna
#3	7.x-3.x: PHP 5.3 & MySQL 5.5, D7 2 pass

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

4 February 2018 at 15:49

Rick J created an issue. See original summary.

Comment #2

DamienMcKenna

NH, USA

CreditAttribution: DamienMcKenna at Mediacurrent commented 5 February 2018 at 02:30

Category:	Task	» Bug report
Parent issue:		» #2939266: Plan for Backup and Migrate 7.x-3.5

Oh no! Dangit :-(

Yeah, we need to fix this immediately and roll a new release.

Comment #3

DamienMcKenna

NH, USA

CreditAttribution: DamienMcKenna at Mediacurrent commented 5 February 2018 at 13:53

Version:	7.x-3.4	» 7.x-3.x-dev
Status:	Active	» Needs review

File	Size
backup_migrate-n2941981-3.patch	1.74 KB
7.x-3.x: PHP 5.3 & MySQL 5.5, D7 2 pass

Does this solve the problem?

Comment #4

DamienMcKenna

NH, USA

CreditAttribution: DamienMcKenna at Mediacurrent commented 5 February 2018 at 14:01

File	Size
backup_migrate-n2941981-4.patch	2.32 KB
7.x-3.x: PHP 5.3 & MySQL 5.5, D7 2 pass
backup_migrate-n2941981-4.interdiff.txt	589 bytes

Comment #5

RickJ CreditAttribution: RickJ commented 5 February 2018 at 14:09

File	Size
backup_migrate-n2941981-5.patch	2.32 KB
7.x-3.x: PHP 5.3 & MySQL 5.5, D7 2 pass

1 file was hidden/shown/deleted

File	Size
backup_migrate-n2941981-3.patch	1.74 KB
7.x-3.x: PHP 5.3 & MySQL 5.5, D7 2 pass

Looks OK, good idea with the message on update.

However, I would keep the order of the options in the UI as they were, with timestamp as the last. This is because it expands to show the timestamp format, and it looks better if that doesn't come in the middle of the checkboxes. It doesn't matter that the enum order in the code is 0, 2, 1.

I've tweaked the patch.

Comment #6

DamienMcKenna

NH, USA

CreditAttribution: DamienMcKenna at Mediacurrent commented 5 February 2018 at 14:15

@Ric: Could you please upload an interdiff?

Comment #7

RickJ CreditAttribution: RickJ commented 5 February 2018 at 14:26

File	Size
backup_migrate-n2941981-5.interdiff.txt	470 bytes

Sorry! here it is

Comment #8

camhoward

New Hampshire, USA

CreditAttribution: camhoward commented 5 February 2018 at 14:34

Thanks for working on this!

Can the spelling of "seperate" be corrected to "separate" in the list of options where it says:
0 => t('Create seperate backups even if `Backup file name` is the same.'),

A minor issue, but perhaps easier done now than later.

Comment #9

DamienMcKenna

NH, USA

CreditAttribution: DamienMcKenna at Mediacurrent commented 5 February 2018 at 17:21

File	Size
backup_migrate-n2941981-9.interdiff.txt	840 bytes
backup_migrate-n2941981-9.patch	2.55 KB
7.x-3.x: PHP 5.3 & MySQL 5.5, D7 2 pass

4 files were hidden/shown/deleted

File	Size
backup_migrate-n2941981-4.patch	2.32 KB
7.x-3.x: PHP 5.3 & MySQL 5.5, D7 2 pass
backup_migrate-n2941981-4.interdiff.txt	589 bytes
backup_migrate-n2941981-5.patch	2.32 KB
7.x-3.x: PHP 5.3 & MySQL 5.5, D7 2 pass
backup_migrate-n2941981-5.interdiff.txt	470 bytes

Some some general wording improvements.

Comment #10

RickJ CreditAttribution: RickJ commented 5 February 2018 at 17:59

File	Size
backup_migrate-n2941981-10.patch	2.72 KB
7.x-3.x: PHP 5.3 & MySQL 5.5, D7 2 pass
backup_migrate-n2941981-10.interdiff.txt	413 bytes

One last fix needed. The timestamp box in the UI was expanding on the overwrite checkbox, I've updated the patch.

Comment #11

camhoward

New Hampshire, USA

CreditAttribution: camhoward commented 5 February 2018 at 21:04

I've now had time to apply the patch and test.

Here's how I tested:

Upgraded a Drupal 7.56 site from Backup and Migrate 7.x-3.4 to Backup and Migrate 7.x-3.x-dev
Applied the patch in note #10
Checked the Save mode settings and changed them to "Append the timestamp" because they had changed to "Overwrite the existing backup file" (as expected with patch)
Added a test backup schedule to create backups every 10 minutes

Questions and observations:

Should the "Leave a message to explain the mixup over the backup option" display as a string $type 'warning' rather than the default 'status' in order to call more attention to it? The message itself is helpful. Since this is a critical issue, I wonder if a 'warning' message would alert people that it requires action on their part.

The backup schedule is creating backups with an appended timestamp as expected.

Thanks.

Comment #12

DamienMcKenna

NH, USA

CreditAttribution: DamienMcKenna at Mediacurrent commented 5 February 2018 at 21:58

File	Size
backup_migrate-n2941981-12.patch	2.79 KB
7.x-3.x: PHP 5.3 & MySQL 5.5, D7 2 pass
backup_migrate-n2941981-12.interdiff.txt	772 bytes

4 files were hidden/shown/deleted

File	Size
backup_migrate-n2941981-9.interdiff.txt	840 bytes
backup_migrate-n2941981-9.patch	2.55 KB
7.x-3.x: PHP 5.3 & MySQL 5.5, D7 2 pass
backup_migrate-n2941981-10.patch	2.72 KB
7.x-3.x: PHP 5.3 & MySQL 5.5, D7 2 pass
backup_migrate-n2941981-10.interdiff.txt	413 bytes

@camhoward: That's a good idea, thanks.

Comment #13

5 February 2018 at 22:51

DamienMcKenna committed 9c703b6 on 7.x-3.x

Issue #2941981 by DamienMcKenna, Rick J, camhoward: Backup files being...

Comment #14

DamienMcKenna

NH, USA

CreditAttribution: DamienMcKenna at Mediacurrent commented 5 February 2018 at 22:51

Status:

Needs review

» Fixed

Committed. Thanks for all of your help today.

Comment #15

DamienMcKenna

NH, USA

CreditAttribution: DamienMcKenna at Mediacurrent commented 6 February 2018 at 00:48

FYI I released 3.5 today with this fix. Thank you again!

Comment #16

camhoward

New Hampshire, USA

CreditAttribution: camhoward commented 6 February 2018 at 06:08

@DamienMcKenna and @Rick J: Thanks for the quick fix on this!

Comment #17

jackalope CreditAttribution: jackalope commented 16 February 2018 at 02:38

Version:	7.x-3.x-dev	» 7.x-3.5
Status:	Fixed	» Needs work

I'm afraid that the fix included in the 7.x-3.5 release isn't working properly!

Confirmed that the Drupal site is running backup_migrate-7.x-3.4.
Checked all settings profiles listed on /admin/config/system/backup_migrate/settings/profile and confirmed that on each profile the "Save modes." field is set to "Append timestamp."
Updated to backup_migrate-7.x-3.5, ran database updates and cleared caches.
Reloaded each settings profile page; on each profile, the "Save mode" field (now renamed) is once again set to "Overwrite the existing backup file."

I was able to successfully change the "Save mode" setting to "Append the timestamp" again, save my change and see my changes preserved upon an additional edit; the change seems to have stuck properly, so it seems like this is a problem with the 7.x-3.5 update rather than an issue with the module not saving settings properly at all.

I've double-checked and have seen the same results on four different Drupal sites so far after the 7.x-3.5 update.

This seems especially concerning given the message included in this update that reassures users that this very bug has been fixed!

Comment #18

DamienMcKenna

NH, USA

CreditAttribution: DamienMcKenna at Mediacurrent commented 16 February 2018 at 03:39

Issue tags:

+Needs tests

Dangit, I'm sorry :-(

I'll see if I can use some time at FLDrupalcamp to work on test coverage.

Comment #19

camhoward

New Hampshire, USA

CreditAttribution: camhoward commented 16 February 2018 at 05:17

I believe the issue @jackalope raises is related to the enumeration of the options as @Rick J noted in his original post when he says:

Commit b05a282 added the option to overwrite. The problem is that the enumeration of the options has been changed. Rather than overwrite being added as enum 2, it's been inserted as enum 1, moving timestamp to enum 2. This means all existing settings that stored the value 1 have changed their meaning.

The options in 7.x.3.4 are:

0 => t('Create seperate backups even if `Backup file name` is the same.'),
1 => t('Overwrite backup.'), 
2 => t('Append timestamp.'),

7.x.3.5 changed the enumeration to:

0 => t('Create separate backups if `Backup file name` already exists'),
2 => t('Overwrite the existing backup file'),
1 => t('Append the timestamp'),

So 7.x.3.5 changes the meaning of value 1 again, back to 'Append the timestamp' as in versions prior to 7.x.3.4. This helps the people who updated to 7.x.3.4 and didn't check or change their settings and didn't know backups were being overwritten.

It also means people who updated to 7.x.3.4, checked their settings and changed them will need to check their settings and change them again when they update to 7.x.3.5.

I understood the emphasis of the message that displays after the update to 7.x.3.5 to be check your settings.

Comment #20

RickJ CreditAttribution: RickJ commented 16 February 2018 at 11:19

That's correct, 7.x.3.5 restores the behaviour of 7.x.3.3, but with one extra option. 7.x.3.4 was a slight aberration. On the basis that most people who upgraded to .4 won't have checked their settings, a subsequent upgrade to .5 ensures their settings work as before.