Options module uses '_none' as a special value

Can't see why FAPI options uses '_none' when NULL seems a more logical (and less painful) method of adding an empty option. +1 here.

I can't think of any good reason why '_none' is used either. Rerolled the patch as it didn't apply anymore.
Assigning to yched however because maybe he has some more historical background, I'll let him push this RTBC or not.

Ouch - bad memories around this, i'm not too thrilled to poke in there :-).
This probably had to do with what FAPI select / radios / checkboxes allowed back in D6 / D7. Maybe those elements are fine with a NULL / empty string value now, but that would need to be thoroughly tested.

Also
- patch uses NULL as value in the FAPI code but empty strings in submitted forms values in tests ? Not ideal, but I guess that's how it works...
- could we settle on '' (single quotes) rather than "" (double quotes) for empty string ?

We're dealing with this over at Select or Other #1830090: Align with Drupal core select list, NULL (-None-) value. as well, so we're anxious to see where this one goes.

an array key should be an empty string, since PHP will cast NULL to one anyway.

i'd like to see this change too. another problem with how it is now is that you can create your own explicit empty-string key|label pair - eg "|None" - in the list of values for the field, but users end up seeing both the Drupal-generated "_none|N/A" and your "|None", both of which produce the same end result.

this already was an empty string up through 6.x, so presumably somebody changed that to '_none' for some reason. it would probably be good to know why that was done.

i'd like to also suggest allowing users to more easily customize the option_none value's label by declaring their own explicitly in the field's value list, as with "|None". that would work if we just swap the way the Drupal-generated value is added to the option list in _options_get_options():

$options = $options + array('' => $label);

instead of

$options = array('' => $label) + $options;

In D7, I find it odd that the options module doesn't just pass #empty_value and #empty_option to the underlying select, but instead does its own thing with theme_options_none(). Wouldn't it make more sense to set these standard FAPI options from the field settings instead? At least this way it would be done in a way people would expect, plus #empty_value defaults to an empty string (can't be NULL according to docs), so the question of "" or "_none" seems handled.

In D8 this also seems to apply, but I'm not up-to-date with D8 development.

In 8 you implement a isEmtpy() method I believe. https://drupal.org/node/1985716

I'm going to take a crack at converting this module to 8.x pretty soon. Unless someone beats me to it of course.

what is this issue waiting on? does it have to be fixed in 8.x and then backported to 7.x?

reroll

This affects the drupal.org "gender" profile field on d.o, so I'm marking it as such. As a result, I'm also closing a duplicate: #2412393: "Prefer not to share" gender option option does not display correct in profile form.

Taking it off yched's plate as it hasn't been touched in a year.

This could use an updated issue summary to make it easier for someone to help with this. See instructions: https://drupal.org/contributor-tasks/write-issue-summary

Yes, #8 is spot on. Let's use #empty_option and #empty_value please!

In Drupal 7 pretty weird is happening.

I've got field like:

  '#entity_type' => string 'entityform' (length=10)
  '#field_name' => string 'field_declaration_checkbox_2' (length=28)
  '#required' => boolean true
  '#delta' => int 0
  '#type' => string 'checkbox' (length=8)
  '#default_value' => int 0
  '#on_value' => int 1
  '#off_value' => int 0
  '#value_key' => string 'value' (length=5)
  '#element_validate' => 
    array (size=2)
      0 => string 'options_field_widget_validate' (length=29)
  '#value' => int 0
  '#checked' => boolean false

where before I start the form, the value is string, when submitting, it's integer. When it's integer, this code doesn't make any sense:

function options_field_widget_validate($element, &$form_state) {
  if ($element['#required'] && $element['#value'] == '_none') {

When you comparing integer with string, string becomes integer (zero in this case). So in this case #value equals to '_none', but it's not really none. I don't know if it's my custom code causing it, but probably not.

Re-rolled against 8.2.x.

Re-roll.

The last patch threw an error when trying to submit a form with a select list that is not required.

One question on #24: Are there any inherent problems still associated with the patch? Or are the tests only failing because the tests themselves have to be adapted? TIA!

The HTML
required Attribute only triggers client-side validation if the value isn't set so we should implement this issue and remove '_none'. See https://www.w3schools.com/tags/att_select_required.asp

The patch in #24 no longer applies to 8.3.5

I agree, mpp. Client-side validation doesnt work if there's a "_none" value. Furthermore, you'll get W3C validation errors if there's not a decent empty value. TBH, I think it was an extremely bad idea to introduce this magic _none value. It ought to be removed ASAP. Hopefully, this patch is included in future Drupal versions.

The patch, BTW, doesnt apply, because the test files dont seem to exist anymore and - as usual - the Array syntax has changed to just brackets. I've added a patch for 8.3.x. Perhaps it will also apply for later versions.

Patch #33 solved the issue for me Drupal core 8.4.4, this should be committed to core

This should be set to RTCB

The patch does not apply.

I have re-rolled the patch for supporting the latest version of Drupal 8.5.x.

Removed Needs Reroll tag.

Updated tests to check for empty string instead of _none.

This should make the unit tests happy.

Patch applies cleanly. Usage of empty strings seems alright instead of _none.
RTBC good to go!

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/OptionsWidgetBase.php
   @@ -67,7 +67,7 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   -    if ($element['#required'] && $element['#value'] == '_none') {
   +    if ($element['#required'] && $element['#value'] == '') {
   

   I'm not sure this is the same thing.

   $element[#value] of FALSE or possibly 0 would match here.

   I think we need === not ==

2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/OptionsWidgetBase.php
   @@ -85,7 +85,7 @@ public static function validateElement(array $element, FormStateInterface $form_
   -    $index = array_search('_none', $values, TRUE);
   +    $index = array_search('', $values, TRUE);
   

   we're being strict about type here, so that makes me more sure ^ should be strict too

3. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/OptionsWidgetBase.php
   @@ -117,7 +117,7 @@ protected function getOptions(FieldableEntityInterface $entity) {
   +        $options = ['' => $empty_label] + $options;
   
   +++ b/core/modules/options/options.api.php
   @@ -32,7 +32,7 @@ function hook_options_list_alter(array &$options, array $context) {
   +    $options[''] = t('== Empty ==');
   
   +++ b/core/modules/options/tests/src/Functional/OptionsSelectDynamicValuesTest.php
   @@ -25,7 +25,7 @@ public function testSelectListDynamic() {
   +      if ($value != '') {
   
   +++ b/core/modules/options/tests/src/Functional/OptionsWidgetsTest.php
   @@ -125,7 +125,7 @@ public function testRadioButtons() {
   +    $edit = ['card_1' => ''];
   

   I think we should use a constant here instead of ''.

   OptionsWidgetBase::EMPTY_VALUE or something.

   That way, if we change it in the future, we do it once

   Would bring it in line with #2448545: Convert '_none' option to a constant, deprecate form_select_options(), deprecate form_get_options() for removal, move form_select_options() to a new class. too

4. +++ b/core/modules/options/tests/src/Functional/OptionsSelectDynamicValuesTest.php
   @@ -25,7 +25,7 @@ public function testSelectListDynamic() {
   -      if ($value != '_none') {
   +      if ($value != '') {
   

   same comment about strictness applies here

Thanks @larowlan for quick feedback.

Fixed the issues related to strict checking.

About replacing '_none' or '' with class constant - I would request to keep it in #2448545: Convert '_none' option to a constant and deprecate form_select_options() only as it will be out of scope for current ticket + changes required for #2448545 are even outside the Options core module. There are many references to '_none' outside the module so it makes more sense to create the constant in core itself (outside the module). Approach and solution in #2448545 look good to me (and for sure it is going through it's own review cycle), only issue I see is bit of rework required in one of the patches after the other patch is merged, which should be fine IMO.

We should change OptionsSelectWidget to use the #empty_option and #empty_value properties of the select FAPI element (\Drupal\Core\Render\Element\Select), like already mentioned in #8 and #16 :)

Also, this is kind of a BC break for modules which implement hook_options_list_alter() so we need a CR at least.

Closed #2942806: HTML5 Validation not working for Field type List as a duplicate issue.

Adding the other issue as related.

(Although, personally, I don't see the point of continuing to maintain this module in core, when Taxonomy always seems to be the better choice. But maybe I'm missing something?)

Patch applied successfully for 8.8.4.

rerolled patch 46

Rerolled patch for latest head for 9.1.x

@anushrikumari The test cases are failing. hence the correct status is "Needs Work".

Fixing tests!

Fixing failed test.

Re: #51. Emphasis mine...

(Although, personally, I don't see the point of continuing to maintain this module in core, when Taxonomy always seems to be the better choice. But maybe I'm missing something?)

That's a fairly opinionated choice; I'd tend to disagree here. Which one you choose really depends on your needs.

Taxonomy is a good choice if you need:

• The ability for content managers to add new options later on.
• Listing pages for content with a particular term. (Taxonomy module comes with some pre-made views, although you could easily create ones based on options fields too.)
• "Big-T" taxonomy features - such as hierarchical vocabularies, preferred terms, synonym collapsing, glossary of term and definition, controlled vocabularies with authority codes, etc.

On the other hand, Taxonomy module brings in a lot of extra baggage, if all you want is a simple enumerated property on a node.

• An "options" field implemented using taxonomy terms will have dependencies on content entities (the terms), not just field config entities. So deployment of new features is complicated by the need to create the term entities in advance (using an update hook, say), and the need for a term ID to be the same on development/production environments.
• Using terms for a simple options field carries a risk of breakage if a term is deleted. So you need to take steps to prevent this, with permissions.
• Terms have a taxonomy/term/% route of their own. Unless your "options" need a page of their own, then you'll need to take steps to hide this from site visitors.

Reroll patch #62 against head of 9.2.x
Also applies to 9.1.0

I'm confused between this and #2448545: Convert '_none' option to a constant, deprecate form_select_options(), deprecate form_get_options() for removal, move form_select_options() to a new class. as they both seem to be going in the same direction but the patches are quite different.

I don't believe we can just change this constant as there could be JavaScript or custom code depending on the existing special value. However, I also don't know how we would be able to deprecate it.

> I don't believe we can just change this constant as there could be JavaScript or custom code depending on the existing special value.

Yes good point.

> However, I also don't know how we would be able to deprecate it.

We can use the "Only new installs get it" pattern.

A imho thorough solution would make this a parameter of the render array (like '#empty_key'), which defaults to '_none' for old installs, and '' for new ones. (Which also means i'd suggest to close the other issue as dup.)

patch 64 needed a reroll

Rerolled patch #68.

fixed the failed test case.

Needs work based on #67. This is going to break any custom code that uses form options, such as submit and validation handlers.

The BC policy says that the form and render API are public, though individual pages and forms aren't. That means that the value you get from a form element is part of our public API.

Re: #67

The problem with "new installs get it" is that you effectively prevent people with centralized custom modules from doing new installs without a major refactor, or manipulating state somewhere.

I'd tend to lean toward a new element being the safest option, as no BC concerns present themselves.

> A imho thorough solution would make this a parameter of the render array (like '#empty_key'), which defaults to '_none' for old installs, and '' for new ones. (Which also means i'd suggest to close the other issue as dup.)

That would make it different for each form, which is probably needed in case a site has a mixture of contrib modules that have updated to the new empty key and modules that haven't.

> The problem with "new installs get it" is that you effectively prevent people with centralized custom modules from doing new installs without a major refactor, or manipulating state somewhere.

But could we also set a global setting in an update hook, so existing sites don't need to do anything? A container parameter would do it, but I don't think there's a way that could be set permanently by an update hook?

FWIW, the patch at #72 also applies against 9.2.x and works as expected with clientside validation. Having html5 validation for list elements is a huge win.

Patch at #72 no longer applies to 9.2.7. I'll possibly attempt a backport sometime soon, unless anyone else cares to do so.

Rerolled the patch in #72 as requested in #78. Retaining status as "Needs work" for the reviews mentioned in #74 to #77.

Thanks!

Rerolled against latest version

Rerolled against the patch 158590_55 for version 9.x

The patch has been rerolled for Drupal 10.1.x

Patch 84 still applies cleanly to the HEAD of 9.5.x, 10.0.x and 10.1.x, I just checked vs the latest in each branch.

Patch 84 passes.

Patch 85 and 87 were not needed and were incorrectly rolled and fail testing.

https://www.drupal.org/docs/develop/git/using-git-to-contribute-to-drupa...

We still need to address the backward compatibility concerns from #66 onwards: we need a way to allow keep '_none' for existing sites that depend on it, while allowing '' for sites that want to use that instead.

patch 84 no longer applies to 10.0.x, will need a new patch for 10.0.10

Thank you @joseph.olstad
Patch #93 working clearly with D10.1.4 version.

Fixed link to project page.