Problem/Motivation

Currently it is possible to create a list field without any allowed values in the user interface. This is non-sensical.

Proposed resolution

Make the allowed values textarea required.

Remaining tasks

User interface changes

API changes

Data model changes

Issue fork drupal-2825712

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tstoeckler created an issue. See original summary.

idebr’s picture

Should I have to fill out the allowed values in the textarea if I want to use an allowed values function?

tstoeckler’s picture

If you have an allowed values function, the allowed values textarea is disabled anyway, there should be no change in this case.

idebr’s picture

While that is correct, the allowed values function can only be set after the field has been created. If I plan on using an allowed values function, I would have to enter temporary data in the textarea because it is required.

tstoeckler’s picture

But if you're setting the allowed_values_function via code, you can also remove the allowed_values, right?

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

tim.plunkett’s picture

Version: 10.1.x-dev » 11.x-dev
Issue tags: +Field UX

Utkarsh_33 made their first commit to this issue’s fork.

Utkarsh_33’s picture

Added the required field attribute for Allowed value list in list(float) field type.

Utkarsh_33’s picture

Status: Active » Needs review
Lendude’s picture

I agree with @idebr on this, while it might be sensical for UX, it's a -1 for DX. And it's not hurting anybody if you leave it empty, so I don't see this as helping anybody. It's not like users will be surprised to find an empty select field if they don't supply any allowed values ¯\_(ツ)_/¯

And if somebody is populating these options using something else than an allow values callback (using a form alter or something), they might also want this empty.

Utkarsh_33’s picture

@Lendude if we try to proceed without entering any values in allowed value list for list(float) field type then it cannot proceed and an error pops up as shown in the screenshot attached.We can atleast do this for list(float) field type as i did in #20.
If we don't want it for all the list types we can roll-back the changes in last commit.

smustgrave’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

This seems like something that would need test coverage.

lauriii’s picture

#23 I don't fully grasp how is this UI change regressing DX? If you know how to generate options dynamically, making the field not required doesn't seem like a big ask. I also don't think we should be optimizing for the edge cases, but rather the majority use case. Having list field without allowed values is still valid config and it will be possible to create fields programmatically without the allowed values.

And it's not hurting anybody if you leave it empty, so I don't see this as helping anybody. It's not like users will be surprised to find an empty select field if they don't supply any allowed values ¯\_(ツ)_/¯

It is hurting if you don't know where you are supposed to configure the options. Since it's not required and someone doesn't understand what it's for, they might ignore it and forget that they were presented with this option. If you don't understand where you were supposed to configure the available options, an empty select field could come up as a surprise.

This issue is problematic in particular in combination with #2521800: List key|label entry field is textarea, which doesn't give guidance towards the expected input.

bnjmnm’s picture

Perhaps I'm missing something but wouldn't it be possible to address both the UX and DX concerns with this?

 public function storageSettingsForm(array &$form, FormStateInterface $form_state, $has_data) {
    $allowed_values = $this->getSetting('allowed_values');
    $allowed_values_function = $this->getSetting('allowed_values_function');

    $element['allowed_values'] = [
      '#type' => 'textarea',
      '#title' => $this->t('Allowed values list'),
      '#default_value' => $this->allowedValuesString($allowed_values),
      '#rows' => 10,
      '#access' => empty($allowed_values_function),
      '#element_validate' => [[static::class, 'validateAllowedValues']],
      '#field_has_data' => $has_data,
      '#field_name' => $this->getFieldDefinition()->getName(),
      '#entity_type' => $this->getEntity()->getEntityTypeId(),
      '#allowed_values' => $allowed_values,
      '#required' => empty($allowed_values_function), // <<<<<<< Add this line and required will only be true when no 
                                                      // allowed values function is present.
    ];

If that doesn't take care of it I also think the DX hit is negligible compared to the UX benefit. It's going to benefit a more common use case that better serves a persona who benefits more from the help. It's the kind of system feedback that effectively trains a new user through gradual exposure. The allowed-values-function-using persona that objects to having the field required is less common and likely knows how to address it, (and the allowed values function message could be tailored if there's any concerns they don't).

lauriii’s picture

I missed before that it looks like the field already has a #access property defined based on whether allowed_values_function has been set or not. Because of this, it doesn't seem like there is any DX hit from this, and we don't need to add any additional checks for #required because the field is not visible when allowed_values_function is defined.

Utkarsh_33’s picture

Status: Needs work » Needs review
Utkarsh_33’s picture

bnjmnm’s picture

Status: Needs review » Needs work
Utkarsh_33’s picture

Status: Needs work » Needs review
smustgrave’s picture

Status: Needs review » Needs work

Some new open threads.

lauriii’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
srishtiiee’s picture

Status: Needs review » Reviewed & tested by the community

All the feedback has been addressed. The MR looks good to go.

lauriii’s picture

Adding to #28, I think #4 is actually describing a valid use case where there is hit for DX from this. In this use case developer would create a field config through the UI and then modify the config to add the allowed_values_function. Even in this case the DX hit seem negligible since you could always add a single value to the options that you remove when you edit the config.

BramDriesen’s picture

Status: Reviewed & tested by the community » Needs work

Added one nit-ish comment to the MR.

BramDriesen’s picture

Status: Needs work » Reviewed & tested by the community

That reads a lot better! Thanks @bnjmnm 😉

One uber nit: allowed value or allowed values like used everywhere in the test file for example.

  • lauriii committed 18432dac on 11.x
    Issue #2825712 by Utkarsh_33, lauriii, bnjmnm, tstoeckler, idebr,...

  • lauriii committed 0b49065c on 10.1.x
    Issue #2825712 by Utkarsh_33, lauriii, bnjmnm, tstoeckler, idebr,...
lauriii’s picture

Version: 11.x-dev » 10.1.x-dev
Status: Reviewed & tested by the community » Fixed

Discussed with @Lendude on Slack and he pointed out #1909744: Allow adding predefined lists to the options fields as a follow-up to address #23. That seems like the way to go moving forward. 👍

Committed 18432da and pushed to 11.x. Also cherry-picked to 10.1.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.