system_settings_form_submit() still saves to variables

[ghost]

function system_settings_form_submit($form, &$form_state) {
  // Exclude unnecessary elements.
  form_state_values_clean($form_state);

  foreach ($form_state['values'] as $key => $value) {
    if (is_array($value) && isset($form_state['values']['array_filter'])) {
      $value = array_keys(array_filter($value));
    }
    variable_set($key, $value);
  }

  backdrop_set_message(t('The configuration options have been saved.'));
}

Should this be changed to config_set() instead, or are variables still used/available in Backdrop?

[quicksketch]

Variables are going to still exist in Backdrop 1.0 as part of the commitment to backwards compatibility, but variable_set() and variable_get() should be marked deprecated. This function is almost eliminated from core, we've still got usage of it in: system_file_system_settings(), system_image_toolkit_settings(), and system_clean_url_settings().

Creating a generic save function is more complicated in Backdrop that it was in Drupal 7, because while variables are global, in Backdrop you have to map the settings into a particular config file. We attempted to make a generic function at one point, but ultimately it was more messy and "magical" than before, so we went with just manually assembled submit buttons and functions to keep things direct and simple.

[quicksketch]

So to answer the actual question, remaining usages should be removed, then this function and system_settings_form() itself should be moved into core/includes/drupal.inc, as it should only be available through the backwards compatibility layer.

[quicksketch]

As of today, we are now 100% variable free (w00t!) We moved over the last variable, mail_system in #416. We should move system_settings_form() to drupal.inc, as it is no longer being used anywhere in core.

[quicksketch]

This would be a good thing to get documented, so tagging with needs change notice.

Previously, when writing a simple settings form, it was common to use system_settings_form() to quickly save values into the "variables" table. Since all variables are deprecated in Backdrop and should not be used, system_settings_form() is deprecated as well. Now when creating settings forms, submit handlers must be individually written to save values into the respective configuration file.

Old:

function my_settings_form($form, $form_state) {
  $form['my_setting'] = array(
    '#type' => 'textfield',
    '#title' => t('My setting'),
    '#default_value' => variable_get('my_setting', ''),
  );
  return system_settings_form($form);
}

New:

function my_settings_form($form, $form_state) {
  $config = config('my_module.settings');
  $form['my_setting'] = array(
    '#type' => 'textfield',
    '#title' => t('My setting'),
    '#default_value' => $config->get('my_setting'),
  );
  $form['actions']['#type'] = 'actions';
  $form['actions']['submit'] = array(
    '#type' => 'submit',
    '#value' => t('Save configuration'),
  );
}

function my_settings_form_submit($form, &$form_state) {
  $config = config('my_module.settings');
  $config->set('my_setting', $form_state['values']['my_setting']);
  $config->save();
  backdrop_set_message(t('The configuration options have been saved.'));
}

[rudiedirkx]

