[DX] Form API: checkboxes should be saving 1 or 0 instead of the #options key.

[klonos]

So with this for example:

$form['radios'] = array(
  '#type' => 'radios',
  '#title' => t('A set of options'),
  '#options' => array(
    1 => t('1st option'),
    OPTION_TWO => t('2nd option'),
    'option_three' => t('3rd option'),
  ),
);

$form['single_checkbox'] = array(
  '#type' => 'checkbox',
  '#title' => t('A single setting'),
);

$form['multiple_checkboxes'] = array(
  '#type' => 'checkboxes',
  '#title' => t('A series of settings'),
  '#options' => array(
    'setting_one' => t('1st setting'),
    'setting_two' => t('2nd setting'),
    'setting_three' => t('3rd setting'),
  ),
);

...you get this:

{
    "_config_name": "my_config.settings",
    "radios": 1,
    "single_checkbox": 0,
    "multiple_checkboxes": {
        "setting_one": "setting_one",
        "setting_two": "setting_two",
        "setting_three": 0
    }
}

In the above:

• possible values for the radios are: 1, OPTION_TWO, "option_three" (depends on how the keys were defined; you get the point) 👍
• possible values for the single checkbox are: 0 or 1 👍
• possible values for the multiple checkboxes should be 0 or 1 (for checked/unchecked status); not 0 or "setting_one" 👎

In other words, this is what we should be getting instead:

{
    "_config_name": "my_config.settings",
    "radios": 1,
    "single_checkbox": 0,
    "multiple_checkboxes": {
        "setting_one": 1,
        "setting_two": 1,
        "setting_three": 0
    }
}

As it is now, this requires writing a form submit handler function in order to properly save the values of checkboxes as 0/1 instead of 0/%key%, but we could avoid all that and create better DX if we saved the #options for '#type' => 'checkboxes' as 1/0 instead.

[klonos]

Is there any specific reason that eludes me why we are saving key names instead of 1? I mean, checkboxes only have two states (checked/unchecked), right?

[jlfranklin]

Yes, ease of coding. With foo => foo instead of foo => 1, we can iterate through the list and use the actual value instead of needing to reference the keys. For example:

$enabled_values = array_filter($multiple_checkboxes);
foreach ($enabled_values as $value) { 
  $thing = load_my_thing_by_machine_name($value);
  // do stuff with $thing
}

This is a common when enabling a module's feature for selected node types or taxonomy vocabularies.

Also, when searching for an enabled value:

if (array_search('my_value', $multiple_checkboxes)) { ... }

Setting unchecked array elements to zero prevents undefined index warnings. Set or not, the following code works cleanly, assuming 'foo' is a known key.

if ($multiple_checkboxes['foo']) { ... }

And any non-empty string will return TRUE in the above line, just as much as 1 will.

[klonos]

@jlfranklin I think that ease of coding should not "impose" config inconsistency because that is confusing for beginner devs. Besides, adding an array_keys does not make it harder to code::

$enabled_values = array_keys(array_filter($multiple_checkboxes));
foreach ($enabled_values as $value) { 
  $thing = load_my_thing_by_machine_name($value);
  // do stuff with $thing
}

...nor does using array_key_exists instead of in_array:

if (array_key_exists('my_value', $multiple_checkboxes)) { ... }

...and if ($multiple_checkboxes['foo']) { ... } would work just the same for 1 as it would for any other non-empty key as you point out.

[klonos]

..and speaking of easy of coding, let me reiterate:

As it is now, this requires writing a form submit handler function in order to properly save the values of checkboxes as 0/1 instead of 0/%key%...

[klonos]

...and if you ask "Yes, but why save as 0/1 in the first place?", then the answer is that as a beginner dev, when I see that a single checkbox in a form is saved as 0/1 and I can do:

$thing = $config->get('thing');
if ($thing) {
 // do stuff with $thing
}

...then I expect this to work just the same with multiple checkboxes:

$things = $config->get('things');
foreach ($things as $thing => $enabled) {
  if ($enabled) {
   // do stuff with each $thing
  }
}

then I expect this to work just the same with multiple checkboxes

It would work. if ($enabled) { will still return TRUE whether $enabled is 1 or 'setting_one'.

I personally don't see the need for a change here. And any change would be an API change anyway (some people might be relying on the option names as values).

[klonos]

I am willing to drop this, but it still annoys me to see this:

    "content_types": {
        "page": "page",
        "post": "post",
        "whatever": "whatever"
    },

and then this:

    "content_types": {
        "page": "page",
        "post": 0,
        "whatever": "whatever"
    },

I would either expect this:

    "content_types": {
        "page": 1,
        "post": 0,
        "whatever": 1
    },

...or this (array format where unticked checkboxes are omitted):

    "content_types": [ "page", "whatever" ]

@klonos I see your point, it can be confusing/inconsistent. I'm just not sure how to change/fix this while keeping backwards-compatibility...

[klonos]

Yeah, this is similarly annoying as having some numbers saved as strings, and then some others as numbers:

{
...
    "search_minimum_word_size": "3",
    "search_overlap_cjk": 1,
    "search_cron_limit": "100",
...
    "search_and_or_limit": 7,
    "search_default_module": "node",
    "search_logging": false,
    "node_rank_comments": "0",
    "node_rank_relevance": "0",
    "node_rank_sticky": "0",
    "node_rank_promote": "0",
    "node_rank_recent": "0",
    "transliteration": 0
}

As for maintaining compatibility, thanks for setting the milestone on this issue to 2.x ...means that we won't need to worry about it for a long time. Just something to consider when we are doing our clean-up for 2.0 😉

[klonos]

...here's another pickle I've come across while working on #4197...

I have some code that is to save whatever input is coming from a checkbox in the content type edit form. I would expect things to be as simple as this:

function search_node_type_form_submit($form, &$form_state) {
  if (isset($form_state['values']['search_options'])) {
    $node_type = $form_state['values']['type'];
    $config = config('search.settings');
    $config->set("content_types.$node_type", $form_state['values']['search_options']);
    $config->save();
  }
}

$form_state['values']['search_options'] can either be 1 or 0, depending on whether the checkbox is ticked or not. So the above code results in either of these in the .json config file:

{
    "_config_name": "search.settings",
    "content_types": {
        "page": "page",
        "post": 0,
        "test": 1
    },
...

{
    "_config_name": "search.settings",
    "content_types": {
        "page": "page",
        "post": 0,
        "test": 0
    },
...

...but with this behaviour (the one I'm reporting in this issue as a "bug"), I now have to "translate" the input of the checkbox when ticked (value is 1) into the machine name of the content type, like so:

function search_node_type_form_submit($form, &$form_state) {
  if (isset($form_state['values']['search_options'])) {
    $node_type = $form_state['values']['type'];
    $search_index_setting = $form_state['values']['search_options'] ? $node_type : $form_state['values']['search_options'];
    $config = config('search.settings');
    $config->set("content_types.$node_type", $search_index_setting);
    $config->save();
  }
}

(that's in order to achieve saving "test": "test" instead of "test": 1)

Anyway, still up for discussion/feedback.