Posted by bangpound on August 24, 2009 at 1:15pm
6 followers
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | field system |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
After #367595: Translatable fields, list fields cause WSOD when a bundle with the field is edited.
It could be options widget. Maybe plach can illuminate.
Comments
#1
plach:
I added a list field / select widget and it works. If I change that select widget to an checkbox/radio widget, WSOD.
#2
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.
#3
The last submitted patch failed testing.
#4
Head was broken. Re-testing.
#5
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:
This does look like the problem fixed by the first part of the patch. However, the second part of the patch does this:
<?php- $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:
<?php'#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:
<?php'#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.
#6
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.
#7
#8
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.
#9
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.
#10
The last submitted patch failed testing.
#11
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.
#12
This is no longer about translatable fields.
#13
The last submitted patch failed testing.
#14
Ready to review, from duplicate #595702: Warnings when adding taxonomy term-field with checkboxes wodget
#15
Re-roll.
#16
Tested this deeply. #11 exactly describes (checkboxes is array, radios is integer) - so tests cover both cases
#17
Committed to CVS HEAD. Thanks!
#18
Automatically closed -- issue fixed for 2 weeks with no activity.