Option for _none is removed once a field has a value and can cause accidental data corruption

@mstrelan Maybe the right approach to this is to warn first the user before term deletion? Right bellow the message "Deleting a term will delete all its children.." should be a message/warning that the specific term is referenced from an entity (if it is). If the user proceed with the deletion, then the new reference should be to '_none'.

Btw the code on function getEmptyLabel() adds a '- Select a value -' option for required fields that do not come with a value.

Working on this one.

Interesting. I'm gonna try also to reproduce the issue with user ref fields

The issue comes from the fact that after the delete of the term (entity in general) the database still keeps the reference for the given entity (and there is no check if the deleted term exists). There is a discussion here
https://www.drupal.org/project/drupal/issues/2978521 about deleting orphaned references when an entity is deleted

@mstrelan You are absolutely right on this one. For now, I think I can make a patch for the term content entity type, and we can move on with the rest. A broader solution could be to check if the previously selected key is in the new options list, and if not, then display the - Select one - for required fields. Just a quick thought ofc, this needs some more investigation.

Here is a quick patch

Dummy me, forgot the _none value, here is the correct patch

Took a shot at the test case

Applied patch #17 on 9.5.x.The patch fixes the issue of showing first item in the list as default one.

Before Patch:

After patch:

If the issue is resolved and code looks good to you please move to RTBC

Addressed point in #22

Tested on Drupal 10.1.x, 9.5.x along with php 8.1

- The patch applies for both the Drupal versions
- Issue summary is clear and provides testing steps
- After applying the patch, once we delete a referenced term, the select field reverts to "Select a value"
- Unless we select a different term, the form can't be saved.
- Did a code review and no major issues were identified

My only suggestion is to break the comments near 80 chars, but marking it to RTBC anyways

