"An illegal choice has been detected" when a radio/checkbox option key contains a quote character

Max, thanks for reporting the error. I'm moving this issue to 7.x-dev, because that's where bugs are fixed. Also marking this as major, because this is obviously annoying, but it doesn't break entire websites.

I cannot reproduce this bug using Drupal 7 beta 3.
MXT, can you check if this error persists in the latest dev version, and if so, re-open this issue?

MXT: sorry, I don't quit understand what you mean with "TEST #2". Are the instructions from comment #3 still the way to reproduce the error?

Another question: when you tested this with beta 3, did you create a completely clean install, with fresh code and a completely new database? Or did you update an older test site?

OK, I have reproduced the problem.

Steps:
- Install D7 dev, checked out from CVS today, with the minimal install profile
- Enable field_ui, list and opttions modules
- Create a content type
- Add a field of type 'List (text)' with widget 'Check boxes/radio buttons'
- Define the following allowed values:

test0
te'st1
2|test2
3|te'st3
test4|test4
te'st5|te'st5
te"st6
test7|te'st7

- Try to create 6 new nodes, one for each list field option

Results:

test0         - ok
te'st1        - error
2|test2       - ok
3|te'st3      - ok
test4|test4   - ok
te'st5|te'st5 - error
te"st6        - error
test7|te'st7  - ok

Conclusion so far:
The error occurs in the handling of the option key, not the option label. Both single and double quotes trigger the error.

To be continued. I'd like a committer to decide if this is critical. Wondering what the best fix would be - validating the field settings form, or automatically escaping option keys?

Just checked if this works in D6. It does, so this is a regression.

Slightly off topic:
That said, it doesn't work great in D6, because the quotes produce loads of invalid HTML, for instance:

<div class="form-item" id="edit-field-radiotest-value-te"st6-wrapper">
 <label class="option" for="edit-field-radiotest-value-te"st6"><input type="radio" id="edit-field-radiotest-value-te"st6" name="field_radiotest[value]" value="te&quot;st6"   class="form-radio" /> te"st6</label>
</div>

Actually, I think this is not critical, because #11 shows a usable workaround: removing the quotes from the option key, for example test7|te'st7, solves the problem (I will update comment #11 to show that explicitly).

Of course, "not critical" doesn't mean we can't fix it before RC1 is released :-)

