Provide settings page for the hidden variables

Which settings do you mean?

See the referenced issue #3060980: Add settings options for custom smart backup delete.

I have found the following settings in the code.

Skip to the next comment for a list of candidates for an admin settings page.

Values already in uninstall function

function backup_migrate_uninstall() {
  variable_del('backup_migrate_backup_max_time');
  variable_del('backup_migrate_copy_destination_id');
  variable_del('backup_migrate_data_bytes_per_line');
  variable_del('backup_migrate_data_rows_per_line');
  variable_del('backup_migrate_data_rows_per_query');
  variable_del('backup_migrate_destination_id');
  variable_del('backup_migrate_disable_cron');
  variable_del('backup_migrate_memory_limit');
  variable_del('backup_migrate_profile_id');
  variable_del('backup_migrate_schedule_last_run_');
  variable_del('backup_migrate_source_id');
  variable_del('backup_migrate_timeout_buffer');
  variable_del('backup_migrate_verbose');
  variable_del('nodesquirrel_endpoint_urls');
  variable_del('nodesquirrel_schedule');
  variable_del('nodesquirrel_schedule_enabled');
  variable_del('nodesquirrel_schedule_source_id');
  variable_del('nodesquirrel_secret_key');
}

* Of those, the following values are listed in README.txt as settings that users should set in a site's settings.php:

backup_migrate_memory_limit
backup_migrate_backup_max_time
backup_migrate_verbose
backup_migrate_disable_cron
backup_migrate_data_rows_per_query
backup_migrate_data_rows_per_line
backup_migrate_data_bytes_per_line

* The following values are used for internal housekeeping and should presumably not be user-editable:

backup_migrate_source_id
backup_migrate_destination_id
backup_migrate_copy_destination_id
backup_migrate_profile_id

(These are used to remember the most recent setting a user used in the Quick Backup form.)

backup_migrate_schedule_last_run_

(Note that this receives a schedule ID as suffix.)

* The following values are never set (as far as I can tell) and therefore always use the variable_get() default value:

backup_migrate_timeout_buffer

* NodeSquirrel values - considering what is said in https://www.drupal.org/project/backup_migrate/issues/3051821 ("Disable NodeSquirrel integration") it seems inopportune to create an admin interface for these values:

nodesquirrel_endpoint_urls
nodesquirrel_schedule
nodesquirrel_schedule_enabled
nodesquirrel_schedule_source_id
nodesquirrel_secret_key

for the following reasons: 1) uncertainty about testing against production servers, and 2) uncertainty that these values will be used in upcoming versions of B&M.

Values not (yet) in the uninstall function

The following values do not occur in the uninstall function:

backup_migrate_default_schedule
nodesquirrel_schedule_keep
? backup_migrate_destinations_defaults (see includes/crud.inc:918 and :939)
? backup_migrate_profiles_defaults (see includes/crud.inc:918 and :939)
? backup_migrate_schedules_defaults (see includes/crud.inc:918 and :939)
? backup_migrate_sources_defaults (see includes/crud.inc:918 and :939)
backup_migrate_max_email_size
backup_migrate_allow_backup_to_download
nodesquirrel_activate_url
nodesquirrel_manage_url
nodesquirrel_failover
nodesquirrel_default_endpoint_urls
backup_migrate_cleanup_temp_files
backup_migrate_cleanup_time
backup_migrate_smart_keep_hourly
backup_migrate_smart_keep_daily
backup_migrate_smart_keep_weekly
backup_migrate_schedule_buffer
backup_migrate_smart_keep_subhourly

* Of these, I suggest to ignore the following for now, for the above mentioned reasons:

nodesquirrel_schedule_keep
nodesquirrel_activate_url
nodesquirrel_manage_url
nodesquirrel_failover
nodesquirrel_default_endpoint_urls

* It has been suggested that the following values be turned into per-schedule settings:

backup_migrate_smart_keep_hourly
backup_migrate_smart_keep_daily
backup_migrate_smart_keep_weekly
backup_migrate_smart_keep_subhourly

See https://www.drupal.org/project/backup_migrate/issues/3060980, "Add settings options for custom smart backup delete".

I don't care either way. There is a precedent in per-schedule settings in the variables that start with 'backup_migrate_schedule_last_run_'.

