Options widget broken

This issue was partially fixed by #557932: Taxonomy term field autocomplete widgets never validate, always lose values.. The current patch should fix a broken behavior in options.module which should not derive from the refactoring introduced by #367595: Translatable fields. A field api guru's feedback is badly needed here.

Edit: If the remaining issue is actually an options.module issue please change the title and remove the tag translatable fields.

Head was broken. Re-testing.

I'm not a wizard on the FAPI-related parts of Field API. But I tried a List field (not numeric or text, just list) on a node type, with (single and unlimited values) x (checkboxes/radios and select list). I got no WSODs, but I do get a warning on single-value radio buttons:

Error message
Warning: Invalid argument supplied for foreach() in options_buttons_elements_process() (line 141 of /Users/bjaspan/drupal/head/modules/field/modules/options/options.module).

This does look like the problem fixed by the first part of the patch. However, the second part of the patch does this:

-      $value[$key] = 1;
+      $value[] = $key;

and $value becomes #default_value in the FAPI element of type checkboxes or radios. I do not think part of the patch is right. If $multiple is TRUE, the form element is 'checkboxes', and form_process_checkboxes() does:

          '#default_value' => isset($value[$key]),

So $value[$key] = 1 looks right; #default_value should be an array keyed by box keys to be enabled. If $multiple is FALSE, the form element is 'radios', and form_process_radios() does:

          '#default_value' => isset($element['#default_value']) ? $element['#default_value'] : NULL,

so #default_value should be a single value, the key of the radio button to enable. In neither of these cases should #default_value be an array of values of box keys to be enabled.

But I am not a Field API FAPI wizard.

I tested the patch on a fresh install with all the core fields having the checkboxes/radio buttons widget and I can confirm I experience an improved behavior with the patch applied. Without it I get an Apache crash (!) on initially saving the field settings and the following errors after submitting the field configuration form:

    *  Notice: Array to string conversion in drupal_validate_utf8() (line 1154 of E:\www\test\includes\bootstrap.inc).
    * Warning: preg_match() expects parameter 2 to be string, array given in drupal_validate_utf8() (line 1160 of E:\www\test\includes\bootstrap.inc).
    * Notice: Array to string conversion in drupal_validate_utf8() (line 1154 of E:\www\test\includes\bootstrap.inc).
    * Warning: preg_match() expects parameter 2 to be string, array given in drupal_validate_utf8() (line 1160 of E:\www\test\includes\bootstrap.inc).
    * Warning: Invalid argument supplied for foreach() in options_buttons_elements_process() (line 139 of E:\www\test\modules\field\modules\options\options.module).

Moreover I get the following error after creating a node:

Warning: Invalid argument supplied for foreach() in options_buttons_elements_process() (line 139 of E:\www\test\modules\field\modules\options\options.module).

The patch seems to fix all the errors. Changing to CNR again because we need more eyes on this.

Given the subtleties and the fact that plach and I are seeing different behavior, I think this is a great case for some unit tests. I'll see if I can do that on the plane to France today.

I finally made progress on this! I've implemented basic unit tests for the options widgets, and I must say I think they are pretty sweet. I'm really happy to have them. Here is what I've learned:

1. The patch in #2 is right. There is no WSOD any more, but without the patch, checkboxes do not get initialized properly when there is existig data; with the patch, checkboxes do get initialized. So, the patch fixes a bug.
2. The new tests seem to reveal that something is wrong with radio buttons. When there is no existing data, they cause PHP warnings. The problem is that when theme_radio() is called, $variables['element']['#value'] is an array, but the theme function passes it to check_plain() which of course expects a string.

So, the patch from #2 is right and is included in this new patch, but the tests still fail because of the radio button issue.

Note that this patch creates a new file, options.test.

The problem described in #9 item 2 is that an options_buttons element's #default_value for radio buttons is correctly initialized to an integer if there is a default value, but is left as an array() if there is no default value because the loop that would change $value from its initial value of array() to an integer never runs. This version of the patch fixes that. I am not 100% sure that setting #default_value to the empty string if there is no default is "right" but it does seem to work, as proved by the tests.

This is no longer about translatable fields.

Ready to review, from duplicate #595702: Warnings when adding taxonomy term-field with checkboxes wodget

Re-roll.

Tested this deeply. #11 exactly describes (checkboxes is array, radios is integer) - so tests cover both cases

Committed to CVS HEAD. Thanks!