The following FAPI code creates a set of radio buttons with none selected. If the user then submits the form without selecting an option, drupal outputs "An illegal choice has been detected. Please contact the site administrator." instead of "test is required". This error only happens when #required is set to TRUE.

<?php
$form
['test'] = array(
   
'#type' => 'radios',
   
'#title' => 'test',
   
'#options' => array(
     
0 => 'cat',
     
1 => 'dog',
     
2 => 'turkey',
     
3 => 'chicken',
    ),
   
'#required' => TRUE,
  );
?>

Doing a little digging, it seems that #needs_validation is only set to TRUE when #required is set to TRUE (As #value is apparently not set if not value is posted), which is why the error only happens when #required is set.

I have no idea how to go about fixing this, but it works as expected in Drupal 6, so I'd say this is a regression.

Files: 
CommentFileSizeAuthor
#265 radios-regression-811542-265-TESTS-ONLY.patch1.43 KBDavid_Rothstein
FAILED: [[SimpleTest]]: [MySQL] 39,267 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#265 radios-regression-811542-265.patch4.03 KBDavid_Rothstein
PASSED: [[SimpleTest]]: [MySQL] 39,262 pass(es).
[ View ]
#261 interdiff-249-261.txt1.45 KBDavid_Rothstein
#261 radios-regression-811542-261-D7-do-not-test.patch4.03 KBDavid_Rothstein
#261 radios-regression-811542-261.patch4.22 KBDavid_Rothstein
PASSED: [[SimpleTest]]: [MySQL] 37,111 pass(es).
[ View ]
#249 radios-regression-811542-249.patch4.13 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 37,106 pass(es).
[ View ]
#248 i811542-failing-D8-do-not-test.patch1.13 KBeffulgentsia
#248 i811542-failing-D7-do-not-test.patch1010 byteseffulgentsia
#247 i811542-failing.patch1.67 KBattiks
FAILED: [[SimpleTest]]: [MySQL] 37,082 pass(es), 41 fail(s), and 25 exception(s).
[ View ]
#244 radios-regression-811542-244-D7-do-not-test.patch4.4 KBDavid_Rothstein
#244 radios-regression-811542-244-TESTS-ONLY.patch1.59 KBDavid_Rothstein
FAILED: [[SimpleTest]]: [MySQL] 37,101 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#244 radios-regression-811542-244.patch4.59 KBDavid_Rothstein
PASSED: [[SimpleTest]]: [MySQL] 37,105 pass(es).
[ View ]
#239 radios-regression-811542-239.patch18.81 KBDavid_Rothstein
FAILED: [[SimpleTest]]: [MySQL] 37,323 pass(es), 6 fail(s), and 0 exception(s).
[ View ]
#232 radios-regression-811542-232-D7-do-not-test.patch6.47 KBDavid_Rothstein
#232 radios-regression-811542-232-TESTS-ONLY.patch12.9 KBDavid_Rothstein
FAILED: [[SimpleTest]]: [MySQL] 37,052 pass(es), 8 fail(s), and 0 exception(s).
[ View ]
#232 radios-regression-811542-232.patch19.41 KBDavid_Rothstein
PASSED: [[SimpleTest]]: [MySQL] 37,037 pass(es).
[ View ]
#231 radios-regression-811542-231.patch5.78 KBDavid_Rothstein
PASSED: [[SimpleTest]]: [MySQL] 36,807 pass(es).
[ View ]
#228 radios-regression-811542-228.patch4.39 KBDavid_Rothstein
FAILED: [[SimpleTest]]: [MySQL] 36,814 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#224 radios-regression-811542-224.patch834 bytesDavid_Rothstein
FAILED: [[SimpleTest]]: [MySQL] 36,827 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#204 drupal-8.x-811542-204.patch1.36 KBthekevinday
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#191 i811542-191.patch592 bytesattiks
PASSED: [[SimpleTest]]: [MySQL] 36,578 pass(es).
[ View ]
#188 i811542-188.patch571 bytesattiks
FAILED: [[SimpleTest]]: [MySQL] 38,670 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#179 issue_811542_after_d7.14-1.patch671 bytesthekevinday
FAILED: [[SimpleTest]]: [MySQL] 39,054 pass(es), 8 fail(s), and 0 exception(s).
[ View ]
#168 drupal-811542-168.patch11.34 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-811542-168.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#166 drupal-811542-161.patch11.35 KBpillarsdotnet
PASSED: [[SimpleTest]]: [MySQL] 38,306 pass(es).
[ View ]
#162 drupal8.radios-test-language.patch553 bytessun
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.radios-test-language.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#161 drupal-811542-161.patch11.35 KBDamienMcKenna
PASSED: [[SimpleTest]]: [MySQL] 38,299 pass(es).
[ View ]
#157 Screen Shot 2012-03-07 at 11.57.55 AM.png20.27 KBryanissamson
#157 Screen Shot 2012-03-07 at 11.57.44 AM.png16.27 KBryanissamson
#156 drupal-811542-156.patch11.43 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 35,149 pass(es).
[ View ]
#141 811542-141.patch11.45 KBxjm
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 811542-141.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#141 interdiff-138-141.patch1.32 KBxjm
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch interdiff-138-141.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#138 drupal8.form-radios-value.138.patch11.29 KBsun
FAILED: [[SimpleTest]]: [MySQL] 34,114 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#138 interdiff.txt10.64 KBsun
#136 interdiff.txt12.52 KBxjm
#134 drupal8.form-radios-value.134.patch12.47 KBsun
FAILED: [[SimpleTest]]: [MySQL] 34,110 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#133 radio-required-regression-811542-132.test.patch7.27 KBloganfsmyth
FAILED: [[SimpleTest]]: [MySQL] 34,402 pass(es), 4 fail(s), and 0 exception(es).
[ View ]
#133 radio-required-regression-811542-132.patch11.19 KBloganfsmyth
PASSED: [[SimpleTest]]: [MySQL] 34,086 pass(es).
[ View ]
#133 radio-required-regression-811542-interdiff-119-132.txt5.78 KBloganfsmyth
#128 radio-required-regression-811542-128.test.patch7.27 KBloganfsmyth
FAILED: [[SimpleTest]]: [MySQL] 34,243 pass(es), 21 fail(s), and 8 exception(es).
[ View ]
#128 radio-required-regression-811542-128.patch11.19 KBloganfsmyth
FAILED: [[SimpleTest]]: [MySQL] 34,258 pass(es), 11 fail(s), and 4 exception(es).
[ View ]
#128 radio-required-regression-811542-interdiff-119-128.txt7.03 KBloganfsmyth
#126 radio-required-regression-811542-126.test.patch7.27 KBloganfsmyth
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/field/tests/field.test.
[ View ]
#126 radio-required-regression-811542-126.patch11.19 KBloganfsmyth
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/field/tests/field.test.
[ View ]
#126 radio-required-regression-811542-interdiff-119-126.txt7.03 KBloganfsmyth
#119 811542-119.patch9.96 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 34,127 pass(es).
[ View ]
#118 811542-118.patch9.96 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 34,128 pass(es).
[ View ]
#118 interdiff-115-118.txt1.9 KBxjm
#115 811542-two-tests-combined.patch10.04 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 34,127 pass(es).
[ View ]
#113 811542-113-tests.patch4.6 KBxjm
FAILED: [[SimpleTest]]: [MySQL] 34,093 pass(es), 2 fail(s), and 0 exception(es).
[ View ]
#113 811542-113-combined.patch8.01 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 34,102 pass(es).
[ View ]
#113 interdiff-103-113.txt3.43 KBxjm
#110 811542-110-tests.patch5.77 KBxjm
FAILED: [[SimpleTest]]: [MySQL] 34,098 pass(es), 4 fail(s), and 0 exception(es).
[ View ]
#110 811542-110-combined.patch9.18 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 34,104 pass(es).
[ View ]
#110 interdiff-103-110.txt4.86 KBxjm
#107 required_radio_multivalue_test-107.patch2.03 KBxjm
FAILED: [[SimpleTest]]: [MySQL] 34,109 pass(es), 2 fail(s), and 0 exception(es).
[ View ]
#106 required_radio_multivalue_test.patch2.03 KBxjm
FAILED: [[SimpleTest]]: [MySQL] 34,102 pass(es), 2 fail(s), and 0 exception(es).
[ View ]
#103 helpful_error_on_required_radios-811542-103.test.patch4.04 KBloganfsmyth
FAILED: [[SimpleTest]]: [MySQL] 34,112 pass(es), 2 fail(s), and 0 exception(es).
[ View ]
#103 helpful_error_on_required_radios-811542-103.patch7.79 KBloganfsmyth
PASSED: [[SimpleTest]]: [MySQL] 34,120 pass(es).
[ View ]
#99 helpful_error_on_required_radios-811542-99.test.patch4.05 KBloganfsmyth
FAILED: [[SimpleTest]]: [MySQL] 34,029 pass(es), 2 fail(s), and 0 exception(es).
[ View ]
#99 helpful_error_on_required_radios-811542-99.patch7.72 KBloganfsmyth
PASSED: [[SimpleTest]]: [MySQL] 34,029 pass(es).
[ View ]
#86 helpful_error_on_required_radios-811542-86.patch7.29 KBloganfsmyth
PASSED: [[SimpleTest]]: [MySQL] 33,817 pass(es).
[ View ]
#86 helpful_error_on_required_radios-811542-86.test.patch4.05 KBloganfsmyth
FAILED: [[SimpleTest]]: [MySQL] 33,818 pass(es), 2 fail(s), and 0 exception(es).
[ View ]
#83 helpful_error_on_required_radios-811542-83.patch7.33 KBloganfsmyth
PASSED: [[SimpleTest]]: [MySQL] 33,661 pass(es).
[ View ]
#80 helpful_error_on_required_radios-811542-70.patch7.11 KBpillarsdotnet
PASSED: [[SimpleTest]]: [MySQL] 33,318 pass(es).
[ View ]
#76 helpful_error_on_required_radios-811542-76.patch7.09 KBeosrei
FAILED: [[SimpleTest]]: [MySQL] 33,312 pass(es), 1 fail(s), and 0 exception(es).
[ View ]
#75 helpful_error_on_required_radios-811542-74.patch8.14 KBpillarsdotnet
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in modules/field/modules/options/options.module.
[ View ]
#75 interdiff-70-74.txt808 bytespillarsdotnet
#72 helpful_error_on_required_radios-811542-72.patch7.95 KBpillarsdotnet
FAILED: [[SimpleTest]]: [MySQL] 33,307 pass(es), 1 fail(s), and 0 exception(es).
[ View ]
#70 helpful_error_on_required_radios-811542-70.patch7.11 KBpillarsdotnet
PASSED: [[SimpleTest]]: [MySQL] 33,292 pass(es).
[ View ]
#69 radio_throw_illegal-811542-69.patch712 bytesidflood
PASSED: [[SimpleTest]]: [MySQL] 33,268 pass(es).
[ View ]
#63 drupal-7.x-dev_form.inc-811542-53_v2.patch1.31 KBapatrinos
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-7.x-dev_form.inc-811542-53_v2.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]
#61 helpful_error_on_required_radios-811542-61-testsonly.patch5.52 KBpillarsdotnet
FAILED: [[SimpleTest]]: [MySQL] 33,641 pass(es), 2 fail(s), and 0 exception(es).
[ View ]
#61 helpful_error_on_required_radios-811542-61-tests+fix.patch7.11 KBpillarsdotnet
PASSED: [[SimpleTest]]: [MySQL] 33,643 pass(es).
[ View ]
#59 helpful_error_on_required_radios-811542-59-testsonly.patch5.53 KBpillarsdotnet
FAILED: [[SimpleTest]]: [MySQL] 33,658 pass(es), 2 fail(s), and 0 exception(es).
[ View ]
#59 helpful_error_on_required_radios-811542-59-tests+fix.patch8.3 KBpillarsdotnet
PASSED: [[SimpleTest]]: [MySQL] 33,638 pass(es).
[ View ]
#58 helpful_error_on_required_radios-811542-56.patch8.3 KBpillarsdotnet
PASSED: [[SimpleTest]]: [MySQL] 33,637 pass(es).
[ View ]
#57 helpful_error_on_required_radios-811542-56.patch8.3 KBpillarsdotnet
PASSED: [[SimpleTest]]: [MySQL] 35,834 pass(es).
[ View ]
#57 811542-45-56-interdiff.txt2.18 KBpillarsdotnet
#56 Radio-Button-0-label.jpg27.81 KBsirtet
#53 drupal-7.x-dev_form.inc-811542-53.patch887 bytesapatrinos
PASSED: [[SimpleTest]]: [MySQL] 35,788 pass(es).
[ View ]
#45 helpful_error_on_required_radios-811542-45.patch6.97 KBpillarsdotnet
PASSED: [[SimpleTest]]: [MySQL] 29,891 pass(es).
[ View ]
#44 field_list_boolean_test_fail.png175.66 KBpillarsdotnet
#41 helpful_error_on_required_radios-811542-41.patch6.37 KBpillarsdotnet
FAILED: [[SimpleTest]]: [MySQL] 29,907 pass(es), 4 fail(s), and 0 exception(es).
[ View ]
#39 helpful_error_on_required_radios-811542-39.patch7.49 KBpillarsdotnet
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in modules/simpletest/tests/form.test.
[ View ]
#36 helpful_error_on_required_radios-811542-36.patch8.64 KBpillarsdotnet
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in includes/form.inc.
[ View ]
#29 811542_has_garbage.patch5.29 KBchx
FAILED: [[SimpleTest]]: [MySQL] 31,607 pass(es), 4 fail(s), and 0 exception(es).
[ View ]
#18 drupal.form-radios-required.17.patch5.89 KBsun
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#16 drupal.form-radios-required.16.patch3.98 KBsun
FAILED: [[SimpleTest]]: [MySQL] 30,441 pass(es), 2 fail(s), and 0 exception(es).
[ View ]
#13 811542_12.patch6.89 KBmlncn
PASSED: [[SimpleTest]]: [MySQL] 30,383 pass(es).
[ View ]
#12 811542.patch2.45 KBbojanz
PASSED: [[SimpleTest]]: [MySQL] 30,456 pass(es).
[ View ]
#11 811542.patch2.46 KBbojanz
PASSED: [[SimpleTest]]: [MySQL] 30,402 pass(es).
[ View ]

