backdrop-contrib / backup_migrate

Port of the Drupal backup_migrate module to Backdrop
GNU General Public License v2.0
7 stars 14 forks source link

Default values make no sense for config directories #125

Closed indigoxela closed 11 months ago

indigoxela commented 11 months ago

If you go to /admin/config/system/backup_migrate/export/advanced on a fresh install and expand one of the config related fieldsets, you'll see that "Exclude the following files or directories" contains:

backup_migrate
styles
css
js
ctools
less

These are the defaults for the files directory, but they make no sense for a directory, that's supposed to only contain json files and no nesting at all.

backup-settings-config-dir

While there may eventually be a use-case to exclude a JSON file (like backup_migrate.settings.json), I don't think, above values ever make sense.

The values are from includes/sources.filesource.inc (exclude_filepaths).

argiepiano commented 11 months ago

Thanks for filing! I agree it doesn't make sense to list those. This is an interesting problem, as these defaults are defined for the class backup_migrate_destination_filesource that is used for all types of directory/file backup sources, so this class is agnostic as to whether this is the config directory or any other directory in the installation.

I think the solution might be to extend backup_migrate_destination_filesource into a subclass that is used exclusively for config directories.

indigoxela commented 11 months ago

I think the solution might be to extend backup_migrate_destination_filesource into a subclass that is used exclusively for config directories.

That might be.

Maybe, if the only difference is this single method, extend backup_migrate_destination_filesource and override just that single method? I'm not familiar enough with B&M to really understand possible pitfalls for either approach.

argiepiano commented 11 months ago

@indigoxela, @cellear, I've implemented these suggestions. Now, the "Configuration Files Directory" sources do not include the non-sensical folders and file names pointed out by @indigoxela. To test, visit (for example) admin/config/system/backup_migrate/settings and create a new Profile. Then expand the fieldset for Configuration Files Directory (Active) Backup Options. The "Exclude" fieldset is now blank.

If you have saved a Profile (even the Default one) before the patch, the folders saved then will still appear when you edit (as expected). The fieldset will only be blank when you are creating a new profile, or when you revert the Default profile back to its original defaults.

Thank you for testing!

indigoxela commented 11 months ago

@argiepiano many thanks for the quick fix - works for me! :+1:

I started with a fresh install (to prevent previous data from showing up). Without the patch - unusable excludes show up, with patch applied - no values in that texarea.

The I ran an active config backup with a custom profile that excludes one file - works! This one file was excluded.

(I found an unrelated issue, but have to check existing issue before reporting.)

argiepiano commented 11 months ago

Thanks for testing! Merging...