Regression: Required radios throw illegal choice error when none selected

Radio buttons imho should *always* be filled in by default. They are weird in that once you select a radio you can never unselect them. The select or checkbox might be more appropriate in situations where one is using a radio without a default value...

You cannot *unselect* a radio I guess is my point - they are best used for anything which has a singular state which cannot be "null". In your example, you could set a default as "None of the above".

Ideally radios will be able to select the correct default e.g. #default_value => key($form['radios']['options']);
However, my gut is to postpone this issue because the work around is relatively straightforward, and there be scary things that live in the deep caverns of the formapi that i'm not sure we want to toy with at this stage.

Anyways, example code explaining the workaround

  // try unselecting this radio without hacking in firebug...
  // if you do hack it - it will throw no error - and presumably show up as null in whatever script eats it
  $form['typical'] = array(
    '#type' => 'radios',
    '#title' => 'The Funk Status',
    '#options' => array(
      0 => 'its off',
      1 => 'its on!',
    ),
   '#default_value' =>0
  );
  // in this example we use radios because we insist on answer 
  // however using the required flag will correctly catch anyone who hacks the values via firebug
  $form['paranoid'] = array(
    '#type' => 'radios',
    '#title' => 'You a communist?',
    '#options' => array(
      0 => 'None of your damn business',
      1 => 'No, im a dudist',
      2 => 'Yes, and you are a capitalist pig',
    ),
   '#default_value' => 0,
   '#required' => TRUE
  );

If anything it seems like this needs to be documented:
"#default_value is *required* for radios."

p.s.
While reviewing the issue I did uncover some additional weirdness.... If you want to set the default as "0" in a checkbox set, you actually have to set it as NULL - which is different then a radio which apparently understands what "0" means but not NULL.

  $form['paranoid'] = array(
    '#type' => 'checkboxes',
    '#title' => 'You a communist?',
    '#options' => array(
      0 => 'No, im an anarchist',
      1 => 'Yes, and you are a capitalist pig',
      2 => 'Dude, chill out - your messing up my vibes'
    ),
   // array(0) will no aworky 
   '#default_value' => array(null),
   '#required' => TRUE
  );

Bugs should be against Drupal 7, and Drupal 8 issues are unlikely to be postponed on any particular other issue at this point.... so I think something is wrong with the status here :)

At a minimum it seems worth fixing the "An illegal choice has been detected" issue mentioned in the original report - there is no reason Drupal should ever say that in this situation.

But yeah, I agree that radio buttons with nothing selected are a bad idea that Drupal should try to help people avoid (and possibly have the form API prevent them from doing altogether). I think that if a set of radio buttons does not have a default value, having the form API automatically select the first one would be reasonable. That would basically mimic the way a select element works.

For a dropdown select list in which a value must be entered, and we do not want the first one to be a default if no default is desired, so #735426: Fields with select widget: first option is selected by default even if no 'default value' set for the field had to specially provide a 'None' option.

Radio buttons have the natural capacity to handle this situation (no default, but choosing at least one is required) elegantly.

That it is not, and throwing an unhelpful error, is a bug.

Steps to recreate:

1. Create a content type with two Options module provided fields, they can be a list or a term reference.
2. Make one, 'test', a select list. Set required and set the maximum number of values to 1.
3. Make another, 'goodluckfindingthisonalargeform', a radio button / checklist field, also required and number of values of 1 (meaning it will display as radio buttons).

Here's what we get:

* An illegal choice has been detected. Please contact the site administrator.
* test field is required.

(Note also that the failing field is below the test field, but its error message is displayed first.)

If a radio button element is *not* required, then Drupal adds an NA option, which is necessary, if ugly, to allow people to unselect after selecting. (Would be interested in making an unselect link replace this when JavaScript is enabled, maybe in contrib for D7.) The NA option is not set as a default. To then say required radios must have a default, or have a useless NA or -None- field that then triggers a proper error like dropdown selects need, is definitely a regression.

There must be a way to get FormsAPI to identify what element triggered the error... right?

/me goes to make offerings to the gods of the FormsAPI... five listed in MAINTAINERS.txt!

Form API issues like this should start with extending or adding tests, i.e., move your expectations into tests. Patches will fail. Afterwards, we'll fix Form API to match those expectations.

A test AND stop gap fix for this bug happens to be contained in my patch over in #742344-18: Allow forms to set custom validation error messages on required fields by coincidence.

Let's keep this issue to figure out the desired behavior. If you'd ask me today, I'd say that

- #type 'radios' without a #default_value is bogus. Perhaps should even throw a PHP exception during form building.

- #type 'radios' should "automatically" be #required. It makes no sense that you can successfully submit a form containing #type 'radios' without a value for it. Radio buttons require input to be valid.

- it's therefore technically even valid to show a required marker on the #type 'radios' label/heading.

Use case for unselected radios (without default value) are different type of pools/questionnaires when you require some user input but don't want to suggest any answer or you want to check if all questions has been answered.

Bugs that affect error feedback on every single form in Drupal, to me sound like major issues. Which should be fixed before release, or after release.

Here's a fix.

Pulled one part (that fixes generic form radios) from sun's patch at #742344: Allow forms to set custom validation error messages on required fields
The other part (fixing Field radios) is mine.

Still needs tests.

Had trailing whitespace.

Now with tests! Based originally off Sun's tests in #742344: Allow forms to set custom validation error messages on required fields but pretty heavily modified. They fail without bojanz patch, and pass with it. It's a beautiful thing...

Patch in #13 looks good and is working for me.

We need to restart from scratch.

This is what we should do..... umph.

#18: drupal.form-radios-required.17.patch queued for re-testing.

Changing the meaning of #default_value = NULL across all form elements (as the above patches do) seems a little risky at this point in the D7 cycle.

Isn't there a way to fix this that involves adding a #value_callback function for radios? For example, like http://api.drupal.org/api/drupal/includes--form.inc/function/form_type_c... does for checkboxes? (There is already special-case code for validating selects and checkboxes scattered across the form API, so if we had to add some code that specifically targeted radios in the same places to make that value callback work, that doesn't sound like it would be the end of the world.)

