From f85a37c3076a3145aa291439b713e33825adfd83 Mon Sep 17 00:00:00 2001 From: Dries Buytaert Date: Mon, 4 Oct 2010 18:00:46 +0000 Subject: - Patch #140783 by sun, chx, effulgentsia, David_Rothstein, webchick: a select list without #default_value() always passes form validation. --- includes/form.inc | 97 +++++++++++------------------ includes/install.core.inc | 1 - modules/aggregator/aggregator.processor.inc | 1 - modules/block/block.admin.inc | 2 - modules/field_ui/field_ui.admin.inc | 11 ---- modules/filter/filter.module | 1 + modules/image/image.admin.inc | 2 - modules/image/image.field.inc | 3 - modules/locale/locale.admin.inc | 1 - modules/menu/menu.admin.inc | 2 - modules/simpletest/tests/form.test | 5 +- modules/simpletest/tests/form_test.module | 37 ++++++++--- modules/system/system.module | 5 -- 13 files changed, 70 insertions(+), 98 deletions(-) diff --git a/includes/form.inc b/includes/form.inc index ed5fa2850..3d468391c 100644 --- a/includes/form.inc +++ b/includes/form.inc @@ -1166,21 +1166,14 @@ function _form_validate(&$elements, &$form_state, $form_id = NULL) { // does not change the form, it will be the value of the first option. // Because of this, form validation for the field will almost always // pass, even if the user did not select anything. To work around this - // browser behavior, select fields without a #default_value get an - // additional, first empty option by default. In case the submitted - // value is identical to the empty option's value, we reset the - // element's value to NULL to trigger the regular #required handling - // below. + // browser behavior, required select fields without a #default_value get + // an additional, first empty option. In case the submitted value is + // identical to the empty option's value, we reset the element's value + // to NULL to trigger the regular #required handling below. // @see form_process_select() - elseif ($elements['#type'] == 'select' && !$elements['#multiple'] && !isset($elements['#default_value']) && $elements['#value'] === (string) $elements['#empty_value']) { - if ($elements['#required']) { - $elements['#value'] = NULL; - form_set_value($elements, NULL, $form_state); - } - else { - $elements['#value'] = $elements['#empty_value']; - form_set_value($elements, $elements['#empty_value'], $form_state); - } + elseif ($elements['#type'] == 'select' && !$elements['#multiple'] && $elements['#required'] && !isset($elements['#default_value']) && $elements['#value'] === $elements['#empty_value']) { + $elements['#value'] = NULL; + form_set_value($elements, NULL, $form_state); } elseif (!isset($options[$elements['#value']])) { form_error($elements, $t('An illegal choice has been detected. Please contact the site administrator.')); @@ -2172,6 +2165,15 @@ function form_type_select_value($element, $input = FALSE) { return (isset($element['#default_value']) && is_array($element['#default_value'])) ? $element['#default_value'] : array(); } } + // Non-multiple select elements may have an empty option preprended to them + // (see form_process_select()). When this occurs, usually #empty_value is + // an empty string, but some forms set #empty_value to integer 0 or some + // other non-string constant. PHP receives all submitted form input as + // strings, but if the empty option is selected, set the value to match the + // empty value exactly. + elseif (isset($element['#empty_value']) && $input === (string) $element['#empty_value']) { + return $element['#empty_value']; + } else { return $input; } @@ -2307,14 +2309,23 @@ function _form_options_flatten($array) { * Whether this first option is a valid option depends on whether the field * is #required or not. * - #required: (optional) Whether the user needs to select an option (TRUE) - * or not (FALSE). Defaults to TRUE. + * or not (FALSE). Defaults to FALSE. * - #empty_option: (optional) The label to show for the first default option. * By default, the label is automatically set to "- Please select -" for a * required field and "- None -" for an optional field. * - #empty_value: (optional) The value for the first default option, which is - * used to determine whether the user submitted a value or not. Defaults to - * '' (an empty string). To be used in case the field is optional and the - * empty default value should have a special value (e.g., a constant). + * used to determine whether the user submitted a value or not. + * - If #required is TRUE, this defaults to '' (an empty string). + * - If #required is not TRUE and this value isn't set, then no extra option + * is added to the select control, leaving the control in a slightly + * illogical state, because there's no way for the user to select nothing, + * since all user agents automatically preselect the first available + * option. But people are used to this being the behavior of select + * controls. + * @todo Address the above issue in Drupal 8. + * - If #required is not TRUE and this value is set (most commonly to an + * empty string), then an extra option (see #empty_option above) + * representing a "non-selection" is added with this as its value. * * @see _form_validate() */ @@ -2323,57 +2334,21 @@ function form_process_select($element) { if ($element['#multiple']) { $element['#attributes']['multiple'] = 'multiple'; $element['#attributes']['name'] = $element['#name'] . '[]'; - // If not explicitly set, #required has to default to FALSE (see below). - if (!isset($element['#required'])) { - $element['#required'] = FALSE; - } } // A non-#multiple select needs special handling to prevent user agents from // preselecting the first option without intention. #multiple select lists do // not get an empty option, as it would not make sense, user interface-wise. else { - $add_empty_option = FALSE; - // Select elements always have a value in HTML, so the expectation is that - // the user has to choose an option. Therefore, a select element is set to - // #required TRUE, unless it has been explicitly set to FALSE. This differs - // from every other element which gets a default of #required FALSE via - // form_builder(). To avoid this default, system_element_info() sets - // #required to NULL. - // If #required has been explicitly set to FALSE, the user may optionally - // choose an option, which can be "None". - if (isset($element['#required']) && !$element['#required']) { - $element['#required'] = FALSE; - $element += array( - '#empty_value' => '', - '#empty_option' => t('- None -'), - ); - $add_empty_option = TRUE; - } - // 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'])) { - // By only conditionally setting #required to TRUE, we additionally ensure - // that the select element does not get a required marker if it already - // has a value. - $element['#required'] = TRUE; + $required = $element['#required']; + // If the element is required and there is no #default_value, then add an + // empty option that will fail validation, so that the user is required to + // make a choice. Also, if there's a value for #empty_value or + // #empty_option, then add an option that represents emptiness. + if (($required && !isset($element['#default_value'])) || isset($element['#empty_value']) || isset($element['#empty_option'])) { $element += array( '#empty_value' => '', - '#empty_option' => t('- Select -'), + '#empty_option' => $required ? t('- Select - ') : t('- None -'), ); - $add_empty_option = TRUE; - } - // If there is a #default_value and #required is not explicitly FALSE, then - // there is no point in adding an empty option which is never valid, and we - // just retain API compatibility. - if (!isset($element['#required'])) { - $element['#required'] = FALSE; - } - // If one of the above conditions is met, add a first empty default option, - // which is always invalid for #required select lists that do not specify a - // #default_value. - // @see _form_validate() - if ($add_empty_option) { // The empty option is prepended to #options and purposively not merged // to prevent another option in #options mistakenly using the same value // as #empty_value. diff --git a/includes/install.core.inc b/includes/install.core.inc index 9cbfd059c..fcf8812e1 100644 --- a/includes/install.core.inc +++ b/includes/install.core.inc @@ -1759,7 +1759,6 @@ function _install_configure_form($form, &$form_state, &$install_state) { $form['server_settings']['site_default_country'] = array( '#type' => 'select', '#title' => t('Default country'), - '#required' => FALSE, '#default_value' => variable_get('site_default_country', NULL), '#options' => $countries, '#description' => st('Select the default country for the site.'), diff --git a/modules/aggregator/aggregator.processor.inc b/modules/aggregator/aggregator.processor.inc index a7a87dd15..cd74c0337 100644 --- a/modules/aggregator/aggregator.processor.inc +++ b/modules/aggregator/aggregator.processor.inc @@ -91,7 +91,6 @@ function aggregator_form_aggregator_admin_form_alter(&$form, $form_state) { $form['modules']['aggregator']['aggregator_summary_items'] = array( '#type' => 'select', '#title' => t('Number of items shown in listing pages'), - '#required' => FALSE, '#default_value' => variable_get('aggregator_summary_items', 3), '#empty_value' => 0, '#options' => $items, diff --git a/modules/block/block.admin.inc b/modules/block/block.admin.inc index db5b72100..b6a8919c5 100644 --- a/modules/block/block.admin.inc +++ b/modules/block/block.admin.inc @@ -129,7 +129,6 @@ function block_admin_display_form($form, &$form_state, $blocks, $theme, $block_r ); $form['blocks'][$key]['region'] = array( '#type' => 'select', - '#required' => FALSE, '#default_value' => $block['region'] != BLOCK_REGION_NONE ? $block['region'] : NULL, '#empty_value' => BLOCK_REGION_NONE, '#title_display' => 'invisible', @@ -302,7 +301,6 @@ function block_admin_configure($form, &$form_state, $module, $delta) { $form['regions'][$key] = array( '#type' => 'select', '#title' => $theme->info['name'], - '#required' => FALSE, '#default_value' => !empty($region) && $region != -1 ? $region : NULL, '#empty_value' => BLOCK_REGION_NONE, '#options' => system_region_list($key, REGIONS_VISIBLE), diff --git a/modules/field_ui/field_ui.admin.inc b/modules/field_ui/field_ui.admin.inc index 8a7f04340..3ba38d81b 100644 --- a/modules/field_ui/field_ui.admin.inc +++ b/modules/field_ui/field_ui.admin.inc @@ -335,7 +335,6 @@ function field_ui_field_overview_form($form, &$form_state, $entity_type, $bundle 'parent_wrapper' => array( 'parent' => array( '#type' => 'select', - '#required' => FALSE, '#options' => $table['#parent_options'], '#attributes' => array('class' => array('field-parent')), '#parents' => array('fields', $name, 'parent'), @@ -402,7 +401,6 @@ function field_ui_field_overview_form($form, &$form_state, $entity_type, $bundle 'parent_wrapper' => array( 'parent' => array( '#type' => 'select', - '#required' => FALSE, '#options' => $table['#parent_options'], '#attributes' => array('class' => array('field-parent')), '#parents' => array('fields', $name, 'parent'), @@ -458,7 +456,6 @@ function field_ui_field_overview_form($form, &$form_state, $entity_type, $bundle 'parent_wrapper' => array( 'parent' => array( '#type' => 'select', - '#required' => FALSE, '#options' => $table['#parent_options'], '#attributes' => array('class' => array('field-parent')), '#prefix' => '
 
