A select list without #default_value always passes form validation

+1 In favor of this! Example case can be when sometimes user just overlooks a form and by default behavior of forms system, in registration form, Male is chosen as gender, but it's a she :P Hence, it would be good to make the user make a right choice.

+1 from me. Sounds like a great idea.

Michelle

+1 this sounds like a good thing.

+1

+1

Hasn't this feature already been included in core?

In which issue was this fixed?

Not really. However, there is no option pre-selected as there was in 5.x. So if the user tries to submit the form it will complain.

IMHO, this satisfies the spirit of the request without the need for a setting somewhere that needs to be translated.

+1

Feature request go to HEAD.

I already see a non-selectable (does not validate if "required") option in most fields in Drupal 6. Which was/were the issue(s) where this was fixed?

After referring to those, please close this issue marking as duplicate.

There is a "-Please choose-" if the vocabulary is required, but not multi-select. If the vocab is multi-select, then there is nothing to prompt a choice other than the little star, which most people don't actually see.

Would have been nice, but too late for D7.

Actually not. Required to solve #358437: Filter system security fixes from SA-2008-073 not applied to Drupal 7.x -- I'll try to come up with a quick, but proper solution.

Gurpartap Singh wrote:
> +1 In favor of this! Example case can be when sometimes user just overlooks a form and by default behavior of forms system, in registration form, Male is chosen as gender, but it's a she :P Hence, it would be good to make the user make a right choice.

Yes, this bug is similar to another one already fixed, #289957: Afghanistan should not be the default value shown in the country field, which affected the Drupal.org country demographics, with Afghanistan as one of the first Drupal countries in number of members... because of the alphabetical order. ;)

It's good this is going to be fixed as well.

Wow, much much easier than I thought. The previous issue title made me totally implement a totally wrong behavior at first, but now it totally works like it should:

Improved condition, clarified comments.

Let's see whether we can get beyond the installer ;)

Thanks to chx, we can describe this bug a bit more precisely. :)

Sure, putting in <Select> instead of the first value makes complete sense. I mean, what if this was a country list? It would be silly to make assumptions on this one.

Just a note : a couple zeeks ago we added some code in list.module to handle this very case for the limited scope of selects as field widgets (de facto fixing the OP in D7).
This code might need to be reconsidered if the issue is adressed at FAPI level ?

(limited web access from a web bar, sorry for not providing an issue # - should be one of the most recent changes in list.module)

Looks like this needs a bit more tweaking. Also changed the text to "- Please select -".

yched, maybe you mean #735426: Fields with select widget: first option is selected by default even if no 'default value' set for the field?

Sorry, the status changed inadvertently.

Ugh. That patch in #735426: Fields with select widget: first option is selected by default even if no 'default value' set for the field is too complex to revert here. Let's re-open that issue after this one gets in.

During debugging the test failures I found out that optional select fields badly need this handling, too. So. Meet "- Please select -". Meet "- None -"!

I am going to do some research what the standard is here.

I got some responses on twitter, from pboersma. One suggestion is "Select <type of thing>" like "Select your number" or "Select your profession". In terms of usability it makes much more sense for this option to be explanatory, rather than merely <Select> which doesn't say much about its contents. All we have to figure out if this is the decided road, how to display this form in the field ui and what not filled out default is (let me know if this is possible in the release cycle).

Thanks, Bojhan! That sounds like a good idea, and the current patch even allows to define a custom string for the empty default option. (Though modules have to specify the entire string, i.e., "- Select your foo -" or "- Select a value -", instead of just the <type>.) But properly updating all select lists to use custom empty options accordingly would have to be done in a separate issue, as I'm not sure whether that is still on the table for D7.

That said, I'm not sure whether it makes sense in the end, especially for screen reader users. Because, in an ideal world, the select label precisely denotes the kind of option values already, so a label of "Your default country" would be duplicate with a first default option that states "- Select your default country -".

We also have to take into account that, most often, this first empty default option text is going to be the most lengthy option value in the list (e.g., consider a list that only contains numbers). It looks very odd if you see a select list like this:

- Select the number of items to show -
1
2
3

So we want to avoid overlengthy select lists, as they can also break a site's theme/layout very easily. (Cutting the width via CSS is no option, as that would hide parts of the empty default option text)

Lastly, "Please select" contains a "Please", which we may want to avoid. While updating a couple of select lists throughout core, I found an alternative in Image module's administrative style effect configuration: "- Select a value -".

We should also take translatability of the agreed on empty default strings into account.

I highly recommend to apply this patch for manual testing and look at some select lists throughout Drupal. Seeing the results, I guess that this is both an awesome improvement and security hardening fix at the same time.

Will try to fix those tests and will probably revert some of the additional select list conversions (leaving to a follow-up issue).

P.S.: Make sure to write < instead of < when wrapping text into something that looks like a HTML tag ;) I've fixed your follow-up.

You are right, the label preseeding the option would make it somewhat of a duplicate. Then again not reading the labels - going straight to the select list, is not a bad idea. I am on the fence, it is duplicating but it's also allowing the user to more quickly scan the exposed fields. Given that we need to change many many modules, to fit this new pattern - I would argue going for a diffrent pattern - but on pure principle this one should be better.

I think we should keep it simple and go for something like < Select > there is no reason to be lengthy or to add the word "value". The word value is also one that belongs primarily in the computer vocabulary and does not make much sense to people understanding the select as different "values" mostly its understood as options.

I scanned several books and the most occurring one was < select >

Thanks!

Personally, I'd really prefer to replace those angle brackets with hyphens, i.e., instead of < Select >, which I think is derived from computer shell/command line programming, where <angle brackets> denote a required parameter and [square brackets] denote an optional parameter, e.g.,

drush <command> [--option]

So instead of that, just simply - Select - and if a value is optional, just simply - None -

Note that there are already two screnshots to compare the two: <Select> and - Select -, whereas the latter looks much more "human" to me.

Sure, makes sense.

Stupid me.

Yay, it passed! I'm going to transfer my markup test into a unit test now.

As visible from the test failures, there's a small API change here, but which only affects single (not #multiple) select lists that happen to be optional (i.e., there is never a #default_value by default).

critical, because this is postponed #358437: Filter system security fixes from SA-2008-073 not applied to Drupal 7.x

... which is one of the last two issues blocking Drupal 7 beta.

This looks important, but I don't think it's critical. The patch shows that other parts of core are already working around this, and #358437: Filter system security fixes from SA-2008-073 not applied to Drupal 7.x can too (and already has code to do so).

We're ready to rumble! Attached patch adds extensive select list validation tests.

ok, rather this one.

So this looks RTBC to me now. Summary:

Problem

• Select lists always validate successfully, even if the user did not select anything at all.

Goal

• Make Form API properly validate select lists that require a user-defined value.
• Make the first empty select list option use a standardized default string, depending on whether a value is required or not.

Details

• Select lists always have a value in HTML. That is, because a user agent/browser renders a select list with the first option being pre-selected, in case no other option has the selected="selected" HTML attribute. A select list option can only have that attribute, if it is the #default_value for the select list.
• This means that a user is able to submit a valid value for a select list by just submitting a form, without even having looked at the select list. However, certain forms require the user to choose a value for a select list, and no safe default can or should be applied. This was also the main reason for having so many people from Afghanistan on drupal.org. A work around has been applied to the user registration form in the meantime, but the proper fix would have been this patch.
• This patch resolves the overall problem by automatically injecting a first empty default option to select lists that do not specify a #default_value. In other words: If a select list already has a #default_value, then the user does not have to select one.
• Effectively, only select lists that do not specify a #default_value are affected by the API change of this patch.

  Normally, such selects are rarely used. The patch converts the corresponding select lists throughout core, and additionally converts some more locations, because I had to find out whether the logic and optional customization works as intended, and whether it can be used and understood easily by developers. There are more select lists throughout Drupal core that ideally should be converted to achieve a consistent UX, but I'm leaving those to a separate issue.