+      // Add empty label in case the default value
+      // is missing from the options list.

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/OptionsSelectWidget.php
   @@ -29,9 +29,18 @@ class OptionsSelectWidget extends OptionsWidgetBase {
   +    $default_value = $this->getSelectedOptions($items);
   ...
          '#default_value' => $this->getSelectedOptions($items),
   

   we can use $default_value here now, instead of calculating it twice

2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/OptionsSelectWidget.php
   @@ -78,4 +88,18 @@ protected function getEmptyLabel() {
   +  protected function getEmptyLabelMissingValue() {
   

   Let's add a return type for new code

3. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/OptionsSelectWidget.php
   @@ -78,4 +88,18 @@ protected function getEmptyLabel() {
   +      return t('- None -');
   ...
   +    return t('- Select a value -');
   

   Let's not use t in OO code, either use $this->t or if that's not available, new TranslatableMarkup

Just nits

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/OptionsSelectWidget.php
   @@ -59,6 +69,7 @@ protected function supportsGroups() {
      protected function getEmptyLabel() {
   +
        if ($this->multiple) {
   

   stray newline?

2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/OptionsSelectWidget.php
   @@ -78,4 +89,18 @@ protected function getEmptyLabel() {
   +  /**
   +   * {@inheritdoc}
   +   */
   +  protected function getEmptyLabelMissingValue(): TranslatableMarkup {
   

   There is no parent version of this, so needs a real docblock

Made changes , As per the comment #30.

Manually tested following the issue summary and confirmed it is working now.

1. Nice find - tricky bug! And great to see test coverage.

2. diff --git a/core/interdiff-3095257-29_31.txt b/core/interdiff-3095257-29_31.txt
   new file mode 100644
   index 0000000000..72da122d71
   --- /dev/null
   +++ b/core/interdiff-3095257-29_31.txt
   @@ -0,0 +1,21 @@
   +diff --git a/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/OptionsSelectWidget.php b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/OptionsSelectWidget.php
   +index 6b7832b921..12f26d0a88 100644
   +--- a/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/OptionsSelectWidget.php
   ++++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/OptionsSelectWidget.php
   +@@ -69,7 +69,6 @@ protected function supportsGroups() {
   +    * {@inheritdoc}
   +    */
   +   protected function getEmptyLabel() {
   +-
   +     if ($this->multiple) {
   +       // Multiple select: add a 'none' option for non-required fields.
   +       if (!$this->required) {
   +@@ -90,7 +89,7 @@ protected function getEmptyLabel() {
   +   }
   + ¶
   +   /**
   +-   * {@inheritdoc}
   ++   * Handles label in case of missing value.
   +    */
   +   protected function getEmptyLabelMissingValue(): TranslatableMarkup {
   +     // Add a 'none' option for non-required fields,
   

   This shouldn't be part of the patch - it's an interdiff.

3. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/OptionsSelectWidget.php
   @@ -29,10 +30,19 @@ class OptionsSelectWidget extends OptionsWidgetBase {
   +    $options = [];
   +    $default_value = $this->getSelectedOptions($items);
   +    if (!$default_value) {
   +      // Add empty label in case the default value is missing from the options
   +      // list.
   +      $options['_none'] = $this->getEmptyLabelMissingValue();
   +    }
   +    $options += $this->getOptions($items->getEntity());
   +
        $element += [
          '#type' => 'select',
   -      '#options' => $this->getOptions($items->getEntity()),
   -      '#default_value' => $this->getSelectedOptions($items),
   +      '#options' => $options,
   +      '#default_value' => $default_value,
          // Do not display a 'multiple' select box if there is only one option.
          '#multiple' => $this->multiple && count($this->options) > 1,
        ];
   @@ -78,4 +88,18 @@ protected function getEmptyLabel() {
   
   @@ -78,4 +88,18 @@ protected function getEmptyLabel() {
        }
      }
    
   +  /**
   +   * Handles label in case of missing value.
   +   */
   +  protected function getEmptyLabelMissingValue(): TranslatableMarkup {
   +    // Add a 'none' option for non-required fields,
   +    // and a 'select a value' option for required fields that do not come
   +    // with a value selected.
   +    if (!$this->required) {
   +      return $this->t('- None -');
   +    }
   +    // Return value for required fields.
   +    return $this->t('- Select a value -');
   +  }
   

   I think we can do better than this with less code duplication - especially of text strings.

   The problem here is that $this->has_value is being determined incorrectly. It's tricky because we use $this->has_value in determining the option list and we use that to determine the selected option - so there is a bit of chicken and egg - but I think the method could look like this:

     /**
      * {@inheritdoc}
      */
     public function formElement(FieldItemListInterface $items, $delta, array $element, array &$form, FormStateInterface $form_state) {
       $element = parent::formElement($items, $delta, $element, $form, $form_state);
       $options = $this->getOptions($items->getEntity());
       $default_value = $this->getSelectedOptions($items);
   
       // @todo some descriptive comment of what is going on here.
       if ($this->has_value && empty($default_value)) {
         $this->has_value = FALSE;
         // Add an empty option if the widget needs one.
         if ($empty_label = $this->getEmptyLabel()) {
           $options = ['_none' => $empty_label] + $options;
         }
       }
   
       $element += [
         '#type' => 'select',
         '#options' => $options,
         '#default_value' => $default_value,
         // Do not display a 'multiple' select box if there is only one option.
         '#multiple' => $this->multiple && count($options) > 1,
       ];
   
       return $element;
     }
   

   This way the intention of \Drupal\Core\Field\Plugin\Field\FieldWidget\OptionsWidgetBase::getOptions() and any overrides of \Drupal\Core\Field\Plugin\Field\FieldWidget\OptionsSelectWidget::getEmptyLabel() will continue to work as expected.

4. +++ b/core/modules/options/tests/src/Functional/OptionsWidgetsTest.php
   @@ -372,6 +378,52 @@ public function testSelectListSingle() {
   +    $this->assertTrue($this->assertSession()->optionExists($fieldName, '_none')->isSelected());
   

   It would be great to test what the text is here - given that it is programatically determined.

Another option that would definitely play nice with anything that extends from this class is to do this:

  /**
   * {@inheritdoc}
   */
  public function formElement(FieldItemListInterface $items, $delta, array $element, array &$form, FormStateInterface $form_state) {
    $element = parent::formElement($items, $delta, $element, $form, $form_state);
    $default_value = $this->getSelectedOptions($items);

    // The default value has been removed but the option list has been
    // calculated assuming that the element has a value. Force it to be
    // recalculated.
    if ($this->has_value && empty($default_value)) {
      unset($this->options);
      $this->has_value = FALSE;
    }

    $element += [
      '#type' => 'select',
      '#options' => $this->getOptions($items->getEntity()),
      '#default_value' => $default_value,
      // Do not display a 'multiple' select box if there is only one option.
      '#multiple' => $this->multiple && count($this->options) > 1,
    ];

    return $element;
  }

The downside of this approach is that we have to work out the $this->options static again...

And here is the potential half way between the two above solutions...

  /**
   * {@inheritdoc}
   */
  public function formElement(FieldItemListInterface $items, $delta, array $element, array &$form, FormStateInterface $form_state) {
    $element = parent::formElement($items, $delta, $element, $form, $form_state);
    $default_value = $this->getSelectedOptions($items);

    // @todo some text to explain...
    if ($this->has_value && empty($default_value)) {
      $this->has_value = FALSE;
      // Add an empty option if the widget needs one.
      if ($empty_label = $this->getEmptyLabel()) {
        $this->options = ['_none' => $empty_label] + $this->options;
      }
    }

    $element += [
      '#type' => 'select',
      '#options' => $this->getOptions($items->getEntity()),
      '#default_value' => $default_value,
      // Do not display a 'multiple' select box if there is only one option.
      '#multiple' => $this->multiple && count($this->options) > 1,
    ];

    return $element;
  }

This relies on $this->options and fixes it. So it has the advantage of #34 in that $this->options is correct but doesn't do the recalculation. But then it is relying on $this->getSelectedOptions() setting up $this->options...

I'm not sure which solution I like best. And then there is the question of hook_options_list_alter() and how this plays into that.

I liked the last proposal from @alexpott, as he said it is the best of both worlds by reducing code duplication and avoid options recalculation.

I think that will work ok when hook_options_list_alter(), because it is part of the options calculation so this fix can also help when someone removes from the options the default value.

I improved multiple things in the test to make it more readable and consistent. Also, I found it was false positive because the vocabulary was created but never saved, so all the times the only option was "_none", that issue was coming from here:

+    $vocab = Vocabulary::create([
+      'name' => 'Views testing tags',
+      'vid' => 'views_testing_tags',
+    ]);

Removed the interdiff from previous patch.

Fixed CS issue, only difference with previous patch is removing following line from test:
use Drupal\taxonomy\Entity\Vocabulary;

So \Drupal\Core\Field\Plugin\Field\FieldWidget\OptionsButtonsWidget is not affect by this because its \Drupal\Core\Field\Plugin\Field\FieldWidget\OptionsButtonsWidget::getEmptyLabel() does not use $this->has_value but some things in contrib are. For example, \Drupal\entity_reference_override\Plugin\Field\FieldWidget\EntityReferenceOverrideSelect will have exactly the same problem.

I'm not sure how that affects the solution we're going for here. Any general solution would have to fix $this->has_value to be always correct but to do that we have to work out the list first - where we need $this->has_value. The empty option stuff would need to be completely separated from the list of allowed values for that to work. Which in turn would probably affect even more things. This is very tricky. Perhaps the best thing here is to fix the core element and file a follow-up to try to get to a more general solution. We'll have to refactor \Drupal\Core\Field\Plugin\Field\FieldWidget\OptionsWidgetBase to deal with this.

@alexpott should this go back to NW for a different approach? Or least to have a follow-up created.

Created the followup for #40

Bumping to critical since it's a data integrity bug.

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/OptionsSelectWidget.php
@@ -28,11 +28,21 @@ class OptionsSelectWidget extends OptionsWidgetBase {
+
+    // Set has_value to FALSE and empty label when default value is empty.
+    if ($this->has_value && empty($default_value)) {
+      $this->has_value = FALSE;

I think we need to improve the code comments here - i.e. should we talk about reference fields with 'missing' entities explicitly as one of the reasons the default value can be empty?

This issue has been flagged as a "hard problem" in the Bug Smash Initiative "hard problems meeting".

I'm unclear why this was moved to Active instead of Needs work as the feedback in #44 is referring to the patch in #39.

So... moving to Needs work to incorporate feedback from #44 and #40.

@smustgrave You mention creating a followup in #42 but I don't see the issue noted as a related issue here. Please add when you get a chance.

Triaging the options queue. This will need an IS update but seems like we need a new solution based on #40?

Actually I tried replicating this and was not able to. Can someone else try?

I just followed the steps in the summary

Thanks for the video.

Cleaning up the file list some (hiding everything)

Got this slightly started but believe we need to reset $this->options but that causes phpstan warning.

Used AI to help get around the phpstan error I encountered in #56

Cannot unset property
Drupal\Core\Field\Plugin\Field\FieldWidget\OptionsWidgetBase::$option
s because it might have hooks in a subclass.
🪪 unset.possiblyHookedProperty

I ran the test file locally on the main branch (local equivalent of the test-only test in the pipeline). Here is the output:

------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
https://drupal.ddev.site/sites/simpletest/browser_output/Drupal_Tests_op...

Time: 01:12.153, Memory: 10.00 MB

There was 1 failure:

1) Drupal\Tests\options\Functional\OptionsWidgetsTest::testSelectListSingle
Behat\Mink\Exception\ElementNotFoundException: Option with id|name|label|value "_none" not found.

/var/www/html/core/tests/Drupal/Tests/WebAssert.php:246
/var/www/html/core/modules/options/tests/src/Functional/OptionsWidgetsTest.php:424

FAILURES!
Tests: 8, Assertions: 210, Failures: 1.

---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

Could the assertion be more specific? and specify one of the 'id|name|label|value'?

I have reviewed the issue. Made 2 code comments. There are a few instances of $options = []; in core but none of $options = NULL; Is this a PHP 8.5 way of handling empty arrays?

Apart from that seems close to RTBTC.

Thanks but I've pinged people for review already

At lines 311 to 313 of OptionsWidgetsTest.php is:

    // With no field data, nothing is selected.
    $this->assertTrue($this->assertSession()->optionExists('card_1', '_none')->isSelected());
    $this->assertFalse($this->assertSession()->optionExists('card_1', 0)->isSelected());

Could that pattern be repeated in the test coverage to assert which option is selected and which is not?

Re: #62

Thanks but I've pinged people for review already

Sorry, I don't understand? The issue was marked 'Needs review' I reviewed it at #59 to #60. And added 2 code comments.

I have followed the steps to reproduce from the mp4 video. The result is that the tags select list shows '-Select a value-' not 'Bar' as before the fix. But you cannot now save the article until you change '-Select a value-' to a real tag value.

If you view the article just after deleting 'Foo' it displays with a blank tags field.

If you try to edit the article and save it without changing any values, validation of the tags field prevents you. You have to select a real tag value from the list, ie 'Bar'.

This seems ready for RTBTC.

I concur with the RTBC status. I was able to follow the steps to reproduce the issue. Applying the MR fixes the problem. I reviewed the code and didn't find anything to comment about.

I've added some comments to the MR that need to be addressed.

Thanks @alexpott!

Random JS failure.

Failure is from #3623728: Node.js v24 upgrade fails all Nightwatch test runs

Copied the

Re the auto-selecting when there is a single value. This is what actually happens in HEAD and there is a single value and you are creating an entity rather than editing it. The current fix regresses this untested behaviour.

Re: #74: But if you are creating a new entity there is no danger of data corruption since there is no data stored for the new entity in the db yet?

We have to prevent regression when fixing this issue, however. After creating the entity with the only value 'foo', so it is stored in the db, more terms are added to the vocab, when the entity is then edited those terms need to be available. If one of the new terms where to flip the value like happens in mstrelan's mp4 then that would be a regression. So long as we have test coverage for that..

Thinking about it we could allow the flip from foo to bar as shown in the mp4 so long as a (danger!) message is presented to the user? The message would only appear on entity edit as in the mp4 but not on entity create. Avoiding the regression highlighted by alexpott in #74.

Re the auto-selecting when there is a single value. This is what actually happens in HEAD and there is a single value and you are creating an entity rather than editing it. The current fix regresses this untested behaviour.

OOPSS.... I'm wrong - In my head we were adding the _none in \Drupal\Core\Field\Plugin\Field\FieldWidget\OptionsSelectWidget::getEmptyLabel when $this->has_value was true but that's not the case.

I think the current MR is a really great position to land in and much better than the previous attempts. Thanks @smustgrave and @mstrelan for all the effort here.

Thank you @alexpott for all your work in this issue.

Re: #78:

I think the current MR is a really great position to land in and much better than the previous attempts.

edit: mstrelen's code comment 16 hours ago:

In fact, the best action would be to show a message that the previously selected value is no longer available. We possibly should have UX review here.

I agree about the message at least.

So here's the funny thing about #74 and #78... there is actually a situation in HEAD where I'm correct! If you set your field to be multi-cardinality then it will auto select the one and only choice if there is only one option :)

This problem is caused by the fact that whether or not something is a multiple select also depends on the number of options. So actually our code needs to look like this:

  /**
   * {@inheritdoc}
   */
  public function formElement(FieldItemListInterface $items, $delta, array $element, array &$form, FormStateInterface $form_state) {
    $element = parent::formElement($items, $delta, $element, $form, $form_state);

    $options = $this->getOptions($items->getEntity());
    $selected = $this->getSelectedOptions($items);
    // Do not display a 'multiple' select box if there is only one option.
    $multiple = $this->multiple && count($options) > 1;

    // If the selected option is empty and the field is required, add an option
    // to force the user to choose.
    if (!isset($options['_none']) && empty($selected) && $this->required && !$multiple) {
      // Ensure there is an empty option if the widget needs one.
      $empty_label = $this->t('- Select a value -');
      $this->sanitizeLabel($empty_label);
      $options = ['_none' => $empty_label] + $options;
    }

    $element += [
      '#type' => 'select',
      '#options' => $options,
      '#default_value' => $selected,
      '#multiple' => $multiple,
    ];

    return $element;
  }

FWIW I still think when there is only one possible option and the field is required, it is better UX to auto select the one option. I think it falls into the bucket of "Don't make me think" - a mandatory question with exactly one acceptable answer isn't a question. But I think we can defer this discussion to a follow-up.

Re: #81 Thank you mstrelan. Interesting video, tricky bug.

alexpott if you can please refer to #19 and #20 especially the screenshots before and after, at that time we had code that fixed the bug: the before screenshot shows 'bar' highlighted in green and the after screenshot shows '- Select an option -' highlighted in green. At some point there has been scope creep and we are now considering things like how the widget should behave when the field is multiple cardinality (as well as single).

Scope creep is okay so long as there is agreement and the IS is updated accordingly. I think we have a split vote 2:2 at the moment.

But I think we can defer this discussion to a follow-up.

This is a critical bug that risks data loss. alexpott you have not explicitly taken issue with that (yet). Do you want to downgrade the status of the ticket and edit the title to remove the stuff about data loss?

edit: The President uses a Drupal site to make decisions during a missile crisis. There is a taxonomy field: single value, 'What to do'. There are 2x terms: 'Nuke Em!' 'Don't Nuke em!'. And a body field detailing the current state of the crisis. Vice President creates a new entity. Sets term to 'Don't Nuke em!'. Adds latest updates to the body field. Naughty computer engineer removes the 'Don't Nuke em!' term. New developments so VP edits the entity body field. Saves. Does not notice the 'Nuke Em!'.. President opens the entity, reaches for the button. Kaboom! My point is though many sites may use taxonomy/ vocabs for like do I want my new sweater in medium, large, small etc. But other sites eg in medical research field may use taxonomies in ways that make correct term selection more critical.

Patient needs leg/ arm/ hows your fathers amputated. Waking up from operation. Doc, where's my ??

In the examples above the fault could be laid at the door of the site builder. However, the data corruption risk increases in relation to the number of terms in the vocab. Say there are 100 terms in the field. Each term is an obscure latin name. Only the prof. knows what they mean so only he edits that field. Junior researcher edits an entity. Does not notice that 'lex ad astram' got deleted by prof. Next in list has been auto-selected, 'lex ex astram'. Saves the entity with wrong value. Completely oblivious.

@alexpott restored behavior to what it does currently.

edit: Thanks smustgrave now we can progress.

The next step is to issue the person editing the entity a warning message that data loss may occur. That could justify removing the 'accidental' aspect of this issue (see the title).

We could then aim for RTBTC. If we want to take it further and force the user to select an option before saving that can be in a follow-up.

@oily - the bug in the original report happens when there widget is in multiple mode - it is not scope creep. And we have to consider the changes made and how they impact both multiple and single select mode. That is also part of the scope.

WRT to data loss that's actually not what is happening here. This bug is the result of someone, with permission to, deleting something on your site and thereby making other things on your site broken. The point at which things have gone wrong is when the delete happened. That's when you have a data integrity issue and not a data loss bug. It is not when the edit form is opened. So this fix is not actually fixing the critical part of the data integrity bug. We should check for existing entity reference issues about deleting referenced entities that are used in field values and if an issue does not exist we should create one.

Also FWIW usability studies have shown that select lists are problematic, and multiple select lists even more so... and required fields are a UX nightmare too... see https://design-system.service.gov.uk/components/select/ for some of this...

The select component should only be used as a last resort in public-facing services because research shows that some users find selects very difficult to use.

... so if you put a nuclear option on a select list... perhaps that's your fault :)

Thanks alexpott for #91 read with interest.

The IS states

When you create a required single-value entity reference field

The steps to reproduce include

Change the Tags field to Required, Allowed number of values to 1

You state

the bug in the original report happens when there widget is in multiple mode -

Are the first two statements using different words to refer to the same thing as the third statement?

@oily note given how concerned you are #86... try this against HEAD without the MR.

1. Create a taxonomy with 1 term
2. Create a node type with a required entity reference to this taxonomy with multiple cardinality.
3. Create a node of this type...

With the current code, the term will be auto-selected. You don't even need the data integrity issue with multiple cardinality fields for this bug. If the field is not multiple cardinality, you'll get a select list with a "please select" option as well as the single term.

With this MR we now have consistent behaviour when $selected is empty, regardless of whether you are editing or creating, and regardless of the field cardinality.

The code we are changing runs for both multiple and single selects and therefore when we change that code we have to ensure that it works consistently for both. And to make this issue more fun, we even have a situation when a multiple select becomes a single select. This exacerbates the issue because only single selects have the UX problem described in the issue summary - automatically selecting the first option if none is selected. That said, multiple selects have a plethora of other UX issues though, so ¯\(ツ)/¯ ...

Additionally, the current behaviour of the select field when it is required is untested, hence all of the changes to tests being additions.

Re: #93 Interesting points, alexpott. But

Create a taxonomy with 1 term
Create a node type with a required entity reference to this taxonomy with multiple cardinality.
Create a node of this type...

Why? Those are not the steps to reproduce. They would be the STR of a different issue.

If you are saying that the '- Select an option -' fix we got to at one point in the commits breaks the widget when it is configured as multi-select. If so then that would be a regression. Fix one thing, break another. But we could then look at the simpler solution of present a warning message to the user so removing the 'accidental' factor as in 'We did warn you mate but you went ahead and done it anyways!'

In order to only run our code in the case of single select so multiselect does not get broken maybe the approach is wrong? Have we explored all event listener or validation function possibilities?

If what you are saying is that even if we user our fix for the single select our solution will not fix multiselect well turtles on the beach!

You and chum arrive at beach. 15 turtles appear lodged in the rocks. You hatch a plan to save the 15 turtles figure out the order you will do it in, which ones to start with and how to extract each one. You realise it is impossible. You cannot save all the turtles. So you abandon the whole project and go for a swim?

Should you? If you realised you could only save 2 out of 15 turtles would you not go and save them? So if we are fixing the widget when it is set to single select (from data loss: see title) but eg if the same site also uses multiselect for other fields, that is still an improvement. An incremental one. Which is the part of the whole ethos of Agile development.

I am not sure of your overall argument? Are you saying we could save 2 turtles but there are also 15 healthy turtles on the beach and in the act of saving the 2 we will kill healthy turtles? That would be pointless.

But if that turns out to be the case I think a warning message is not going to break anything.

After pondering further, I wonder if what you are getting at @alexpott is that if you apply the fix to the widget you should apply it consistently however the widget is configured.

Inconsistency in UX terms is not helpful.

Scenario:
Site builder creates a vocab. Creates 1 entity ref field for it in an entity configured as single value. Users get used to the 'safety mechanism' of '- Select an option -' if the term is deleted.

12 months later site builder creates another entity ref field for the vocab in the same entity or a different entity configured as multi-select. The same users assume that the field works the same way and when auto-select of term happens on term deletion they save the entity they are editing feeling safe that no term has been deleted (else they would be seeing '- Select an option -'. As developers we would be giving users a false sense of security. The net effect might be that the users lose more data than if we had left things as they are.

Your thoughts @alexpott?

Let's proceed here with the scope @alexpott proposed. (As a core framework manager, alexpott can make decisions about issue scoping that are final unless overruled by a release manager or the project lead.)

Additional tangents about turtles and nuclear armageddon are not helpful and do not bring this issue closer to mitigation.

@xjm This issue was created by @mstrelan. As far as the scope goes @mstrelan made it clear that he and myself are in agreement. @mstrelan suggested that this issue confine get a UX review. I still agree with him on that.

Additional tangents about turtles and nuclear armageddon are not helpful and do not bring this issue closer to mitigation.

Well that is your opinion. I disagree.

What would

bring this issue closer to mitigation

would be alexpott acknowleding the position taken by mstrelan and myself and all four of us compromising to a degree. doxing people is not the answer.

alexpott you waving the white flag? Had enough?

@oily, I am not stalking you and I have not doxxed you. (The term doesn't even make sense in context.)

As a release manager it is one of my responsibilities to help manage problematic contribution patterns.

I sent you a single private contact form message because I'd heard numerous complaints about your long tangents, argumentative behavior, etc. on core issues, from core contributors and maintainers. Others reported that they had repeatedly tried to give you mentoring about how to work on issues, about the contribution policy, etc. without it impacting your behavior, and that it was increasingly difficult to resolve nearly any issue you were involved in.

The next step we take when public mentoring doesn't work is a private warning. (It's better to correct people in private when possible.)

Instead of trying to listen and understand my feedback, you proceeded to reply with two emails (without any further contact from me) and accuse me of harassment and threaten me with legal action. With your comments above, it is therefore necessary to document what actually happened in public.

Thereafter, I made comments on exactly two critical issues that you were disrupting, documenting and explaining the core governance for you.

Regarding:

Well that is your opinion. I disagree.

It's fine for you to disagree, but that doesn't mean you also can also go on doing it. You still should follow issue management guidelines, including recommendations from the core committers, if you wish to participate in core issues.

@alexpott is not "waiving the white flag". This is not a war and he doesn't need to surrender to a tide of pages and pages of text that are not part of attempting to solve a difficult problem. He is a core framework manager attempting to lead solving a complicated critical bug, which is his responsibility within the governance.

I am going to ask, as with the CKEditor update, that you stop commenting on this issue.

@alexpott

Re: #91 you stated

@oily - the bug in the original report happens when there widget is in multiple mode - it is not scope creep. And we have to consider the changes made and how they impact both multiple and single select mode. That is also part of the scope.

I do not understand what you mean. By 'multiple mode' do you mean multiple cardinality? So if the vocab container 3 terms, 'A', 'B' and 'C', the field will contain 3 select boxes and you can select 'C' for the 1st one, 'B' for the 2nd one and 'A' for the 3rd one (ignoring the secenario where 1 or more of the terms 'A', 'B' or 'C' is deleted?

Okay, so I think the problems you have identified are with single cardinality and multiple cardinality where more than one value can be selected (multi-select). So what can make it even more complicated is where there is multiple cardinality and multi-select configured on each. Then when a term gets deleted the effect is pretty chaotic. Hence, your quote about research finding that there are inherent problems with select fields.

So, it does seem like this is a much tougher bug to fix than the single value, single cardinality + term deletion scenario in the IS video and STR. In my last comment on this issue I did observe that we should ensure that the behaviour of the field is consistent.

You did mention that the root cause of this is the deletion of the vocab term. Would be great to see where @mstrelan stands on this. If the user deleting the term could be warned of data loss that might be easier to implement than trying to make the select field change its spots?

@oily if you make an entity reference select list field multiple cardinality (any cardinality greater than 1)... the select changes from a drop-down to a multiple select. As far as I know there is no option to have a single select for each cardinality... how are you configuring that? That feels like it could get extremely messy.

The code we are changing runs for both the single selects and the multiple selects. That's why, regardless of the issue summary, we need to ensure that any changes we make don't make things worse for any way of configuring \Drupal\Core\Field\Plugin\Field\FieldWidget\OptionsSelectWidget. And that's what we've done here. With the current MR there is no auto-select, for either multiple select or single select regardless if the required field has an invalid value or no value. Everything is now consistent and tested.

With respect to warning a user when you delete an entity that is referenced in a required field, that is something that will prove extremely hard to do in core. If you want this functionality I would recommend using entity_usage to achieve this. This MR makes it less surprising when you edit the entity whose reference field is now invalid. It puts the editor back in the position of having to choose rather than having the first available value selected (as if is had that value already). Unfortunately solving the data integrity issue at the time the entity is deleted in core is going be super super hard. This UX improvement will help a little bit.

@mstrelan has also pointed out that entity access can cause a version of this bug. This occurs when a user has permission to edit an entity but not view an entity that is referenced from one of its fields. This issue goes part of the way to fixing this situation because we're no longer auto-selecting but I would argue that we should consider having different messages in #3623828: Add message that previously select value is no longer available depending on whether the reference exists or is inaccessible - but that will need careful consideration - hence it is a follow-up. In many situations, I would argue that a site should be configured to prevent access to editing an entity if the user does not have permission to view the entity in the entity reference field to prevent data integrity issues like this.

@oily it feels like you are commenting on this issue without testing the MR. I feel this because the turtle analogy was made after we fixed, for both multiple and single selects, any possibility of auto-selection. And we've added test coverage for all the situations (single select, multiple select with 1 option and multiple select with more than 1 option). I'm not waving any flags, I just consider the MR complete, well tested and ready.

Found 1 minor issue (missing return on getEmptyLabel), verified the remaining open threads had been fixed and resolved them.

Marking RTBC

Re: #103 Thank you for the detailed explanation alexpott. It may be that we have been using slightly different terms for things.

Way back at #19 this issue was RTBTC by smustgrave. #19 contains before and after screenshots. Those screenshots seem to be still valid. By #103 I take you to mean that the After screenshot is what the current fix does in the case that the term stored for the entity gets deleted. IN which case, I agree there has been a misunderstanding.

When you create a required single-value entity reference field, for example a term reference, then the referenced entity is deleted, the _none option disappears. When the widget is loaded up it defaults to the first option in the list, and the user can easily save the new value without noticing. Steps to reproduce below.

This stuff in the IS needs to be edited and I think the Proposed Resolution, also.

Updated the issue summary.

@oily please refrain from the analogies and metaphors - they add nothing and make it harder for people to contribute either because English is not a first language or they can trigger. I do not come to drupal.org to be reminded of our inhumanity to animals. Screenshots, videos and steps to reproduce are more useful if you believe an issue is taking a wrong direction.

@oily, credit on core issues is granted according to the issue credit policy. Committers assess a contributor’s overall activity on the issue, not individual comments on their own. The test is whether that activity helped move the issue forward.

Looking at your activity here as a whole:

• Your early review (#59–#66) included some useful manual testing.
• Since then, most of your comments have been disputes about scope after the scope had been decided (#84–#85, #94, #97), hypothetical scenarios (#86–#88, #94, #110), and arguing with the feedback you were given about them (#112). A release manager and the community health team both had to step in (#96, #99, #101). The policy specifically excludes rants and off-topic comments from credit.

Taken together, your participation has made this issue harder to resolve, not easier. So the Core Leadership team has agreed not to give you credit on this issue.

If you want your future reviews to earn credit, keep them short and specific: say what you tested, what you found, and include steps, screenshots or code review comments. Leave out the tangents, and once a maintainer has decided the scope, work within it.

Credited

Not credited

Committed and pushed to main, 12.0.x, 11.x and 11.4.x. Thanks!

Moving this back to fixed.

Hoi @oily, I know you are not happy with how the discussion ended. Is not getting no contribution credits for this issue so bad that you need to make the discussion going on? Could you be so kind and drop it. Accept that this did not ended the way you would like to have it ended. Try to be the bigger person. Let’s work together on other issues and make Drupal a better product. It should be fun to work together on making Drupal better. Know that it is easier said than done. I have my own problems in that area. What is another issue you care about and how can I help you with that issue?

Hi oily,
Your story gives me the impression that at the moment it is for you (and others) not fun to work on Drupal core. That makes me sad. It should be fun. Maybe it is a good idea for you to take a break from working on Drupal core. Maybe work on a contrib module or take a break from Drupal for a time. Go and do something else, give it some time and see if working on Drupal core is still something for you. If it is not, then I hope very much that you find something that is fun for you. And if you come back after a break, we shall start fresh.
Again working on Drupal core should be fun for everyone including you. We should be kind to each other. Try to see it from the position of the other person. I hope we shall meet each other in person in the future.

Please stop using this issue to continue the interpersonal dispute, discuss CWG matters, or revisit the conflict with other contributors. This issue has been resolved and should remain focused on the technical work.

Do not change the issue status from Fixed again.

Further concerns about community conduct should be sent directly to the Community Working Group rather than discussed here.

@oily.. Please be aware that @volkswagenchick is speaking as a member of the Conflict Resolution Team and the CWG.