', @@ -482,7 +479,6 @@ function field_ui_field_overview_form($form, &$form_state, $entity_type, $bundle ), 'type' => array( '#type' => 'select', - '#required' => FALSE, '#options' => $field_type_options, '#empty_option' => t('- Select a field type -'), '#description' => t('Type of data to store.'), @@ -491,7 +487,6 @@ function field_ui_field_overview_form($form, &$form_state, $entity_type, $bundle ), 'widget_type' => array( '#type' => 'select', - '#required' => FALSE, '#options' => $widget_type_options, '#empty_option' => t('- Select a widget -'), '#description' => t('Form element to edit the data.'), @@ -530,7 +525,6 @@ function field_ui_field_overview_form($form, &$form_state, $entity_type, $bundle 'parent_wrapper' => array( 'parent' => array( '#type' => 'select', - '#required' => FALSE, '#options' => $table['#parent_options'], '#attributes' => array('class' => array('field-parent')), '#prefix' => '
 
', @@ -544,7 +538,6 @@ function field_ui_field_overview_form($form, &$form_state, $entity_type, $bundle ), 'field_name' => array( '#type' => 'select', - '#required' => FALSE, '#options' => $existing_field_options, '#empty_option' => t('- Select an existing field -'), '#description' => t('Field to share'), @@ -554,7 +547,6 @@ function field_ui_field_overview_form($form, &$form_state, $entity_type, $bundle ), 'widget_type' => array( '#type' => 'select', - '#required' => FALSE, '#options' => $widget_type_options, '#empty_option' => t('- Select a widget -'), '#description' => t('Form element to edit the data.'), @@ -897,7 +889,6 @@ function field_ui_display_overview_form($form, &$form_state, $entity_type, $bund 'parent_wrapper' => array( 'parent' => array( '#type' => 'select', - '#required' => FALSE, '#options' => $table['#parent_options'], '#attributes' => array('class' => array('field-parent')), '#parents' => array('fields', $name, 'parent'), @@ -1051,7 +1042,6 @@ function field_ui_display_overview_form($form, &$form_state, $entity_type, $bund 'parent_wrapper' => array( 'parent' => array( '#type' => 'select', - '#required' => FALSE, '#options' => $table['#parent_options'], '#attributes' => array('class' => array('field-parent')), '#parents' => array('fields', $name, 'parent'), @@ -1713,7 +1703,6 @@ function field_ui_field_edit_form($form, &$form_state, $instance) { '#default_value' => !empty($instance['description']) ? $instance['description'] : '', '#rows' => 5, '#description' => t('Instructions to present to the user below this field on the editing form.
Allowed HTML tags: @tags', array('@tags' => _field_filter_xss_display_allowed_tags())), - '#required' => FALSE, '#weight' => 0, ); diff --git a/modules/filter/filter.module b/modules/filter/filter.module index f31a5d32c..aaba2b11f 100644 --- a/modules/filter/filter.module +++ b/modules/filter/filter.module @@ -850,6 +850,7 @@ function filter_process_format($element) { // If the stored format does not exist, administrators have to assign a new // format. if (!$format_exists && $user_is_admin) { + $element['format']['format']['#required'] = TRUE; $element['format']['format']['#default_value'] = NULL; // Force access to the format selector (it may have been denied above if // the user only has access to a single format). diff --git a/modules/image/image.admin.inc b/modules/image/image.admin.inc index 43cb479d1..5a40e6782 100644 --- a/modules/image/image.admin.inc +++ b/modules/image/image.admin.inc @@ -127,7 +127,6 @@ function image_style_form($form, &$form_state, $style) { ); $form['effects']['new']['new'] = array( '#type' => 'select', - '#required' => FALSE, '#options' => $new_effect_options, '#empty_option' => t('Select a new effect'), ); @@ -295,7 +294,6 @@ function image_style_delete_form($form, $form_state, $style) { $form['replacement'] = array( '#title' => t('Replacement style'), '#type' => 'select', - '#required' => FALSE, '#options' => $replacement_styles, '#empty_option' => t('No replacement, just delete'), ); diff --git a/modules/image/image.field.inc b/modules/image/image.field.inc index 221c5959d..11b49aa63 100644 --- a/modules/image/image.field.inc +++ b/modules/image/image.field.inc @@ -269,7 +269,6 @@ function image_field_widget_settings_form($field, $instance) { $form['preview_image_style'] = array( '#title' => t('Preview image style'), '#type' => 'select', - '#required' => FALSE, '#options' => image_style_options(FALSE), '#default_value' => $settings['preview_image_style'], '#description' => t('The preview image will be shown while editing the content.'), @@ -423,7 +422,6 @@ function image_field_formatter_settings_form($field, $instance, $view_mode, $for $form['image_style'] = array( '#title' => t('Image style'), '#type' => 'select', - '#required' => FALSE, '#default_value' => $settings['image_style'], '#empty_option' => t('None (original image)'), '#options' => $image_styles, @@ -436,7 +434,6 @@ function image_field_formatter_settings_form($field, $instance, $view_mode, $for $form['image_link'] = array( '#title' => t('Link image to'), '#type' => 'select', - '#required' => FALSE, '#default_value' => $settings['image_link'], '#options' => $link_types, ); diff --git a/modules/locale/locale.admin.inc b/modules/locale/locale.admin.inc index 9d71ed629..c748164a0 100644 --- a/modules/locale/locale.admin.inc +++ b/modules/locale/locale.admin.inc @@ -854,7 +854,6 @@ function locale_translation_filter_form() { $form['filters']['status'][$key] = array( '#title' => $filter['title'], '#type' => 'select', - '#required' => FALSE, '#empty_value' => 'all', '#empty_option' => $filter['options']['all'], '#size' => 0, diff --git a/modules/menu/menu.admin.inc b/modules/menu/menu.admin.inc index e18162c97..622df38ab 100644 --- a/modules/menu/menu.admin.inc +++ b/modules/menu/menu.admin.inc @@ -683,7 +683,6 @@ function menu_configure() { $form['menu_main_links_source'] = array( '#type' => 'select', '#title' => t('Source for the Main links'), - '#required' => FALSE, '#default_value' => variable_get('menu_main_links_source', 'main-menu'), '#empty_option' => t('No Main links'), '#options' => $menu_options, @@ -694,7 +693,6 @@ function menu_configure() { $form['menu_secondary_links_source'] = array( '#type' => 'select', '#title' => t('Source for the Secondary links'), - '#required' => FALSE, '#default_value' => variable_get('menu_secondary_links_source', 'user-menu'), '#empty_option' => t('No Secondary links'), '#options' => $menu_options, diff --git a/modules/simpletest/tests/form.test b/modules/simpletest/tests/form.test index 70cf19b86..4661ad25f 100644 --- a/modules/simpletest/tests/form.test +++ b/modules/simpletest/tests/form.test @@ -164,6 +164,8 @@ class FormsTestCase extends DrupalWebTestCase { // Posting without any values should throw validation errors. $this->drupalPost(NULL, array(), 'Submit'); $this->assertNoText(t($error, array('!name' => $form['select']['#title']))); + $this->assertNoText(t($error, array('!name' => $form['select_required']['#title']))); + $this->assertNoText(t($error, array('!name' => $form['select_optional']['#title']))); $this->assertNoText(t($error, array('!name' => $form['empty_value']['#title']))); $this->assertNoText(t($error, array('!name' => $form['empty_value_one']['#title']))); $this->assertText(t($error, array('!name' => $form['no_default']['#title']))); @@ -194,7 +196,8 @@ class FormsTestCase extends DrupalWebTestCase { 'empty_value' => 'one', 'empty_value_one' => 'one', 'no_default' => 'three', - 'no_default_optional' => '', + 'no_default_optional' => 'one', + 'no_default_optional_empty_value' => '', 'no_default_empty_option' => 'three', 'no_default_empty_option_optional' => '', 'no_default_empty_value' => 'three', diff --git a/modules/simpletest/tests/form_test.module b/modules/simpletest/tests/form_test.module index 57a96863c..0e704d25c 100644 --- a/modules/simpletest/tests/form_test.module +++ b/modules/simpletest/tests/form_test.module @@ -789,44 +789,65 @@ function form_test_select($form, &$form_state) { '#title' => '#default_value one', '#default_value' => 'one', ); + $form['select_required'] = $base + array( + '#title' => '#default_value one, #required', + '#required' => TRUE, + '#default_value' => 'one', + ); + $form['select_optional'] = $base + array( + '#title' => '#default_value one, not #required', + '#required' => FALSE, + '#default_value' => 'one', + ); $form['empty_value'] = $base + array( - '#title' => '#default_value one, #empty_value 0', + '#title' => '#default_value one, #required, #empty_value 0', + '#required' => TRUE, '#default_value' => 'one', '#empty_value' => 0, ); $form['empty_value_one'] = $base + array( - '#title' => '#default_value = #empty_value', + '#title' => '#default_value = #empty_value, #required', + '#required' => TRUE, '#default_value' => 'one', '#empty_value' => 'one', ); $form['no_default'] = $base + array( - '#title' => 'No #default_value', + '#title' => 'No #default_value, #required', + '#required' => TRUE, ); $form['no_default_optional'] = $base + array( '#title' => 'No #default_value, not #required', '#required' => FALSE, - '#description' => 'Should result in an empty string (default of #empty_value), because it is optional.', + '#description' => 'Should result in "one", because it is not required and there is no #empty_value requested, so default browser behavior of preselecting first option is in effect.', + ); + $form['no_default_optional_empty_value'] = $base + array( + '#title' => 'No #default_value, not #required, #empty_value empty string', + '#empty_value' => '', + '#required' => FALSE, + '#description' => 'Should result in an empty string (due to #empty_value), because it is optional.', ); $form['no_default_empty_option'] = $base + array( - '#title' => 'No #default_value, #empty_option', + '#title' => 'No #default_value, #required, #empty_option', + '#required' => TRUE, '#empty_option' => '- Choose -', ); $form['no_default_empty_option_optional'] = $base + array( '#title' => 'No #default_value, not #required, #empty_option', - '#required' => FALSE, '#empty_option' => '- Dismiss -', '#description' => 'Should result in an empty string (default of #empty_value), because it is optional.', ); $form['no_default_empty_value'] = $base + array( - '#title' => 'No #default_value, #empty_value 0', + '#title' => 'No #default_value, #required, #empty_value 0', + '#required' => TRUE, '#empty_value' => 0, '#description' => 'Should never result in 0.', ); $form['no_default_empty_value_one'] = $base + array( - '#title' => 'No #default_value, #empty_value one', + '#title' => 'No #default_value, #required, #empty_value one', + '#required' => TRUE, '#empty_value' => 'one', '#description' => 'A mistakenly assigned #empty_value contained in #options should not be valid.', ); diff --git a/modules/system/system.module b/modules/system/system.module index d83bd4c84..096442067 100644 --- a/modules/system/system.module +++ b/modules/system/system.module @@ -399,11 +399,6 @@ function system_element_info() { ); $types['select'] = array( '#input' => TRUE, - // In order to be able to determine whether a select list needs an empty - // default option, #required has to be NULL by default, as form_builder() - // preemptively sets #required to FALSE for all elements. - // @see form_process_select() - '#required' => NULL, '#multiple' => FALSE, '#process' => array('form_process_select', 'ajax_process_form'), '#theme' => 'select', -- cgit v1.2.3