WireDatabaseBackup::restore() does not drop tables that don't exist in backup

@Toutouwai commented on Apr 10, 2019, 3:32 AM UTC:

Short description of the issue

If I make a database backup using WireDatabaseBackup::backup() at time 1, and then later restore that backup at time 2 using WireDatabaseBackup::restore(), I expected the database to be put into the same state as at time 1.

But if there is some new table at time 2 that didn't exist when the backup was made at time 1 then that table is not dropped and persists in the database after the restore. One way this is a problem is with field tables. If a new field "foo" was created between time 1 and time 2 then restoring the backup removes the field from the PW admin (expected) but the "field_foo" table persists (unexpected) which creates a database error when attempting to create a field with that name.

I noticed the problem using ProcessDatabaseBackups but the issue occurs also with the core WireDatabaseBackup::backup() / WireDatabaseBackup::restore() methods.

Field tables at time of backup:

Creating backup:

New field "cost" is created.

Restoring backup:

Field tables after backup is restored:

Setup/Environment

• ProcessWire version: 3.0.129

This issue was moved by ryancramerdesign from processwire/processwire-issues#846.

@adrianbj commented on Apr 10, 2019, 4:11 AM UTC:

For reference: ryancramerdesign/ProcessWire#714

The current behavior makes this feature very problematic and can result in major errors. Thanks Toutouwai for bringing it up again.

@Toutouwai commented on Apr 10, 2019, 4:21 AM UTC:

I guess the database backup feature in AdminActions is affected by this too. That's something I rely on heavily so would be great to get a fix for this issue.

A dropAll option for WireDatabaseBackup::restore() seems like a good solution, like what's been proposed in the earlier issue.

@szabeszg commented on Apr 10, 2019, 7:12 AM UTC:

I expected the database to be put into the same state as at time 1.

Mee too. At least now I know I was wrong.
I also support the request for the option to be able to drop all tables in order to restore the previous state which I though was doing all the time... (I should have checked how it works in the first place, I know.)

@LostKobrakai commented on Apr 10, 2019, 7:55 AM UTC:

Afaik this is intended functionality by ryan, as processwire is very cautious about databases being shared between multiple projects. E.g. there might be a wordpress install with some table prefix running on the same db. This is a common practice for shared hosting, which only allow for a single database. But this "feature" really needs to be properly documented and at best have a way to be disabled. It causes more issues than it solves for people not being in such a situation.

@ryancramerdesign commented on Apr 10, 2019, 7:08 PM UTC:

LostKobrakai is right that this is intended. However, I'm okay to add a dropAll function for those that want it. I just didn't want the PW installer (which is what the class was originally intended for) to be deleting any tables. I'll move this to the processwire-requests repo. Thanks.

[ryancramerdesign]

@Toutouwai This has been added

[adrianbj]

I guess the database backup feature in AdminActions is affected by this too

@Toutouwai - I have added this new option to the restore process of AdminActions.

[adrianbj]

@ryancramerdesign - should the DB Backups module get an extra Restore icon option that calls this new option?

[Toutouwai]

Thanks @ryancramerdesign.
I second @adrianbj's request for a new "Drop all and restore" icon option in ProcessDatabaseBackups.

[adrianbj]

@ryancramerdesign - any thoughts on the update to the backup module?