* The following settings are currently not being accessed by the module:

backup_migrate_default_schedule (commented out)

* The following settings are explicitely defined as settings.php only:

TABLENAME_defaults (i.e. backup_migrate_destinations_defaults for default destinations, backup_migrate_profiles_defaults for default profiles and so on)

A code comment says: "Get any items stored as a variable. This allows destinations to be defined in settings.php"

The reason why this would not make sense as a user administered setting, is because if a user wants to create a destination, they can already do so. Having a separate set of defaults would likely only be confusing.

Also, there is a hook for defining default settings, namely HOOK_backup_migrate_ITEMTYPE().

It would perhaps be useful to document the structure of these settings though.

* Security settings:

backup_migrate_allow_backup_to_download

This is a setting that allows someone with access to the B&M UI to download a backup to the browser. Since the person that would be allowed to download would also have permission to change this setting, it would make no sense to make this a DB setting.

* Other values:

backup_migrate_max_email_size
backup_migrate_cleanup_temp_files
backup_migrate_cleanup_time
backup_migrate_schedule_buffer

The following values are candidates for settings in an admin interface:

backup_migrate_memory_limit
backup_migrate_backup_max_time
backup_migrate_verbose
backup_migrate_disable_cron
backup_migrate_data_rows_per_query
backup_migrate_data_rows_per_line
backup_migrate_data_bytes_per_line
backup_migrate_max_email_size
backup_migrate_cleanup_temp_files
backup_migrate_cleanup_time
backup_migrate_schedule_buffer

Of these, the final four would need to be added to backup_migrate_uninstall().

Here is a patch that implements the feature. (Update: patch in the next comment.)

Some notes:

- I wasn't sure if this should be implemented on the Settings tab or on its own tab. I opted for the latter.

- There is probably a better name than Global Settings out there.

- I have checked a number of Drupal core modules: the practice appears to be to read setting form's field values using variable_get(). This means that if an override exists, this gets displayed rather than the database value. I am not overly happy about this, but if that is the way things are done...

- This patch reduces code readability, because values will be defined away from where they are used, but I fear that is unavoidable.

Thank you for the quick review!

I looked at and tested your changes and saw nothing wrong.

I came across a bug in my code and also wanted to improve some other things, so here is a new patch. This changes the following:

1) Include warning from README.txt at the top of the settings form.

2) I mixed up the mathematical meaning of "positive integer" with the computing meaning of (unsigned) integer. The former excludes 0, the latter includes 0. Function element_positive_integer_validate() already did what backup_migrate_positive_nonzero_integer_validate() attempted to do. I replaced those instances. I replaced backup_migrate_positive_integer_validate() by backup_migrate_unsigned_integer_validate().

3) Better descriptions, comments and names.

4) Removed unused variables.

5) Removed extraneous comments.

6) The assertion function now returns a result (TRUE if the assertion succeeds, FALSE if it fails). I did this because that is what the Simpletest module appears to do.

Furthermore, I have the following comment and question.

A) The reason I put the memory limit test in a separate function is because that function could be useful for this issue: ""A non-numeric value encountered" in backup_migrate_to_bytes()"

B) What is the problem this feature is trying to solve? Does this approach solve it?

CC Test Site

Started testing with an existing site running lastest dev version of module - 7.x-3.6+11-dev, and current version of Drupal core 7.67
Used existing feature module with a schedule and an added destination.

- Applied patch #13, and ran update.php and cleared caches.
- Confirmed advanced settings page appears in module settings
- Updated feature module, using default values provided by backup_migrate module
- Confirmed that new default values were saved as part of feature module settings
- Updated values on advanced settings page
- Confirmed that changed values appeared in the DIFF of the feature module
- Recreated the feature module, and confirmed that the new values were stored
- Set memory limit to a new value, and set backup_migrate_verbose to true
- Created backup, which generated verbose output, and displayed the increased memory setting

Testing confirmed that new values are set and available as ctools exportable values, and that changes are reflected in the operations of the module.

I won't change status of the module to RTBC because the patch is failing testing. BUT, it does look like the patch is working as intended and providing the functionality intended to be included.

If additional RTBC testing is required, please assign this ticket to me for further action.

Patch #13 is not failing testing...

https://www.drupal.org/pift-ci-job/1341092