Bug: on multi-select, if last option is checked, then first (empty) option gets checked on reload

maniqui commented 12 years ago

This affects 1.2.4 and 1.3.0.

Basically, on a multi-select created, the function glz_selectInput adds an empty first option (unless you pass the $default_value parameter as true, which doesn't seem correct at all, from a logic/semantic POV. See note on next comment).

Then, there is the bug: if user selects the last option in the multi-select, and the saves the article, then, on page reload, this first empty option is also selected. Problem is, if the user saves the article again, without noticing this, both the empty option and the last option get saved into the database.

The causes of this bug are two:

first, the $selected variable in the foreach loop is not being set/reset to empty after the last loop. This means that if the user selects the last option of a multi-select, then, after the loop, the $selected variable will hold some non-empty value.
second, in the return line of the function glz_selectInput , you can spot this code "<option value=\"\"$selected> </option>". That's the code that adds a first empty option to select/multi-select. Thus, if the $selected variable holds something (basically, a selected="selected"), then it's going to get printed there.

So, I think there are two solutions. A) Set the $selected variable to empty after the foreach loop, and/or B) Change this "<option value=\"\"$selected> </option>" to this "<option value=\"\"> </option>".

The original function is the following. I'll add some comments inline for you to spot the issue.

function glz_selectInput($name = '', $id = '', $arr_values = '', $custom_value = '', $default_value = '', $multi = '') {
  if ( is_array($arr_values) ) {
    global $prefs;
    $out = array();

    // if there is no custom_value coming from the article, let's use our default one
    if ( empty($custom_value) )
      $custom_value = $default_value;

    foreach ($arr_values as $key => $value) {
      $selected = glz_selected_checked('selected', $key, $custom_value, $default_value); // NOTE (not related to the issue): the $default_value there isn't necessary at all. The function only receives 3 parameters.
      $out[] = "<option value=\"$key\"{$selected}>$value</option>";
    }

    // NOTE: After the foreach, if the selected option is the last one, the $selected variable holds 'selected="selected"'.
    // A simple $selected='' would be enough to set it to empty after the loop.

    // we'll need the extra attributes as well as a name that will produce an array
    if ($multi) {
      $multi = ' multiple="multiple" size="'.$prefs['multiselect_size'].'"';
      $name .= "[]";
    }

    return "<select id=\"".glz_idify($id)."\" name=\"$name\" class=\"list\"$multi>".
      // NOTE: here, the $selected variable is used again. If it's not empty, the bug is triggered: first empty option (on a multi-select) will get selected.
      ($default_value ? '' : "<option value=\"\"$selected>&nbsp;</option>").
      ( $out ? join('', $out) : '').
      "</select>";
  }
  else
    return glz_custom_fields_gTxt('field_problems', array('{custom_set_name}' => $name));
}

maniqui commented 12 years ago

Extra note about multi-select and having a first empty option.

When you create a multi-select field by calling the glz_selectInput function in a "Custom Script" custom field, you get an empty option as the first option, unless you set $default_value to true in the function call (which doesn't makes sense from a code logic POV)

First comment, on having a "first empty option" on a multi-select input: personally, I consider it a "semantic" error: a multi-select doesn't need an empty option. The "emptiness" of a multi-select is selected by, precisely, not selecting any option.

Someone could argue that it's easier for non-tech-savvy users to select an "first empty option" on a multi-select, than having to "un-select" (usually, by doing Ctrl+click). OK, I can agree with that, although that doesn't make it correct.

In any case, maybe we all can get along with the first empty option, but for that, we need:

this bug to be fixed.
improved logic on glz_selectInput to be able to disable/enable the first option for multi-selects (and maybe for single selects too). Currently, the "set $default_value to true to disable first empty option" doesn't seem correct, imo.

I go into more detail in this issue: https://github.com/gerhard/glz_custom_fields_public/issues/30

maniqui commented 12 years ago

One more thing: this bug also affects "built-in" multi-selects that doesn't have a {default value} configured (and thus, they are rendered with a first empty option) . By "built-in" multi-selects I'm talking about those created directly via "Extensions -> Custom fields" tab.

maniqui commented 12 years ago

Just in case anyone wants to patch the plugin, this is a version of function glz_selectInput, based on the possible fixes I suggested on the opening post.

function glz_selectInput($name = '', $id = '', $arr_values = '', $custom_value = '', $default_value = '', $multi = '') {
  if ( is_array($arr_values) ) {
    global $prefs;
    $out = array();

    // if there is no custom_value coming from the article, let's use our default one
    if ( empty($custom_value) )
      $custom_value = $default_value;

    foreach ($arr_values as $key => $value) {
      $selected = glz_selected_checked('selected', $key, $custom_value, $default_value);
      $out[] = "<option value=\"$key\"{$selected}>$value</option>";
    }

    // Empty the variable. Maybe unset($selected) is an option too?
    $selected='';

    // we'll need the extra attributes as well as a name that will produce an array
    if ($multi) {
      $multi = ' multiple="multiple" size="'.$prefs['multiselect_size'].'"';
      $name .= "[]";
    }

    return "<select id=\"".glz_idify($id)."\" name=\"$name\" class=\"list\"$multi>".
      ($default_value ? '' : "<option value=\"\"&nbsp;</option>").
      ( $out ? join('', $out) : '').
      "</select>";
  }
  else
    return glz_custom_fields_gTxt('field_problems', array('{custom_set_name}' => $name));
}

You may also want to look at other possible fixes here: https://github.com/gerhard/glz_custom_fields_public/issues/30#issuecomment-2046077 And here: https://github.com/gerhard/glz_custom_fields_public/issues/30#issuecomment-2046190

gerhard / glz_custom_fields_public

Bug: on multi-select, if last option is checked, then first (empty) option gets checked on reload #29