This is also not a major bug. It's extremely rare to have a set of radio buttons that don't start off with something selected in the first place. (And as discussed above, doing so can itself be considered a bug on the part of whoever built the form.)

For the future, it's something to either prevent completely or deal with like #140783: A select list without #default_value always passes form validation does. But for now, just getting things back to the way they worked in Drupal 6 would be good enough.

@David_Rothstein the test-passing patch in #13 does not add default_value = NULL to all form elements, it is for options widget only.

@sun can you explain why it needs to start from scratch?

and at everybody-- the patch in #13 is targeted, passes tests, and prevents the horribly useless generic form error that users and administrators will hate.

^{benjamin, agaric}

I don't think sun's patch is going to fix the Field radios bug, because the Field module is passing FALSE there, no default is going to matter.

@David_Rothstein the test-passing patch in #13 does not add default_value = NULL to all form elements, it is for options widget only.

The patch in #13 changes the meaning of #default_value = NULL across all form elements via this code:

@@ -1952,7 +1952,7 @@ function _form_builder_handle_input_elem
       // Avoid image buttons (which come with garbage value), so we only get value
       // for the button actually clicked.
       if (!isset($element['#value']) && empty($element['#has_garbage_value'])) {
-        $element['#value'] = isset($element['#default_value']) ? $element['#default_value'] : '';
+        $element['#value'] = array_key_exists('#default_value', $element) ? $element['#default_value'] : '';

Now it may be the case that people who have code that sets #default_value to NULL don't care much one way or the other whether they start getting back NULL instead of an empty string, but it's a behavior change so we should be careful with it, and if there is a way to limit it to radios only I think it would be better at this stage.

@David_Rothstein - right, if someone *sets* default value to NULL, it returns it to them that way, not as an empty string. I can't imagine anyone relying on the other behavior.

Let me see.

Thanks for the test. I kept that one. But form.inc, that can be patched radios specific. It's a bit khm but it works.

Whether non-required-radios make sense is a different issue and discussion.

Note that this breaks AJAX as the option checker always fires so even if you #limit_validation_errors , this bug will kill your AJAX. I am still investigating another issue where we did did not see the error message from this during AJAX.

Has to go to D8 and come back unfortunately.

I'm still getting the unhelpful error message.

An illegal choice has been detected. Please contact the site administrator.

... on the structure/types/manage/article/fields/field_test page, for not selecting a default radio item on the taxonomy terms! Didn't even get to test on creating content. :-/

subscribe

subscribe

Re-roll of #29 for current 8.x checkout. Looks like part of it went in already.

Trying again...

And again...

The errors seem to come from the field_ui_field_edit_form() function in field_ui.admin.inc:

  // Add handling for default value if not provided by any other module.             
  if (field_behaviors_widget('default value', $instance) == FIELD_BEHAVIOR_DEFAULT && empty($instance['default_value_function'])) {
    $form['instance']['default_value_widget'] = field_ui_default_value_widget($field, $instance, $form, $form_state);
  }

Apparently the boolean field is a radio, but the field_ui_default_value_widget() function doesn't know what to do with it, so it adds a required radio button for the default value, labeled N/A and containing a value of '_none'.

The failing test code doesn't set this bizarrely required value, so fails with the famous "An illegal choice has been detected."

Attached is a screenshot produced during local testing.

I'm not sure what to do about this, so leaving as CNW for now.

This one lets tests pass but the change looks completely bogus. Can somebody help with this?

If you make a radios field required but dont set a default then submitting empty must give you an error and this error message is as good as any. So, that change does make sense.

@chx: Here's the change that I think is bogus:

interdiff helpful_error_on_required_radios-811542-41.patch helpful_error_on_required_radios-811542-45.patch 
only in patch2:
unchanged:
--- a/modules/field/modules/list/tests/list.test
+++ b/modules/field/modules/list/tests/list.test
@@ -291,6 +291,7 @@ class ListFieldUITestCase extends FieldTestCase {
     $off = $this->randomName();
     $allowed_values = array(1 => $on, 0 => $off);
     $edit = array(
+      'field_list_boolean[und]' => '_none',
       'on' => $on,
       'off' => $off,
     );

Can you explain why that particular naming convention is required for the test to pass? And can you explain how the test author could have anticipated that particular requirement? Because the only way I found it was through reverse-engineering, and that ain't the right way to do it.

sub

./modules/options/options.module:245: $options = array('_none' => $label) + $options;

The asked for explanation. Short of retooling options this won't happen. Also OMFG that this was help up for a month on this and noone helped :(

Okay, assume that I'm stupid. Really, I am, because your explanation makes absolutely no sense to me. Neither does the code to which you refer.

It appears to my feeble, uneducated mind that the forms API as modified by this patch requires that any radio buttons with "On" and "Off" choices must also include a third choice named "field_list_boolean[und]".

Will this documented in the forms API guide?

I agree that at the very least we could use a comment there to clarify what's going on. I'd write it myself before commit, but I too can't quite parse meaning out of #49.

Otherwise, though, the tests look great! This won't make it into 7.3, but should be able to get into next month's release.

After discussion with catch on IRC, I believe that the test should be setting the '#empty_option' key rather than doing what it does. Will re-roll when I get a chance.

Hi, I think I've corrected the validation error in a very simple manner, which seems to work in my admittedly limited testing. I'm attaching the patch. This is my first patch submission, so please be merciful :-). The patch is against the current dev version of form.inc in drupal 7 core.

Thanks for the patch! A couple of things:

a) Patches should be in "unified diff" format (with +s and -s instead of >s and <s) for readability. If you use git, git diff > foo.patch will do the right thing. If you're using just "diff", then "diff -up" should do it.

b) The patch in #45 comes with automated tests to prove that it works. We don't want to lose those in the final version, so could you please make a new patch that includes both your fix and the tests? This would also show that your fix is working properly.

c) Could you outline the reason that you chose a different approach than #45?

Thanks!

Oops. And restoring tag.

Before finding this here, i had opened another issue http://drupal.org/node/1229970 on the (partly) same topic, which i now closed.
But there's an aspect in it, which i am not sure if it is addressed here, that is regarding options with Label '0' not being rendered (see attachment).
As i am not good enough in programming to understand all talk here, i am not sure if i talk about the same effect as mentioned in #1, but i suspect it is another 'side'-bug that i found.

