Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comment | File | Size | Author |
---|---|---|---|
#24 | Screenshot 2023-05-17 at 1.26.52 PM.png | 371.22 KB | Utkarsh_33 |
#21 | Screenshot 2023-05-17 at 10.42.56 AM.png | 349.6 KB | Utkarsh_33 |
Issue fork drupal-2825712
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:
- 2825712-the-allowed-values changes, plain diff MR !4004
Comments
Comment #2
idebr CreditAttribution: idebr at One Shoe commentedShould I have to fill out the allowed values in the textarea if I want to use an allowed values function?
Comment #3
tstoecklerIf you have an allowed values function, the allowed values textarea is disabled anyway, there should be no change in this case.
Comment #4
idebr CreditAttribution: idebr at iO commentedWhile 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.
Comment #5
tstoecklerBut if you're setting the allowed_values_function via code, you can also remove the allowed_values, right?
Comment #18
tim.plunkettComment #21
Utkarsh_33 CreditAttribution: Utkarsh_33 at Acquia commentedAdded the required field attribute for Allowed value list in list(float) field type.
Comment #22
Utkarsh_33 CreditAttribution: Utkarsh_33 at Acquia commentedComment #23
LendudeI 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.
Comment #24
Utkarsh_33 CreditAttribution: Utkarsh_33 at Acquia commented@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.
Comment #25
smustgrave CreditAttribution: smustgrave at Mobomo commentedThis seems like something that would need test coverage.
Comment #26
lauriii#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.
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.
Comment #27
bnjmnmPerhaps I'm missing something but wouldn't it be possible to address both the UX and DX concerns with this?
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).
Comment #28
lauriiiI missed before that it looks like the field already has a
#access
property defined based on whetherallowed_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 whenallowed_values_function
is defined.Comment #29
Utkarsh_33 CreditAttribution: Utkarsh_33 at Acquia commentedComment #30
Utkarsh_33 CreditAttribution: Utkarsh_33 at Acquia commentedComment #31
bnjmnmComment #32
Utkarsh_33 CreditAttribution: Utkarsh_33 at Acquia commentedComment #33
smustgrave CreditAttribution: smustgrave at Mobomo commentedSome new open threads.
Comment #34
lauriiiComment #35
srishtiiee CreditAttribution: srishtiiee at Acquia commentedAll the feedback has been addressed. The MR looks good to go.
Comment #36
lauriiiAdding 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.Comment #37
BramDriesenAdded one nit-ish comment to the MR.
Comment #38
BramDriesenThat reads a lot better! Thanks @bnjmnm 😉
One uber nit:
allowed value
orallowed values
like used everywhere in the test file for example.Comment #41
lauriiiDiscussed 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!