- Patch #654796 by chx, c960657, sun, effulgentsia: fix bugs with checkbox element.

6 files changed, 193 insertions, 47 deletions

diff --git a/includes/form.inc b/includes/form.inc
index 930f8b512..8bf2ba072 100644
--- a/includes/form.inc
+++ b/includes/form.inc
@@ -2073,11 +2073,29 @@ function form_type_image_button_value($form, $input, $form_state) {
  *   for this element. Return nothing to use the default.
  */
 function form_type_checkbox_value($element, $input = FALSE) {
-  if ($input !== FALSE) {
-    // Successful (checked) checkboxes are present with a value (possibly '0').
-    // http://www.w3.org/TR/html401/interact/forms.html#successful-controls
-    // For an unchecked checkbox, we return integer 0, so we can explicitly
-    // test for a value different than string '0'.
+  if ($input === FALSE) {
+    // Use #default_value as the default value of a checkbox, except change
+    // NULL to 0, because _form_builder_handle_input_element() would otherwise
+    // replace NULL with empty string, but an empty string is a potentially
+    // valid value for a checked checkbox.
+    return isset($element['#default_value']) ? $element['#default_value'] : 0;
+  }
+  else {
+    // Checked checkboxes are submitted with a value (possibly '0' or ''):
+    // http://www.w3.org/TR/html401/interact/forms.html#successful-controls.
+    // For checked checkboxes, browsers submit the string version of
+    // #return_value, but we return the original #return_value. For unchecked
+    // checkboxes, browsers submit nothing at all, but
+    // _form_builder_handle_input_element() detects this, and calls this
+    // function with $input=NULL. Returning NULL from a value callback means to
+    // use the default value, which is not what is wanted when an unchecked
+    // checkbox is submitted, so we use integer 0 as the value indicating an
+    // unchecked checkbox. Therefore, modules must not use integer 0 as a
+    // #return_value, as doing so results in the checkbox always being treated
+    // as unchecked. The string '0' is allowed for #return_value. The most
+    // common use-case for setting #return_value to either 0 or '0' is for the
+    // first option within a 0-indexed array of checkboxes, and for this,
+    // form_process_checkboxes() uses the string rather than the integer.
     return isset($input) ? $element['#return_value'] : 0;
   }
 }
@@ -2109,7 +2127,7 @@ function form_type_checkboxes_value($element, $input = FALSE) {
     // NULL elements from the array before constructing the return value, to
     // simulate the behavior of web browsers (which do not send unchecked
     // checkboxes to the server at all). This will not affect non-programmatic
-    // form submissions, since a checkbox can never legitimately be NULL.
+    // form submissions, since all values in $_POST are strings.
     foreach ($input as $key => $value) {
       if (!isset($value)) {
         unset($input[$key]);
@@ -2807,7 +2825,7 @@ function theme_checkbox($variables) {
   element_set_attributes($element, array('id', 'name', '#return_value' => 'value'));
 
   // Unchecked checkbox has #value of integer 0.
-  if (isset($element['#return_value']) && isset($element['#value']) && $element['#value'] !== 0 && $element['#value'] == $element['#return_value']) {
+  if (!empty($element['#checked'])) {
     $element['#attributes']['checked'] = 'checked';
   }
   _form_set_class($element, array('form-checkbox'));
@@ -2859,6 +2877,33 @@ function form_pre_render_conditional_form_element($element) {
   return $element;
 }
 
+/**
+ * Sets the #checked property of a checkbox element.
+ */
+function form_process_checkbox($element, $form_state) {
+  $value = $element['#value'];
+  $return_value = $element['#return_value'];
+  // On form submission, the #value of an available and enabled checked
+  // checkbox is #return_value, and the #value of an available and enabled
+  // unchecked checkbox is integer 0. On not submitted forms, and for
+  // checkboxes with #access=FALSE or #disabled=TRUE, the #value is
+  // #default_value (integer 0 if #default_value is NULL). Most of the time,
+  // a string comparison of #value and #return_value is sufficient for
+  // determining the "checked" state, but a value of TRUE always means checked
+  // (even if #return_value is 'foo'), and a value of FALSE or integer 0 always
+  // means unchecked (even if #return_value is '' or '0').
+  if ($value === TRUE || $value === FALSE || $value === 0) {
+    $element['#checked'] = (bool) $value;
+  }
+  else {
+    // Compare as strings, so that 15 is not considered equal to '15foo', but 1
+    // is considered equal to '1'. This cast does not imply that either #value
+    // or #return_value is expected to be a string.
+    $element['#checked'] = ((string) $value === (string) $return_value);
+  }
+  return $element;
+}
+
 function form_process_checkboxes($element) {
   $value = is_array($element['#value']) ? $element['#value'] : array();
   $element['#tree'] = TRUE;
@@ -2868,6 +2913,13 @@ function form_process_checkboxes($element) {
     }
     foreach ($element['#options'] as $key => $choice) {
       if (!isset($element[$key])) {
+        // Integer 0 is not a valid #return_value, so use '0' instead.
+        // @see form_type_checkbox_value().
+        // @todo For Drupal 8, cast all integer keys to strings for consistency
+        //   with form_process_radios().
+        if ($key === 0) {
+          $key = '0';
+        }
         $element[$key] = array(
           '#type' => 'checkbox',
           '#processed' => TRUE,
diff --git a/modules/field/modules/options/options.module b/modules/field/modules/options/options.module
index 75683a9fb..e536c08c6 100644
--- a/modules/field/modules/options/options.module
+++ b/modules/field/modules/options/options.module
@@ -152,7 +152,6 @@ function options_field_widget_validate($element, &$form_state) {
  */
 function _options_properties($type, $multiple, $required, $has_value) {
   $base = array(
-    'zero_placeholder' => FALSE,
     'filter_xss' => FALSE,
     'strip_tags' => FALSE,
     'empty_option' => FALSE,
@@ -190,9 +189,6 @@ function _options_properties($type, $multiple, $required, $has_value) {
     case 'buttons':
       $properties = array(
         'filter_xss' => TRUE,
-        // Form API 'checkboxes' do not suport 0 as an option, so we replace it with
-        // a placeholder within the form workflow.
-        'zero_placeholder' => $multiple,
       );
       // Add a 'none' option for non-required radio buttons.
       if (!$required && !$multiple) {
@@ -238,18 +234,6 @@ function _options_get_options($field, $instance, $properties) {
  * The function is recursive to support optgroups.
  */
 function _options_prepare_options(&$options, $properties) {
-  // Substitute the '_0' placeholder.
-  if ($properties['zero_placeholder']) {
-    $values = array_keys($options);
-    $labels = array_values($options);
-    // Use a strict comparison, because 0 == 'any string'.
-    $index = array_search(0, $values, TRUE);
-    if ($index !== FALSE && !is_array($options[$index])) {
-      $values[$index] = '_0';
-      $options = array_combine($values, $labels);
-    }
-  }
-
   foreach ($options as $value => $label) {
     // Recurse for optgroups.
     if (is_array($label)) {
@@ -273,14 +257,6 @@ function _options_storage_to_form($items, $options, $column, $properties) {
   $items_transposed = options_array_transpose($items);
   $values = (isset($items_transposed[$column]) && is_array($items_transposed[$column])) ? $items_transposed[$column] : array();
 
-  // Substitute the '_0' placeholder.
-  if ($properties['zero_placeholder']) {
-    $index = array_search('0', $values);
-    if ($index !== FALSE) {
-      $values[$index] = '_0';
-    }
-  }
-
   // Discard values that are not in the current list of options. Flatten the
   // array if needed.
   if ($properties['optgroups']) {
@@ -302,14 +278,6 @@ function _options_form_to_storage($element) {
     $values = array($values[0] ? $element['#on_value'] : $element['#off_value']);
   }
 
-  // Substitute the '_0' placeholder.
-  if ($properties['zero_placeholder']) {
-    $index = array_search('_0', $values);
-    if ($index !== FALSE) {
-      $values[$index] = 0;
-    }
-  }
-
   // Filter out the 'none' option. Use a strict comparison, because
   // 0 == 'any string'.
   if ($properties['empty_option']) {
diff --git a/modules/field/modules/options/options.test b/modules/field/modules/options/options.test
index cd85e6579..110ab5767 100644
--- a/modules/field/modules/options/options.test
+++ b/modules/field/modules/options/options.test
@@ -112,9 +112,6 @@ class OptionsWidgetsTestCase extends FieldTestCase {
    * Tests the 'options_buttons' widget (multiple select).
    */
   function testCheckBoxes() {
-    // Checkboxes do not support '0' as an option, the widget internally
-    // replaces it with '_0'.
-
     // Create an instance of the 'multiple values' field.
     $instance = array(
       'field_name' => $this->card_2['field_name'],
@@ -142,7 +139,7 @@ class OptionsWidgetsTestCase extends FieldTestCase {
 
     // Submit form: select first and third options.
     $edit = array(
-      "card_2[$langcode][_0]" => TRUE,
+      "card_2[$langcode][0]" => TRUE,
       "card_2[$langcode][1]" => FALSE,
       "card_2[$langcode][2]" => TRUE,
     );
@@ -157,7 +154,7 @@ class OptionsWidgetsTestCase extends FieldTestCase {
 
     // Submit form: select only first option.
     $edit = array(
-      "card_2[$langcode][_0]" => TRUE,
+      "card_2[$langcode][0]" => TRUE,
       "card_2[$langcode][1]" => FALSE,
       "card_2[$langcode][2]" => FALSE,
     );
@@ -172,7 +169,7 @@ class OptionsWidgetsTestCase extends FieldTestCase {
 
     // Submit form: select the three options while the field accepts only 2.
     $edit = array(
-      "card_2[$langcode][_0]" => TRUE,
+      "card_2[$langcode][0]" => TRUE,
       "card_2[$langcode][1]" => TRUE,
       "card_2[$langcode][2]" => TRUE,
     );
@@ -181,7 +178,7 @@ class OptionsWidgetsTestCase extends FieldTestCase {
 
     // Submit form: uncheck all options.
     $edit = array(
-      "card_2[$langcode][_0]" => FALSE,
+      "card_2[$langcode][0]" => FALSE,
       "card_2[$langcode][1]" => FALSE,
       "card_2[$langcode][2]" => FALSE,
     );
diff --git a/modules/simpletest/tests/form.test b/modules/simpletest/tests/form.test
index 4661ad25f..1f76318f5 100644
--- a/modules/simpletest/tests/form.test
+++ b/modules/simpletest/tests/form.test
@@ -1352,3 +1352,84 @@ class FormsFileInclusionTestCase extends DrupalWebTestCase {
     $this->assertText('Submit callback called.');
   }
 }
+
+/**
+ * Tests checkbox element.
+ */
+class FormCheckboxTestCase extends DrupalWebTestCase {
+
+  public static function getInfo() {
+    return array(
+      'name' => 'Form API checkbox',
+      'description' => 'Tests form API checkbox handling of various combinations of #default_value and #return_value.',
+      'group' => 'Form API',
+    );
+  }
+
+  function setUp() {
+    parent::setUp('form_test');
+  }
+
+  function testFormCheckbox() {
+    // Ensure that the checked state is determined and rendered correctly for
+    // tricky combinations of default and return values.
+    foreach (array(FALSE, NULL, TRUE, 0, '0', '', 1, '1', 'foobar', '1foobar') as $default_value) {
+      // Only values that can be used for array indeces are supported for
+      // #return_value, with the exception of integer 0, which is not supported.
+      // @see form_process_checkbox().
+      foreach (array('0', '', 1, '1', 'foobar', '1foobar') as $return_value) {
+        $form_array = drupal_get_form('form_test_checkbox_type_juggling', $default_value, $return_value);
+        $form = drupal_render($form_array);
+        if ($default_value === TRUE) {
+          $checked = TRUE;
+        }
+        elseif ($return_value === '0') {
+          $checked = ($default_value === '0');
+        }
+        elseif ($return_value === '') {
+          $checked = ($default_value === '');
+        }
+        elseif ($return_value === 1 || $return_value === '1') {
+          $checked = ($default_value === 1 || $default_value === '1');
+        }
+        elseif ($return_value === 'foobar') {
+          $checked = ($default_value === 'foobar');
+        }
+        elseif ($return_value === '1foobar') {
+          $checked = ($default_value === '1foobar');
+        }
+        $checked_in_html = strpos($form, 'checked') !== FALSE;
+        $message = t('#default_value is %default_value #return_value is %return_value.', array('%default_value' => var_export($default_value, TRUE), '%return_value' => var_export($return_value, TRUE)));
+        $this->assertIdentical($checked, $checked_in_html, $message);
+      }
+    }
+
+    // Ensure that $form_state['values'] is populated correctly for a checkboxes
+    // group that includes a 0-indexed array of options.
+    $results = json_decode($this->drupalPost('form-test/checkboxes-zero', array(), 'Save'));
+    $this->assertIdentical($results->checkbox_off, array(0, 0, 0), t('All three in checkbox_off are zeroes: off.'));
+    $this->assertIdentical($results->checkbox_zero_default, array('0', 0, 0), t('The first choice is on in checkbox_zero_default'));
+    $this->assertIdentical($results->checkbox_string_zero_default, array('0', 0, 0), t('The first choice is on in checkbox_string_zero_default'));
+    $edit = array('checkbox_off[0]' => '0');
+    $results = json_decode($this->drupalPost('form-test/checkboxes-zero', $edit, 'Save'));
+    $this->assertIdentical($results->checkbox_off, array('0', 0, 0), t('The first choice is on in checkbox_off but the rest is not'));
+
+    // Ensure that each checkbox is rendered correctly for a checkboxes group
+    // that includes a 0-indexed array of options.
+    $this->drupalPost('form-test/checkboxes-zero/0', array(), 'Save');
+    $checkboxes = $this->xpath('//input[@type="checkbox"]');
+    foreach ($checkboxes as $checkbox) {
+      $checked = isset($checkbox['checked']);
+      $name = (string) $checkbox['name'];
+      $this->assertIdentical($checked, $name == 'checkbox_zero_default[0]' || $name == 'checkbox_string_zero_default[0]', t('Checkbox %name correctly checked', array('%name' => $name)));
+    }
+    $edit = array('checkbox_off[0]' => '0');
+    $this->drupalPost('form-test/checkboxes-zero/0', $edit, 'Save');
+    $checkboxes = $this->xpath('//input[@type="checkbox"]');
+    foreach ($checkboxes as $checkbox) {
+      $checked = isset($checkbox['checked']);
+      $name = (string) $checkbox['name'];
+      $this->assertIdentical($checked, $name == 'checkbox_off[0]' || $name == 'checkbox_zero_default[0]' || $name == 'checkbox_string_zero_default[0]', t('Checkbox %name correctly checked', array('%name' => $name)));
+    }
+  }
+}
diff --git a/modules/simpletest/tests/form_test.module b/modules/simpletest/tests/form_test.module
index fbeb6de45..4d717c04b 100644
--- a/modules/simpletest/tests/form_test.module
+++ b/modules/simpletest/tests/form_test.module
@@ -175,6 +175,13 @@ function form_test_menu() {
     'access callback' => TRUE,
     'type' => MENU_CALLBACK,
   );
+  $items['form-test/checkboxes-zero'] = array(
+    'title' => 'FAPI test involving checkboxes and zero',
+    'page callback' => 'drupal_get_form',
+    'page arguments' => array('form_test_checkboxes_zero'),
+    'access callback' => TRUE,
+    'type' => MENU_CALLBACK,
+  );
 
   return $items;
 }
@@ -1395,3 +1402,44 @@ function form_test_load_include_custom($form, &$form_state) {
   $form_state['cache'] = TRUE;
   return $form;
 }
+
+function form_test_checkbox_type_juggling($form, $form_state, $default_value, $return_value) {
+  $form['checkbox'] = array(
+    '#type' => 'checkbox',
+    '#return_value' => $return_value,
+    '#default_value' => $default_value,
+  );
+  return $form;
+}
+
+function form_test_checkboxes_zero($form, &$form_state, $json = TRUE) {
+  $form['checkbox_off'] = array(
+    '#type' => 'checkboxes',
+    '#options' => array('foo', 'bar', 'baz'),
+  );
+  $form['checkbox_zero_default'] = array(
+    '#type' => 'checkboxes',
+    '#options' => array('foo', 'bar', 'baz'),
+    '#default_value' => array(0),
+  );
+  $form['checkbox_string_zero_default'] = array(
+    '#type' => 'checkboxes',
+    '#options' => array('foo', 'bar', 'baz'),
+    '#default_value' => array('0'),
+  );
+  $form['submit'] = array(
+    '#type' => 'submit',
+    '#value' => 'Save',
+  );
+  if ($json) {
+    $form['#submit'][] = '_form_test_checkbox_submit';
+  }
+  else {
+    $form['#submit'][] = '_form_test_checkboxes_zero_no_redirect';
+  }
+  return $form;
+}
+
+function _form_test_checkboxes_zero_no_redirect($form, &$form_state) {
+  $form_state['redirect'] = FALSE;
+}
diff --git a/modules/system/system.module b/modules/system/system.module
index d450eadc5..296c06714 100644
--- a/modules/system/system.module
+++ b/modules/system/system.module
@@ -408,8 +408,8 @@ function system_element_info() {
   $types['checkbox'] = array(
     '#input' => TRUE,
     '#return_value' => 1,
-    '#process' => array('ajax_process_form'),
     '#theme' => 'checkbox',
+    '#process' => array('form_process_checkbox', 'ajax_process_form'),
     '#theme_wrappers' => array('form_element'),
     '#title_display' => 'after',
   );