One thing we have learned so far, is that the error occurs only with the 'List (text)' type of field, not with the normal 'List' field (that's also why I couldn't reproduce this in #5). Going through the code, I found a description of this field type in list_field_info():
"This field stores keys from key/value lists of allowed values where the stored key has significance and must be a varchar, i.e. \'US States\': IL|Illinois, IA|Iowa, IN|Indiana"
(Makes me wonder where in the UI this description is displayed, but that's a different matter.) Anyway, because it is explicitly meant for significant alphanumerical keys, I think we should A) try not to restrict the allowed characters, and B) not silently remove the quotes from the keys. That means we shouldn't make the form validation more strict, but leave the quotes as they are and improve the way keys with quotes are handled.

I'll just keep documenting everything I find :-)
The error message is generated in form.inc, _form_validate(), around line 1200:

elseif (!isset($options[$elements['#value']])) {
  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);
}

What happens is that the submitted form value (here available as $elements['#value']) is encoded, so te'st becomes te'st1. The allowed values in the $options array are not encoded, so !isset($options[$elements['#value']]) returns true and the error is triggered.

The encoding of the radio value happens when the following functions are called: theme_radio() -> drupal_attributes() -> check_plain() -> htmlspecialchars().

By the way, commenting the elseif-block above triggers an error in list_field_validate(): "[fieldname]: illegal value." In other words, the form value is validated twice and both validators do not take into account that the value has been encoded along the way.

Looks like we have to introduce htmlspecialchars_decode() somewhere to fix this.

Reassigning to form API then.

FAPI radios / checkboxes take #options, but if an option contains html special chars, you end up with a submitted #value that's not part of #options.
It sounds reasonable that FAPI radios / checkboxes should decode entities before assigning a #value.

Doesn't need Dries / webchick feedback at this point.

Hmm, I guess I'm a bit stuck here, I got lost in the Form API internals...
First, I tried fixing this with inserting this at line 1186 of form.inc, in _form_validate():

if ($elements['#type'] == 'radios') {
  $elements['#value'] = htmlspecialchars_decode($elements['#value']);
}

That works for the "An illegal choice has been detected" error generated by _form_validate(), but the "[fieldname]: illegal value" error from list_field_validate() is still triggered. What happens is this:
- drupal_validate_form() calls _form_validate($form, $form_state, $form_id);
- in _form_validate(), a recursive function, the $form array or one of its elements is called $elements and gets validated
- near the end of _form_validate(), it calls form_execute_handlers('validate', $elements, $form_state);
- form_execute_handlers() passes on $form_state to be validated, which eventually leads to list_field_validate()
WTF? Why does _form_validate() validate the $form array while list_field_validate() validates $form_state? Is that correct? I don't get it.

So now I'm wondering, should the fix above simply be repeated in list_field_validate()? Or is there a better place to decode the value, preferably only once? Or is there a completely different method to solve this?

I don't think this is a problem with FormAPI. The following code works fine:

function checkbox_apostrophe_form() {
  $form = array();
  
  $form['names'] = array(
    '#type' => 'checkboxes',
    '#title' => t('Names'),
    '#options' => array(
      "o'brien" => t("O'brien"),
      'smith' => t('Smith'),
    ),
  );  
  
  $form['actions']['submit'] = array('#type' => 'submit', '#value' => t('Save permissions'));    
  
  return $form;
}

function checkbox_apostrophe_form_submit($form, &$form_state) {
  dsm($form_state);
}

This produces a form with a checkbox

<input type="checkbox" class="form-checkbox" value="o'brien" name="names[o'brien]" id="edit-names-obrien">

and it submits just fine.

Hence I think this is specific to FieldAPI.

I'm sorry but I can reproduce the bug with just the code posted above

$form['names'] = array(
    '#type' => 'checkboxes',
    '#title' => t('Names'),
    '#options' => array(
      "o'brien" => t("O'brien"),
      'smith' => t('Smith'),
    ),
  ); 

The HTML for the input does contain value="o'brien" (which seems only natural since this all runs through drupal_attributes() / check_plain().
Select "O'brien", submit --> "An illegal choice has been detected" - o'brien is not in #options.

You were not supposed to break checkboxes ever again. There are a lot of tests. Add more.

It seems intermittent!

I've put the above code into its own module (rather than hack it into whatever I was working on at the time), and I got the error once, but now I don't get it at all.

[crosspost with #23]

Looks like we'll need to add a form_type_radio_value() callback as well, then ?

Yes, we need to do something for radio values as well.

I'm not sure if the patch in #22 is what we need, I wasn't able to reproduce the error with checkboxes. Still, I did try an identical approach for radio's, adding the following function to form.inc:

function form_type_radio_value($element, $input = FALSE) {
  if ($input === FALSE) {
    return isset($element['#default_value']) ? $element['#default_value'] : 0;
  }
  else {
    return isset($input) ? htmlspecialchars_decode($element['#return_value'], ENT_QUOTES) : 0;
  }
}

Unfortunately that doesn't work.

I also tried changing line 2824 in form_process_radios():
'#return_value' => check_plain($key),
to this:
'#return_value' => $key,
That does solve the problem, but I'm not sure if it's allowed to remove the check_plain. (Can anyone explain what check_plain is doing there in the first place? It cannot be for rendering the radio button value, because drupal_attributes takes care of that, including the check_plain.) When fixing it like this, I also needed to make a small change in theme_radio in order to get the 'checked' attribute right. Patch attached.

My mistake. Try again.

#27: 954628-27-radio-value-quotes.patch queued for re-testing.

I guess this patch needs to fix the test as well.

Like marcvangend, I too was unable to reproduce the checkbox issue.
Looks like that is fixed.

With chx's consent, I am marking this duplicate in favor of #971120: Radio button values get run through check_plain() twice, since that issue has the same fix as this one (for the radios), but with tests included.

A related bugfix is at #811542: Regression: Required radios throw illegal choice error when none selected.

bojanz, that's great, thanks.

I have the same problem - however its for drupal 6.

i added img html in the allowed value list so users could pick an image to go with their article.

<img src='http://www.example.com/images/halfstars.png'></img>
<img src='http://www.example.com/images/onestars.png'></img>

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

Steps to reproduce:

Manage Fields-->New Field-->Text-->Check Boxes/Radio Buttons

Number of Values: 2

Allowed Values:

<img src='http://www.example.com/images/halfstars.png'></img>
<img src='http://www.example.com/images/onestars.png'></img>

When creating/editing the node, the images are present with the check box, but when saving you get the illegal choice error. Just putting plain text minus ' works.

Is it related at all to this in bootstrap.inc?

function check_plain($text) {
  return drupal_validate_utf8($text) ? htmlspecialchars($text, ENT_QUOTES) : '';
}

I'm no programmer so...