Make optgroups work with choice checker

please use isset() instead of !== NULL . Also why $value needs to be an array? form_radios? Not-multiple selects? I thoguht these return a string.

while I definitely like your patch, there are minor issues, like $msg !== null still does not use isset. Also is it possible that choices are not an array? Is this something we need to check? After all choices are set by the developer not by the end user IMO.

um, essential is an understatement. this is a security hole waiting to happen. possibly more validation code can be removed once this hits. i know that some will be removed from og. probably many more modules. this validation one of the main reasons we were doing form api, at least in my mind.

i checked with Adrian and chx and they both agreed that this is worthwhile.

my patch really needs some review. we are now settings errors in the form_legal_choice_check() function instead of returning error messages.

um, essential is an understatement. this is a security hole waiting to happen. possibly more validation code can be removed once this hits. i know that some will be removed from og. probably many more modules. this validation one of the main reasons we were doing form api, at least in my mind.

i checked with Adrian and chx and they both agreed that this is worthwhile.

my patch really needs some review. we are now settings errors in the form_legal_choice_check() function instead of returning error messages.

I deleted a debug chunk.

In my first effort to test this, it appears to fail. Not sure why.

I simply applied the patch to a fresh Head, altered a story form to create a -99 value for the menu option list and submitted it. It saved just fine, no error messages. I added some debug to form_legal_choice_check() to see if it was being called, and it was. Did not get further with trying to find where the failure was before I was called away to dinner. :-)

This sounds like a must have. Actually, I believed we already had this build in.

Do we really need SKIP_CHOICE_CHECK and SKIP_VALIDATION?

Please write $message, not $msg.

The $value, $v and %val stuff is somewhat confusing. Also, write %value, not %val.

Is it 'Illegal choice' or 'Invalid choice'? Can we make the error messages a tad more verbose?

Please write explicit if-statements here:
$msg ? form_error($element, $msg) : form_error($element, t('Illegal choice (none provided).'));
$msg ? form_error($element, $msg) : form_error($element, t('Illegal choice in %title: %val.', array('%title' => theme('placeholder', $element['#title']), '%val' => theme('placeholder', $v))));

It appears that my test failed because form_legal_choice_check() was not even called. In fact, in none of the normal user select lists that I tried (profile, taxonomy, menu) is it even called, although illegal values are silenty discarded. I assume valid values for those lists are being checked elsewhere in the code.

The only time I saw form_legal_choice_check() being called was from admin screen select lists. And there, it seems form_error() is never called.

I think maybe these 2 lines:

foreach ($value as $key => $v) {
    if ($key && !array_key_exists($key, $choices)) {

Need to be changed to read like this:

foreach ($value as $key => $v) {
    if ($v && !array_key_exists($v, $choices)) {

Thanks for the review Chris. There is likely a problem in the lines that you highlight. But I don't think your proposed change is right. In a select or checkbox field, $v is the display text and $key is the value which will be saved. I don't have a devel environment today to play with this more.

Right, which is why I wasn't so sure about my suggestion. But it appears that the $value array is not structured like a select or checkbox array. Its keys appear to be 1, 2, 3... and its values appear to be the keys in the select/checkbox array $choices.

I patched my install to use $v as suggested and for my _one_ simple-minded test, it seems to work correctly (and my debug output looks right, too).

I think this works now. While at it, I corrected a bug with multiple selects: if there was no default_value and there was a 0 keyed element then it was always selected.

http://drupal.org/node/41518 was marked as a dupe of this but after checking the patch on #15 it still seems to be a problem.

Right, I even deleted the relevant lines from my patch here, do not mix things.

Checkboxes are just too different -- I needed to add special code for them.

*sigh*

*sigh*

Major cleanup. There is no need for a separate function or a possibility to not call this. Also, #options is not something the user submits, no need to check it whether it's an array.

Another couple bytes!

I checked the theme settings and created some selects -- all seems well.

Committed to HEAD. Thanks.

This is broken - doesn't support optgroups (like those used in the category select of admin/node, or the project select of project module's add/issue).

#default_values is botched. We change it to be array(key1 => return_value , key2 => return_value) usually array(key1=>1, key2=>1) because that's submitted back by browsers and it's a chaos that the two input form clashes.

Several issues have been opened because of this.

Patch for aggregator, blogapi and profile modules. comment module looks ok, which makes me suspect I'm confused.

I'll merge patches later.

I merged chrisxj patch, and even added some salt from matt's patch. Matt have found out most of this alone, which is pretty fantastic.

The work is done, review please.

I have not forgot optgroups, but that's much less a problem.

Code looks good to me. Thoroughly test, this indeed fixes egregious problems I keep running into under the permission pages and while uploading files. This patch is quite critical, as it is very easy to get the "illegal pointer" error (I ran into it 3 times today, and once during a demo to my boss, eek!).

There are more changes in this patch than merely the fix of these bugs. New version of patch attached which adds the flipping of node_options in an update.

Screens changed: language management screen, aggregator categories, content types screens, blogapi content types, admin themes, admin modules, vocabulary edit, upload. profile block configure needs testing too: a) have it kept your old settings? b) regardless of old settings, can you set it up now?

Check boxes seem totally borked in HEAD right now so I thought I'd try out this patch to see if it fixed any of it.

Comparing the user edit form: before, it all checkboxes were checked, now, they're all unchecked, and settings don't seem to be saved.

I threw a couple var_dumps in and noticed that it seems like the form is being built correctly:

#default_value array
  'authenticated user' => 1


#options array
  2 => 'authenticated user'
  4 => 'dj'
  3 => 'staff'

But when it renders they're all unchecked.

The problem seems to be in form.inc : expand_checkboxes(). When setting #default_value it's using $value[$key] rather than $value[$choice]. I've attached a patch for just form.inc because I'm afraid if I roll a big patch I'll introduce some un-related changes. Once this is posted I'll try it against a clean checkout of Drupal.

disregard drewish's patch. the user edit screen is another (though may be related issue).

#options is built of key => title pairs (the $choice name in expand_checkboxes is not the most fortunate)
#default_value is now default pairs of key => return_value . So the only thing in common is they key.

while half a dozen issues are a duplicate of this , http://drupal.org/node/42057 is not! at least not yet :)

I really can't understand people, I list all screens which needs testing and they carefully disregard my words.

I'll carefully disregard good taste and say "Awesome way to encourage folks, chx, whOo!"

This patch solves the role checkboxes problem, too.

This makes expand_checkboxes a bit easier to understand by renaming $choice to $title.

Added phpdoc to form_checkboxes_default.

I applied checkboxes_5 earlier today but I still get errors when editing or creating a node. When I check a box in a checkboxes group, I get an "Illegal choice" error when submitting or previewing. With all boxes unchecked, I get no error. Before the application of the patch, I had just the opposite behavior: no boxes gave error while having boxes checked produced an error.

I'm new to forms API so it could be me. So here's ther relevant part of my code in case there is something wrong with it:

$form['city_council']['checked_pols'] = array(
    '#type' => 'checkbox_columns',
    '#description' => 'Check off all the city councilors you\'d like to make the request of.',
    '#default_value' => is_array($node->checked_pols) ? array_keys($node->checked_pols) : array(),
    '#options' => $pols, //an array with nids as the key and the label as the value.
);

One other clue: the checkboxes work just fine on my admin/settings page. No errors produced at all.

You can see my actual form at http://drupalcvs.dondley.com/node/add/pol_tracker_request. Use "steve dondley" in the authored by field to avoid the no author error. $_POST['edit'] is output to the screen to help troubleshoot.

Hope this can be helpful in some way. Thanks!

Instead of array_keys, use form_checkboxes_default (since #6). You need an array which is in the form of key1 => 1, key2 => 1.

I checked that these forms load correctly and that changes are saved:
* admin/modules
* user/*/edit roles
* admin/themes
* admin/block (though, i'm not sure if this was on your to test list)

The only wierdness I noticed was on admin/settings/content-types/*. The settings weren't loaded the first time, but I was able to save the settings (I'm not sure if this is unrelated to this patch).

Yes, node options is a known issue, I flipped them around, there is the update SQL to keep your settings. So we still have aggregator categories, blogapi content types, vocabulary edit, upload (ie try to attach a file and change its checkboxes later) and profile block configure (not general block configure, but specifically profile's). Thanks for the help though, we are getting closer!

Tested the following:
* admin/block/configure/profile/0 (aka 'Author information' block, aka profile block configure): it loaded and saved correctly.
* admin/taxonomy/edit/vocabulary/*: the settings weren't being loaded, there were saving though.
* upload attaching to a node: the delete checkbox worked but the "list" checkbox wouldn't save if it was unchecked.
* admin/aggregator/edit/feed/*: seemed to work fine on some trivial test data.
* admin/settings/blogapi: gave me "Call to undefined function form_checkboxes_default() in drupal/modules/blogapi.module on line 566"... not sure if it's a problem on my end.

Redid the whole thing. Now it's nice and VERY simple.

The user module patch has nothing to do with this issue.

when #default_value is not set, we do not want a warning.

when this goes in, I'll give some love to optgroups. Their options needs some flattening because HTML hands you a flattened array. The necessary function can be copy-pasted from the php manual user comments. Piece of cake.

I understand that this is the best and simplest solution to fixing the problem, but I don't like the idea
of adding type specific code to the form_builder function.

What happens if, in that opt-group patch you mentioned, you need to make more of these type specific changes.

Why not do

if (function_exist($form['#type'] . '_values')) {
  ${$form['#type'] . '_values'}($form);
}
else {
   $form['#value'] = $form['#default_value'];
}

[?
function checkbox_values(&$form) {
$value = array();
foreach ($form['#default_value'] as $key) {
$value[$key] = 1;
}
$form['#value'] = $value;
}
?]

Same thing, more flexible code.

of course i meant

function checkboxes_values(&$form) {
$value = array();
foreach ($form['#default_value'] as $key) {
$value[$key] = 1;
}
$form['#value'] = $value;
}

Adrian, optgroups options flatten is needed in validate not here. But I understand your concerns, and you are still the master in these matters. Minor nitpick: it's _value not _values , we have #value and #default_value.

I checked a few screens and all is in order, which is not a surprise -- the change only makes default_value structure consistent with value.

Committed to HEAD. Does this allow us to close any other issues?

Committed, whee! No other issues can be closed IMO, m3avrck and I constantly marking duplicate related issues, but I'll take another look.

The only issue that was not closed or marked as a duplicate was this one, nested checkboxes: http://drupal.org/node/42008 ... now this might be related to the optgroup chx spoke about and if so, we can close that issue. Not sure what to do about that one so it is still open.

Now that I have changed the title, the issue m3avrck mentions is a duplicate.

Here is a patch which intends to make selects more readable and solves the optgroups problem via a nice recursive flatten function.

Haven't tested (heading to bed) but you've got a "$selectd" (sic) in the final part of the patch.

Hm, well, i have not seen select being 'stuck' but still, Morbus is right.

I've tested chx's patch in #61, and it works. It should be committed.

However, the only place I know we use optgroups in core (category filter in admin/content) is also broken, and has been for a bit - even before the original patches of this issue. The attached patch fixes admin/content, and has been informally reviewed by chx. This patch can ONLY BE COMMITTED if chx's #61 gets in, as it relies on the flattening. Project.module also uses optgroups, and may need a similar patch for that, but it'll be a bit before I can take a look.

Gah. admin/node, I mean.

chx:
It seems to me form_flatten_array() is broken if you have more than one select box: the options for the first will be considered valid for the second?

Also, this is a bit off-topic, but shouldn't we check_plain() the choice keys too? They are rarely going to be freeform strings, but the possibility exists.

Morbus:
Your admin/node changes don't go deep enough. In fact, that code got severely mangled by some patch before:

function node_filters() {
/// ... SNIP ... ///
  if ($taxonomy = module_invoke('taxonomy', 'form_all')) {
    $terms = array();
    foreach ($taxonomy as $value) {
      $terms = $terms + $value;
    }
    $filters['category'] = array('title' => t('category'), 'where' => 'tn.tid = %d',
                                 'options' => $terms, 'join' => 'INNER JOIN {term_node} tn ON n.nid = tn.nid');
  }
  if (isset($filters['category'])) {
    $filters['category']['options'] = $taxonomy;
  }

Note that $filters['category'] is never set when this code is called. So, we fetch the (optgroupted) taxonomy tree, flatten it, assign it to $filter['category']['options'], and then we replace the flattened options list again with the optgroupted one.

With your patch, when we validate the value, we flatten that array again, while the reverse lookup of tid to term name (for the UI) uses a complicated foreach loop.

I wrote the original code with the least amount of flattening in mind, but after the function split somebody obviously mangled the logic for this. It needs to be cleaned up properly and not by adding in more hacks and special handling.

the select_cleanup_0.patch worked for my issues with the select dropdowns. i have multiple dropdowns on the same page, but they have the same set of options. +1 on commiting and fixing Stevens problem in a new revision, as this does fix at least some of the problem (all of the problem i am having).

By the way CVS is still broken for checkboxes if you don't select anything 'Illegal choice' is returned even when not required.

Sincerely,

YAPC

Reset introduced.

Node admin does NOT belong to this issue.

This patch does indeed fix the problems on node/admin for me, no more illegal choice errors.

Looking over the code, I don't see any glaring problems and seeing how critical this is, let's bump it up to RTC.

This worked for me with a HEAD from about 24 hours ago.

Committed to HEAD. Thanks.

Sorry for this late intervention - I just found out this thread.
The "legal choice" validation step breaks a functionality in one of my (tailor-made) modules, where the options of a "select" are automatically (client-side) generated based on the value of another form element. Without delving into the details, it is really useful in terms on user experience.

The early posts of this thread mention a SKIP_CHOICE_CHECK flag, and Dries wondered whether it would be needed - well, I need it.
Any chance of having that reconsidered ?

You surely have the legal options in advance, right? So, add #options in your form and create a custom theme for this element, which unsets the #options and calls theme('select', $element); . This will keep safety.

If you do not know... welll... this became so important a cornerstone in Drupal's safety that I am a bit reluctant to add the possibility to skip it. Even if it turns out that you really need this one, then I will call it DANGEROUS_SKIP_CHECK_CHOICE .

You surely have the legal options in advance, right?

Well, the number of legal options is finite, but quite huge. The user enters a (french) zip-code in a textarea, and an ajax call queries the db and fills the select with (possibly up to 10) matching towns. At form-description time, there are about 39.000 french cities being "legal choices"...

So I guess I won't be able to use the workaround you suggest :-)

I use this "db query - select form input" mechanism instead of a plain textarea for the city name because i don't want to allow any mispelling or abbreviation mismatch or whatever - In other words, I have my own "legal option check" here, so I could really use a way to skip the built-in one when it doesn't fit.

The involved module is specifically designed for a single website, it won't (as far as I can say right now) benefit the community as a contrib, so I feel a litle awkward asking for a core feature just to meet my needs.

Then again, I think the pattern I'm descibing here might not be that rare ?

OK, I found a way to bypass the 'legal option check' for my problematic case...
If you're interested :
- I use hook_elements to define my own 'select_no_validation' form element type,
- in the form description, I give the involved element no '#options', but i set a non-semantic '#options_no_validation' array instead.
- the 'select_no_validation' element is themed with :

function theme_select_no_validation($element) {
  $element['#options'] = $element['#options_no_validation'];
  return theme_select($element);
}

So the case is closed for my own personal problem : i don't NEED any modification of forms.inc.

The question rather is, from a general point of view :
- are there legitimate cases where a module coder might want to disable the 'legal choice check' (i think my recent posts here provide for one) ?
- should those cases require using the workaround I described above, or should a more simple 'DANGEROUS_SKIP_CHECK_CHOICE' flag be provided ?

I mean, a bypass is feasible anyway. It uses pure drupal-style coding, yet I can't help finding it a bit 'hackish'. So why not provide a plain and simple flag ? The DANGEROUS part making it explicit enough...

Would be good to submit this as a tip somewhere in the Forms API docs, just for those that might need it, insetad of keepng it buried in this thread.

I think this is a legitimate use case, and that we should add the DANGEROUS_SKIP flag.

Let there be one.

Better wording.

Do we _really_ need this?

I really do not know but I defer to moshe.

IMO it is legitimate. Validation is there as an aid to security, not to limit your options (as a programmer).

Committed to HEAD.

It's enough to have the word dangerous in the attribute; let's not capitalize it.

Do we really want to have to lowercase attributes before we check them? No! All form API attributes should be lowercased.

This patch makes it #dangerous_skip_check, not #DANGEROUS_SKIP_CHECK.

I'd say this is now a task, since it is changing the API. It will need review and documentation of the change, but I think this is an okay change.

No, no, no. Nothing can change me. Of course, the greated gods are free to overwrite my opinion but this attribute has so serious consequences that it needs to stand like a sore thumb.

OK I revoke that. I do not care. My policy was that we do not babysit broken code. Well, then be it.

But if this goes through then you automatically accept that I will remove quite some function_exists from form API because I will automatically presume that we really do not want to babysit broken code.

no idea what this is about, but it looks as if it shoul dbe in the cvs queue.

Unless someone cares about this more than chx, its not really worth it to change it since we never see it anyway.

@chx: i think we should remove those function_exists() too. masking problems does more harm than good.

i'm fine with marking this as 'by design'

i was led to this patch (dangerouslc.patch) -- which i guessed was for form.inc, but it's dang hard to tell -- by node 76141, 63734, & 39179#comment-65918. none of the fixes -- the patch, or commenting out lines 222-223 of form.inc -- works.

my webform (4.7.3) is fine until i add a textarea. i have, in weight order, 2 textfields, 1 email, a select (multiple) and then the textarea for comments. this exact form has worked in 4.6.2.

in 4.7.3, when i add the textarea at the end, i get "An illegal choice has been detected. Please contact the site administrator."

when i delete the textarea, the form works again. hard to add comments to the form this way, however.

** oy. i added the textarea back in, but placed it at the top (-8) of the form -- it worked. moved it to right above the select -- worked. right below -- it worked. several steps below -- worked. changed line height from 3 to 6 -- worked. wtf? anyway, it's working. but this is goofyweirdshit. go figure.

when y'all get talking fixes & patches, you have a tendency to start skipping steps cuz you know what you're talking about. when we poor slobs just trying to get our sites to work with the upgrade try to wade thru your convos, we get lost. i ask you: is that polite?

tia.