So maybe someone can look at this (now closed) issue and decide if that has to go here, or into yet another (new) issue?

As a User, my opinion is, that if drupal lets me create options with no default, then it should be able to validate it's requiredness correctly.
But if you want to adhere to W3C standards as issue http://drupal.org/node/621366 requests, then i am not sure if it makes sense to handle radios and checkboxes in the same widget 'Check boxes/radio buttons' (Which i don't understand the reason for anyways, but as i said, i am not an expert).

Updated patch incorporating suggested improvement from #53 and also adds the following code to the createListField() function:

diff --git a/modules/field/modules/list/tests/list.test b/modules/field/modules/list/tes
index dec0956..eade6eb 100644
--- a/modules/field/modules/list/tests/list.test
+++ b/modules/field/modules/list/tests/list.test
@@ -341,6 +341,10 @@ class ListFieldUITestCase extends FieldTestCase {
       'entity_type' => 'node',
       'bundle' => $this->type,
     );
+    if ($type === 'list_boolean') {
+      $instance['widget']['type'] = 'options_onoff';
+    }
+
     field_create_instance($instance);
 
     $this->admin_path = 'admin/structure/types/manage/' . $this->hyphen_type . '/fields

Passes tests locally, but I'm still not sure this is the correct approach.

Should have set version to 8.x-dev before uploading that patch. Not sure if it applies to 7.x.

Should have uploaded both testsonly and tests+fix patches.

Again, I'm not sure whether this is the right approach. Shouldn't the widget type be set in a field info hook somewhere?

Something looks wrong with the latest patch; it seems to contain both the previous form_type_radios_value() solution as well as the solution from #53, all in the same patch. We only need one of them :)

Regarding #53, it looks like it would probably solve the bug, but it seems like maybe it's working around the bug more than fixing it? An illegal choice error shouldn't occur in this situation at all, so ideally we want to fix the root cause of why it's occurring rather than catching the error right before it's displayed. A practical effect of this is that if a hacker does actually try to submit an illegal choice, with #53 we would have no way to distinguish it from someone who merely left the form blank, so the things that are supposed to happen for an actual illegal choice (e.g., the watchdog message) would be bypassed in the case of radios.

Okay, re-rolled. Still don't know whether this chunk is correct; would appreciate some feedback:

--- a/modules/field/modules/list/tests/list.test                                     
+++ b/modules/field/modules/list/tests/list.test                                     
@@ -341,6 +341,10 @@ class ListFieldUITestCase extends FieldTestCase {
       'entity_type' => 'node',                                                      
       'bundle' => $this->type,                                                      
     );                                                                              
+    if ($type === 'list_boolean') {                                                 
+      $instance['widget']['type'] = 'options_onoff';                                
+    }                                                                               
+                                                                                    
     field_create_instance($instance);                                               
                                                                                     
     $this->admin_path = 'admin/structure/types/manage/' . $this->hyphen_type . '/fields/' . $this->field_name;                                                          

subscribing for #1239910: META: tracking other issues about validation

Hi webchick. I 've looked at the approach of #45 and it seems more elegant than mine, which I came up with by stepping with a debugger through the code and intercepting the generic error thrown at the approrpiate moment. I'm reattaching my patch implemented with git -diff in case you or anyone else wants to take a look, its only a acouple of lines. Sorry, I don't know how, nor do I have the time to learn at this point how to add the testing code, but I guess the tests for the other patch should apply.
Best regards,
Tony Patrinos

CNR for patch in #61

Subscribe

I had this problem (or at least a similar one) discovered to be the cause of an incorrect usage of the 'reset()' function on the $default_value variable during form generation.
See: http://drupal.org/node/919348#comment-4544950

#61 works as-is to correct this on D7.8. Don't have time to test on 8.x-dev though; I need this project done...

the patch proposed by #67 seems to be really good and is much more simple. Thanks thekevinday.

Here is a reroll for d8 of the patch in http://drupal.org/node/919348#comment-4544950

When you decide to completely change the direction and scope of an issue, (highly discouraged), you should at least explain the difference between the formerly proposed patch and the new one, and justify why the new approach is superior.

Since the patch in #61 includes a test to prove that the change fixes the problem and to ensure that the problem does not re-appear in future versions, and since the patch in #69 does not include such a test, the patch in #61 should be presumed superior until proven otherwise.

Note that even if it included tests, the logic in #69 would be identical to the logic in #13, which also included tests but which was abandoned in favor of the current approach.

Therefore, re-rolling and re-uploading the patch from #61.

#69 applies cleanly to 7.8, but does not fix the issue.

#70 applies cleanly to both 7.8 and 8.x. It correctly fixes the issue.

Added one-line patch from #1301596-1: An illegal choice has been detected on Check boxes/radio buttons widget with radios

If you're going to do that IMO it needs to go back to CNR.

Sorry; you are correct, of course.

Simplified logic by only setting the '#default_value' property if one was actually available.

Wonder if this will pass tests?

Also including interdiff from the previously RTBC version, since the change is no longer a single line.

#75 causes [06-Oct-2011 11:24:34] PHP Parse error: syntax error, unexpected ';', expecting ')' in /var/www/modules/field/modules/options/options.module on line 113

Fixed patch attached.

