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.
$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.
Comments
Comment #1
Nick Lewis CreditAttribution: Nick Lewis commentedRadio 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
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.
Comment #2
Nick Lewis CreditAttribution: Nick Lewis commentedComment #3
cdale CreditAttribution: cdale commentedI 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.
Comment #4
David_Rothstein CreditAttribution: David_Rothstein commentedBugs 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.
Comment #5
mlncn CreditAttribution: mlncn commentedFor 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:
Here's what we get:
(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!
Comment #6
Anonymous (not verified) CreditAttribution: Anonymous commentedI patched with this http://drupal.org/files/issues/735426-options-required-default-2.patch but the unhelpful error message is still there.
Comment #7
sunForm 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.
Comment #8
sunA 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.
Comment #9
vito_swat CreditAttribution: vito_swat commentedUse 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.
Comment #10
Bojhan CreditAttribution: Bojhan commentedBugs 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.
Comment #11
bojanz CreditAttribution: bojanz commentedHere'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.
Comment #12
bojanz CreditAttribution: bojanz commentedHad trailing whitespace.
Comment #13
mlncn CreditAttribution: mlncn commentedNow 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...
Comment #14
idflood CreditAttribution: idflood commentedPatch in #13 looks good and is working for me.
Comment #15
mlncn CreditAttribution: mlncn commentedComment #16
sunWe need to restart from scratch.
Comment #18
sunThis is what we should do..... umph.
Comment #20
mlncn CreditAttribution: mlncn commented#18: drupal.form-radios-required.17.patch queued for re-testing.
Comment #22
David_Rothstein CreditAttribution: David_Rothstein commentedChanging 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.)
Comment #23
David_Rothstein CreditAttribution: David_Rothstein commentedThis 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.
Comment #24
mlncn CreditAttribution: mlncn commented@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
Comment #25
bojanz CreditAttribution: bojanz commentedI 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.
Comment #26
David_Rothstein CreditAttribution: David_Rothstein commentedThe patch in #13 changes the meaning of #default_value = NULL across all form elements via this code:
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.
Comment #27
mlncn CreditAttribution: mlncn commented@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.
Comment #28
chx CreditAttribution: chx commentedLet me see.
Comment #29
chx CreditAttribution: chx commentedThanks 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.
Comment #30
chx CreditAttribution: chx commentedNote 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.
Comment #32
rfayHas to go to D8 and come back unfortunately.
Comment #33
mlncn CreditAttribution: mlncn commentedI'm still getting the unhelpful error message.
... 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. :-/
Comment #34
effulgentsia CreditAttribution: effulgentsia commentedsubscribe
Comment #35
Isostar CreditAttribution: Isostar commentedsubscribe
Comment #36
pillarsdotnet CreditAttribution: pillarsdotnet commentedRe-roll of #29 for current 8.x checkout. Looks like part of it went in already.
Comment #37
pillarsdotnet CreditAttribution: pillarsdotnet commentedComment #39
pillarsdotnet CreditAttribution: pillarsdotnet commentedTrying again...
Comment #41
pillarsdotnet CreditAttribution: pillarsdotnet commentedAnd again...
Comment #42
pillarsdotnet CreditAttribution: pillarsdotnet commentedComment #44
pillarsdotnet CreditAttribution: pillarsdotnet commentedThe errors seem to come from the
field_ui_field_edit_form()
function infield_ui.admin.inc
: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, labeledN/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.
Comment #45
pillarsdotnet CreditAttribution: pillarsdotnet commentedThis one lets tests pass but the change looks completely bogus. Can somebody help with this?
Comment #46
chx CreditAttribution: chx commentedIf 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.
Comment #47
pillarsdotnet CreditAttribution: pillarsdotnet commented@chx: Here's the change that I think is bogus:
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.
Comment #48
HnLn CreditAttribution: HnLn commentedsub
Comment #49
chx CreditAttribution: chx commented./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 :(
Comment #50
pillarsdotnet CreditAttribution: pillarsdotnet commentedOkay, 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?
Comment #51
webchickI 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.
Comment #52
pillarsdotnet CreditAttribution: pillarsdotnet commentedAfter 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.Comment #53
apatrinos CreditAttribution: apatrinos commentedHi, 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.
Comment #54
webchickThanks 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!
Comment #55
webchickOops. And restoring tag.
Comment #56
sirtetBefore 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).
Comment #57
pillarsdotnet CreditAttribution: pillarsdotnet commentedUpdated patch incorporating suggested improvement from #53 and also adds the following code to the
createListField()
function:Passes tests locally, but I'm still not sure this is the correct approach.
Comment #58
pillarsdotnet CreditAttribution: pillarsdotnet commentedShould have set version to 8.x-dev before uploading that patch. Not sure if it applies to 7.x.
Comment #59
pillarsdotnet CreditAttribution: pillarsdotnet commentedShould 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?
Comment #60
David_Rothstein CreditAttribution: David_Rothstein commentedSomething 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.
Comment #61
pillarsdotnet CreditAttribution: pillarsdotnet commentedOkay, re-rolled. Still don't know whether this chunk is correct; would appreciate some feedback:
Comment #62
attiks CreditAttribution: attiks commentedsubscribing for #1239910: META: tracking other issues about validation
Comment #63
apatrinos CreditAttribution: apatrinos commentedHi 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
Comment #65
pillarsdotnet CreditAttribution: pillarsdotnet commentedCNR for patch in #61
Comment #66
robertom CreditAttribution: robertom commentedSubscribe
Comment #67
thekevinday CreditAttribution: thekevinday commentedI 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
Comment #68
13rac1 CreditAttribution: 13rac1 commented#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...
Comment #69
idflood CreditAttribution: idflood commentedthe 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
Comment #70
pillarsdotnet CreditAttribution: pillarsdotnet commentedWhen 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.
Comment #71
13rac1 CreditAttribution: 13rac1 commented#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.
Comment #72
pillarsdotnet CreditAttribution: pillarsdotnet commentedAdded one-line patch from #1301596-1: An illegal choice has been detected on Check boxes/radio buttons widget with radios
Comment #73
rfayIf you're going to do that IMO it needs to go back to CNR.
Comment #75
pillarsdotnet CreditAttribution: pillarsdotnet commentedSorry; 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.
Comment #76
13rac1 CreditAttribution: 13rac1 commented#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.
Comment #77
pillarsdotnet CreditAttribution: pillarsdotnet commentedThanks, eosrei, but ... now that you've touched it, you can't mark it RTBC anymore!
:-(
Comment #78
13rac1 CreditAttribution: 13rac1 commentedYep, I know.
Comment #80
pillarsdotnet CreditAttribution: pillarsdotnet commentedLooks like that change causes a test error, which I don't want to debug, so back to the (previously-RTBC) version in #70.
Comment #81
catchIt'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?
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
Comment #82
pillarsdotnet CreditAttribution: pillarsdotnet commented@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.
Comment #83
loganfsmyth CreditAttribution: loganfsmyth commented@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
Comment #84
pcambraThe patch in #83 fixes the issue and applies correctly in 7.9
Comment #85
xjmThanks for your work on this patch! I read through it and I have a few suggestions:
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.
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.)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.
Comment #86
loganfsmyth CreditAttribution: loganfsmyth commentedI'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.
Comment #88
pillarsdotnet CreditAttribution: pillarsdotnet commentedNext 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".
Comment #89
pcambraI'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.
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.
Comment #90
loganfsmyth CreditAttribution: loganfsmyth commentedI 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.
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.
Comment #91
baby.hack CreditAttribution: baby.hack commentedThough 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.
Comment #92
loganfsmyth CreditAttribution: loganfsmyth commentedI'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.
Comment #93
thekevinday CreditAttribution: thekevinday commentedWhat @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.
Comment #94
loganfsmyth CreditAttribution: loganfsmyth commented@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.
Comment #95
TonyK CreditAttribution: TonyK commentedsubscribe
Comment #96
loganfsmyth CreditAttribution: loganfsmyth commented@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.
Comment #97
xjmAlright, #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:
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()
.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!
Comment #98
xjmDo 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?
Comment #99
loganfsmyth CreditAttribution: loganfsmyth commentedGood 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.
Comment #100
xjmAlright, #99 looks RTBC to me. Thanks @loganfsmyth.
Comment #101
catchAbove 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?
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.
Comment #102
xjm#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.
Comment #103
loganfsmyth CreditAttribution: loganfsmyth commentedYeah, 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.
Comment #104
xjmMaybe 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?
Comment #105
xjmI'm also working on a test based on #1162712: AJAX submissions try to validate other form fields: "An illegal choice has been detected.".
Comment #106
xjmHere's that other test. Locally, it fails without #103 and passes with it.
Comment #107
xjmWithout trailing whitespace and with better comments.
Comment #109
xjmComment #110
xjmHere's what I think we'd need to do with a validator to not rely on the specific error message text. Attached:
_form_validate()
and prints a custom debug message under the conditions that would trigger the "illegal choice" error.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:
This hunk here is actually a mistake and should be removed.
Comment #111
xjm_form_validate()
repeats this bit twice: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.
Comment #112
xjmI 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.
Comment #113
xjmSimpler test as per #112.
Comment #114
xjmGrr.
I should roll a patch with the other test anyway.
Comment #115
xjm#113 plus #107, both tests + the fix.
Comment #116
David_Rothstein CreditAttribution: David_Rothstein commentedNot a full review, but a couple things:
Typo.
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.
Comment #117
xjmThat 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.)
Comment #118
xjmCorrecting 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).
Comment #119
xjmFixing docblock indentation, no other changes from #118.
Comment #120
David_Rothstein CreditAttribution: David_Rothstein commentedOK, 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:
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:
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.
Comment #121
xjmAh, 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:
And then probably one more sentence about why the empty string is bad?
Comment #122
xjmHow about:
So:
Edited a few times.
Edit: Ah, maybe we could use some of the explanation in #44.
Comment #123
sunShouldn'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).
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.
Comment #124
xjmThat 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'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?Comment #125
David_Rothstein CreditAttribution: David_Rothstein commentedRegarding wording, something like #122 looks good to me, after fixing minor grammatical errors :)
Comment #126
loganfsmyth CreditAttribution: loganfsmyth commentedI 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.
Comment #128
loganfsmyth CreditAttribution: loganfsmyth commentedOKAY, one more time without the typo.
Comment #129
xjmRe: Strings being wrapped in
t()
-- Not wrapping test assertions int()
was deliberate. No one is going to translate test assertions.Edit: See #500866: [META] remove t() from assert message.
Comment #130
loganfsmyth CreditAttribution: loganfsmyth commentedYou are probably right for the string output in the submit handler, but all the rest should still have been.
Comment #131
xjmOkay, I read it again and you're probably right about the FAPI texts. However, I think these changes aren't needed:
...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.Comment #133
loganfsmyth CreditAttribution: loganfsmyth commentedI'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.
Comment #134
sunI do not like the usage of
#has_garbage_value
here. So far,#has_garbage_value
is only used by _form_button_was_clicked() in order to signify that, although there is a#value
for an (image) button, it is "garbage" and not to be used as actual value.However, I do not see a way to fix this differently - without entirely rewriting Form API's options checker. Thus, I'm fine with this change (but will create a follow-up issue for D8 to rewrite the options checker).
Furthermore, the change of #default_value from NULL to FALSE in form_process_radios() looks bogus to me. While theme_radio() currently checks that the #value is not FALSE before adding the 'checked' attribute, a #value of FALSE was never possible. I've double-checked the code throughout form.inc and I'm confident that this special condition for FALSE is just simply stale code and obsolete.
#1381140: A #default_value = 0 for #type radios checks all radios partially depends on this patch, and in fact, has quite some overlap as it also concerns the #value/#default_value handling of #type radios, so I merged the additional one-line tweak into this patch.
Lastly, I've slightly improved the tests and comments.
This looks pretty much ready to fly for me.
Comment #136
xjmSo 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.
Comment #137
xjmOops.
Comment #138
sunReverted the merged-in changes and re-opened #1381140: A #default_value = 0 for #type radios checks all radios
Interdiff is against #133.
Comment #139
xjmMost 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.Comment #141
xjmAlright, 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.
Comment #142
xjmAnd I named my interdiff .patch again. darnit. :P
Comment #144
xjmComment #145
xjmMarked #1426146: Add another item button causes an error, "An illegal choice has been detected. Please contact the site administrator" as duplicate of this issue.
Comment #146
Pedro Lozano CreditAttribution: Pedro Lozano commentedThe patch at #141 works for me in D8.
If anyone needs the patch for D7, the one at #45 still aplies cleanly and works.
Comment #147
Pedro Lozano CreditAttribution: Pedro Lozano commentedThe 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.
Comment #148
DamienMcKenna+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?
Comment #149
xjmRe: #148: Baking cookies or making snarky remarks won't help, but reviewing the latest patch and marking it RTBC if appropriate will.
Comment #150
DamienMcKennaCommit time?
Comment #151
mrfelton CreditAttribution: mrfelton commentedWith 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.
Comment #152
hanoii#141 works on D7, haven't tested context module though.
Comment #153
Fidelix CreditAttribution: Fidelix commentedmrfelton, I have tested the patch in #141 with context.module, and I was able to add/remove blocks with no problems.
Comment #154
catch#141: 811542-141.patch queued for re-testing.
Comment #155
xjmWeird; why did the bot not NW it?
Needs a reroll I guess.
Comment #156
tim.plunkettReroll.
Docblock for
form_type_tableselect_value()
changed.Comment #157
ryan.gibson CreditAttribution: ryan.gibson commentedI can confirm that before the patch the steps reproduced the error.
The patch applied cleanly.
After the patch, I can confirm that the illegal choice message was removed and a required field replaced it.
Looks good to me!
Comment #158
xjmReroll is OK. Thanks @ryanissamson and @tim.plunkett!
Comment #159
chx CreditAttribution: chx commentedIndeed good to go.
Comment #160
webchickWow! 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.
Comment #161
DamienMcKennaThe patch from #156 rerolled for D7.
Comment #162
sunComment #163
webchickThanks, sun. :) Committed/pushed #162 to un-break 8.x. Back to 7.x and needs review.
Comment #164
chx CreditAttribution: chx commented#161 is green.
Comment #166
pillarsdotnet CreditAttribution: pillarsdotnet commentedRe-uploading #161.
Comment #167
xjmmeep meep
Comment #168
tim.plunkettPatch 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.
Comment #169
TonyK CreditAttribution: TonyK commentedI think this really needs backport to D6 either (I'm nto sure if I may add the tag).
Comment #170
xjm@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?
Comment #171
webchickCommitted and pushed to 7.x. Thanks!
Comment #172
webchickThis is probably worth mentioning in the release notes, actually.
Comment #173
jeremypinto CreditAttribution: jeremypinto commented#168: drupal-811542-168.patch queued for re-testing.
Comment #175
tim.plunkettUhm.
Comment #177
timmetj CreditAttribution: timmetj commentedI'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
Comment #178
timmetj CreditAttribution: timmetj commentedEdit: 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
Comment #179
thekevinday CreditAttribution: thekevinday commentedIt 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:
However, this produces a new problem.
The line that has the reset() looks like:
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().
Comment #181
bhauff CreditAttribution: bhauff commentedI 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.
Comment #182
kimu CreditAttribution: kimu commentedI 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.
Comment #183
quicksketchThis 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).
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:
Comment #184
timmetj CreditAttribution: timmetj commentedI 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?
Comment #185
webchickEscalating. Sorry for breaking stuff. :( :(
Comment #186
fenstratMarked #1560062: After Drupal 7.14 core update, getting error Illegal choice 0 in notify_type element as a duplicate of this.
Comment #187
catchThis will need to be fixed in 8.x again then backported.
Comment #188
attiks CreditAttribution: attiks commentedWas trying to fix this in drupal7, so just to make sure I didn't break anything. Will move it back to D8 after bot
Comment #189
attiks CreditAttribution: attiks commentedBTW: the patch in 188 solves the problem for webforms on drupal7 (but might break other things)
Comment #191
attiks CreditAttribution: attiks commentedPatch against D8
Comment #192
attiks CreditAttribution: attiks commentedFYI: I cannot reproduce the failing test for D7, mine fails on
But it works for the testbot or if I do it manually ...
Comment #193
fenstratThe patch in #188 fixes the issue with comment_notify module in #1560062: After Drupal 7.14 core update, getting error Illegal choice 0 in notify_type element
Comment #194
chx CreditAttribution: chx commentedThanks 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?
Comment #195
attiks CreditAttribution: attiks commented@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.No idea, I assume it means the element was defined but no value was received.
No idea, I assume it's set by form_builder.
Can't see any real problem (for now), but will try to add some tests.
Not that I know of.
Can't see any real problem (for now)
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:
Comment #196
attiks CreditAttribution: attiks commentedWebform 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 checkedComment #197
attiks CreditAttribution: attiks commentedAnother problem is that if you use the following form, the first one will be selected
even ifif default value == '''#default_value' => NULL
EDIT: Sorry all, was checking with a change made to core
Comment #198
attiks CreditAttribution: attiks commentedWhich of the following #default_values are supposed to work?These should all work with the form from above.
This one does not work with the form from above.
EDIT: Sorry all, was checking with a change made to core
Comment #199
thekevinday CreditAttribution: thekevinday commentedBut 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.
Comment #200
timmetj CreditAttribution: timmetj commentedLooks 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.
Comment #202
attiks CreditAttribution: attiks commented@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:
The only thing I don't know for sure is if this how it's meant to be?
Comment #203
timmetj CreditAttribution: timmetj commented@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?)
My other case just doesn't sets a #default_value (which brings me to 'testa') and now also works.
And now i can again submit my forms without having to select a radio button.
Thanks.
Comment #204
thekevinday CreditAttribution: thekevinday commented@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:
Comment #206
attiks CreditAttribution: attiks commented@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.
Comment #207
attiks CreditAttribution: attiks commented#204: drupal-8.x-811542-204.patch queued for re-testing.
Comment #209
attiks CreditAttribution: attiks commentedbot: "'failed to set variable [simpletest_clear_results] in checkout database'"
Comment #210
muka CreditAttribution: muka commentedJust a note, #188 gives meaningful messages in D7 when no raidos option is selected.
Comment #211
thekevinday CreditAttribution: thekevinday commented#204: drupal-8.x-811542-204.patch queued for re-testing.
Comment #213
thekevinday CreditAttribution: thekevinday commentedOkay, so looking for "simpletest_clear_results", leads me to this:
modules/simpletest/simpletest.pages.inc
: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.
Comment #214
webchick.
Comment #215
attiks CreditAttribution: attiks commentedBot 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
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.
Comment #216
seb.pisarski CreditAttribution: seb.pisarski commented#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.
Comment #217
attiks CreditAttribution: attiks commented@seb.pisarski see my comment #215, I'm not sure we need a patch
Comment #218
seb.pisarski CreditAttribution: seb.pisarski commentedsubscribe
Comment #219
seb.pisarski CreditAttribution: seb.pisarski commentedWell 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?
Comment #220
attiks CreditAttribution: attiks commentedAFAIK all problems can be solved in the modules, rolling back is not an option since the patch from #166 fixes other problems.
Comment #221
gregglesThe 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.
Comment #222
attiks CreditAttribution: attiks commented@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 :/
Comment #223
greggles@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.
Comment #224
David_Rothstein CreditAttribution: David_Rothstein commentedYeah, 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:
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.
Comment #225
David_Rothstein CreditAttribution: David_Rothstein commentedAlso, @thekevinday, I don't think I understand your proposed change or why it's needed:
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?
Comment #226
sunHrm. Yeah, this totally circles back into #134, in which I found that FALSE value extremely suspicious.
Comment #228
David_Rothstein CreditAttribution: David_Rothstein commentedOK, here's the fix I had in mind. Let's see if it works.
I also reworked and clarified several of the code comments.
Comment #229
sun#228 looks RTBC to me, in case it will pass tests.
Comment #231
David_Rothstein CreditAttribution: David_Rothstein commentedOK, 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.
Comment #232
David_Rothstein CreditAttribution: David_Rothstein commentedHere are new patches. Changes:
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:
That's because they all defined radio buttons something like the following:
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:
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:
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.
Comment #233
mgiffordSo 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.
Comment #234
jpstrikesback CreditAttribution: jpstrikesback commentedBreaks views contextual filters...just putting that there for posterity's sake
Comment #235
effulgentsia CreditAttribution: effulgentsia commented@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?
Comment #236
effulgentsia CreditAttribution: effulgentsia commentedWhat 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.]
Comment #237
jpstrikesback CreditAttribution: jpstrikesback commentedI'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.
Comment #238
David_Rothstein CreditAttribution: David_Rothstein commentedLooking again at #232, I'm starting to think this change to form_type_radios_value() might be a problem:
Because the empty($form_state['input']) check does not match what _form_builder_handle_input_element() uses to decide whether input is being processed:
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:
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.
Comment #239
David_Rothstein CreditAttribution: David_Rothstein commentedHere'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).
Comment #241
mgifford#239: radios-regression-811542-239.patch queued for re-testing.
Comment #243
mgiffordI 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?
Comment #244
David_Rothstein CreditAttribution: David_Rothstein commentedOK, 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:
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.
Per above, the thing that caused major contrib modules to break was typically that code like this stopped working:
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:
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:
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.
Comment #245
David_Rothstein CreditAttribution: David_Rothstein commentedNow 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 :)
Comment #246
chx CreditAttribution: chx commentedWhile 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.Comment #247
attiks CreditAttribution: attiks commentedI 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."
Comment #248
effulgentsia CreditAttribution: effulgentsia commented#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.
Comment #249
effulgentsia CreditAttribution: effulgentsia commentedThis changes the form.inc hunk based on #246 feedback, but the tests from #244 are unchanged.
Comment #250
effulgentsia CreditAttribution: effulgentsia commentedComment #251
David_Rothstein CreditAttribution: David_Rothstein commentedWe 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.
Comment #252
chx CreditAttribution: chx commented#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.
Comment #253
effulgentsia CreditAttribution: effulgentsia commented0 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.
Comment #254
effulgentsia CreditAttribution: effulgentsia commentedRe #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.
Comment #255
chx CreditAttribution: chx commentedAh so. That's good, then.
Comment #256
attiks CreditAttribution: attiks commented#248: I created #1699644: tableselect and required: An illegal choice has been detected and thanks for cleaning the patch.
Comment #257
David_Rothstein CreditAttribution: David_Rothstein commentedIn 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):
I think we could also have done something like this:
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.
I still only see one call to $element['#has_garbage_value'] with that patch applied, but as mentioned, I think that is OK.
Comment #258
chx CreditAttribution: chx commentedOMG 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.
Comment #259
attiks CreditAttribution: attiks commentedI can no longer break this, except for the bug found in #247, but since it out-of-scope ... RTBC
Comment #260
David_Rothstein CreditAttribution: David_Rothstein commentedNeeds work for #251/#257 still. But that should be a very quick reroll which I can do tonight. I think we're extremely close :)
Comment #261
David_Rothstein CreditAttribution: David_Rothstein commentedHere'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?
Comment #262
effulgentsia CreditAttribution: effulgentsia commentedYes.
Comment #263
sun#261 looks clean + ready to me, too.
Comment #264
webchickAwesome. 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.
Comment #265
David_Rothstein CreditAttribution: David_Rothstein commentedThanks 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.)
Comment #266
David_Rothstein CreditAttribution: David_Rothstein commentedCommitted 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.
Comment #268
than1337 CreditAttribution: than1337 commentedIs there any kind of similar solution for the select boxes?
Thanks.