Comments

Nick Lewis’s picture

Version:7.x-dev» 8.x-dev
Component:forms system» documentation
Status:Active» Postponed

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

<?php
 
// 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.

<?php
  $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
 
);
?>
Nick Lewis’s picture

Component:documentation» forms system
cdale’s picture

I agree with your argument entirely. Making sure that radios always have a default value is probably a good call. Not sure about where to take this issue from here, or if something like this should be enforced? I think the documentation approach is not a bad idea.

As for the checkboxes issue, that is a little weird, but I think I saw some other issues dealing with checkbox weirdness, but can't seem to find them just now, so perhaps those issues will fix the checkboxes.

David_Rothstein’s picture

Version:8.x-dev» 7.x-dev
Status:Postponed» Active

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.

mlncn’s picture

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!

Anonymous’s picture

I patched with this http://drupal.org/files/issues/735426-options-required-default-2.patch but the unhelpful error message is still there.

sun’s picture

Issue tags:+Needs tests

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.

sun’s picture

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.

vito_swat’s picture

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.

Bojhan’s picture

Priority:Normal» Major

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.

bojanz’s picture

Status:Active» Needs review
StatusFileSize
new2.46 KB
PASSED: [[SimpleTest]]: [MySQL] 30,402 pass(es).
[ View ]

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.

bojanz’s picture

StatusFileSize
new2.45 KB
PASSED: [[SimpleTest]]: [MySQL] 30,456 pass(es).
[ View ]

Had trailing whitespace.

mlncn’s picture

StatusFileSize
new6.89 KB
PASSED: [[SimpleTest]]: [MySQL] 30,383 pass(es).
[ View ]

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...

idflood’s picture

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

mlncn’s picture

Status:Needs review» Reviewed & tested by the community
Issue tags:-Needs tests
sun’s picture

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new3.98 KB
FAILED: [[SimpleTest]]: [MySQL] 30,441 pass(es), 2 fail(s), and 0 exception(es).
[ View ]

We need to restart from scratch.

Status:Needs review» Needs work

The last submitted patch, drupal.form-radios-required.16.patch, failed testing.

sun’s picture

Status:Needs work» Needs review
StatusFileSize
new5.89 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, drupal.form-radios-required.17.patch, failed testing.

mlncn’s picture

Status:Needs work» Needs review

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

Status:Needs review» Needs work

The last submitted patch, drupal.form-radios-required.17.patch, failed testing.

David_Rothstein’s picture

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.)

David_Rothstein’s picture

Priority:Major» Normal

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.

mlncn’s picture

@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

bojanz’s picture

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’s picture

@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.

mlncn’s picture

@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.

chx’s picture

Assigned:Unassigned» chx

Let me see.

chx’s picture

Status:Needs work» Needs review
StatusFileSize
new5.29 KB
FAILED: [[SimpleTest]]: [MySQL] 31,607 pass(es), 4 fail(s), and 0 exception(es).
[ View ]

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.

chx’s picture

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.

Status:Needs review» Needs work

The last submitted patch, 811542_has_garbage.patch, failed testing.

rfay’s picture

Version:7.x-dev» 8.x-dev
Issue tags:+needs backport to D7

Has to go to D8 and come back unfortunately.

mlncn’s picture

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. :-/

effulgentsia’s picture

subscribe

Isostar’s picture

subscribe

pillarsdotnet’s picture

StatusFileSize
new8.64 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in includes/form.inc.
[ View ]

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

pillarsdotnet’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, helpful_error_on_required_radios-811542-36.patch, failed testing.

pillarsdotnet’s picture

Status:Needs work» Needs review
StatusFileSize
new7.49 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in modules/simpletest/tests/form.test.
[ View ]

Trying again...

Status:Needs review» Needs work

The last submitted patch, helpful_error_on_required_radios-811542-39.patch, failed testing.

pillarsdotnet’s picture

StatusFileSize
new6.37 KB
FAILED: [[SimpleTest]]: [MySQL] 29,907 pass(es), 4 fail(s), and 0 exception(es).
[ View ]

And again...

pillarsdotnet’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, helpful_error_on_required_radios-811542-41.patch, failed testing.

pillarsdotnet’s picture

StatusFileSize
new175.66 KB

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.

pillarsdotnet’s picture

Status:Needs work» Needs review
StatusFileSize
new6.97 KB
PASSED: [[SimpleTest]]: [MySQL] 29,891 pass(es).
[ View ]

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

chx’s picture

Status:Needs review» Reviewed & tested by the community

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.

pillarsdotnet’s picture

Status:Reviewed & tested by the community» Needs work

@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.

HnLn’s picture

sub

chx’s picture

Status:Needs work» Reviewed & tested by the community

./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 :(

pillarsdotnet’s picture

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?

webchick’s picture

Status:Reviewed & tested by the community» Needs work

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.

pillarsdotnet’s picture

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.

apatrinos’s picture

Version:8.x-dev» 7.x-dev
Status:Needs work» Needs review
Issue tags:-needs backport to D7
StatusFileSize
new887 bytes
PASSED: [[SimpleTest]]: [MySQL] 35,788 pass(es).
[ View ]

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.

webchick’s picture

Status:Needs review» Needs work

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!

webchick’s picture

Issue tags:+needs backport to D7

Oops. And restoring tag.

sirtet’s picture

StatusFileSize
new27.81 KB

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).

pillarsdotnet’s picture

Status:Needs work» Needs review
StatusFileSize
new2.18 KB
new8.3 KB
PASSED: [[SimpleTest]]: [MySQL] 35,834 pass(es).
[ View ]

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.

pillarsdotnet’s picture

Version:7.x-dev» 8.x-dev
Assigned:chx» Unassigned
StatusFileSize
new8.3 KB
PASSED: [[SimpleTest]]: [MySQL] 33,637 pass(es).
[ View ]

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

pillarsdotnet’s picture

StatusFileSize
new8.3 KB
PASSED: [[SimpleTest]]: [MySQL] 33,638 pass(es).
[ View ]
new5.53 KB
FAILED: [[SimpleTest]]: [MySQL] 33,658 pass(es), 2 fail(s), and 0 exception(es).
[ View ]

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?

David_Rothstein’s picture

Status:Needs review» Needs work

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.

pillarsdotnet’s picture

Status:Needs work» Needs review
StatusFileSize
new7.11 KB
PASSED: [[SimpleTest]]: [MySQL] 33,643 pass(es).
[ View ]
new5.52 KB
FAILED: [[SimpleTest]]: [MySQL] 33,641 pass(es), 2 fail(s), and 0 exception(es).
[ View ]

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;                                                         
attiks’s picture

apatrinos’s picture

StatusFileSize
new1.31 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-7.x-dev_form.inc-811542-53_v2.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, drupal-7.x-dev_form.inc-811542-53_v2.patch, failed testing.

pillarsdotnet’s picture

Status:Needs work» Needs review

CNR for patch in #61

robertom’s picture

Subscribe

thekevinday’s picture

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

eosrei’s picture

#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...

idflood’s picture

StatusFileSize
new712 bytes
PASSED: [[SimpleTest]]: [MySQL] 33,268 pass(es).
[ View ]

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

pillarsdotnet’s picture

StatusFileSize
new7.11 KB
PASSED: [[SimpleTest]]: [MySQL] 33,292 pass(es).
[ View ]

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.

eosrei’s picture

Status:Needs review» Reviewed & tested by the community

#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.

pillarsdotnet’s picture

StatusFileSize
new7.95 KB
FAILED: [[SimpleTest]]: [MySQL] 33,307 pass(es), 1 fail(s), and 0 exception(es).
[ View ]
rfay’s picture

Status:Reviewed & tested by the community» Needs review

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

Status:Needs review» Needs work

The last submitted patch, helpful_error_on_required_radios-811542-72.patch, failed testing.

pillarsdotnet’s picture

Status:Needs work» Needs review
StatusFileSize
new808 bytes
new8.14 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in modules/field/modules/options/options.module.
[ View ]

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.

eosrei’s picture

StatusFileSize
new7.09 KB
FAILED: [[SimpleTest]]: [MySQL] 33,312 pass(es), 1 fail(s), and 0 exception(es).
[ View ]

#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.

pillarsdotnet’s picture

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

eosrei’s picture

Yep, I know.

Status:Needs review» Needs work

The last submitted patch, helpful_error_on_required_radios-811542-76.patch, failed testing.

pillarsdotnet’s picture

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new7.11 KB
PASSED: [[SimpleTest]]: [MySQL] 33,318 pass(es).
[ View ]

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

catch’s picture

Status:Reviewed & tested by the community» Needs work
+    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

pillarsdotnet’s picture

@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.

loganfsmyth’s picture

Status:Needs work» Needs review
StatusFileSize
new7.33 KB
PASSED: [[SimpleTest]]: [MySQL] 33,661 pass(es).
[ View ]

@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

pcambra’s picture

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

xjm’s picture

Status:Needs review» Needs work

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.

loganfsmyth’s picture

Status:Needs work» Needs review
StatusFileSize
new4.05 KB
FAILED: [[SimpleTest]]: [MySQL] 33,818 pass(es), 2 fail(s), and 0 exception(es).
[ View ]
new7.29 KB
PASSED: [[SimpleTest]]: [MySQL] 33,817 pass(es).
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, helpful_error_on_required_radios-811542-86.test.patch, failed testing.

pillarsdotnet’s picture

Status:Needs work» Needs review

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".

pcambra’s picture

Status:Needs review» Needs work

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.

<?php
   
// 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.

loganfsmyth’s picture

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.

<?php
   
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.

baby.hack’s picture

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.

loganfsmyth’s picture

Status:Needs work» Needs review

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.

thekevinday’s picture

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.

loganfsmyth’s picture

@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.

TonyK’s picture

subscribe

loganfsmyth’s picture

@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.

xjm’s picture

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!

xjm’s picture

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?

loganfsmyth’s picture

StatusFileSize
new7.72 KB
PASSED: [[SimpleTest]]: [MySQL] 34,029 pass(es).
[ View ]
new4.05 KB
FAILED: [[SimpleTest]]: [MySQL] 34,029 pass(es), 2 fail(s), and 0 exception(es).
[ View ]

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.

xjm’s picture

Status:Needs review» Reviewed & tested by the community

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

catch’s picture

Status:Reviewed & tested by the community» Needs work
+ * @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.

xjm’s picture

#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.

loganfsmyth’s picture

Status:Needs work» Needs review
StatusFileSize
new7.79 KB
PASSED: [[SimpleTest]]: [MySQL] 34,120 pass(es).
[ View ]
new4.04 KB
FAILED: [[SimpleTest]]: [MySQL] 34,112 pass(es), 2 fail(s), and 0 exception(es).
[ View ]

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.

xjm’s picture

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?

xjm’s picture

xjm’s picture

StatusFileSize
new2.03 KB
FAILED: [[SimpleTest]]: [MySQL] 34,102 pass(es), 2 fail(s), and 0 exception(es).
[ View ]

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

xjm’s picture

StatusFileSize
new2.03 KB
FAILED: [[SimpleTest]]: [MySQL] 34,109 pass(es), 2 fail(s), and 0 exception(es).
[ View ]

Without trailing whitespace and with better comments.

Status:Needs review» Needs work

The last submitted patch, required_radio_multivalue_test-107.patch, failed testing.

xjm’s picture

Status:Needs work» Needs review
xjm’s picture

StatusFileSize
new4.86 KB
new9.18 KB
PASSED: [[SimpleTest]]: [MySQL] 34,104 pass(es).
[ View ]
new5.77 KB
FAILED: [[SimpleTest]]: [MySQL] 34,098 pass(es), 4 fail(s), and 0 exception(es).
[ View ]

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.

xjm’s picture

_form_validate() repeats this bit twice:

<?php
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.

xjm’s picture

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.

xjm’s picture

StatusFileSize
new3.43 KB
new8.01 KB
PASSED: [[SimpleTest]]: [MySQL] 34,102 pass(es).
[ View ]
new4.6 KB
FAILED: [[SimpleTest]]: [MySQL] 34,093 pass(es), 2 fail(s), and 0 exception(es).
[ View ]

Simpler test as per #112.

xjm’s picture

+++ 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.

xjm’s picture

StatusFileSize
new10.04 KB
PASSED: [[SimpleTest]]: [MySQL] 34,127 pass(es).
[ View ]

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

David_Rothstein’s picture

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.

xjm’s picture

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.)

xjm’s picture

StatusFileSize
new1.9 KB
new9.96 KB
PASSED: [[SimpleTest]]: [MySQL] 34,128 pass(es).
[ View ]

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).

xjm’s picture

StatusFileSize
new9.96 KB
PASSED: [[SimpleTest]]: [MySQL] 34,127 pass(es).
[ View ]

Fixing docblock indentation, no other changes from #118.

David_Rothstein’s picture

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.

xjm’s picture

Status:Needs review» Needs work

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?

xjm’s picture

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.

sun’s picture

+++ 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.

xjm’s picture

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?

David_Rothstein’s picture

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

loganfsmyth’s picture

Status:Needs work» Needs review
StatusFileSize
new7.03 KB
new11.19 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/field/tests/field.test.
[ View ]
new7.27 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/field/tests/field.test.
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, radio-required-regression-811542-126.patch, failed testing.

loganfsmyth’s picture

Status:Needs work» Needs review
StatusFileSize
new7.03 KB
new11.19 KB
FAILED: [[SimpleTest]]: [MySQL] 34,258 pass(es), 11 fail(s), and 4 exception(es).
[ View ]
new7.27 KB
FAILED: [[SimpleTest]]: [MySQL] 34,243 pass(es), 21 fail(s), and 8 exception(es).
[ View ]