• In order to be able to figure out whether a value for a select list is required or not, this patch sets #required => NULL for all select lists. That is, because unlike other form elements, all select lists in Drupal basically need to be considered as #required by default, since a select list should always have a value. So if a developer only adds a #type 'select' to his form, then the expectation is that a valid value is submitted for it. Whether the user actively has to choose a value or not merely depends on whether the select list already has a #default_value or not. Accordingly, the element only conditionally gets a form element required marker.
• Attached screenshot outlines the various possible incarnations of select lists and their behavior, which depends on the #default_value and #required properties. It is the form that has been added to form_test.module for the tests.

chx reviewed and asked for some code clean-ups, including a clean-up for the overly lengthy #required condition in _form_validate(), which just happens to be touched here.

Reverted the sweeping API change of #required not being set to FALSE for all form elements. We can intelligently apply it to #type 'select' only.

EDIT: Also updated the issue summary accordingly.

Final comment improvements.

d'oh! Attached patch should pass again.

Removed some technically unrelated changes.

This issue is the blocker of #358437: Filter system security fixes from SA-2008-073 not applied to Drupal 7.x, one of the two last beta blockers. So I think that, in fact, this issue is a beta blocker itself, needing to appear in the beta blocker queue: the fix is ready for review.

Any feedback on the patch? I consider it RTBC. Summary in #46.

Revised comments some more.

I spent hours with sun debating the comments. The code was already solid. I hope we did not break anything w the latest minor code cleanups, the bot will tell :)

Very minor: Removed stale @see, no other change.

Committed to CVS HEAD. One less beta blocker. :)

I'll announce based on the excellent summary in #46. Here all this time I thought #required was just frivolous for FAPI and Selects.

So do I understand correctly:

All #type = select are now effectively #required regardless of the setting of #required.

If you don't want #required behavior, you must set #default_value (and if the user unsets the default, they'll get a validation error on submit)

rfay, you can set #required to FALSE and Drupal will obey. Not that makes any sense but still. It's just that if you dont set anything then it'll be required and now it's a meaningful required because the empty option won't pass. Indeed, if the empty option is valid you MUST set #required to FALSE.

This is a good change for the long run, but not a beta blocker (see above). Please reserve that tag for issues that actually block a beta release.

This also needs to be documented in the module upgrade guide - adding a tag for that.

Additional issues and questions:

1. +    // Otherwise, if #required is TRUE (or not set) and there is no
   +    // #default_value, then the user has to choose an option, which makes this
   +    // element #required.
   +    elseif (!isset($element['#default_value'])) {
   

   What about if #default_value is set, but not valid (i.e., not on the list of #options)? Currently this falls back to the pre-patch behavior (automatically selecting the first element), which doesn't seem right. Shouldn't we be catching that case here too?

2. + *     By default, the label is automatically set to "- Please select -" for a
   + *     required field and "- None -" for an optional field.
   

   The code uses "Select", not "Please select"...

   Actually, "Select" really looks too abrupt and unclear on the page in many cases. Maybe "- Select one -"?

3. @@ -680,22 +680,24 @@ function menu_configure() {
      $menu_options = menu_get_menus();
    
      $main = variable_get('menu_main_links_source', 'main-menu');
   -  $main_options = array_merge($menu_options, array('' => t('No Main links')));
   

   This variable was removed but it's still in use, leading to a PHP notice on the menu settings page.

4. @@ -177,7 +177,6 @@ function trigger_assign_form($form, $for
      );
      // List possible actions that may be assigned.
      if (count($options) != 0) {
   -    array_unshift($options, t('Choose an action'));
        $form[$hook]['parent']['aid'] = array(
   

   On this particular form, the default "- Select -" looks very odd, since otherwise it's not made clear that the choices in the dropdown are, in fact, actions. We should restore the "Choose an action" text as #empty_option.

   The above may be true in other places too - that is just the one that stood out to me. I am not sure what the reasoning is for the patch keeping the original text in some places but switching to the default "- Select -" or "- None -" in others; was there a specific reasoning? We shouldn't be making lots of changes to the UI at this point in the cycle.

5. I haven't looked closely at the block.module changes in this patch, but they appear to have caused a bug on the dashboard configuration page where an empty row is now inserted at the bottom (see attached screenshot).

What about if #default_value is set, but not valid (i.e., not on the list of #options)?

Tested this manually. You get a An illegal choice has been detected. Please contact the site administrator. form validation error and the select resets itself to the empty option.

All other issues are fixed in attached patch.

#62: drupal.form-select-required.62.patch queued for re-testing.

1. Tested this manually. You get a An illegal choice has been detected. Please contact the site administrator. form validation error and the select resets itself to the empty option.

   Strange, when I tried it I got the first #option silently pre-selected (regardless of what it was), and no errors on form submission.

   How did you interpret my question? It sounds like maybe you tried to send something in $_POST that was not in #options? I meant if, in the code, #default_value is set to "3" but #options contains only "1" and "2", that kind of situation.

   In any case, neither behavior sounds right to me. If the desired #default_value isn't there, then the only thing we can reasonably do is set it to "-Select-" (if required) or "-None-" (if not required). The attached patch asserts that behavior and should fail. If we agree it's testing the correct thing, then we can write the code to make it pass.

2. All other issues are fixed in attached patch.

   Mostly looks good, but the menu settings page was still throwing a PHP notice if "No Main links" was selected (worked OK with the others). Frankly, I think the text on that page (that generates the PHP notice) is so confusing that rather than fix this bug I'd rather just try to rewrite and simplify the text :) I had a patch somewhere that did that once, and I've included those text changes in the attached.

   The rest of the patch looked good to me - I still think we want to consider "-Select one-" over "-Select-" and also check other places to minimize the UI changes introduced here, in case there are other examples like the trigger.module one.

Attached patch implements the suggestions of #67.

So what we now revealed is an incompatibility Options module's custom empty value handling. Somehow, I even wonder whether select lists of List fields actually ever showed the stored value upon editing... Attached patch reverts that special handling for select lists, which makes it work based on my manual testing.

Randy's post to the devel list says:

Select elements in forms now require a default value by default. In the past, #required on a select element was kind of silly. Now it's on by default. So if you have a select element which is not supposed to allow submission without having a selection chosen, you have to set #required = FALSE. Look carefully at this one if you have a select element in a form that you do not want to be #required.

Is this true? Have we just made select elements behave totally counter to every other element in the Form API where #required=> TRUE is a deliberate decision by the module developer? If so, this seems like a huge DX wtf.

Nope, I've just read that, too, and it's incorrect. Sorry. My fault - seems like I should have explained it better. Cross-posting my mail:

Select lists mostly behave as usual. However, if a select list does not have a #default_value or the #default_value is NULL, then the select list automatically gets an empty default option that asks the user to select an option.

This is a security hardening improvement, because browsers automatically select the first available option in a select list, if no option has the "selected" attribute. This means that users were able to simply submit a form without ever having looked at the select list, and the form validated successfully.

For your code, this means that you should set no #default_value (or NULL), in case you do not have a stored value for the select list, so the user is forced to choose a value.

The empty default option only appears when there is no #default_value. Usually, you do not want to mark a select list explicitly as #required = TRUE (but of course you can do that).

The empty default option can be tweaked with #empty_value (its value) and #empty_option (its label). See http://api.drupal.org/api/function/form_process_select/7 for details.

Basically the same behavior has been implemented for select lists that are #required = FALSE. However, in that case, the empty option is always contained, so the user can choose "no value", whereas that actual value can be set via #empty_value (which defaults to an empty string).

Nope, I've just read that, too, and it's incorrect.

Hm. Except when I just tried this with the following form definition:

<?php
  $form['select'] = array(
    '#type' => 'select',
    '#title' => t('Regular old select'),
    '#options' => array(t('Banana'), t('Monkey'), t('Dishwasher')),
  );
  $form['select_default'] = array(
    '#type' => 'select',
    '#title' => t('Select with default'),
    '#options' => array(t('Banana'), t('Monkey'), t('Dishwasher')),
    '#default_value' => 1,
  );
  $form['select_required'] = array(
    '#type' => 'select',
    '#title' => t('Required select'),
    '#options' => array(t('Banana'), t('Monkey'), t('Dishwasher')),
    '#required' => TRUE,
  );
  $form['select_default_required'] = array(
    '#type' => 'select',
    '#title' => t('Required select w/ default'),
    '#options' => array(t('Banana'), t('Monkey'), t('Dishwasher')),
    '#required' => TRUE,
    '#default_value' => 1,
  );
?>

I get:

I think that's terribly confusing. The first example shouldn't have a required star. Its empty value should probably be "None" and it should be selected by default.

Right, like when I do this:

<?php
  $form['select_not_required'] = array(
    '#type' => 'select',
    '#title' => t('Select explicitly set to not required'),
    '#options' => array(t('Banana'), t('Monkey'), t('Dishwasher')),
    '#required' => FALSE,
  );
?>

I get:

That's exactly what should happen in the default 'select' case.

And I'm afraid I don't really understand the security hardening improvement argument for this change.

"This means that users were able to simply submit a form without ever having looked at the select list, and the form validated successfully."

Yes. So? If I wanted users to care about filling out that select box, I would mark it #required in my module. Just as if I care about them entering a textfield or a set of radios.

That said, the changes here where #required => TRUE defaults to ' - Select - ' and with #required => FALSE defaults to ' - None - ' are great. We just need to fix that default value of #required in form_process_select() so that it conforms to the rest of the form API.

@webchick: No - the common sense and understanding of select lists is that it should lead to a value. That was actually the part that chx confused most in my earlier patches, which contained an inline comment along the lines of "By default, select lists in Drupal are treated as if they were #required." We slightly reworded that, but it's possible that we've lost an essential detail: Everywhere in Drupal, regardless of core or contrib, select lists are expected to have a valid value, unless they were explicitly marked as #required FALSE. That is partially also, because a single SELECT list HTML element that is rendered by a browser only allows the conclusion that one option will be submitted. So all code throughout Drupal assumes that there is always a value for a select list, regardless of whether it has been explicitly marked as #required or not.

For that sake, this patch implemented special handling for the #required property on select lists. If you don't explicitly set #required to FALSE, then the select list is treated as if #required was TRUE. But it's not even limited to that: The select list is actually only really #required, if there is not a valid #default_value already. Because, if there is a valid value already, then it's only logical that the user does not have to choose one.

All of this was done to retain the visual appearance of select lists throughout Drupal, and to minimize the API change impact of this patch.

To see all possible combinations of select lists and their properties, I recommend to install the form_test.module (drush en form_test) and look at the dedicated test helper form that appears as "Select" menu link in the Navigation menu.

Yes. So? If I wanted users to care about filling out that select box, I would mark it #required in my module. Just as if I care about them entering a textfield or a set of radios.

You need to understand that select lists *always* have a value in HTML. The browser simply selects the first available option. And since *any* option is therefore selected, the value is *valid*. Your #required had absolutely zero meaning, because the browser always submits a value.

You need to understand that select lists *always* have a value in HTML. The browser simply selects the first available option. And since *any* option is therefore selected, the value is *valid*. Your #required had absolutely zero meaning, because the browser always submits a value.

Yes. I do understand that. Which is why the change for - None - and - Select - is good. If #required is set, the form API will bomb out, because NULL is not a valid value. If #required is not set, the value of NULL will be passed through, which my module can use isset() to check and know it means "They didn't select anything." Again, +1 to those changes.

But changing #required to default to TRUE on select element is completely backwards from what every single other element in Drupal does. In order to get a form like this:

I need to do this:

<?php
  $form['radios'] = array(
    '#type' => 'radios',
    '#title' => t('Radios'),
    '#options' =>  array(t('Banana'), t('Monkey'), t('Dishwasher')),
  );
  $form['checkboxes'] = array(
    '#type' => 'checkboxes',
    '#title' => t('Checkboxes'),
    '#options' =>  array(t('Banana'), t('Monkey'), t('Dishwasher')),
  );
  $form['checkbox'] = array(
    '#type' => 'checkbox',
    '#title' => t('Checkbox'),
  );
  $form['textfield'] = array(
    '#type' => 'textfield',
    '#title' => t('Text field'),
  );
  $form['textarea'] = array(
    '#type' => 'textarea',
    '#title' => t('Text area'),
  );
  $form['select'] = array(
    '#type' => 'select',
    '#title' => t('Select'),
    '#options' => array(t('Banana'), t('Monkey'), t('Dishwasher')),
    # One of these things is not like the other...
    '#required' => FALSE,
  );
?>

This needlessly introduces a DX WTF since, #required => FALSE is doing the right thing automatically now, from the perspective of a module developer. $form_state['values'] on all of the elements except for checkbox/checkboxes is "String, 0 characters".

So, can we please revert that default value to FALSE like the rest of the form API?

Btw, when I make the above form definition, "checkboxes" has a default value of "Banana", even though I don't specify a #default_value. That seems like a separate bug.

Sorry if I'm mistaken, could it be that the patch in #57 introduced the changes which produce the following error:
Undefined variable: main_options in menu_configure() (line 702 of ...\modules\menu\menu.admin.inc).
(while opening Administer > Structure > Menus > Settings tab)

@webchick: No. No. And no. We cannot default select lists to #required FALSE. That would be an insanely large API change, which is far too late for D7. You can open a separate issue for D8 to request that change. Let me repeat: If developers did not explicitly specify #required, then neither previous HEAD nor current HEAD treated the element as not required. The expectation is and always has been that the element receives a valid value. All select lists still work in the same way. Except for the case when there is no #default_value, so a user *has to* choose one, because all code *expects* that the user has actively chosen a valid value.

@mertskeli: Please read #61-3. It should be fixed in the patch in 62.

Wait. What?!

cvs up -dPC -r DRUPAL-7-0-ALPHA7

The code to get the above form element is:

  $form['select'] = array(
    '#type' => 'select',
    '#title' => t('Select'),
    '#options' => array('' => t(' - None - '), t('Banana'), t('Monkey'), t('Dishwasher')),
  );

Now, there's a nice DX improvement that tacks t('- None -') on there for me automatically. Nice! But never do I need to set #required => FALSE to make it not required, just as I didn't in Drupal 6 or any other point in HEAD's history. It's a basic tenant of the Form API that elements such as #default_value and #required are strictly opt-in, regardless of the element type.

If gazillions of places in core need to change as a result of this DX improvement in order for select elements to conform to this basic "opt-in" tenant of all form API elements, well, then that's a good argument for rolling this back and pushing it to D8. Introducing inconsistency to Form API elements is not something I can stand behind, particularly at this point in the release cycle. Sorry.

If Dries wants to overrule me, I'll stand down. But my "day job" is explaining the Drupal API to new module developers. Trust me. This change is going to completely confuse the crap out of new developers, just as any area where we have to say things like "oh, except when..." confuses new developers. The DX trade-off is not worth it, IMO.

In other words, I'm quite curious what this breaks.

@webchick: Either you have a #default_value, or you don't. If you don't have any, then we have to prevent the browser from choosing one on behalf of you. This patch was not a DX improvement, this patch was badly required security hardening.

Almost all existing code throughout core and contrib specifies a #default_value, pre-selecting whatever choice makes sense for the respective module/form.

You either have

  $form['select'] = array(
    '#type' => 'select',
    '#title' => t('Select'),
    '#options' => drupal_map_assoc(array(t('Banana'), t('Monkey'), t('Dishwasher'))),
    '#default_value' => 'Banana',
  );

or you have

  $form['select'] = array(
    '#type' => 'select',
    '#title' => t('Select'),
    '#options' => array(t('Banana'), t('Monkey'), t('Dishwasher')),
  );

If there is no value by default, then the user must select one, because the rendered HTML element by the browser enforces this behavior on us.

Most select lists specify a default value, even if no value has been stored.

All code that does not explicitly set the element to be not required expect a valid value to be submitted.

Really, I think you're overcomplicating this patch.

In other words:

1. You specify this:
   

     $form['select'] = array(
       '#type' => 'select',
       '#title' => t('Select'),
       '#options' => array(t('Banana'), t('Monkey'), t('Dishwasher')),
     );
   

2. So would you expect that you get no value for the select?

No, you don't. You expect a valid value. More specifically, one of the available options.

Furthermore, you arguments are not valid. Compare:

  $form['radios'] = array(
    '#type' => 'radios',
    '#title' => t('Radios'),
    '#options' => array(t('Banana'), t('Monkey'), t('Dishwasher')),
  );
  $form['select'] = array(
    '#type' => 'select',
    '#title' => t('Select'),
    '#options' => array(t('Banana'), t('Monkey'), t('Dishwasher')),
  );

So you also expect that submitting no value for 'radios' is valid?

Reading the official HTML 4.01 Specification (that is, the current HTML version, given that HTML 5 is still a working draft), it looks like the ideal behavior and the real behavior are different things for current browsers.

In the Forms section we can read, for the SELECT element:

A SELECT element must contain at least one OPTION element. (...)

If no OPTION element has the selected attribute set, user agent behavior for choosing which option is initially selected is undefined.

Note. Since existing implementations handle this case differently, the current specification differs from RFC 1866 ([RFC1866] section 8.1.3), which states:

    "The initial state has the first option selected, unless a SELECTED attribute is present on any of the <OPTION> elements."

Since user agent behavior differs, authors should ensure that each menu includes a default pre-selected OPTION.

And something similar happens for radio buttons.

Sorry, the status changed while I was writing.

Hm. We clearly have a communication breakdown happening here, but I'm not sure how I can rectify it. Let me try one more time.

re: #89 and 90, I expect Drupal's form API to do exactly what HTML does (or as close to it) in both cases:

Yields the following form:

Without me having to specify a "selected" attribute on <option>, my browser automatically chose the first option. That's what HTML does. This is what I would expect Drupal to do.

If I submit the form without changing any elements, and print out $_POST in form.php, I get:

'radios' doesn't even come through in $_POST because nothing was submitted. The Form API way to do that is to create a record for it in $form_state['values'] that defaults to empty string. So this is what I would expect Drupal to do.

So the DX improvement to add that initial "- None -" option that will come through select elements the same way that radios comes through is nice! That way I know that they explicitly didn't choose anything, as opposed to ignoring it and leaving it at the default value. But that's absolutely no justification for changing the API so that select elements are required by default.

Yes, defaulting #required => TRUE on selects is just not cool at all. The default elements are great, but this late API change to selects needs to be rolled back.

To sum up what webchick says (in her own words) and she is right.

1. Adding - None - / - Select - is a good idea.
2. Selecting that gives us an empty string same as an empty textbox.
3. There is no need to make selects #required TRUE.

sun disagrees on 3. based on the fact how browsers behave, namely that they select something always so the expectation should be that a select element always reutrns something.

Now... here is the deal. This is not the expectation. This is more of a nuisance. Changing from radios to select does not change the expectations. I agree with webchick #3 needs a rollback.

And sorry, but I feel strongly enough about this regression that I'm adding this to the beta blockers list. :\

Again, select lists do not default to #required TRUE. They behave identically to before, as long as there is a #default_value.

But anyway, I'll try to come up with something.

The workaround to fix the similar Afghanistan bug (#289957: Afghanistan should not be the default value shown in the country field) was to add a first value to the country select in profiles, which currently is "not specified".

I've just tried it in My account -> Edit -> Personal information -> Country. It's a required field, but I've just submitted it with "not specified", and it has been accepted.

On the other hand, chx explained (#60) about this issue:

"rfay, you can set #required to FALSE and Drupal will obey. Not that makes any sense but still. It's just that if you dont set anything then it'll be required and now it's a meaningful required because the empty option won't pass. Indeed, if the empty option is valid you MUST set #required to FALSE."

But there is the API special case. So it seems there is not a perfect solution, maybe just different opinions about the better, not perfect, solution.

Well, current browsers should behave better...

Sorry again, form values changed while writing several people at the same time (I had not refreshed for a long while).

Whatever. Since we now change the behavior of select elements that developers commonly expected since at least Drupal 5, I naturally expect quite some breakage.

You can fix the testing profile while I'm working. Thanks.

Well I like this patch already, based on the removal of 20+ lines of comments justifying the special-casing. ;)

I just wish I could understand "Since we now change the behavior of select elements that developers commonly expected since at least Drupal 5." :\ I'm really, really not trying to frustrate you on purpose. :\

Still 10 minutes to go to see test results of the first patch. 70 minutes more to see results of #103. On highly optimized testbots. And I still see no progress on the testing profile issue.

Well, I'd love to, but #913086: Allow modules to provide default configuration for running tests is currently "needs work" and currently blocked waiting for Dries's feedback. Try as I might with the hair gel, I still cannot seem to turn myself into Dries. :)

Since I have to wait, I can also write.

I don't care whether you like this patch or not. It frustrates me, because it's straight nonsense, adds unnecessary burden on countless of modules and developers, and entirely changes expectations. Whoever thinks we try to avoid major API changes, thinks wrongly. It took a giant amount of time to get the current implementation in HEAD right, for the whole sake of changing the API as little as possible. The arguments are moot, the examples are moot, everything moot. I'm solely working on this now, because you think it's right, without understanding the consequences, and I have to please you. Top down forever. I'm seriously disgusted. Spending more time on talking and debating totally hypothetical and artificial, entirely made up barriers instead of simply fixing bugs. Dead simple, but apparently not in the insanity of Drupal core. One hundred bugs are ready to be fixed, but nothing happens. I spared 5 hours yesterday to double-check and re-review all of the RTBC issues, from which most are languishing around since January 2010. Drupal core seems to prefer bugs instead of fixing them. So what is the point of contributing to Drupal core and trying to fix them? All of that slows down and delays the release of Drupal 7, and no one in this community currently understands why that happens. Patches totally needlessly need to be re-rolled and re-tested, all over and all over and all over again. Everyone wants to get Drupal 7 out of the door, but it's like running against a wall of people that somehow do not want to release it. It's like upside-down, white turned black, it's driving me completely mad. I completely fail to see any logic in what happened lately. Lately? Since more than 6 months.

Now back to the really required fixes.

Yes, I'm running amok. But I'm also working my ass off to get Drupal 7 out of the door. Every single fucking day. But nothing happens.

I don't care whether you like this patch or not. It frustrates me, because it's straight nonsense, adds unnecessary burden on countless of modules and developers, and entirely changes expectations.

I really, really wish you would qualify this. You keep saying it. I have posted evidence upon evidence to defend my view that the change to select boxes without default values is unexpected and inconsitent, both from the perspective of what core used to do before this patch, as well as the perspective of what HTML does natively. And yet I have received no code snippets back from you that counter my opinions. I would welcome them, because then I could maybe understand your perspective. But right now, I can't. And all I'm getting back in response to me asking for clarification is anger. This doesn't help either one of us.

I'm solely working on this now, because you think it's right, without understanding the consequences, and I have to please you.

I'm not "ordering" you to do anything. I'm making a technical argument. This is my job, as both a core maintainer and also as a patch reviewer. When I make technical arguments, I expect technical (and not emotional), arguments in return. I don't expect subordination. I'm perfectly happy to be pointed out why I'm wrong.

One hundred bugs are ready to be fixed, but nothing happens. I spared 5 hours yesterday to double-check and re-review all of the RTBC issues, from which most are languishing around since January 2010.

I can't argue with your criticisms about the RTBC queue languishing. They're valid. I'm not paid to work on core. Dries isn't paid to work on core. And yes, I fully realize, neither are you. But as a result, we all need to explicitly chisel out time in amongst other things going on (work, life, family, travel, illness, other promises/obligations) to focus on the Drupal 7 issue queue.

Therefore, if I have an hour to spend on core, it goes towards critical and major bugs (and removing bugs that are falsely marked critical/major, which ends up with me being a battle-ax a lot of the time ;)). This means normal and minor bugs, unless they're dead simple, do tend to fall by the wayside, currently. What gets focus right now is the beta blockers until we hit beta 1, and the bugs actively blocking release of 7.0 once we hit RC. Normal and minor bug fixes can follow in 7.1 and beyond (or during downtime when I'm waiting for critical/major issues to become RTBC), from my POV.

So why do I drop everything to expend all of this energy on this issue? It's not merely to frustrate you, I can assure you. I hugely value the core work that you're doing, and it's really unfortunate that our run-ins lately have apparently implied the opposite. I am loud in this issue because I fundamentally believe the DX regression is a critical, beta-blocking issue that we need to resolve in order to ship Drupal 7.

I realize you disagree. I would really love to know why. I'd also love if you could approach me one on one if you have a problem with me. I'm on IRC every hour that I'm awake.

I don't have a problem with you, and you know that. I have a problem with needlessly debating and delaying bug fixes. I can provide an entire list of issues that went into so called "silly-core-debate" mode instead of simply hitting the commit button. So instead of making progress, we are debating. For nothing. It takes more time to follow up on an issue to state that a patch is "not important" right now, plus the endless re-rolls and re-tests and re-debate and whatnot afterwards. Exactly those core developers that are working on those "magic beta blockers" still need to care for their other patches, to make sure they still apply, and whatnot. All of that sucks time, a sheer endless amount of time. Much more time than committing. Dead simple.

Hm... let's take a step back.

@sun is correct that the latest patches he posted will definitely break lots of things, regardless of how many tests fail :) To avoid these things breaking, a lot of people building forms will have to add '#required' => TRUE to their select elements (to avoid the new "None" option, which they do not want, appearing). By default, doing that will also add a "*" (required flag) to the form element (which looks very silly for select lists that already have a valid value chosen) unless we do something to fix that at the theming layer.

Is it really necessary to make all these API changes?

The "-None-" option that was added in the original committed patch did not fix any bugs by itself (in fact, it now looks to me like it caused some, in the Field UI specifically). It was only a DX improvement, and it's not even clear it's that (it does save the developer some typing, but the contrary argument is that it's nice to be able to look at #options and know that it corresponds to reality, without the form API automatically sticking something else unexpected in there).

So what if we simply rolled THAT PART back?

The main bug that this issue is supposed to be addressing (in my opinion at least) is that setting #required to TRUE on a select element but not setting a valid #default_value did not force the user to make a conscious choice. That was inconsistent with other elements in the form API, and arguably is a security weakness (at least in certain use cases).

So perhaps for D7 we can just fix that and be done with it, with the bonus that there is then no actual API change at all...

In that case, you'd have this behavior for select elements:

1. Not #required, no valid #default_value: Same behavior as we had a couple weeks ago (before the committed patch). That is, we render exactly what was in #options and simply let the browser auto-select the first element, or auto-select it ourselves. We do not theme it as a required form element; hence, no confusion for developers. (We could optionally have the form API treat this as "required" behind the scenes only, since we do not ever expect a legitimate browser to submit without it; not sure.)
2. Not #required, with a valid #default_value: Same behavior as we had before; render exactly what was in #options, pre-select the #default_value one and leave it at that.
3. #required, no valid #default_value: Automatically add the "- Select a value -" choice and force the user to choose something (same as in the patch that was committed here).
4. #required, with a valid #default_value: Same as before; render exactly what was in #options, pre-select the #default_value one and theme it as a required form element.

Would this be a reasonable compromise? It fixes the bug (or at least the main part of it), does not introduce API changes, and also could probably be made to work with radio buttons too, which really are similar enough that they should behave similarly to select elements, and currently have their own set of bugs that are related to this (see #811542: Regression: Required radios throw illegal choice error when none selected).

Of course, I forgot to update Drupal core according to the major API change that webchick wants to do.

@David: No, I do not go backwards. I learned to go forwards, and I will it keep it that way.

I don't want to make a huge API change. I want to restore the consistency to our FAPI elements that we had before this patch. If an extensive API change is required to do that, we need to roll this patch back and push it to D8 for consideration.

However, David's idea sounds interesting as a compromise.

Sorry. Cross-post.

Also, @David, the remaining failures are caused by List/Options module. @yched already mentioned in #23 that we happened to commit a custom workaround for this bug to List/Options module a few weeks ago. That patch basically just needs to be reverted here, but that's not as simple as it may sound.

@webchick: Current HEAD works exactly like it did before. Only if, and ONLY IF you do not specify a #default_value, we HAVE TO prevent the browser from choosing an option on behalf of you. The same is true if your example code does not specify a #default_value. Drupal developers EXPECT a value for a select list, and one that is valid. And we do that since like ever. It's just not mentioned or documented or explicitly coded somewhere.

I am really sad to see Daniel being frustrated (and I perfectly understand why), but I'm afraid I agree with Angie on technical vs emotional arguments. I hate to see Daniel's hard work constantly being (re)rolled back and forth, so I think perhaps what David suggests in #144 sounds like the best solution (a reasonable compromise in his own words). The rest of the problems can be addressed latter on and perhaps in separate issues or even in a contrib module (Checkbox validate perhaps?). I say sounds like the best solution, because I wouldn't know in terms of reviewing and all. I am simply saying this as a person looking at this argument from an "outsider's" point of view.

Other than that, I am coming from #654796: Identify fix bugs with checkbox element and initially from seeking a solution for #259292: Required radios/checkboxes are not validated (D6). Right now I am rather confused and I need to know if this is supposed to fix these issues (in other words... is any of them duplicate to this one?) and if there is need/intention of this fix to be backported to D6. Perhaps the first issue is this one's equivalent for D6(?). I'm honestly totally lost here.

"Only if, and ONLY IF you do not specify a #default_value, we HAVE TO prevent the browser from choosing an option on behalf of you."

But... why? HTML doesn't work like this, as evidenced in #94. If select options are given without a selected attribute, the default value is selected by the browser, and is passed in $_POST. That's how HTML works.

You seem dead set on "fixing" HTML via the Form API. I am dead set against it, both because it's too late for this kind of behaviour change in D7, and also because it introduces special "Drupalisms" that people already familiar with working with HTML forms need to learn and creates inconsistency in our Form API.

Oh, but for required elements? I absolutely agree we HAVE TO prevent the browser from choosing an option on behalf of you, and am happy to see this fixed.

However, #required needs to be opt-in, as it was before this patch, and as it always has been on every form element ever since Drupal 4.7.

...but then Daniel replied in #116 before I even got to type my previous post:

No, I do not go backwards. I learned to go forwards, and I will it keep it that way.

...boy is this issue on fire! ;)

Also, why don't we simply spend two more years with debates about nonsense that are not grounded in any way?

Yeah, please keep on stating that you consider to roll back this patch. But yeah, don't forget to re-open that other critical issue http://drupal.org/node/358437 then. And stop counting on me.

There is no consensus here to close the issue, but also seems like the issue and comments are getting too personal. Let's remember we all want the same thing: Drupal World Domination!

That's right. I'm sorry. But in other news, the RTBC queue just decreased by ~20 issues. That seems to be the desired way to get D7 done. Good luck.

sun wrote:
> That's right. I'm sorry. But in other news, the RTBC queue just decreased by ~20 issues. That seems to be the desired way to get D7 done. Good luck.

So, right now, probably there are just some different opinions and misunderstandings between hard-working, valuable people, all trying to do their best... IRC has been suggested to communicate one to one about this...

It's natural, this open source project is on an energetic, unstoppable wave, and we can't help coming back to it. We are at the doors...

For example, about this issue, if the API change -with select required by default- is not pretty -because browser behavior isn't pretty-, and the Afghanistan workaround used in drupal.org is not enough, in this case there is the middle point solution (#114).

However, remember that the official HTML 4.01 Specification recommends (#92), for select elements (and in a similar way for radio buttons):

Since user agent behavior differs, authors should ensure that each menu includes a default pre-selected OPTION.

In this case, the author would be Drupal. Setting the field as required would validate any submitted value excepting a possible empty option such as "please select". And not required would validate any of the values including an empty option like "none" or "any".

Again, there are not perfect, just reasonable solutions. And other developers have worked on this problem before in various ways, just search for PHP form validation, or JavaScript form validation. So, considering advantages/disadvantages, we should try to choose the best one between the available solutions, and that's it.

You know, many are grateful to all the great people here for your so good, passionate work.

This implements the compromise in #114. Observe how well it works with the filter module change: all I needed there was the flip the form to #required TRUE. It's one of the (very few) forms where we want to coerce the user to make a conscious decision. Most of the time, just the first option is fine. Or if it's not we have the code already to cope with that, observe for example how little the change is in modules/trigger/trigger.admin.inc -- the conditions in validate / submit that protect against a 0 choice were not changed by the committed patch. We can later investigate which forms need to be #required TRUE and then clean up. If these changes happen in D8 and get backported, that's fine.

Written doxygen, simplified form_process_select now it's really just the possible (and necessary) minimum.

Note that most changes are rollback. My changes are in the #required validator, form_process_select, tests and filter module.

I think you guys are amazing! Obviously busy people who still manage to put many hours of their time into getting Drupal 7 out of the door. However, all that dedication obviously requires emotional involvement and so we get the occasional issue, like this one, that starts to generate lots of heat! Maybe an outsider can help.

I have read through the issue and, as I understand it, the problem centres on the behaviour of the HTML <SELECT> element: come hell or high water it *ALWAYS* returns a value. If a default is specified, fine, that is returned. Otherwise the first value in the option list is returned.

The case that the issue is about concerns a field that is required, but no explicit default value is given. The "security hardening" bug that is fixed here is to force the user to make a choice in this case. The (nice) solution is to add an item "-select-" at the top of the option list. If this is returned when the form is submitted, then the user hasn't made a choice - so reload the page with an error telling them that they must choose.

But the argument is about a different case where:

• The field is NOT required,
• There is NO explicit default value
• and the field cannot accept an empty/null value.

What happened in this case, before the patch if the user did not choose an option, was that Drupal chose the first option for you and that was what got saved in the field. Sun argues that this is wrong and his solution is to treat it as a required field and force the user to make an explicit choice.

Webchick's screenshot in #75 shows that this results in the field displaying with an orange * - despite the fact that the developer didn't specify #REQUIRED = TRUE. If the developer wants the previous behaviour, they have to explicitly set #REQUIRED = FALSE. She points out in #81/#82 that this differs from the result of setting the same set of options as radio buttons. The user is not then required to make an explicit choice, but the field will get set to a value as if the first radio button had been chosen. Her comment in #94:

"Without me having to specify a "selected" attribute on , my browser automatically chose the first option. That's what HTML does. This is what I would expect Drupal to do."

I think that what she is arguining is that the normal expectation is that the first option in the list is treated as a de facto default.

So, I think the decision that has to be made in this particular case is between:

• Sun: treat the field as required and force the user to make a choice
• Webchick: treat the first item in the option list as a de facto default

Both are good arguments. For my money, I agree with Webchick. I think I would expect the first option to be treated as the default and I find the imposition of a required field I didn't set to be intrusive. But, Sun has a good point: I bet that, in most cases, the developer simply didn't spot that a default was needed and didn't set up the option list with "first option = default" in mind. Difficult!

About browsers selecting the first option of a select element by default, this happens only for normal one-line drop-down selections. It does not happen for multiple select or when the size is > 1, multiple or not.

Because of this, the already quoted HTML 4.01 recommendation is "Since user agent behavior differs, authors should ensure that each menu includes a default pre-selected OPTION."

More details: SELECT tag issues. It's an old document, but I've just made its included tests with current versions of Firefox, IE, Opera, Chrome and Safari, and they behave like the IE5.5 test on that page.

Also, I admit making a mistake by approving the patch in the first place, I was occupied by the FAPI implementation and did not consider the bigger picture. Sorry.

+++ includes/form.inc	2010-09-30 05:10:11 +0000
@@ -1225,24 +1205,18 @@ function _form_validate(&$elements, &$fo
+      $is_empty_value = isset($elements['#empty_value']) && $elements['#value'] === $elements['#empty_value'];

Blatantly fails if #empty_value is not an empty string. This patch only passes tests, because you removed the corresponding test cases.

+++ includes/form.inc	2010-09-30 05:10:11 +0000
@@ -2292,94 +2266,30 @@ function _form_options_flatten($array) {
-      $element += array(
-        '#empty_value' => '',
-        '#empty_option' => t('- None -'),
-      );

+++ modules/field_ui/field_ui.admin.inc	2010-09-30 02:57:17 +0000
@@ -304,7 +304,7 @@ function field_ui_field_overview_form($f
-    '#parent_options' => array(),
+    '#parent_options' => array('' => t('<none>')),

Why on earth is this reverted?

+++ includes/form.inc	2010-09-30 05:10:11 +0000
@@ -2292,94 +2266,30 @@ function _form_options_flatten($array) {
+  elseif (!empty($element['#required']) && !isset($element['#default_value'])) {

empty() is needless.

+++ modules/simpletest/tests/form_test.module	2010-09-30 04:45:58 +0000
@@ -800,56 +800,21 @@ function form_test_select($form, &$form_
-  $form['no_default'] = $base + array(

Your tests only pass because you removed essential test cases.

+++ modules/system/system.module	2010-09-30 03:47:44 +0000
@@ -399,11 +399,7 @@ function system_element_info() {
+    '#size' => 0,

#size was removed, because it ends up as bogus attribute. Doesn't look like you would have looked once at the resulting markup...? Oh, of course, how could you see it - you removed the test cases.

Powered by Dreditor.

I am still trying to slavage whatever we can instead of asking for a straight rollback. Took me several hours to get the tests pass. Please stay with me and explain. I have removed a huge number of test cases because this patch is now only about #required forms without #default_value.The empty_value_one test case is still in there. Care to explain how "blatantly fails if #empty_value is not an empty string?" and, why does it matter whether it's empty string or something else the user designates? The code makes #empty_value the first option in #process, that's what the browser chooses and that's we deem illegal during validation. What does this have to do whether #empty_value is '', 0 or 'one'? I do not see the need for change here. If it would always be the empty string then why on earth allow a property to "change" it in the first place? This variable must have the value '' , what sort of variable is that?

My reasoning for reverting everything outside of form.inc is explained above: we would need to evaluate every form whether they require a pick or just the first one would be fine and then clean up the surrounding code which before patch have protected submission of an invalid value. This does not belong to this patch, it's a series of followups and it should not have been here in the first place. This patch is now about adding a - Select - into required selects when there is no default value but nothing more. The filter case is a valid case when a user choice must be enforced and so that's rightly set to #required TRUE and its tests pass without a change. We are good there.

Obviously, either you support a custom #empty_value or you don't. Current patch tries to, but fails to do so.

ENOUGH OF THIS! I HAVE ASKED FOR AN EXPLANATION AND YOU PROVIDED NONE. GET LOST.

I asked effulgentsia to look at this in case there is an issue I overlooked because I do not see and sun refused to help.

@chx: So sorry. Ran out of time, and I have other plans I'm committed to tonight. This is my very next priority, when I'm able to carve out time for core work again. Not sure when that will be, but I'll try to make it as soon as possible.

[EDIT: good chance I'll be able to review this tomorrow]

Something that might help reviewers here is an interdiff showing the difference between a straight rollback patch and @chx's patch - in other words, a patch file that shows the net changes that would be introduced as a result of this issue if the current patch gets committed.

I've attached that here - as you can see, it's a lot smaller so hopefully will help anyone who wants to review this :)

Thanks David! Yes, that's very familiar to me -- I started with rolling back the patch then this is indeed what I added, some of it is sun's work some of it is mine.

Right, and if you now look at the resulting test, then you see that a custom #empty_value that can potentially be 0 is no longer tested at all. In other words, the following is entirely missing, and the patch claims to support it, but it doesn't:

  $form['no_default_empty_value'] = $base + array(
    '#title' => '#required, no #default_value, #empty_value 0',
    '#required' => TRUE,
    '#empty_value' => 0,
  );

Ah so what you mean is that check needs to be $is_empty_value = isset($elements['#empty_value']) && (string) $elements['#value'] === (string) $elements['#empty_value'];! Gotcha.

I just spent a bunch of time reviewing this full thread and trying to think through the different implications. I also chatted with sun about this to make sure I understood his perspective on this issue. I'm going to try to summarize what I understand about this issue so far.

First of all, I agree with webchick, and with what appears to be broad consensus on this issue, that what got committed in #58, and is now in HEAD, is flawed, and addressing that flaw is now the goal of this issue, which is critical, and beta blocking. Specifically, the flaw is having #type=>'select' elements having #required default to NULL, which acts like TRUE, as opposed to every other form element type which has #required default to FALSE. This is a huge WTF, as webchick, David Rothstein, and others have already stated.

It's unfortunate that sun's arguments for why this decision was made were buried under so much emotion and frustration, that they weren't clearly conveyed and understood. The argument essentially boils down to wanting to maintain backwards compatibility with existing poorly constructed forms (of which there are many in core and contrib). A "select" control without an "empty" option is effectively a required control, whether #required is set or not, because, at least when submitted by a browser, it always contains a value corresponding to one of the options. Because of this, many contrib modules create forms, whose submit handlers treat the control as required, even if the form constructor doesn't set the #required property. The #57 patch (what is now in HEAD) was a reasonable (but flawed) attempt at making this implicit situation explicit. Unfortunately, the implementation was flawed, and, fortunately, webchick attached follow-up screenshots that expose those flaws.

If we were currently in code thaw, I would recommend changing "select" elements to have #required => FALSE by default (like everything else), and to let this have the logical repurcussions, which is to automatically add a "- None -" (exact text overridable, of course) option, that is validly selectable. And to require contrib modules that don't want this "- None -" option to have to set #required => TRUE, since that's what it means to not have a "- None -" option. As I understand it, this is what #109 does. But we're not in code thaw, so this is not a viable course of action.

If #57 hadn't been committed, I would more or less support #142. I haven't reviewed it carefully enough to RTBC it yet, but it's essentially a sound implementation of #114.3 (minus perhaps minor tweaky things), which is an important improvement that makes #required=>TRUE really mean "the user must make an explicit decision", the lack of which has caused problems discussed in comments leading up to #57.

But #57 was committed, and it contains a nice improvement for adding a "- None -" (exact text overridable, of course) option to select elements that are not required, a feature that prior to #57, had been implemented in one-off ways by Field UI, Webform, and probably many other modules. Was adding FAPI support for this, so that we can stop having all these one-off variants, in-scope for D7, past our alpha7 release? Probably not. But what's done is done, and that FAPI feature is now in HEAD. Sure, we can choose to remove it, as #131 does, but I don't think we have to, at least not on scope grounds, since whether something is in-scope or out-of-scope needs to be evaluated based on HEAD as the baseline.

So what I propose is #109 (see 2 paragraphs back), but with the addition of a "- None -" option being something that needs to be explicitly opted in to by setting #empty_value. So, if you want the "- None -" added, you set #empty_value => '' (or whatever other value you want this option to have, if not empty string) on the element explicitly. And all the various contrib forms out there that don't have this set keep working as they do in D6 and D7 Alpha7: not required, but no "- None -" option (and we continue to look the other way past the logical inconsistency of such a creature). And then we solve this logical inconsistency in D8.

Specifically, here's a table showing the 8 permutations (based on whether #required, #default_value, and #empty_value are explicitly set or not), and what their behavior is in D6 / D7 Alpha 7 (i.e., prior to #57), what their behavior would be if we committed #131 (I think), and what their behavior would be if we go with what I propose (#109 + opt-in for #empty_value).

Thoughts?

effulgentsia's excellent table sounds like a great compromise to me.

As I've maintained the whole way through, I actually do really like the changes that were added in the original patch, except for the inconsistencies in the "no #required, no #default_value, no #empty_value" case. That's it. chx's patch basically rolls back the whole thing, which seems a shame. But my understanding based on sun's later patches after #109 was that this was our only alternative, because we could not afford to make those kinds of even more dramatic changes at this point.

So it sounds like with effulgentsia's method, we can have our cake and eat it, too.

This is just #109 plus a tiny change to form_process_select() to implement the opt-in approach to #empty_value as per #146.

There are likely existing forms that will therefore require an explicit #empty_value => '' to be added. Hopefully, testbot will show us what those are. Go, bot, go!

Install failed locally, so giving bot a better candidate. Running local FAPI tests now as well.

I apologize for not understanding all the consequences of this patch when I committed it. Glad to see we're getting back on track though.

Don't be scared by the growth of the patch. Most of it is just removing now unnecessary '#required' => FALSE lines, since, you know, that's now the default again.

Gonna get some dinner, and come back to see if it's green.

This implements #146 just fine.

Just for disclosure, it is theoretically possible this will cause some regression, like some form that needs a #empty_value set on it and doesn't have it, but doesn't have test coverage to catch it. But that's the price you pay for not writing tests. I can't manually search every form. If there is a regression, we'll get a bug report, and it will be an opportunity to expand our test coverage.

The existing FAPI tests, along with the couple added in the patch, are pretty thorough in testing the basic API.

I'm going wait a bit before committing this patch. I need to spend some more time with it, and I'd love to get constructive feedback from sun and webchick.

Thanks so much, effulgentsia!

I'm re-tagging as a beta blocker, because whatever's decided here impacts every single module developer using a select box, which seems pertinent given that we want people to start (or resume) porting their modules and themes once we hit beta.

Ok, just tested this extensively and it works as I expect now. Wonderful.

However, just tossing this out there, since we're changing the API here either way...

    '#empty_value' => '',
    '#empty_option' => t('- Pick one, jerk! -'),

Having to set two separate properties for this was weird. I actually had to go back and reference one of the existing elements because it wasn't clear to me how to use it.

When I specify #options, I do it like this:

'#options' => array('key' => 'value', 'key2' => 'value2');

So I think the syntax would therefore make more sense if it were:

'#empty_option' => array('' => t('- Choo-choo-choose me -'));

What do you guys think? And how extensive of a change would this create? From eyeballing the patch it seems like every place we specify #empty_option, we also specify #empty_value, but of course the patch only shows the affected lines.

From effulgentsia's summary in #146:

Nothing added. Potential problems if the form submit handlers act as if the element is required.

This is for no #required and no #default_value
Can we please add a bold, blinky comment to the FORMS API reference on api.drupal.org for that? While it may be important to retain backwards compatibility as much as possible, we should make people aware of this problem as much as we can, especially for new modules. Because while it is somehow logical that this problem could occur when you think about it in the context of this issue, I don't think every developer who uses a select list in their module thinks that through properly.

#empty_value was deliberately separate, as most developers will want to use the default "- None -" option label for optional selects. You can set #empty_option, but most often a different label doesn't make sense for optional select lists.

#required select lists without #default_value get a default "- Select -" option. For those it's more likely that developers may want to set #empty_option, but possibly no #empty_value when the its default is fine.

Please bear in mind that all of this is also a giant help for translators, as they no longer need to translate gazillions of different <none> <None> <any> <Any> <choose>... strings.

But look at Issues for Drupal core. The label - Any - is useful for optional selects in search forms.

On the other hand, in Create issue, the label none is used for required fields instead of optional, when - Select - would be more suitable.

What I would do here is allowing #empty_option in itself to call for adding that empty option so that we do not need to specify the #empty_value ''. Patch, interdiff to effulgentsia'patch attached.

Reviewed the interdiff and noticed a "the there" in a comment. Looked sensible otherwise.

The 'no_default_empty_option_optional' test should also be kept.

/me wishes we could stop inventing a huge list of obscure properties that only one or two people will ever understand or know about. But it's our way, I know, and we're solving problems. But it is a "bad thing". No reflection on this particular issue, but I just feel comprelled to whine for a moment.

I removed it because the behavior have changed. Thanks for pointing it out saving me quite some struggling with debugging very similarly named tests that it's actually the same as HEAD so it only differs from effulgentsia's patch... I think we are good, here. So to sum, compared to effulgentsia's patch my patch only differs with #empty_option in itself triggers the empty option which makes a lot of sense.

Here is an interdiff from a straight rollback to my patch.

I've spent some time reading through this and it all looks good and makes sense. (There is a "preprended" which should be "prepended" but whatever.) I am not confident enough in my understanding of FAPI internals to RTBC this, and even if I were I would wait for effulgentsia. But I think it's ready.

I'm also happy with #166 but I'll leave it up to @webchick. Given that she was super involved in this issue that seems appropriate ...

#166: fsr_interdiff.txt looks great. I didn't review the patch file in #166, but I trust that chx posted an accurate interdiff. We'll want to update the param docs of form_process_select() to make it clear that either #empty_value or #empty_option triggers the opt-in behavior, but I have no time to do that tonight, and I think it's fine to tweak docs in a non-beta-blocking follow-up.

To sum up #157 and #160, we have 2 properties, #empty_value and #empty_option, rather than an array with a key/value pair the way #options is, because when setting #options, you need to set keys and values, but with the emptiness functionality, you should rarely need to set either, and if you do, then you may need to set one or the other, but for decoupled reasons, so it's nice to keep them separate. I wouldn't mind changing #empty_option to #empty_option_label, but I don't think it's worth delaying beta to bikeshed this. Whatever the names of the two properties, it's a huge improvement compared to D6.

Re #159 and #165: I agree we'll want to incorporate all this into the FAPI reference, so it's not an esoteric body of knowledge. @rfay: can you link to the issue that's trying to make it easier to document FAPI (using a sensible file structure, not as a monster html file with a giant table); how's that issue going?

--- modules/filter/filter.module	2010-09-29 20:12:51 +0000
+++ modules/filter/filter.module	2010-10-03 18:01:56 +0000
@@ -827,6 +827,7 @@ function filter_process_format($element)
   $element['format']['format'] = array(
     '#type' => 'select',
     '#title' => t('Text format'),
+    '#required' => TRUE,

What's up with that? This makes the red required marker show up on the text format selector all the time, which we don't want.

Flagging this because I want to make sure this patch behaves as expected. We ought to be able to remove it from here (and conditionally add it in the code below, only when we are setting #default_value to NULL to force them to make a choice), like some of the earlier patches here did... right?

***

This patch is also missing all the followup bugfixes and tests we did earlier, but those we can always handle in a non-critical followup.

Per @eff's request in #171: The API module issue about figuring out a way to manage form (and other large array) documentation going forward is #100680: [meta] Make API module generate Form API documentation. Please come and help solve it.

Sorry, don't know how to make an interdiff, but the only change in this relative to #166 is moving the #required within filter.module, as per #172.

This patch is also missing all the followup bugfixes and tests we did earlier, but those we can always handle in a non-critical followup.

David: if you want to roll another patch here, go for it. Otherwise, if you're satisfied with just the filter.module change, please RTBC, and then submit your bug fixes that got lost in a follow-up.

Reviewed again - this looks good.

None of the followups we were working on are critical beta blockers (and none are things caused by the current patch) so no reason to hold it up over those.

Yes, I am fine with moving that around.

My test form now works great, node/add doesn't have a star next to input format as it appears the previous patch did, and it removes all the weird special-casing in system_element_info() and other places. sun's reasoning behind separating the two properties makes sense. What's not to love?

...oh. the fact that testbot's down. :(

I'll check this again tomorrow morning and commit it, unless Dries gets to it first. Thanks for the quick rally on this, folks.

Given this another review and it still looks good -- no surprise. Plus, webchick is happy with it so I committed to CVS HEAD. :-)

If there is ever a book written on Drupal, this patch might get a reference. Community building at work through good times and bad times. Thanks all!

And of course, I forgot to update the 'Status'. Classic.

OhhhKay now. Time for an API change retraction/reiteration. I've buried my head in the sand for this whole thing. New issue summary/API change summary? I can't bear to look and see if there's one buried in there. /me buries head again.

The summary on #46 still mostly applies, except modified by the table in #146. The text in #146 above the table may also be useful to people wanting to understand this more thoroughly. The 2 bold rows in the middle column of the table plus the 2 bold rows in the right column of the table reflect the net change relative to D6. The short version is that #46 is modified as follows:

- "Select" elements go back to having #required=FALSE by default, like all other FAPI elements.
- "Select" elements with #required=FALSE (the default) that specify neither of the new properties #empty_value or #empty_option continue to work just like they did in D6: illogically, because what is the conceptual meaning of an optional element where a browser will always submit a value, but fixing this logical hole is left to Drupal 8, because we wanted to retain backwards compatibility for all the contrib modules out there, given how late we are in the D7 cycle.

Unchanged from #46:
- "Select" elements may specify either #empty_value or #empty_option in order to have an empty option added. See form_process_select().
- Required "select" elements without a #default_value automatically get an empty option added, and this option fails validation, so that the user is required to make a choice, rather than letting the browser pick the first one.

A glitch from this, being treated now in a separate issue: #934110: Regression: Installer defaults country to 'Afghanistan' not '- None -'

#934110 has also just been fixed, and Drupal 7.0 beta 1 released. :)

Just turned up another regression in #936536: Missing #empty_option on a couple selects.

As I said there, I wonder if other select lists now need #empty_option to replace functionality that the patch in #57 originally removed. : ?

Documentation needs to be added to the FAPI reference page.

Agreed, but I think we're calling documentation needs "major" rather than "critical". Correct me if I'm wrong on that.

Sorry about that. I'm not even sure it's major. But it needs to be done. Changing priority to normal.