Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Default values make no sense for config directories #125

Closed
indigoxela opened this issue Dec 7, 2023 · 5 comments · Fixed by #142
Closed

Default values make no sense for config directories #125

indigoxela opened this issue Dec 7, 2023 · 5 comments · Fixed by #142

Comments

@indigoxela
Copy link
Member

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
Copy link
Collaborator

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
Copy link
Member Author

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
Copy link
Collaborator

@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
Copy link
Member Author

@argiepiano many thanks for the quick fix - works for me! 👍

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
Copy link
Collaborator

Thanks for testing! Merging...

argiepiano added a commit that referenced this issue Dec 16, 2023
Issue #125. Remove non-sensible directory defaults for config active and staging.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants