Unable to uncheck rows in default_value. Tableselect needs value callback.

I prefer the values in the default_values array over the keys in that array. This goes along with patch in #2. I was simply pointing out this is different from what it was doing and was not aware if it was intended to use the keys or the values. Seems more intuitive to use the values.

Attached patch:

- incorporates multiple value check in form_type_tableselect_value()
- includes type = tableselect in processing done for checkboxes in drupal_process_form()
- modifies tests when not multiple value

Revised patch:

- handles the form code defining default values and the user clearing the default values (and not checking any other values)
- defines the defaults in the same way as the checkboxes element (i.e. the array value not the key)

Here is a simpler version of the previous patch that simply calls the form_type_checkboxes_value() function.

The only change with using the checkboxes code is if the core code sets the #default_value property on a tableselect item. The only occurrence of this seems to be in node.admin.inc with '#default_value' => array(1 => 1, 2 => 2). Thus, whether keys or values are used to set the value, should it not work the same?

Interestingly, this issue highlights a missing facet to the test for the tableselect widget (and possibly others). None of the tableselect tests set a default value, tested for it, changed it and tested again, etc.

Rerolled with suggestions in IRC from Heine.

Marking RTBC per Heine's comment in #13 as latest patch incorporates the conditions he stated for changing to RTBC.

I downloaded and grepped all the D7 contributed modules on drupal.org for 'tableselect.' Only one other module (besides coder upgrade) sets a default value for this form element. Thus, to change the default value handling (or more accurately to conform it to checkboxes), does not appear to affect a significant base of existing code. We can inform the maintainer of the other module of this change. Also, as mentioned above, there is no instance in core of setting the default value for this form element.

This seems like a "no brainer" to commit.

Rerolled patch from #3 with a comment for D8 and one additional line change in _form_validate().

+++ includes/form.inc	1 Sep 2010 13:45:50 -0000
@@ -2036,6 +2036,49 @@ function form_type_checkboxes_value($ele
+ * @param $element
+ *   The form element whose value is being populated.
+ * @param $input
+ *   The incoming input to populate the form element. If this is FALSE,
+ *   the element's default value should be returned.
+ * @return
+ *   The data that will appear in the $element_state['values'] collection
+ *   for this element. Return nothing to use the default.

Not sure why we document the callback params and return values all over again. I'd be happy to not introduce them here.

(The phpDoc is the same for all form element value callbacks.)

Leaving that decision to you.

+++ includes/form.inc	1 Sep 2010 13:45:50 -0000
@@ -2036,6 +2036,49 @@ function form_type_checkboxes_value($ele
+  // uses the array values. For D8, consider making the two elements consistent
+  // with the following disabled code.
+
+  // If #multiple is FALSE. the tableselect element displays a set of radio
+  // buttons which constitue a single value element, and for which the default
+  // value handling is sufficient.
+  // if (isset($element['#multiple']) && $element['#multiple']) {
+  //   // Multiple value element, a set of checkboxes.
+  //   return form_type_checkboxes_value($element, $input);
+  // }

Please remove that code. It's nice that you think ahead of time, but D8 may turn out to be entirely different, so the entire commented out code might be moot.

+++ includes/form.inc	1 Sep 2010 13:45:50 -0000
@@ -2036,6 +2036,49 @@ function form_type_checkboxes_value($ele
+  if (isset($element['#multiple']) && $element['#multiple']) {

I guess a #type tableselect cannot be non-multiple? Otherwise, we'd be dealing with a serious abuse of the element, and I'd make a call to not support that.

Hence, let's just drop this extra condition.

Powered by Dreditor.

Thanks for looking at this. Responding to your points:

I left the callback parameters as it seems odd to remove them only from one value callback. It would seem another issue should be created to modify all of the callbacks to remove the redundant parameters and add an @see reference to the documentation of the parameters.

Removed the D8 comment and code.

Retained the "$element['#multiple']" condition because this value can be FALSE. If it is, radio buttons are displayed and the default value handling is used.

Attached patch is against current HEAD.

Retained the "$element['#multiple']" condition because this value can be FALSE. If it is, radio buttons are displayed and the default value handling is used.

Please also add this important detail to the inline comment, slightly reworded.

Revised as suggested above.

Rerolled against current HEAD.

#26: 831966-core-tableselect.patch queued for re-testing.

Committed to HEAD.