I think system_settings_form() was used a LOT, so it might be useful to provide its replacement. Something like

  return system_settings_form($form, $form_state, 'my_module.settings);

which adds the buttons and the submit handler, which saves everything into config('my_module.settings').

Or make the $config object the 3rd param since you'll need it for the values in the form function anyway.

[quicksketch]

Yeah, some kind of shortcut would be helpful. At the time when were doing conversions we weren't quite sure what the requirements would be for such a shortcut. In some cases, multiple config files would be necessary, especially if a form is provided by one module then altered by another module. With variables, everything goes into a single bucket. With config, each setting should go into a file based on the module that provided it. Early patches in Drupal 8 tried to do something with $form_state, like this:

$form_state['config_map'] = array(
  'my_setting' => 'my_module.settings',
  'another_setting' => 'second_module.settings',
);

And then the submit handler would loop through the mapping and save into different files. However, this syntax ended up being just about as verbose as just making an explicit submit handler, and was significantly more magical (not a good way).

So making a shortcut would be good, I agree. But finding a shortcut that works in most situations, is easy to follow, and not very magical is the challenge.

[rudiedirkx]

How about saving it in the $form tree? Some custom #property to represent the 'bucket'.

  $form['my_setting'] = array(
    '#type' => 'textfield',
    '#title' => t('My setting'),
    '#default_value' => $config1->get('my_setting'),
    '#config_bucket' => 'my_module.settings',
  );
  $form['my_other_setting'] = array(
    '#type' => 'textfield',
    '#title' => t('My other setting'),
    '#default_value' => $config2->get('my_setting'),
    '#config_bucket' => 'my_module.other_settings',
  );

The system_settings_form(form, form_state, default_bucket) would be the same, because a default location is required, so you can skip the specific #config_bucket. The submit handler could find #config_bucket elements in the form root or maybe recursively until it finds a #tree => TRUE.

Even with only 1 destination it's useful I think, because 1 config form would save a lot to 1 destination. If you need more, you can user a custom submit handler.

I have 0 experience with the new configuration management, so I might be talking crazy. I'm just thinking of the developers here =)

[quicksketch]

I thought about that solution as well, but then the form properties start becoming fuzzy and only existing in certain situations. If we implemented this, should #config_bucket be documented in the FAPI reference document? Probably not, but it'd be unusual to see it widely-used but not documented as a property.

Both the element property and the $form_state map would work (combined with or without the parameter to system_settings_form()), but they perpetuate a problem we already have with system_settings_form(): forms don't always directly map to configuration.

[rudiedirkx]

You can't tackle them all... That's why you can always have your own submit handler.

The docs for system_settings_form() (or its replacement) should contain info about #config_bucket, not the FAPI. Even without per-element storage it's useful. Even if it's only useful 20% of the time, it's still useful. It's up to you of course if it's worth the maintenance and docs tme etc.

[ghost]

I've only been skimming over these last few posts, so I might not have the whole picture, but this comment stood out to me:

Even if it's only useful 20% of the time, it's still useful.

Wasn't that the case with many other features Backdrop has removed? Useful for some people, or some of the time, but because they weren't widely used they were removed (and rightly so). I'd be careful about adding a new system that's only useful 20% of the time...

[quicksketch]

It's up to you of course if it's worth the maintenance and docs tme etc.

I'm not worried so much about maintenance and docs as I am about abstraction and learnability. It's not very much work to write a submit handler in the first place; we did exactly this for at least a dozen forms in core. It makes settings form no different or more special than any other form in Backdrop, and it makes it clear how values are being saved and where they are being saved. It also gives an opportunity to save values consistently (for example casting to integers or filtering checkboxes), and decouples the structure of the form from the structure of the configuration file.

The down-sides here are that it's slightly more work for experienced developers who are familiar with the previous shortcut and it could result in inconsistencies between forms (for example forgetting the $form['actions'] wrapper). We're also marketing Backdrop on being easy to upgrade from Drupal 7, and providing an alternative shortcut could reduce porting time. We would need to name it something else to keep compatibility with the previous API, since system_settings_form() should continue to work and save into variables when the Drupal compatibility mode is enabled.

My feeling on this is that we should take the conservative route and avoid adding the replacement shortcut at this point. We can always add more abstraction later, removing it is difficult.

[docwilmot]

Search "system_settings_form($" (60 hits in 51 files), in a site with 80 contrib modules, so about once per module. So I suspect not a terrible impact to change to drop the function and require a full submit handler. If the only significant advantage of keeping system_settings_form() is to make it "easier" for developers, I dont think theyd miss it that much it seems.

[sentaidigital]

I know I'd miss it. It's not so much that we'll have to write a submit handler, it's that we'll be constantly writing the same commit handler over and over.

The common form is a single mymodule.settings bucket, and I think system_settings_form() should support that. I also like the more complicated $form['my_setting']['#config'] = 'mymodule.special'; to handle the uncommon case.

I suggest taking it a step further and let the #config setting propagate down to sub-elements, so if you set:

<?php
$form['grouping'] = array(
    '#type' => 'fieldset',
    '#config' => 'mymodule.special',
);

... any settings fields in the grouping fieldset would also be saved to mymodule.special. I think the common case for multiple config buckets will be (should be!) to group them together by bucket on the form itself, anyway, so allowing a form to set the config bucket on a fieldset keeps the form DRY.

Now that I think about it, adding $form['#config'] = 'mymodule.settings'; could cover the entire form in the most common case, and would be a degenerate case of adding it to a fieldset. Then, system_config_form() doesn't even need a new parameter, just more smarts.