Remove #process pattern from option widgets

AWESOME.

+++ modules/field/modules/options/options.module	17 Nov 2009 15:56:22 -0000
@@ -67,285 +47,174 @@ function options_field_widget_info() {
+    case 'options_buttons':
+      $type = $multiple ? 'checkboxes' : 'radios';
+      // If required and there is one single option, preselect it.
+      if ($element['#required'] && count($options) == 1) {
+        $default_value = array(key($options));
+      }

This should only happen for radios, not for checkboxes.

+++ modules/field/modules/options/options.module	17 Nov 2009 15:56:22 -0000
@@ -67,285 +47,174 @@ function options_field_widget_info() {
+ * FAPI function to validate options element.

This should be:

Form element validation handler for options element.

+++ modules/field/modules/options/options.module	17 Nov 2009 15:56:22 -0000
@@ -67,285 +47,174 @@ function options_field_widget_info() {
+function options_options($field, $instance, $zero_placeholder) {

ugh. ;)

Can't we name this options_allowed_values() or will Field API freak out then?

+++ modules/field/modules/options/options.module	17 Nov 2009 15:56:22 -0000
@@ -67,285 +47,174 @@ function options_field_widget_info() {
+  // Add an empty choice for :
+  // - non required radios,
+  // - non required selects.

We can remove the trailing colon/white-space here. And, list items like these should not have punctuation.

+++ modules/field/modules/options/options.module	17 Nov 2009 15:56:22 -0000
@@ -67,285 +47,174 @@ function options_field_widget_info() {
+    if (($instance['widget']['type'] == 'options_buttons' && ($field['cardinality'] == 1))
+      || ($instance['widget']['type'] == 'options_select')) {

Strange wrapping here?

+++ modules/field/modules/options/options.module	17 Nov 2009 15:56:22 -0000
@@ -67,285 +47,174 @@ function options_field_widget_info() {
+ * Translate values as stored in the database to the format the widget needs.

"in the database"? That isn't necessarily true, right? So how about:

Transform stored values into the format the widget needs.

+++ modules/field/modules/options/options.module	17 Nov 2009 15:56:22 -0000
@@ -67,285 +47,174 @@ function options_field_widget_info() {
+function _options_data2form($items, $options, $column, $zero_placeholder) {
...
+function _options_form2data($element) {

I wonder:

1) Why these are private?

2) What data means?

3) What the number does in there? ;)

So how about:

options_transform_storage_form()
options_transform_form_storage()

+++ modules/field/modules/options/options.module	17 Nov 2009 15:56:22 -0000
@@ -67,285 +47,174 @@ function options_field_widget_info() {
+ * Translate submitted form values to the field storage format.

Transform submitted form values into field storage format.

+++ modules/field/modules/options/options.module	17 Nov 2009 15:56:22 -0000
@@ -67,285 +47,174 @@ function options_field_widget_info() {
   if (empty($values)) {
-    $values[] = NULL;
+    // @todo Apparently not needed IRL, but tests break without.
+    $values = array(NULL);
   }

erm. 8)

If the tests break without, then we can be sure that some code will break, too.

+++ modules/field/modules/options/options.module	17 Nov 2009 15:56:22 -0000
@@ -67,285 +47,174 @@ function options_field_widget_info() {
+  $result = options_transpose_array_rows_cols(array($element['#value_key'] => $values));

oh - we have a similarly named function already? Nice. :)

I guess this one flips rows to columns and vice-versa?

How about renaming to:

options_array_flip() ?

Or something else, so developers aren't confused by those function names like me currently :P

+++ modules/field/modules/options/options.test	17 Nov 2009 15:16:27 -0000
@@ -190,41 +210,197 @@ class OptionsWidgetsTestCase extends Dru
+    debug($e);
+    debug($values);
+    debug($expected_values);

uh oh

+++ modules/simpletest/drupal_web_test_case.php	17 Nov 2009 13:59:09 -0000
@@ -1610,20 +1610,28 @@ class DrupalWebTestCase extends DrupalTe
+            if (is_array($new_value) && empty($new_value)) {
+              // Multiple select box with no options selected: do not include
+              // any POST data for the element.
...
+                if (is_array($new_value)) {
...
+                elseif ($new_value == $option['value']) {

Can we move this inline comment above the condition, and also add similar ones to the other conditions?

I'm on crack. Are you, too?

Lovely. Thanks for the test coverage too. Subscribe.

Lovely.

+++ modules/field/modules/options/options.module	18 Nov 2009 01:36:19 -0000
@@ -67,290 +47,178 @@ function options_field_widget_info() {
+ * Prepares for a field.
...
+function options_get_options($field, $instance, $zero_placeholder) {
@@ -380,24 +248,10 @@ function options_transpose_array_rows_co
- * Helper function for finding the allowed values list for a field.
- *
- * See if there is a module hook for the option values.
- * Otherwise, try list_allowed_values() for an options list.
...
-function options_options($field, $instance) {

Only remaining issue: This PHPDoc summary is a bit short ;) Also not sure whether the original description wasn't helpful, too.

This review is powered by Dreditor.

Great!

We can tackle any @todos in follow-up issues, if necessary.

This change looks consistent with others that Dries has committed recently, and comes with buckets of nice tests. That _0 thing is too bad; hopefully we can get that sorted out in #179932: Required radios/checkboxes are not validated once and for all.

Committed to HEAD.

Why were the widget tests changed from using drupal_get_form() and drupal_render() to using drupalGet(), or in other words from being pure API unit tests to being page-request system tests?

It also makes the tests much slower, less targeted, and impossible to run on the command line (at least for me). But okay.

BTW, you've done a ton of work recently while I've been MIA. Thanks!