OKAY, one more time without the typo.

xjm’s picture

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.

loganfsmyth’s picture

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

xjm’s picture

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.

Status:Needs review» Needs work

The last submitted patch, radio-required-regression-811542-128.patch, failed testing.

loganfsmyth’s picture

Status:Needs work» Needs review
StatusFileSize
new5.78 KB
new11.19 KB
PASSED: [[SimpleTest]]: [MySQL] 34,086 pass(es).
[ View ]
new7.27 KB
FAILED: [[SimpleTest]]: [MySQL] 34,402 pass(es), 4 fail(s), and 0 exception(es).
[ View ]

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.

sun’s picture

StatusFileSize
new12.47 KB
FAILED: [[SimpleTest]]: [MySQL] 34,110 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

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 (Worst PHP Loose Data Typing Bug Ever) 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.

Status:Needs review» Needs work

The last submitted patch, drupal8.form-radios-value.134.patch, failed testing.

xjm’s picture

Priority:Normal» Major
StatusFileSize
new12.52 KB

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.

xjm’s picture

Priority:Major» Normal

Oops.

sun’s picture

Status:Needs work» Needs review
StatusFileSize
new10.64 KB
new11.29 KB
FAILED: [[SimpleTest]]: [MySQL] 34,114 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
xjm’s picture

+++ 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.

Status:Needs review» Needs work

The last submitted patch, drupal8.form-radios-value.138.patch, failed testing.

xjm’s picture

Status:Needs work» Needs review
StatusFileSize
new1.32 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch interdiff-138-141.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new11.45 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 811542-141.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

xjm’s picture

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

Status:Needs review» Needs work

The last submitted patch, interdiff-138-141.patch, failed testing.

xjm’s picture

Status:Needs work» Needs review
xjm’s picture

Pedro Lozano’s picture

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.

Pedro Lozano’s picture

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.

DamienMcKenna’s picture

+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?

xjm’s picture

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

DamienMcKenna’s picture

Status:Needs review» Reviewed & tested by the community

Commit time?

mrfelton’s picture

Status:Reviewed & tested by the community» Needs work

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.

hanoii’s picture

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

Fidelix’s picture

Status:Needs work» Reviewed & tested by the community

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

catch’s picture

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

xjm’s picture

Status:Reviewed & tested by the community» Needs work

Weird; why did the bot not NW it?

Needs a reroll I guess.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new11.43 KB
PASSED: [[SimpleTest]]: [MySQL] 35,149 pass(es).
[ View ]

Reroll.
Docblock for form_type_tableselect_value() changed.

ryanissamson’s picture

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

The patch applied cleanly.

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

after

Looks good to me!

xjm’s picture

Status:Needs review» Reviewed & tested by the community

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

chx’s picture

Indeed good to go.

webchick’s picture

Version:8.x-dev» 7.x-dev
Status:Reviewed & tested by the community» Patch (to be ported)

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.

DamienMcKenna’s picture

Status:Patch (to be ported)» Needs review
StatusFileSize
new11.35 KB
PASSED: [[SimpleTest]]: [MySQL] 38,299 pass(es).
[ View ]

The patch from #156 rerolled for D7.

sun’s picture

Version:7.x-dev» 8.x-dev
Status:Needs review» Reviewed & tested by the community
StatusFileSize
new553 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.radios-test-language.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
webchick’s picture

Version:8.x-dev» 7.x-dev
Status:Reviewed & tested by the community» Needs review

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

chx’s picture

Status:Needs review» Reviewed & tested by the community

#161 is green.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, drupal8.radios-test-language.patch, failed testing.

pillarsdotnet’s picture

Status:Needs work» Needs review
StatusFileSize
new11.35 KB
PASSED: [[SimpleTest]]: [MySQL] 38,306 pass(es).
[ View ]

Re-uploading #161.

xjm’s picture

Status:Needs review» Reviewed & tested by the community

meep meep

tim.plunkett’s picture

StatusFileSize
new11.34 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-811542-168.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

TonyK’s picture

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

xjm’s picture

@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?

webchick’s picture

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 7.x. Thanks!

webchick’s picture

Issue tags:+7.13 release notes

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

jeremypinto’s picture

Status:Fixed» Needs review
Issue tags:-needs backport to D7, -7.13 release notes

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

Status:Needs review» Needs work
Issue tags:+needs backport to D7, +7.13 release notes

The last submitted patch, drupal-811542-168.patch, failed testing.

tim.plunkett’s picture

Status:Needs work» Fixed

Uhm.

Status:Fixed» Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

timmetj’s picture

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

timmetj’s picture

Status:Closed (fixed)» Needs work

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

thekevinday’s picture

Status:Needs work» Needs review
StatusFileSize
new671 bytes
FAILED: [[SimpleTest]]: [MySQL] 39,054 pass(es), 8 fail(s), and 0 exception(s).
[ View ]

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().

Status:Needs review» Needs work

The last submitted patch, issue_811542_after_d7.14-1.patch, failed testing.

bhauff’s picture

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.

kimu’s picture

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.

quicksketch’s picture

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;
       }
timmetj’s picture

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?

webchick’s picture

Priority:Normal» Critical