Thanks, eosrei, but ... now that you've touched it, you can't mark it RTBC anymore! :-(

Yep, I know.

Looks like that change causes a test error, which I don't want to debug, so back to the (previously-RTBC) version in #70.

+    if ($type === 'list_boolean') {
+      $instance['widget']['type'] = 'options_onoff';
+    }
+

It'd be worth adding a comment here to explain why the widget needs changing - the patch was stalled for a while before this hunk was introduced. That also looks like a minor API change that could affect backport - how come the default is no good there?

+   * This is a functional test while testRequiredFields above attempts unit.

There is no useful information in this comment, we should make it useful (i.e. explain why), or remove it altogether.

No-one listed as maintainer of FAPI has given feedback on this issue since chx last posted a patch

@Catch -- I dunno why that chunk is required, but without it, tests fail. I'm no expert on the code in this issue; just been plugging away at the grunt work. Now it's time for smarter people than me to step in and take charge.

@catch That widget change is there because the default widget, 'option_buttons,' is broken. By fixing the requiredness of radios, it exposes the brokenness of that widget. The widget will pass a #required check even when no value is entered.

pillarsdotnet tried to fix that issue in comment 72 with my patch from #1301596: An illegal choice has been detected on Check boxes/radio buttons widget with radios, but it wasn't working yet. I have now gotten my patch working properly, so here is a combine patch of 70 and my fix.

Elaborating on the changes I've made, the first thing I've tweaked is that the option_buttons widget, when it has no default_value, sets the default value to FALSE instead of setting the default value to NULL. FALSE does not trigger #required errors because #required assumes that when it runs, a value is either an array, string, NULL or 0.
This isn't enough however, because it looks like #type radios doesn't handle #value == NULL well. When the value is NULL, it will select any falsy radio buttons when it themes each radio button, so I have also fixed radios to properly treat NULL to mean no radios selected.

I also fixed the test comment to be more useful.

Hopefully I haven't broken anything else :D

The patch in #83 fixes the issue and applies correctly in 7.9

Thanks for your work on this patch! I read through it and I have a few suggestions:

+++ b/includes/form.incundefined
@@ -2239,6 +2239,39 @@ function form_type_checkbox_value($element, $input = FALSE) {
+ * Helper function to determine the value for a radios form element.
+ *
+ * @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 is returned.
+ * @return
+ *   The data that will appear in the $element_state['values'] collection
+ *   for this element. Return nothing to use the default.

I would just say "Determines the value for a radios form element." Also, there should be a blank line above the @return. (These are things we are cleaning up for existing functions as a part of #1310084: [meta] API documentation cleanup sprint.)

See http://drupal.org/node/1354 for more information.

+++ b/includes/form.incundefined
@@ -2239,6 +2239,39 @@ function form_type_checkbox_value($element, $input = FALSE) {
+function form_type_radios_value(&$element, $input = FALSE) {
+  // User submitted.
+  if ($input !== FALSE) {
+    // User submission and nothing selected.
+    if (!isset($input)) {
+      // We do not want form builder to override our NULL with an empty string.
+      $element['#has_garbage_value'] = TRUE;
+      // There was a user submission so validation is a must. This way, the
+      // required checker will produce an error as necessary.
+      $element['#needs_validation'] = TRUE;
+    }
+    // Whatever the user submission was, that's the value.
+    return $input;
+  }
+  // Default value now.
+  if (isset($element['#default_value'])) {
+    return $element['#default_value'];
+  }
+  // NULL.

The inline comments could use a little work here. Try to be a little more clear and detailed, to explain what's going on for those who aren't familiar with the code. Also, the final comment isn't necessary.

Also, can we change #has_garbage_value to #has_invalid_value? (Unless I'm mistaken and #has_garbage_value already exists. If it does, we should leave it, but if it is being introduced by this patch, let's rename it.)

+++ b/modules/simpletest/tests/form_test.moduleundefined
@@ -338,6 +344,43 @@ function form_test_validate_form_validate(&$form, &$form_state) {
+ * Form constructor for simple #required tests.

Let's say, "Form constructor for a form to test the #required property."

Also, note that the Drupal 8.x patch will need to be rerolled, because the core directory structure for Drupal 8 has now changed. (For more information, see #22336: Move all core Drupal files under a /core folder to improve usability and upgrades). When the patch has been rerolled, please set the issue back to "Needs Review."

If you need help rerolling this patch, you can come to core office hours or ask in #drupal-gitsupport on IRC.

Finally, since we're adding tests to fix a bug, let's also upload the tests by themselves in a separate patch file to demonstrate that they fail without the fix.

I've reworked the comments a lot and moved everything for the new 8.x core location.
#has_garbage_value is already part of the API, so that has to stay as-is.

Also uploading only the tests.

Next time you might want to attach the test-only patch before the test+fix patch. As long as the last patch stays green, the issue stays at "needs review".

I've found that when the default_value of the radio element is '' (empty string) this patch produces the "Illegal action" message when a radio element is not required but no option has been selected.

This happens with opengraph_meta module, i.e. you can create a node with opengraph setting and a required image correctly, but if you edit the node you get the illegal action message, and without the patch, the edit action works just fine.

I've fixed this by adding a check in the form_type_radios_value that forces the default value to be null if it's empty string.

    // Force empty elements to be null.
    if ($element['#default_value'] == '') {
      $element['#default_value'] = NULL;
    }

If this is ok, I'll reroll the patch with this change.
Maybe it is something to solve locally module by module, but I don't know how many can be affected by this.

I feel like that's more a bug in the module, since '' isn't a valid option because there is no '' key in the #options array. I could go either way, though.

If we do do this, it should probably done more like this though, since '' could be a valid option.

    if (!isset($element['#options'][$element['#default_value']])) {
      $element['#default_value'] = NULL;
    }

Also not sure if we'd want to actually alter to #default_value like this, or if it would be better to leave it as is and just return NULL from form_type_radios_value.

Though I'm aware that this changes things a tad, what I would love to see in form creation with required radio fields is a *required* default value, AND the option to disallow the default on form submission.

This default value could be set to something like -none selected- and the value disallowed, where the goal is to force the end user to make an actual selection. Otherwise, *required* radio button fields are either pointless, or don't follow W3C.

Just my two cents, as a new Drupal user once unfamiliar with W3C Standards, and once frustrated by this very issue as it applied to my use case.

I'm setting this back to needs review so others will look it over.

@pcambra Having thought it over more, I feel like that's probably beyond the scope of this issue. Since this is specifically about the regression from D6 -> HEAD, I went back to see how D6 reacts to settings default_value => '' when '' isn't a valid options. It turns out D6 will have an illegal option error message as well. It doesn't print in the UI because of form_set_error ordering, but it would fill your watchdog log up with errors.

As it is now, I feel like having the error pop up very visibly is actually better.

@baby.hack Welcome new Drupaler! I'm not 100% sure I follow what you're trying to say. Do you mean you want a way to have an option show up as a radio button that would signify 'unselected'? While I'm not sure I agree that the change would be that helpful, I think all you should have to do for that is set one of your #options as array('' => '- non selected', ... ). That way it will show up, but if someone chooses it, Drupal will see '' as empty, and still show it's normal required field error message.

What @baby.hack is saying happens to be pretty important.
When it comes to government websites, I find that they don't care about how helpful or unhelpful it is. There may be laws that require a radio button to be unselected, requiring the user to select one and the law must be followed (not logic).

In the case of radio buttons I see three possible states for required fields,
Lets say I have the radio buttons with the following choices: 'A', 'B', and 'C', then the different possible states if the radio buttons field is required are:
1) Forcing a radio button to be selected, such as 'A'. (current drupal behavior)
2) Leave all radio buttons unchecked. (what I have been legally required to do in the past)
3) Append a 'None' checkbox, such that we have: '', 'A', 'B', and 'C', with '' selected.

In the case of a select lists, the same situation exists, but I do not think #2 is a possible state.

I haven't checked up on the accuracy of this in D7, but I have historically had to hack my local D6 core copies to remain compliant with the law.

@thekevinday True enough. I can see the use for this as a feature, but it is beyond the scope of this issue. If you make an issue about implementing this, and post a link in here, I'd be happy to look at it though.

I don't 100% follow the examples you give here though, so please elaborate a bit more in your new issue.

Thanks.

subscribe

@TonyK There is a working patch in #86. Have you tried it out? We need some people to review this.

Also, "subscribe" comments aren't needed anymore. Just click the "Follow" button at the top of the page.

Alright, #89 plus are related to a feature request that should be opened in a separate issue. (One might even exist already, so be sure to search.)

I've reviewed #86 and it seems like the right solution to me. The test is also thorough and covers the relevant scenarios for this issue.

Couple remarks:

+++ b/core/includes/form.incundefined
@@ -2954,7 +2989,7 @@ function form_process_radios($element) {
+        '#default_value' => isset($element['#default_value']) ? $element['#default_value'] : FALSE,

Can we add an inline comment here explaining why we are using FALSE? It's non-obvious if someone hasn't read this issue or seen the bit in options_field_widget_form().

+++ b/core/modules/field/modules/options/options.moduleundefined
@@ -105,7 +105,7 @@ function options_field_widget_form(&$form, &$form_state, $field, $instance, $lan
         // Radio buttons need a scalar value.
-        '#default_value' => $multiple ? $default_value : reset($default_value),
+        '#default_value' => $multiple ? $default_value : ($default_value ? reset($default_value) : NULL),

Adding more detail to the inline comment would be good here. Also, nesting ternary expressions is sometimes hard for code readability. In this case, the line is short enough that it might be okay. On the other hand, though, setting the $default_value a few lines up, above the $element += array(), would allow us to explain the logic a bit.

I think with that added detail, I'd be comfortable marking this RTBC.

Thanks for your continued work on this!

Do we need to add a similar fix for select elements?

Edit: Never mind; the other issue includes a legitimately "illegal choice." However, can we check what the select behavior is when submitting nothing in a required field?

Good idea. Here's a new patch with better documentation on the lines you mentioned.

Select boxes that are required and have no default value automatically add the empty option to the select, so there is always something to select.

Alright, #99 looks RTBC to me. Thanks @loganfsmyth.

+ * @param $input
+ *   The incoming input to populate the form element. If this is FALSE,
+ *   the element's default value is returned.
+ *
+ * @return
+ *   The data that will appear in the $element_state['values'] collection
+ *   for this element. Return nothing to use the default.

Above this it says "pass FALSE as $input to use the default.". Yet also "Return nothing to use the default.". Both of these can't be true at the same time?

+++ b/core/modules/simpletest/tests/form.testundefined
@@ -122,6 +122,45 @@ class FormsTestCase extends DrupalWebTestCase {
+   * This test programmatically submits a test form containing several types of
+   * input elements. The form is submitted twice, once with values and once

Do we need "This test"?

Is there a way to test this other than the absence of "an illegal choice has been detected" - if we change that string in another patch, that assertion will always be true.

#1162712: AJAX submissions try to validate other form fields: "An illegal choice has been detected." has been marked as a duplicate of this issue. (Thanks @beejeebus!)

Edit: The steps to reproduce there could be another way to test this.

Yeah, those comments aren't exactly the best. They are copied directly form the previous value_callbacks.

Really $input = FALSE is just a way to say that there wasn't any submitted value. Thus, most of the time the value returned should be the default value. If the value returned is NULL, then it will attempt to automatically check default_value if the element has not been marked as a garbage value.
I have removed the "Return nothing to use the default" line, since it's not particularly accurate, and I've added a new comment to try to elaborate.

I've removed "This test."

For testing, I'm not sure what a better way would be. This fails more than just the check for absence of that though, so maybe it's not the end of the world? We essentially just want to make sure that the #value ends up being NULL when no radios are selected.

Thoughts? I don't know if this is enough to address catch's points.

Maybe we could add a validation handler to the test form that checks the value and prints a debug message, then check for that debug message in the test?

I'm also working on a test based on #1162712: AJAX submissions try to validate other form fields: "An illegal choice has been detected.".

Here's that other test. Locally, it fails without #103 and passes with it.

Without trailing whitespace and with better comments.

Here's what I think we'd need to do with a validator to not rely on the specific error message text. Attached:

• Makes the assertion that checks for the "illegal choice" error message a little less specific, so that minor changes to the string won't silently break the test. (It will still need to be adjusted if we change the words "illegal choice," which honestly we probably should...)
• Adds a custom element validation handler that basically duplicates the logic from _form_validate() and prints a custom debug message under the conditions that would trigger the "illegal choice" error.
• Does not (yet) include the separate test from #107.

So we have a choice between string matching, code duplication, or both. Another thing we could check is watchdog (all the "illegal choice" errors get logged there), but I'm not sure if that would be overkill... thoughts?

Edit:

+++ b/core/modules/simpletest/tests/form_test.moduleundefined
@@ -346,6 +352,79 @@ function form_test_validate_form_validate(&$form, &$form_state) {
+    elseif ($element['#type'] == 'select' && !$element['#multiple'] && $element['#required'] && !isset($element['#default_value']) && $element['#value'] === $element['#empty_value']) {
+      drupal_set_message("Value is not allowed for {$element['#name']}.");
+    }

This hunk here is actually a mistake and should be removed.

_form_validate() repeats this bit twice:

form_error($elements, $t('An illegal choice has been detected. Please contact the site administrator.'));
watchdog('form', 'Illegal choice %choice in %name element.', array('%choice' => $elements['#value'], '%name' => empty($elements['#title']) ? $elements['#parents'][0] : $elements['#title']), WATCHDOG_ERROR);

I wonder if it should be in its own function? And, should there be some form property we set on the element that marks its value as disallowed? That would make it easier for form handling code (including our test) to react to this type of error.

I guess another (simpler) thing we could do would be to use xpath to simply fail on any warning that is not the "field is required" message.

Simpler test as per #112.

+++ b/core/modules/simpletest/tests/form.testundefined
@@ -122,6 +122,61 @@ class FormsTestCase extends DrupalWebTestCase {
+   * with values. Each submission is checked for relevant error messages. ¶

Grr.

I should roll a patch with the other test anyway.

#113 plus #107, both tests + the fix.

Not a full review, but a couple things:

+      // replace the our NULL with an empty string.

Typo.

+    $this->drupalPost('form-test/validate-required', $edit, 'Submit');
+    $this->assertNoFieldByXpath('//div[contains(@class, "error")]', FALSE, 'No error message is displayed when all required fields are filled.');

How about just testing for a successful form submission instead? For example, adding a submit handler to the form which does something like drupal_set_message("The form_test_validate_required_form form was submitted successfully") and then searching for that string. Seems more straightforward.

How about just testing for a successful form submission instead? For example, adding a submit handler to the form which does something like drupal_set_message("The form_test_validate_required_form form was submitted successfully") and then searching for that string. Seems more straightforward.

That would be straightforward enough for the second submission of the form test, but for the field test the form isn't actually being submitted completely. It's just adding one value to a multivalue field. Since both use the same assertion, I'm not sure whether it's worth changing one but not the other. (Though either way is fine by me, really.)

Correcting the typo @David_Rothstein pointed out in #116 and cleaning up the doxygen a little for clarity. I didn't modify the tests (see #117).

Fixing docblock indentation, no other changes from #118.

OK, I see what you mean in #117. However, the field test doesn't just check for absence of error messages; it already also checks for evidence that the form was successfully submitted, via the "Verify that the widget is added" block below:

+    // Verify that no error is thrown by the radio element.
+    $this->assertNoFieldByXpath('//div[contains(@class, "error")]', FALSE, 'No error message is displayed.');
+
+    // Verify that the widget is added.
+    $this->assertFieldByName("{$this->field_name}[$langcode][0][value]", '', 'Widget 1 is displayed');
+    $this->assertFieldByName("{$this->field_name}[$langcode][1][value]", '', 'New widget is displayed');
+    $this->assertNoField("{$this->field_name}[$langcode][2][value]", 'No extraneous widget is displayed');

So maybe a better thing to do for the form test is to keep the code that checks for no error messages, but also do a positive check that makes sure the form was actually submitted (using the method I described above). Then the two tests will actually be consistent.

***

I also tried reviewing a bit more of the patch. This is crazy complicated stuff, but (a) I think it makes sense, and (b) it isn't really any more complicated than some of the value callbacks that are already in core, so it seems good enough for now.

However, I felt like I was missing some motivation in the code comments of form_type_radios_value(). In particular this part:

+      // Flag as a garbage value so that form builder will not automatically
+      // replace the NULL value with an empty string.
+      $element['#has_garbage_value'] = TRUE;

What I missed here was an explanation of why we need the NULL preserved; i.e., what bad thing might happen if it went ahead and replaced it with an empty string instead? From this issue itself, as well as looking carefully at a few places in the form API code, I eventually understood why, but I think an extra sentence or two explaining that in the code comments would go a long way.

Ah, that makes sense. Submit handler it is.

WRT the inline comment, I thought about moving a hunk from the docblock there, but apparently I didn't.

Here's those two bits glued together:

Flag as a garbage value so that form builder will not automatically replace the NULL value with an empty string. If NULL is returned and #has_garbage_value is not set then, it (what function?) will automatically attempt to use default value or '' (an empty string).

And then probably one more sentence about why the empty string is bad?

How about:

Flag as a garbage value. If the value is set to NULL and #has_garbage_value is not set then, the form builder will automatically attempt to use default value or '' (an empty string). An empty string will fail validation because it is not in the allowed values list.

So:

1. Is this entirely correct?
2. Is this wording okay?

Edited a few times.

Edit: Ah, maybe we could use some of the explanation in #44.

+++ b/core/modules/field/tests/field.test
@@ -1459,6 +1459,47 @@ class FieldFormTestCase extends FieldTestCase {
+  function testFieldFormMultivalueWithRequiredRadio() {
...
+    // Verify that no error is thrown by the radio element.
+    $this->assertNoFieldByXpath('//div[contains(@class, "error")]', FALSE, 'No error message is displayed.');

Shouldn't this test continue one step more and also check that the required radio field actually throws a required error message when trying to submit the form?

Right now, it only tests that a required radio is not validated as required - which seems valid for the "Add another item" button, but not ultimately for the field value(s).

+++ b/core/modules/simpletest/tests/form.test
@@ -122,6 +122,61 @@ class FormsTestCase extends DrupalWebTestCase {
+  function testRequiredCheckboxesRadio() {

I don't see where this test actually determines and validates the expected starting condition: not having any #default_value in #required form elements.

1 days to next Drupal core point release.

Shouldn't this test continue one step more and also check that the required radio field actually throws a required error message when trying to submit the form?

That should be covered by the other test in the patch. The Field UI one is just a special case where the bug prevents the intermediate submission. I don't have a problem with adding it, but it seems to duplicate the main test.

I don't see where this test actually determines and validates the expected starting condition: not having any #default_value in #required form elements.

I'm not quite sure I understand what you mean by this. The test form (defined in form_test_validate_required_form()) does not have default values set on any of the fields. Do you mean that the text should also check that no value is set after the failed validation?

Regarding wording, something like #122 looks good to me, after fixing minor grammatical errors :)

I updated the #has_garbage_value text and added a submit handler, though I'm not sure I 100% followed the suggestions about the submit handlers. I think I did it right though.

I also fixed a bunch of small things, like static strings not wrapped in t(), and fixing the menu settings for the new form.

OKAY, one more time without the typo.

Re: Strings being wrapped in t() -- Not wrapping test assertions in t() was deliberate. No one is going to translate test assertions.

Edit: See #500866: [META] remove t() from assert message.

You are probably right for the string output in the submit handler, but all the rest should still have been.

Okay, I read it again and you're probably right about the FAPI texts. However, I think these changes aren't needed:

+++ b/core/modules/field/tests/field.testundefined
@@ -1493,12 +1493,12 @@ class FieldFormTestCase extends FieldTestCase {
     // Verify that no error is thrown by the radio element.
-    $this->assertNoFieldByXpath('//div[contains(@class, "error")]', FALSE, 'No error message is displayed.');
+    $this->assertNoFieldByXpath('//div[contains(@class, "error")]', FALSE, t('No error message is displayed.'));
 
     // Verify that the widget is added.
-    $this->assertFieldByName("{$this->field_name}[$langcode][0][value]", '', 'Widget 1 is displayed');
-    $this->assertFieldByName("{$this->field_name}[$langcode][1][value]", '', 'New widget is displayed');
-    $this->assertNoField("{$this->field_name}[$langcode][2][value]", 'No extraneous widget is displayed');
+    $this->assertFieldByName("{$this->field_name}[$langcode][0][value]", '', t('Widget 1 is displayed'));
+    $this->assertFieldByName("{$this->field_name}[$langcode][1][value]", '', t('New widget is displayed'));
+    $this->assertNoField("{$this->field_name}[$langcode][2][value]", t('No extraneous widget is displayed'));

+++ b/core/modules/simpletest/tests/form.testundefined
@@ -151,7 +151,7 @@ class FormsTestCase extends DrupalWebTestCase {
-        $this->fail("Invalid error message: {$error[0]}");
+        $this->fail(t("Invalid error message: !error", array('!error' => $error[0])));
       }

@@ -161,7 +161,7 @@ class FormsTestCase extends DrupalWebTestCase {
     foreach ($expected as $not_found) {
-      $this->fail("Error message not shown: $not_found");
+      $this->fail(t("Error message not shown: !error", array('!error' => $not_found)));

@@ -172,8 +172,9 @@ class FormsTestCase extends DrupalWebTestCase {
-    $this->assertNoFieldByXpath('//div[contains(@class, "error")]', FALSE, 'No error message is displayed when all required fields are filled.');
...
+    $this->assertNoFieldByXpath('//div[contains(@class, "error")]', FALSE, t('No error message is displayed when all required fields are filled.'));

...Though, I guess maybe the fail()s should probably use format_string() for sanitization of the variable. Edit: Though, passing them with the ! prefix probably nullifies that in any case.

I've removed the t() except the FAPI elements. I didn't realized that there was a push to move away from having t() calls in asserts, and I just wanted to be consistent with the other tests in the file.

I also changed the two strings with variables to use format_string.

I do not like the usage of #has_garbage_value here. So far, #has_garbage_value is only used by _form_button_was_clicked() in order to signify that, although there is a #value for an (image) button, it is "garbage" and not to be used as actual value.

However, I do not see a way to fix this differently - without entirely rewriting Form API's options checker. Thus, I'm fine with this change (but will create a follow-up issue for D8 to rewrite the options checker).

Furthermore, the change of #default_value from NULL to FALSE in form_process_radios() looks bogus to me. While theme_radio() currently checks that the #value is not FALSE before adding the 'checked' attribute, a #value of FALSE was never possible. I've double-checked the code throughout form.inc and I'm confident that this special condition for FALSE is just simply stale code and obsolete.

#1381140: A #default_value = 0 for #type radios checks all radios partially depends on this patch, and in fact, has quite some overlap as it also concerns the #value/#default_value handling of #type radios, so I merged the additional one-line tweak into this patch.

Lastly, I've slightly improved the tests and comments.

This looks pretty much ready to fly for me.

So I just retested both #134 and #133, and sun's changes do indeed break the poll tests (it's not just a testbot error).

A naïve interdiff of 133 to 134 is attached.

Oops.

Reverted the merged-in changes and re-opened #1381140: A #default_value = 0 for #type radios checks all radios

Interdiff is against #133.

+++ b/core/modules/simpletest/tests/form.testundefined
@@ -139,7 +139,7 @@
-    $this->drupalPost('form-test/validate-required', $edit, t('Submit'));
+    $this->drupalPost('form-test/validate-required', $edit, 'Submit');

@@ -175,7 +185,7 @@
-    $this->drupalPost('form-test/validate-required', $edit, t('Submit'));
+    $this->drupalPost(NULL, $edit, 'Submit');

+++ b/core/modules/simpletest/tests/form_test.moduleundefined
@@ -353,50 +353,49 @@
-    '#title' => t('Textfield'),
+    '#title' => 'Textfield',
...
-    '#title' => t('Checkboxes'),
...
+    '#title' => 'Checkboxes',
...
-    '#title' => t('Select'),
...
+    '#title' => 'Select',
...
-    '#title' => t('Radios'),
...
+    '#title' => 'Radios',
...
-    '#title' => t('Radios (optional)'),
...
+    '#title' => 'Radios (optional)',
...
-  $form['actions']['submit'] = array('#type' => 'submit', '#value' => t('Submit'));
+  $form['actions']['submit'] = array('#type' => 'submit', '#value' => 'Submit');

Most of the changes look like improvements to me, aside from these. Since these are actual interface text elements, many of which have existing strings, shouldn't they use t()? Not that it makes much difference in practice.

Alright, sun convinced me to leave the things in #139. Attached should fix the test failure in #138; I think it was a leftover bit from the other issue.

And I named my interdiff .patch again. darnit. :P

Marked #1426146: Add another item button causes an error, "An illegal choice has been detected. Please contact the site administrator" as duplicate of this issue.

The patch at #141 works for me in D8.

If anyone needs the patch for D7, the one at #45 still aplies cleanly and works.

The patch at #141 also applies and works in D7. You just have to tell patch where the files are located, because of the D8 directory structure changes.

So the backport should be trivial.

+1 for the #141.

So, who do I have to bake cookies for in order to get this thing applied (and backported), it's been a year of debating the tests & documentation?

Re: #148: Baking cookies or making snarky remarks won't help, but reviewing the latest patch and marking it RTBC if appropriate will.

Commit time?

With the patch in #141 applied to Drupal 7, the Context module breaks. It becomes impossible to add blocks to any context - attempting to do so results in a 'An illegal choice has been detected. Please contact the site administrator' error. I don't know if its the fault of context.module, or this patch, but there a problem that needs flagging one way or the other. If you think it's the fault of context module then feel free to mark this RTBC again, but I wonder how many other modules this will break.

#141 works on D7, haven't tested context module though.

mrfelton, I have tested the patch in #141 with context.module, and I was able to add/remove blocks with no problems.

#141: 811542-141.patch queued for re-testing.

Weird; why did the bot not NW it?

Needs a reroll I guess.

Reroll.
Docblock for form_type_tableselect_value() changed.

I can confirm that before the patch the steps reproduced the error.

The patch applied cleanly.

After the patch, I can confirm that the illegal choice message was removed and a required field replaced it.

Looks good to me!

Reroll is OK. Thanks @ryanissamson and @tim.plunkett!

Indeed good to go.

Wow! Great work on this, everyone.

I asked chx for a final sign off as a Form API maintainer, but it turns out he wrote the original patch way a long time ago. :P So since this has sign-off from sun, I think it's good to go. Great job with the tests. I found the documentation clear to explain what was happening.

Committed and pushed to 8.x. WOOHOO!

This will need a re-roll for 7.x, though.

The patch from #156 rerolled for D7.

Thanks, sun. :) Committed/pushed #162 to un-break 8.x. Back to 7.x and needs review.

#161 is green.

Re-uploading #161.

meep meep

Patch won't apply due to #1317620: Clean up API docs for includes directory, files starting with D-G, reroll around that docs change, leaving at RTBC.

I think this really needs backport to D6 either (I'm nto sure if I may add the tag).

@TonyK -- The summary says the issue does not occur on D6. If you are encountering this issue on D6, can you confirm whether the example form in the summary causes the error?

Committed and pushed to 7.x. Thanks!

This is probably worth mentioning in the release notes, actually.

#168: drupal-811542-168.patch queued for re-testing.

Uhm.

I'm sorry to reopen this. But i also had this "bug" for a long time. Never found a solution for it. Whenever i make a form radios and NOT make it required, if no option is selected the "An illegal choice.." error appears.
It sais this has been fixed in 7.13, I am currently on 7.14 and i have checked the latest patched, and the code seems there, but still i get the "An illegal choice.." error.

Is there something special i have to set? I have tried making it required, set the default_value to NULL, every time i get the illegal notice when i don't select an option. In my case it should be possible to not select an option. But i can't continue without selecting one..
Please some help

Edit: This notice only happend SINCE update to 7.14, before this update the notice didn't came up. It showed my custom errors normally. Since this last commit patched (7.14) i see the error again before i could commit without any problem

It is possible that the old issue here: #919348: unset form values, such as radio buttons improperly trigger 'An illegal choice has been detected' does not get fixed by this patch, because of the improper usage of reset() (which requires an array argument).

This is only a guess, but it may be why @timmetj is still having trouble.
Here is a patch against 7.14 that adds an is_array() test.

An alternative method to the supplied patch would be to always cast $default_value to an array on load, such as:

<?php
$default_value = (array) _options_storage_to_form($items, $options, $value_key, $properties);
?>

However, this produces a new problem.
The line that has the reset() looks like:

<?php
$default_value = $default_value ? reset($default_value) : NULL;
?>

Because this is directly checking to see if $default_value is defined, the boolean check would pass and reset($default_value) would be returned.
The problem is that, if $default_value is an empty array, then a non-null value is returned from reset($default_value).
In fact, according to php documentation, FALSE is returned.
FALSE !== NULL and might produce an invalid choice problem because FALSE (aka: 0, which is also !== NULL) may not be a valid choice in the select list or radio buttons.

This is why I do an is_array() check in the patch instead of simply casting $default_value to an array().

I am also seeing this issue when using the Webform module. In Drupal 7.12 I did not have any issue, now I am receiving the "An illegal choice has been detected. Please contact the site administrator." error every time a form is submitted that has radio buttons that are not required and have no default.

A detailed description of our issue can be found in the Webform Issue queue at http://drupal.org/node/1565696

I do think this is a regression from 7.12, and currently it is not possible to upgrade to 7.14 due to forms with this type of component no longer being able to be submitted without values set.

I have the same problem with Delta and Context, as reported here http://drupal.org/node/1512744 and the problem started with the upgrade to 7.14.

FALSE !== NULL and might produce an invalid choice problem because FALSE (aka: 0, which is also !== NULL) may not be a valid choice in the select list or radio buttons.

This hits it right on the head. Prior to Drupal 7.14, submitting radio buttons with no value would return NULL. In 7.14, it returns FALSE. This causes validation problems in some places (such as Webform) that were depending strictly upon NULL for checking (as it should, because values like "0" or (int) 0 are valid).

This is why I do an is_array() check in the patch instead of simply casting $default_value to an array().

I don't think using is_array() is going to help in 100% of scenarios. Since if the array is empty, you'll still get returned FALSE when we want NULL for consistency. Something like this would work but is ugly as sin:

       if (!$multiple) {
-        $default_value = $default_value ? reset($default_value) : NULL;
+        $default_value = $default_value ? reset($default_value) : NULL;
+        $default_value = $default_value !== FALSE ? $default_value : NULL;
       }

I have tested both options, but none worked for me. Still getting the message. But the line you are talking about is for creating the form element with/without a default value not? The error appears when you want to submit a form. So somewhere in the validation functions?

Escalating. Sorry for breaking stuff. :( :(

Marked #1560062: After Drupal 7.14 core update, getting error Illegal choice 0 in notify_type element as a duplicate of this.

This will need to be fixed in 8.x again then backported.

Was trying to fix this in drupal7, so just to make sure I didn't break anything. Will move it back to D8 after bot

BTW: the patch in 188 solves the problem for webforms on drupal7 (but might break other things)