Escalating. Sorry for breaking stuff. :( :(

fenstrat’s picture

catch’s picture

Version:7.x-dev» 8.x-dev

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

attiks’s picture

Version:8.x-dev» 7.x-dev
Status:Needs work» Needs review
StatusFileSize
new571 bytes
FAILED: [[SimpleTest]]: [MySQL] 38,670 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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

attiks’s picture

Version:7.x-dev» 8.x-dev

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

Status:Needs review» Needs work

The last submitted patch, i811542-188.patch, failed testing.

attiks’s picture

Status:Needs work» Needs review
StatusFileSize
new592 bytes
PASSED: [[SimpleTest]]: [MySQL] 36,578 pass(es).
[ View ]

Patch against D8

attiks’s picture

FYI: I cannot reproduce the failing test for D7, mine fails on

Language was installed successfully. Other translation.test 297 TranslationTestCase->addLanguage() Pass
Language has been created. Other translation.test 300 TranslationTestCase->addLanguage() Fail

But it works for the testbot or if I do it manually ...

fenstrat’s picture

chx’s picture

Thanks for the patch. This is a very sensitive area of form API, so I have a barrage of questions: you now want to honor if $element['#value'] is set to NULL. What does it mean if $element['#value'] is set to NULL? Where does it get set to NULL? What if a value callback sets it to NULL? Are there core value callbacks setting it to NULL? What if a contrib does? What does your patch fix and what does it break?

attiks’s picture

@chx, I'm going to try to answer, but to be honest I'm not even sure this is a 'proper' solution and I will look into it in some more detail.

I debugged the flow of a simple form with a radios element without a #default_value and I found out that $element['#value'] is set to NULL for this element, but the original code (if (!isset($element['#value'])) {) will fill in a new value namely FALSE.

What does it mean if $element['#value'] is set to NULL?

No idea, I assume it means the element was defined but no value was received.

Where does it get set to NULL?

No idea, I assume it's set by form_builder.

What if a value callback sets it to NULL?

Can't see any real problem (for now), but will try to add some tests.

Are there core value callbacks setting it to NULL?

Not that I know of.

What if a contrib does?

Can't see any real problem (for now)

What does your patch fix and what does it break?

In Drupal 7, according to testbot it breaks 1 test, but I think not everything is covered by tests.
In Drupal 7 it solves the problems with webform and comment_notify.

In Drupal 8 the testbot is happy, but I think not everything is covered by tests.

To conclude:

  • This needs more investigation
  • This needs more tests
attiks’s picture

Webform defined their radios with a '#default_value' => FALSE and that's what breaking it, if you want to try it, only the last from the following form has to be checked

<?php
$form
['test'] = array(
 
'#type' => 'radios',
 
'#title' => 'test',
 
'#options' => array(
   
0 => 'cat',
   
1 => 'dog',
   
2 => 'turkey',
   
3 => 'chicken',
  ),
);
 
$form['test2'] = array(
 
'#type' => 'radios',
 
'#title' => 'test',
 
'#options' => array(
   
0 => 'cat',
   
1 => 'dog',
   
2 => 'turkey',
   
3 => 'chicken',
  ),
 
'#default_value' => NULL,
);
 
$form['test3'] = array(
 
'#type' => 'radios',
 
'#title' => 'test',
 
'#options' => array(
   
0 => 'cat',
   
1 => 'dog',
   
2 => 'turkey',
   
3 => 'chicken',
  ),
 
'#default_value' => array(),
);
 
$form['test4'] = array(
 
'#type' => 'radios',
 
'#title' => 'test',
 
'#options' => array(
   
0 => 'cat',
   
1 => 'dog',
   
2 => 'turkey',
   
3 => 'chicken',
  ),
 
'#default_value' => FALSE,
);
 
$form['test5'] = array(
 
'#type' => 'radios',
 
'#title' => 'test',
 
'#options' => array(
   
0 => 'cat',
   
1 => 'dog',
   
2 => 'turkey',
   
3 => 'chicken',
  ),
 
'#default_value' => '', // cat is selected
);

$form['test6'] = array(
 
'#type' => 'radios',
 
'#title' => 'test',
 
'#options' => array(
   
1 => 'dog',
   
2 => 'turkey',
   
3 => 'chicken',
  ),
 
'#default_value' => '', // NOT WORKING
);
?>
attiks’s picture

Another problem is that if you use the following form, the first one will be selected even if '#default_value' => NULL if default value == ''

<?php
$form
['test'] = array(
   
'#type' => 'radios',
   
'#title' => 'test',
   
'#options' => array(
     
0 => 'cat',
     
1 => 'dog',
     
2 => 'turkey',
     
3 => 'chicken',
    ),
  );
?>

EDIT: Sorry all, was checking with a change made to core

attiks’s picture

Which of the following #default_values are supposed to work?
These should all work with the form from above.

  1. None specified
  2. NULL
  3. array()
  4. FALSE

This one does not work with the form from above.

  1. ''

EDIT: Sorry all, was checking with a change made to core

thekevinday’s picture

But keep in mind that not all forms use numeric keys for their option keys.
Some might use machine_names.

I am still support what I mentioned in #139 and what @quicksketch suggested in #183 (and confirmed in #193).

If I understand the forms correctly, not selecting a choice gives a value of NULL.
The forms api understands NULL as no choice.
I am not sure how array() would be handled, but FALSE only happens to work if 0 is a valid choice in your options.

The problem stems from the default_value being set to FALSE when 0 is not a valid option.

timmetj’s picture

Looks like my case idd.. I'm using custom forms with string value's:
array('mail' => 'contact by email', 'post' => 'contact by post') (for example), not the default 0,1,2.. array(0 => 'contact by email',..)
Thats why the patches sadly didn't work for me.

attiks’s picture

@thekevinday the patch in #179 is not for the regular form api only for the option widget, so this will not solve the problems of webform, context and others

@timmetj did you specify a #default_value and if so which? I ran the test with strings as keys, results inline

AFAIK: If you don't want a radio selected you have 3 options:

  1. don't specify a default value
  2. use NULL as default
  3. use array() as default

The only thing I don't know for sure is if this how it's meant to be?

<?php
$form
['testa'] = array(
 
'#type' => 'radios',
 
'#title' => 'no defaut',
 
'#options' => array(
   
'a' => 'cat',
   
'b' => 'dog',
   
'c' => 'turkey',
   
'd' => 'chicken',
  ),
);
 
$form['test2a'] = array(
 
'#type' => 'radios',
 
'#title' => 'default null',
 
'#options' => array(
   
'a' => 'cat',
   
'b' => 'dog',
   
'c' => 'turkey',
   
'd' => 'chicken',
  ),
 
'#default_value' => NULL,
);
 
$form['test3a'] = array(
 
'#type' => 'radios',
 
'#title' => 'default empty array',
 
'#options' => array(
   
'a' => 'cat',
   
'b' => 'dog',
   
'c' => 'turkey',
   
'd' => 'chicken',
  ),
 
'#default_value' => array(),
);
 
$form['test4a'] = array(
 
'#type' => 'radios',
 
'#title' => 'default false',
 
'#options' => array(
   
'a' => 'cat',
   
'b' => 'dog',
   
'c' => 'turkey',
   
'd' => 'chicken',
  ),
 
'#default_value' => FALSE, // Does not work
);
 
$form['test5a'] = array(
 
'#type' => 'radios',
 
'#title' => 'default empty string',
 
'#options' => array(
   
'a' => 'cat',
   
'b' => 'dog',
   
'c' => 'turkey',
   
'd' => 'chicken',
  ),
 
'#default_value' => '', // Does not work
);
 
$form['test6a'] = array(
 
'#type' => 'radios',
 
'#title' => 'default empty string, no 0 option',
 
'#options' => array(
   
'b' => 'dog',
   
'c' => 'turkey',
   
'd' => 'chicken',
  ),
 
'#default_value' => '', // Does not work
);
?>
timmetj’s picture

@attiks I can confirm that your patch from #188 worked for me. When i first read your explanation i thought my forms would not work. But i have to correct it :)

I can even modify your explanation telling you that setting the #default_value' => '' DOES WORK with your patch!
So 'test5a' and 'test6a' are my cases and they work. (But i don't understand the difference between test5a and test6a, aren't thay the same?)

<?php
$product_type
= isset($form_state['values']['1']['product_type']) ? $form_state['values']['1']['product_type'] : '';
$form['1']['product_type'] = array(
   
'#type' => 'radios',
   
'#tree' => TRUE,
   
'#options' => array(
       
'elec' => t('offer.elec'),
       
'gas' => t('offer.gas'),
       
'elecgas' => t('offer.elecgas')
      ),
   
'#title' => t('offer.which.product_type'),
   
'#ajax' => array(
       
'callback' => 'product_type_callback',
       
'wrapper' => 'product_info_wrapper',
       
'method' => 'replace',
       
'effect' => 'fade',
       
'progress' => array('type' => 'throbber', 'message' => ''),
    ),
   
'#default_value' => $product_type,
   
'#attributes' => array('class' => array('vertical_radios')),
);
?>

My other case just doesn't sets a #default_value (which brings me to 'testa') and now also works.

<?php
$element
['previousContract'] = array(
   
'#type' => 'radios',
   
'#title' => t('previousContract.title'),
   
'#options' => array(
       
'YES' => t('Yes'),
       
'NO' => t('No')),
   
'#prefix' => '<div class="datagroup">',
   
'#suffix' => '</div>',
);
?>

And now i can again submit my forms without having to select a radio button.
Thanks.

thekevinday’s picture

StatusFileSize
new1.36 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

@attiks I just very much do not want to lose the "reset()" bugfix, just please include it in your patch.

It may be that I am just on edge, but I have been complaining about this issue since drupal 6: #919348: unset form values, such as radio buttons improperly trigger 'An illegal choice has been detected'.
I have very paitently waited for this to be solved.
I do not want it to get lost again and in Drupal 9 have to complain all over again about how '0' is not a valid option because of reset() being improperly used.

Anyway, here is a combined patch.
I took into mind #183's comment and changed the code to:

<?php
       
if (is_array($default_value)) {
         
$default_value = empty($default_value) ? NULL : reset($default_value);
        }
        else {
         
$default_value = NULL;
        }
?>

Status:Needs review» Needs work

The last submitted patch, drupal-8.x-811542-204.patch, failed testing.

attiks’s picture

@timmetj my examples in #196 and #202 were without the patch, the problem is my knowledge of the form API internal mechanisms is limited and I'm hoping that some core developer can let me (us) know what is supposed to work and what isn't.

There's no difference between 5a and 6a, i just copied the ones from #196 and changed the names.

attiks’s picture

Status:Needs work» Needs review
Issue tags:-needs backport to D7, -7.13 release notes

#204: drupal-8.x-811542-204.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+needs backport to D7, +7.13 release notes

The last submitted patch, drupal-8.x-811542-204.patch, failed testing.

attiks’s picture

bot: "'failed to set variable [simpletest_clear_results] in checkout database'"

muka’s picture

Just a note, #188 gives meaningful messages in D7 when no raidos option is selected.

thekevinday’s picture

Status:Needs work» Needs review
Issue tags:-needs backport to D7, -7.13 release notes

#204: drupal-8.x-811542-204.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+needs backport to D7, +7.13 release notes

The last submitted patch, drupal-8.x-811542-204.patch, failed testing.

thekevinday’s picture

Okay, so looking for "simpletest_clear_results", leads me to this:
modules/simpletest/simpletest.pages.inc:

<?php
  $form
['general']['simpletest_clear_results'] = array(
   
'#type' => 'checkbox',
   
'#title' => t('Clear results after each complete test suite run'),
   
'#description' => t('By default SimpleTest will clear the results after they have been viewed on the results page, but in some cases it may be useful t
o leave the results in the database. The results can then be viewed at <em>admin/config/development/testing/[test_id]</em>. The test ID can be found in the database, simpletest table, or kept track of when viewing the results the first time. Additionally, some modules may provide more analysis or features that require this setting to be disabled.'
),
   
'#default_value' => variable_get('simpletest_clear_results', TRUE),
  );
?>

Any ideas how '#default_value' => variable_get('simpletest_clear_results', TRUE) could be a problem with this patch?

The code in question that is probably failing in the testbot is introduced in #204.

I could only guess that the checkbox values are not stored in an array.
Does this mean that reset() should only be called if and only if the variable is an array and otherwise it never be assigned anything?

What I am also considering is that because reset() would produce FALSE under certain circumstances, it is possible that the testbot was previously, improperly succeeding, because it was getting FALSE instead of NULL (and the form was expecting a boolean).
That is to say, reset() happened to fail in a benificial way that looked like a success.

webchick’s picture

Issue tags:+7.15 release blocker

.

attiks’s picture

Bot has a problem setting the variable: "'failed to set variable [simpletest_clear_results] in checkout database'", so my guess that it might be something else.

Regardless of the test result I'm not sure if we need the following change, since forms are working with

  1. None specified
  2. NULL
  3. array()

but they used to work with FALSE as well, so we need someone to decide how it's meant to be, before we can start thinking about a patch.

-    if (!isset($element['#value'])) {
+    if (array_key_exists('#value', $element)) {
seb.pisarski’s picture

#188 breaks views screen (D 7.14, V 3.3) With patch no views are visible in /admin/structure/views, without patch list of views is properly displayed.

#188 breaks also Profile Complete Percent After patching form.inc administration screen of PCP module shows list of checkboxes without their labels.

attiks’s picture

@seb.pisarski see my comment #215, I'm not sure we need a patch

seb.pisarski’s picture

subscribe

seb.pisarski’s picture

Well sth. got messed up with 7.14 update, those who have Comment notify module installed are not able to post comments (illega choice ...). Patch 188 fixes that problem but breaks some other site features. I did not follow closely changes in form.inc addedby 7.14 but maybe it is time to consider some kind of roll back?

attiks’s picture

AFAIK all problems can be solved in the modules, rolling back is not an option since the patch from #166 fixes other problems.

greggles’s picture

The patch in #1560062: After Drupal 7.14 core update, getting error Illegal choice 0 in notify_type element doesn't completely solve the problem. Note that those radios are not actually required, so this seems to affect other situations.

attiks’s picture

@greggles: if I understand you correct, it means it can not be solved in your module? This means we'll need a patch in here :/

greggles’s picture

@attiks that is my feeling, but I'll admit I only read the summary and most recent comments in this issue. Maybe there is some advice I missed.

David_Rothstein’s picture

Status:Needs work» Needs review
StatusFileSize
new834 bytes
FAILED: [[SimpleTest]]: [MySQL] 36,827 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Yeah, we definitely need to do something here.

So from reading the above, it sounds like the root of all problems (or at least most of them) is this change from the original patch:

@@ -2968,7 +3010,9 @@ function form_process_radios($element) {
         // The key is sanitized in drupal_attributes() during output from the
         // theme function.
         '#return_value' => $key,
-        '#default_value' => isset($element['#default_value']) ? $element['#default_value'] : NULL,
+        // Use default or FALSE. A value of FALSE means that the radio button is
+        // not 'checked'.
+        '#default_value' => isset($element['#default_value']) ? $element['#default_value'] : FALSE,

That seems to be what's causing the form validation code to treat FALSE as the selected option and looking for it in #options and failing validation when it's not there. So, we need to try to revert that and get the default value back to NULL like the spaghetti monster intended.

I think I have some ideas on how to make that work, but let's start with a patch that just does that and see exactly where the tests fail. That will be useful to know.

David_Rothstein’s picture

Also, @thekevinday, I don't think I understand your proposed change or why it's needed:

--- a/core/modules/field/modules/options/options.module
+++ b/core/modules/field/modules/options/options.module
@@ -110,7 +110,12 @@ function options_field_widget_form(&$form, &$form_state, $field, $instance, $lan
       // default to NULL so that the form element is properly recognized as
       // not having a default value.
       if (!$multiple) {
-        $default_value = $default_value ? reset($default_value) : NULL;
+        if (is_array($default_value)) {
+          $default_value = empty($default_value) ? NULL : reset($default_value);
+        }
+        else {
+          $default_value = NULL;
+        }

What's the scenario where $default_value won't be an array?

As far as I can see, it comes from _options_storage_to_form() which is always supposed to return an array. So the current code looks correct to me.

And if there is some situation where an array isn't being returned, maybe that needs to be fixed inside _options_storage_to_form() itself?

sun’s picture

Hrm. Yeah, this totally circles back into #134, in which I found that FALSE value extremely suspicious.

Status:Needs review» Needs work

The last submitted patch, radios-regression-811542-224.patch, failed testing.

David_Rothstein’s picture

Status:Needs work» Needs review
StatusFileSize
new4.39 KB
FAILED: [[SimpleTest]]: [MySQL] 36,814 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

OK, here's the fix I had in mind. Let's see if it works.

I also reworked and clarified several of the code comments.

sun’s picture

#228 looks RTBC to me, in case it will pass tests.

Status:Needs review» Needs work

The last submitted patch, radios-regression-811542-228.patch, failed testing.

David_Rothstein’s picture

Status:Needs work» Needs review
StatusFileSize
new5.78 KB
PASSED: [[SimpleTest]]: [MySQL] 36,807 pass(es).
[ View ]

OK, let's try this.

The change to theme_radio() in the attached patch is perhaps a little scary for D7 backport, but I'm hoping it's correct. And we may be able to find a less intrusive way to do that for backport if we need to.

David_Rothstein’s picture

StatusFileSize
new19.41 KB
PASSED: [[SimpleTest]]: [MySQL] 37,037 pass(es).
[ View ]
new12.9 KB
FAILED: [[SimpleTest]]: [MySQL] 37,052 pass(es), 8 fail(s), and 0 exception(s).
[ View ]
new6.47 KB

Here are new patches. Changes:

  1. Added a huge number of tests.
  2. Made some small fixes to the documentation.
  3. Added a replacement for the $element['#needs_validation'] code that was previously there (see below).

I've attached a Drupal 7 patch also (untested), which could really use some manual testing with the affected contrib modules to see how this approach works.

*****

After I looked into the various contrib modules that were affected by the original patch committed here, I came to the conclusion that the biggest part of the patch that hurt them was this:

+      // There was a user submission so validation is a must. If this element is
+      // #required, then an appropriate error message will be output. While an
+      // optional #type 'radios' does not necessarily make sense from a user
+      // interaction perspective, there may be use-cases for that and it is not
+      // the job of Form API to artificially limit possibilities.
+      $element['#needs_validation'] = TRUE;

That's because they all defined radio buttons something like the following:

<?php
$form
['radios'] = array(
 
'#type' => 'radios',
 
'#options' => array(
   
'one' => t('One'),
   
'two' => t('Two'),
  ),
 
// Most of the modules used 0 or FALSE here, but the important point is
  // that they're using something that's not NULL but also not one of the
  // #options.
 
'#default_value' => FALSE,
);
?>

Previously you could use the above method to have the form rendered with no radio buttons selected (whether that method actually makes sense to support I'm not sure, but I think it's always been that way in Drupal 7) and everything worked fine. The #needs_validation check added in Drupal 7.14 caused the form API to try to validate that the #default_value was one of the #options and print a nasty message to the screen because it wasn't.

To replace that, I've added some code instead that just makes sure the #default_value never gets used as a replacement for user input; that way, a #default_value that isn't in #options is treated the same way as a #default_value of NULL.

*****

One side effect of that change, I think, is that if you have something like this:

<?php
$form
['radios'] = array(
 
'#type' => 'radios',
 
'#options' => array(
   
'one' => t('One'),
   
'two' => t('Two'),
  ),
 
'#default_value' => 'one',
);
?>

And if the user submits the form without any input for the radio buttons anyway (which they can really only do by hacking the form in their browser, or if there's JavaScript on the page that allows them to do it, since browsers don't normally allow the user to de-select a radio button), then:

  • Before the patch, I think (haven't verified) that the form API treats it as if they submitted it with #default_value selected anyway.
  • After the patch, it respects the fact that they didn't submit anything and allows NULL to through.

I think that change in behavior makes sense (and the previous behavior is a bug), but we have to think about whether it's OK for Drupal 7. In general, if you don't want NULL input to be allowed you should make an element #required, but for aesthetic reasons some module authors might not do that but still expect that NULL will never be submitted.

mgifford’s picture

So just to recap.. radios-regression-811542-232.patch needs to be tested against D8. @sun already indicated #228 was pretty close to RTBC.

@webchick why was this marked 7.15 release blocker? I'd certainly like this fixed, but want to see 7.15.

jpstrikesback’s picture

Breaks views contextual filters...just putting that there for posterity's sake

effulgentsia’s picture

@jpstrikesback: Can you elaborate please? Are you testing Views 7.x-3.x-dev? What does/doesn't work with Drupal core 7.x branch tip? What does/doesn't work with core 7.x branch tip plus the D7 patch in #232 applied?

effulgentsia’s picture

What is really frustrating here both for HEAD and for what's in #232 is the rampant inconsistency between radio(s) and checkbox(es) with respect to handling of NULL, 0, '0', and FALSE, for the value callbacks, the process callbacks, and the theme functions, making this completely unmaintainable. Given that 7.14 introduced a regression, it's critical that regression get fixed for 7.15, so if we get some confirmation here that #232 does that successfully, I'm +1 for us proceeding with that, and then working on making this maintainable in an 8.x follow-up.

[EDIT: sorry if that comes off as negative. I'm not blaming anyone here (well, maybe myself for not catching this stuff when we were working on all the checkbox logic during D7 code thaw). Just frustrated that I can neither say "#232 looks awesome" nor offer up something constructive at this time.]

jpstrikesback’s picture

I'm on D 7.14, Views 7.x-3.x-dev. WIthout #232 I get "An illegal choice..." error upon submitting anything on the Views Contextual Filters Options/Settings Modal which means I cannot save anything for a UI generated view. With the patch in #232 the errors are gone and I can again submit and save.

David_Rothstein’s picture

Looking again at #232, I'm starting to think this change to form_type_radios_value() might be a problem:

+  // However, because we let NULL values through, the form API may sometimes
+  // request default value handling for radios even when input was submitted.
+  // Make sure we don't replace the submitted NULL value with the default value
+  // in that case.
+  elseif (isset($element['#default_value']) && empty($form_state['input'])) {
     return $element['#default_value'];
   }

Because the empty($form_state['input']) check does not match what _form_builder_handle_input_element() uses to decide whether input is being processed:

<?php
  $process_input
= empty($element['#disabled']) && ($form_state['programmed'] || ($form_state['process_input'] && (!isset($element['#access']) || $element['#access'])));
?>

So I think this patch might cause problems with programmatic form submissions, or elements with #access set to FALSE, etc., although I can't think of how to write a test that exposes that at the moment.

Maybe we need to go back to something more like #231 instead. I think it's OK for now (and in many ways safer for D7 backport), as long as we don't mind that things like this:

<?php
$form
['radios'] = array(
 
'#type' => 'radios',
 
'#options' => array(
   
'one' => t('One'),
   
'two' => t('Two'),
  ),
 
'#default_value' => FALSE,
);
?>

will result in submit handlers seeing FALSE (even though it's not an allowed option) if the user submits the form with no radio button selected.

To recap, that seems to be how earlier versions of Drupal worked and didn't cause any obvious problems (though it's wrong in theory); 7.14 changed that (but did so by preventing the form from being submitted at all, which is what caused the regression). So this part has never worked well in any version of Drupal as far as I can tell, and maybe it doesn't need to for this issue to go forward.

This could badly use reviews from anyone with form API expertise... I'm starting to get a little worried about this issue.

David_Rothstein’s picture

StatusFileSize
new18.81 KB
FAILED: [[SimpleTest]]: [MySQL] 37,323 pass(es), 6 fail(s), and 0 exception(s).
[ View ]

Here's #231, but with the documentation fixes and tests from #232.

The tests definitely won't all pass, but by seeing which ones fail, that will help us decide if those are things we can live with in the next release of Drupal 7 (and therefore whether we can defer fixing them to a later issue).

Status:Needs review» Needs work
Issue tags:-needs backport to D7, -7.13 release notes, -7.15 release blocker

The last submitted patch, radios-regression-811542-239.patch, failed testing.

mgifford’s picture

Status:Needs work» Needs review

#239: radios-regression-811542-239.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+needs backport to D7, +7.13 release notes, +7.15 release blocker

The last submitted patch, radios-regression-811542-239.patch, failed testing.

mgifford’s picture

I tested this locally, but not sure why this is failing.

@David_Rothstein wanted to see "which ones fail, that will help us decide if those are things we can live with in the next release of Drupal 7 (and therefore whether we can defer fixing them to a later issue)."

So, what can be fixed in D8 now and rebundled to D7 for what may be the last release blocker for 7.15?

Unexpected error message: An illegal choice has been detected. Please contact the site administrator. Other FormTest.php 187 Drupal\system\Tests\Form\FormTest->testRequiredCheckboxesRadio()
Unexpected error message: An illegal choice has been detected. Please contact the site administrator. Other FormTest.php 187 Drupal\system\Tests\Form\FormTest->testRequiredCheckboxesRadio()
Found error message: Radios (with an invalid default value) field is required. Other FormTest.php 197 Drupal\system\Tests\Form\FormTest->testRequiredCheckboxesRadio()
Found error message: Radios (with a default value of FALSE) field is required. Other FormTest.php 197 Drupal\system\Tests\Form\FormTest->testRequiredCheckboxesRadio()

    // Press 'add more' button -> 4 widgets
    $this->drupalPost(NULL, $edit, t('Add another item'));
    for ($delta = 0; $delta <= $delta_range; $delta++) {
      $this->assertFieldByName("$this->field_name[$langcode][$delta][value]", $values[$delta], "Widget $delta is displayed and has the right value");
      $this->assertFieldByName("$this->field_name[$langcode][$delta][_weight]", $weights[$delta], "Widget $delta has the right weight");
    }
    ksort($pattern);
    $pattern = implode('.*', array_values($pattern));
    $this->assertPattern("|$pattern|s", 'Widgets are displayed in the correct order');
    $this->assertFieldByName("$this->field_name[$langcode][$delta][value]", '', "New widget is displayed");
    $this->assertFieldByName("$this->field_name[$langcode][$delta][_weight]", $delta, "New widget has the right weight");
    $this->assertNoField("$this->field_name[$langcode][" . ($delta + 1) . '][value]', 'No extraneous widget is displayed');

    // Submit the form and create the entity.
    $this->drupalPost(NULL, $edit, t('Save'));
    preg_match('|test-entity/manage/(\d+)/edit|', $this->url, $match);
    $id = $match[1];
    $this->assertRaw(t('test_entity @id has been created.', array('@id' => $id)), 'Entity was created');
    $entity = field_test_entity_test_load($id);
    ksort($field_values);
    $field_values = array_values($field_values);
    $this->assertIdentical($entity->{$this->field_name}[$langcode], $field_values, 'Field values were saved in the correct order');

No error message is displayed when all required fields are filled. Other FormTest.php 262 Drupal\system\Tests\Form\FormTest->testRequiredCheckboxesRadio()
Validation form submitted successfully. Other FormTest.php 263 Drupal\system\Tests\Form\FormTest->testRequiredCheckboxesRadio()

  function testFieldFormJSAddMore() {
    $this->field = $this->field_unlimited;
    $this->field_name = $this->field['field_name'];
    $this->instance['field_name'] = $this->field_name;
    field_create_field($this->field);
    field_create_instance($this->instance);
    $langcode = LANGUAGE_NOT_SPECIFIED;
David_Rothstein’s picture

Status:Needs work» Needs review
StatusFileSize
new4.59 KB
PASSED: [[SimpleTest]]: [MySQL] 37,105 pass(es).
[ View ]
new1.59 KB
FAILED: [[SimpleTest]]: [MySQL] 37,101 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
new4.4 KB

OK, after looking into this some more, I think the answer is this:

We can remove a lot more from the Drupal 7 patch, and therefore get something that is hopefully reviewable and committable very quickly.

In particular:

  1. We originally thought this change was the one causing problems with all the contrib modules:
    @@ -2968,7 +3010,9 @@ function form_process_radios($element) {
             // The key is sanitized in drupal_attributes() during output from the
             // theme function.
             '#return_value' => $key,
    -        '#default_value' => isset($element['#default_value']) ? $element['#default_value'] : NULL,
    +        // Use default or FALSE. A value of FALSE means that the radio button is
    +        // not 'checked'.
    +        '#default_value' => isset($element['#default_value']) ? $element['#default_value'] : FALSE,

    But it actually seems to have been a red herring (I'm guessing because it only affects the radio buttons themselves, not the group of radios). While it still looks wrong and should be reverted in Drupal 8, we might be able to get away with leaving it as in Drupal 7. And since reverting it without causing other problems required changes to theme_radio(), etc., we should leave it as is if at all possible.

  2. Similarly, let's leave the $element['#needs_validation'] code alone as much as possible, but only make an exception for the part that we know was breaking contrib modules (see below).

Per above, the thing that caused major contrib modules to break was typically that code like this stopped working:

<?php
$form
['radios'] = array(
 
'#type' => 'radios',
 
'#options' => array(
   
'one' => t('One'),
   
'two' => t('Two'),
  ),
 
'#default_value' => FALSE,
);
?>

So, the attached patch simply contains a targeted fix for empty default values in the above situation, but otherwise leaves the #needs_validation code in place.

I have tested the Drupal 7 version of this patch manually with the following modules that were known to have been affected:

  1. Comment Notify: #1560062: After Drupal 7.14 core update, getting error Illegal choice 0 in notify_type element (7.x-1.0 was broken; 7.x-1.1 contained a fix within the module itself)
  2. Webform: #1565696: Select Component non-mandatory with no default throws "An illegal choice has been detected." (7.x-3.17 was broken; 7.x-3.18 contained a fix within the module itself)
  3. Delta: #1512744: Saving Context Issue because no Delta template has been saved. (7.x-3.0-beta9 was broken; 7.x-3.0-beta10 contained a fix within the module itself)

In each case, I verified that the core patch fixed the older (broken) version of the module, and that it continued to work with the newer version of the module as well.

For Views (mentioned in #237 above), I couldn't reproduce an issue with Views alone, but the Views issue queue suggested it's an issue when Flag module is being used. (See #1484352: Illegal choice *relationship* in Flag element causes Views contextual filters to fail; the bug existed in Flag 7.x-2.0-beta6 but was fixed in 7.x-2.0-beta7). In this case, testing with the older version of the module showed that although the patch in #232 fixed things, the patch attached to this comment does not. However, in this case, the buggy code in the module looked a bit different; it was more like this:

<?php
$form
['radios'] = array(
 
'#type' => 'radios',
 
'#options' => array(),
 
'#default_value' => 'something',
);
?>

I don't think we need to support the above; it really seems like a mistake in the module that that form array ever got constructed in the first place. Given that the module fixed that issue itself and we have no other reports, I'm thinking we can live with this as is for now. In the worst case, we'll deal with it in a future version of Drupal 7.

It's notable that even for the case of an empty #default_value (which affected the other modules), all the modules fixed themselves already too; and it's good that they did, since in theory a #default_value of NULL should be the only way to do this. But given how many popular contrib modules broke as a result of this (there could also be custom code out there that broke too) and how much the form API is generally ambiguous about NULL vs. other empty values, I think it's still important that we fix this regression in Drupal 7.

Conclusion: There is still a lot to follow up on here (I think we'll still want to make many of the changes in the above patches for Drupal 8 if not Drupal 7), but I believe those can be in separate followup issues after this one is finally put to rest.

David_Rothstein’s picture

Now that we have a simpler patch, I really need code reviews (assuming it passes tests).

Given that the backport is trivial and that I've already done heavy manual testing of the Drupal 7 patch, I'm going to essentially consider a Drupal 8 RTBC to apply to Drupal 7 also, and get this committed there ASAP. The goal is to commit the patch by this weekend, then have a new Drupal 7 release on August 1.

Thanks :)

chx’s picture

Status:Needs review» Needs work

While I think the logic is solid , the way it is expressed is not something I would want to see in form API because it mixes up the input FALSE and not FALSE cases. https://gist.github.com/3172060 I rather have   $element['#has_garbage_value'] = TRUE; twice than to have those two cases mixed up.

attiks’s picture

StatusFileSize
new1.67 KB
FAILED: [[SimpleTest]]: [MySQL] 37,082 pass(es), 41 fail(s), and 25 exception(s).
[ View ]

I was able to break it with tableselect, apply attached patch after applying #244, enable the form_test module, navigate to /form_test/tableselect/multiple-false and press submit: "An illegal choice has been detected. Please contact the site administrator."

effulgentsia’s picture

#247 has an extra hunk from #244, so doesn't apply on top of it. Here's D8 and D7 patches without that. But, I think that should be made into a separate issue. That's a bug that existed prior to #156/#171 and was unchanged by it. That fix was for type 'radios' only, so that should be the only regression that's a blocker for 7.15, I think.

effulgentsia’s picture

StatusFileSize
new4.13 KB
PASSED: [[SimpleTest]]: [MySQL] 37,106 pass(es).
[ View ]

This changes the form.inc hunk based on #246 feedback, but the tests from #244 are unchanged.

effulgentsia’s picture

Status:Needs work» Needs review
David_Rothstein’s picture

Status:Needs review» Needs work
+    if (!isset($input) && isset($element['#default_value']) && $element['#default_value'] !== FALSE) {
       $element['#needs_validation'] = TRUE;

We can't just check for FALSE; it has to be any empty value. Comment Notify, for example, won't be fixed unless we do. (The contrib modules I looked at were using various variables/constants/etc. here that evaluated to empty: 0's, empty strings, etc. Sorry if my comment above implied that it was always equal to FALSE exactly.)

If we wanted to be extra careful, we could add another test for that, but I don't think it's worth it as long as the code comment explains what we're doing (and this isn't the type of code we want to encourage, or even necessarily support, going forward in Drupal 8).

Otherwise, this looks pretty good. Like @chx above (and during my conversation with @chx in IRC) I thought splitting it up meant we'd need to set $element['#has_garbage_value'] in two locations... but actually looking at the code flow I think you're right that we don't - the default value handling is the correct place.

I'll try to reroll and test this again tomorrow.

chx’s picture

#249 does have the garbage-set twice as I suggested. However, it has $value = isset($element['#default_value']) ? $element['#default_value'] : NULL; if (!isset($value)) { this which I do not quite understand. If $element['#default_value'] is not set or NULL we set $value to NULL and fail the if. If it is set and not NULL then we pass the if. What's the difference to if (!isset($element['#default_value']))? Did this reroll miss the fact that if an array key is set but it is set to NULL then isset happily fails.

effulgentsia’s picture

The contrib modules I looked at were using various variables/constants/etc. here that evaluated to empty: 0's, empty strings, etc.

0 and '0' are valid and common keys in #options arrays of radios. Technically, so is empty string, though that's less common. If a contrib module is setting #default_value to any of those to mean "select nothing" rather than "select the option with that key", that module has a bug. If there's enough of them or they're high profile ones that we want to coddle them in order to maintain BC from 7.13, I'm okay with that, but I also don't think it's inappropriate to say, "This module had a bug that didn't surface. Now it surfaced. Fix it." Especially since the fix is easy: change #default_value to NULL or FALSE if the behavior you want is to select nothing. NULL and FALSE are the only two legitimate ways of expressing that, and breaking FALSE was our only critical regression, IMO.

effulgentsia’s picture

Re #252: we also need to return $value, and I didn't want PHP notices if #default_value doesn't exist as a key. Feel free to submit an alternate patch if you have a way to clean that up.

chx’s picture

Ah so. That's good, then.

attiks’s picture

#248: I created #1699644: tableselect and required: An illegal choice has been detected and thanks for cleaning the patch.

David_Rothstein’s picture

  1. 0 and '0' are valid and common keys in #options arrays of radios. Technically, so is empty string, though that's less common.... NULL and FALSE are the only two legitimate ways of expressing that [select nothing behavior], and breaking FALSE was our only critical regression, IMO.

    In my testing, FALSE seemed to work as well as the others when you put it in #options. And for radios with any empty option (regardless of whether it's FALSE or something else), setting #default_value to NULL didn't even un-select it in Drupal 7.13 (only FALSE did), although NULL does do that in Drupal 7.14...

    So basically, we have a mess, but the general rule that seemed to always work in the past was "use any empty #default_value to have no radios selected by default, unless one of your #options is empty too, in which case you have to use a more special value to avoid conflicts, but which special value you can use is not documented (at least I think it isn't?) and switches between Drupal versions anyway" :)

    Since there is mass confusion around this and real modules that were broken, I definitely think we should keep the code as lenient as possible for now, which means letting any empty value through.

    Handy code for posterity if anyone wants to test on their own (can edit it to add various #default_value's as necessary):

    <?php
    function mymodule_form($form, $form_state) {
     
    $form['false'] = array(
       
    '#type' => 'radios',
       
    '#title' => 'radios with false option',
       
    '#options' => array(
         
    FALSE => 'false',
         
    TRUE => 'true',
        ),
      );
     
    $form['zero'] = array(
       
    '#type' => 'radios',
       
    '#title' => 'radios with zero option',
       
    '#options' => array(
         
    0 => 'zero',
         
    1 => 'one',
        ),
      );
     
    $form['empty_string'] = array(
       
    '#type' => 'radios',
       
    '#title' => 'radios with empty string option',
       
    '#options' => array(
         
    '' => 'empty',
         
    '1' => 'full',
        ),
      );
     
    $form['submit'] = array(
       
    '#type' => 'submit',
       
    '#value' => 'Submit',
      );
      return
    $form;
    }

    function
    mymodule_form_submit($form, $form_state) {
     
    var_dump($form_state['values']);
      exit;
    }
    ?>
  2. Re #252: we also need to return $value, and I didn't want PHP notices if #default_value doesn't exist as a key. Feel free to submit an alternate patch if you have a way to clean that up.

    I think we could also have done something like this:

    <?php
    if (!isset($element['#default_value'])) {
     
    $element['#has_garbage_value'] = TRUE;
    }
    else {
      return
    $element['#default_value'];
    }
    ?>

    since returning nothing from a function is the same as returning NULL, but for clarity it may be better to explicitly return something always (as your above patch does), so I think it's fine as is.

  3. #249 does have the garbage-set twice as I suggested.

    I still only see one call to $element['#has_garbage_value'] with that patch applied, but as mentioned, I think that is OK.

chx’s picture

OMG I got confused. Yes, #249 is fine and has only one garbage value call. It doesn't need two because the case when the user selects nothing, we load defaults AND validate *that*. Niiiiiiice.

attiks’s picture

Status:Needs work» Reviewed & tested by the community

I can no longer break this, except for the bug found in #247, but since it out-of-scope ... RTBC

David_Rothstein’s picture

Status:Reviewed & tested by the community» Needs work

Needs work for #251/#257 still. But that should be a very quick reroll which I can do tonight. I think we're extremely close :)

David_Rothstein’s picture

Status:Needs work» Needs review
StatusFileSize
new4.22 KB
PASSED: [[SimpleTest]]: [MySQL] 37,111 pass(es).
[ View ]
new4.03 KB
new1.45 KB

Here's the patch with that change, and some minor code comment tweaks. I did a final sanity check of the D7 version against Comment Notify, Webform, and Delta, and everything checked out.

So, are we ready to go here?

effulgentsia’s picture

Status:Needs review» Reviewed & tested by the community

Yes.

sun’s picture

#261 looks clean + ready to me, too.

webchick’s picture

Version:8.x-dev» 7.x-dev
Status:Reviewed & tested by the community» Patch (to be ported)

Awesome. Thanks a lot for all the thorough testing on this, folks!

Committed and pushed to 8.x.

I wasn't 100% sure if the same fix was backportable to D7, so marking 7.x and "patch (to be ported)" for now.

David_Rothstein’s picture

Status:Patch (to be ported)» Reviewed & tested by the community
StatusFileSize
new4.03 KB
PASSED: [[SimpleTest]]: [MySQL] 39,262 pass(es).
[ View ]
new1.43 KB
FAILED: [[SimpleTest]]: [MySQL] 39,267 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

Thanks for the quick commit!

Here's the Drupal 7 patch from #261, renamed and reuploaded to make sure it passes tests.

I'm moving this straight to RTBC based on my comment in #245 - it's exactly the same patch as D8 and it's the one that's had all the manual testing. I'll wait until Sunday or so to commit it, though. (That's the day I'm planning to go through and commit the last patches for the next release anyway.)

David_Rothstein’s picture

Status:Reviewed & tested by the community» Fixed
Issue tags:+7.15 release notes

Committed to 7.x - thanks everyone! http://drupalcode.org/project/drupal.git/commit/331a375

There will need to be some D8 followup issues filed here, but I don't have time to do that myself right at this moment.

Status:Fixed» Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

than1337’s picture

Is there any kind of similar solution for the select boxes?

Thanks.