Problem

The Select or Other module is manipulating the field configuration by inserting user entered values into the allowed_values configuration of the field. Besides not being a secure, or scalable pattern, this also breaks config deployments as noted in the comments here.

The most common use of Select or Other for us through the years is with a List (text) field type with configured allowed values. Current behavior breaks the field configuration by manipulating the allowed values configuration.

@daggerhart wrote Select Text Value as a possible replacement, but it only supports Text field types.

Original: After submiting the form, the "Other" value is stored as an option in the select.

Proposed resolution

Still thinking this through, but patch at #18 is one direction.

Rather than a completely new widget, put the functionality behind a default widget setting? Display a checkbox that controls the behavior of user entered values and the form options?

In order to replicate D7 behavior by default, my suggestion would be something like…

[ ] Display “other” values for selection?
User entered “other” values will become available for selection on the form.

Remaining tasks

  • Update storage pattern of user entered values
  • Update widget settings UI
  • Update widget to provide selected options based on settings
  • ???
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bogdan.stamate created an issue. See original summary.

legolasbo’s picture

Category: Bug report » Feature request
Priority: Critical » Normal

By design the other values are saved to the field, but I welcome patches (with automated tests) which allow a user to modify this behaviour.

snufkin’s picture

This can be a fairly serious issue though which does not scale too well. For instance if we used select_or_other on a contact form, people entering values would be exposed to subsequent submitters. The number of alternative suggestions would slowly degrade the UX (e.g. the admin has 3 options + the other), after submissions the pre-defined options would get lost amidst the user submitted values.

Additionally none of the newly entered values can be removed by an admin, since Drupal will not allow you to edit the field storage after content has been entered, which can easily cause issues when e.g. swearing is added as an option.

legolasbo’s picture

I understand your concerns snufkin, which is why I would like to quote part of my comment in #2.

I welcome patches (with automated tests) which allow a user to modify this behaviour

Cameron Tod’s picture

I think one approach to this would be to create a second field, <field_name>_other, which is automatically shown if 'Other' is selected, using the states API.

gbirch’s picture

I understand @legolasbo 's request for contributions on this issue, and will do if I have time, but note that until this issue is fixed, the module is simply not workable in the vast majority of use cases I have met. It may be good for entity reference behavior, but certainly not for text.

joachim’s picture

> I think one approach to this would be to create a second field, _other, which is automatically shown if 'Other' is selected, using the states API.

That's what I had to do manually for a site last month, as the module's current behaviour just wasn't viable.

There's a UX problem, but also security: you might not want users to be able to see the 'other' options other users have entered.

joachim’s picture

> I think one approach to this would be to create a second field, _other, which is automatically shown if 'Other' is selected, using the states API.

Thinking about this some more though, this approach only worked because on my project we were making forms that collected data, without much by way of display output.

Having two fields will cause all sorts of problems with field display, and views filtering and sorting, because you're going to have to mash the two together to look like a single field.

An alternative approach would be to change this widget to be for normal text fields instead of list text fields. The list of options would then be a configuration setting in the widget, and the field's storage would allow you to store anything.

The downside there is that you couldn't start off with a list text field, and then decide you wanted select or other functionality on it later (at least, not without migrating your data to a new field). So there's a loss of architectural flexibility there, but I would say overall a gain because you have a single field.

bmcclure’s picture

I agree with joachim's last recommendation. I think making this work on string fields is the way to go.

Having the allowed values be a part of select_or_other's configuration and allowing it to work on string fields potentially opens up a whole new array of use cases for the module.

The door being closed by not using a list field doesn't seem too terrible, considering right now I struggle to see much of a use case for having a list field that users filling out the form can add allowed values to freely for other users to choose from. If there is such a use case, I'm not sure people will know to look in the "Select or Other" module to find it anyway.

But a third option would be to allow it to work like it does now on list fields, and also give it its own allowed_values config when applied to a string field. It seems like that could support almost every use case.

daggerhart’s picture

I spent about 9 hours unsuccessfully trying to get this module to meet the expectations as laid out in this issue. So instead, I created a new module that takes a fundamentally different approach to a "select or other" widget. It is for plain and formatted text fields.

https://www.drupal.org/project/select_text_value

Dubs’s picture

I'm guessing the "By design" part comes from requiring the validation checks to pass on the form.

In the performRequiredValidation function (core/lib/Drupal/Core/Form/FormValidator.php) the code checks against the element to see if there is #options and #value. #options does not contain the other text box amount, but #value does, so it displays the "Illegal choice" error message.

To workaround this the valueCallback function (select_or_other/src/Element/ElementBase.php) adds the #value to the #options array. This is good as the validation check passes for this element. But if the form validation fails for another reason then we end up with the text box value added to the array of options which is displayed when the form re-renders.

It looks like a some big changes would be required to fix this as we can't override the whole form validation for this one element and the only way to make the validation pass would be to NOT store the input in the #value field. I haven't checked much - there might be a way to override JUST the validation check for this element and then it would be easy to override that function and exclude the check.

Unfortunately I couldn't invest the time into this (we have a very tight delivery date on this project) so I used radios and a textbox with states.

weseze’s picture

Also important to note that anyone using a deployment workflow that includes a config import from a code repository will not work when using this module because this module changes configuration on-the-fly...

I consider this a bug, not a feature request.

timotejl’s picture

Would it be good enough for Text Lists to prepend the string key with let's say '~' and then just filter out all the values that start with '~'?

hanoii’s picture

I just stumbled upon this. Having this on the config is starting to cause me issues with deployment now that tests are content are more frequent.

What an odd decision, I wonder how/why it was done like this.

I am just thinking out loud here, but what if we store this on some other table (or even config if it makes it simpler initially) and feed the select through config overrides on the module? https://www.drupal.org/docs/8/api/configuration-api/configuration-overri... (Providing overrides from modules)

At least the main config would remain the same

hanoii’s picture

Otherwise https://www.drupal.org/project/config_ignore_keys or the like might be an option

safetypin’s picture

It looks to me like the d7 functionality is that a separate record is saved for the 'other' text field, saving the value entered there in the database column for the extra values. Then when the entity is edited, that value (whatever it is) is loaded as the value of the text field, and the 'other' option is automatically selected.

Are we certain that this isn't possible? If so, then perhaps this functionality should be provided by an entirely new field type rather than a field widget.

chriscalip’s picture

Possible to replicate d7 functionality without too much deviation from the current codebase and decisions.
A possible path is for select_or_other widget to have settings original_allowed_values.

Node Add Form, will ALWAYS only have checkboxes values from the original allowed list.
Node Edit Form, will ALWAYS only have checkboxes values from the original allowed list. In addition on the anything NOT IN original allowed list to be on the Other textfield.

chriscalip’s picture

Proof of concept patch.

The approach is to let editors/admin ONLY show list items from a curated widget settings. This approach simulates d7 style without too much deviation from the current codebase and decisions.

Highlights:

a.) Introduction of src/Element/FilteredButtons.php which has an additional property #other_default_value
b.) Introduction of src/Plugin/Field/FieldWidget/FilteredListWidget.php Its a field widget that has a method getOptions which takes into consideration widget setting widget_allowed_values
Furthermore this has logic to put in field value NOT IN widget_allowed_values as other_default_value.
c.) For discussion: src/Plugin/Field/FieldWidget/WidgetBase.php has hard coded setting widget_allowed_valuesI believe FieldWidget should be able to override method settingsForm
d.) Probable followup: js/MultipleSelectStatesHack.js hide logic needs to take into consideration possibility of other_default_value.

jason.bell’s picture

Title: Other option is saved in the actual select » User entered "other" value is being saved to and overwriting the field configuration
Issue summary: View changes
Related issues: +#2875493: Data is removed from the database when the "Select or Other" "Radiobuttons/Checkboxes" widget is used.

I’m with @weseze and consider this a bug. It breaks the expected behavior from D7 usage but does not clearly specify that in documentation. My preference would be to see the previous behavior be the default. The current state might have excellent use cases, but should be a configurable option.

Rewrote the issue summary a bit to further clarify and start discussion on a path forward.

cgoffin’s picture

I have given it a go with the configurable widget setting to save the other value or not. I don't know if it's a good implementation, but it does the trick.

cgoffin’s picture

The previous patch was started from the wrong branch. Here is the adjusted version. The previous patch can be used in combination with issue How can we change the 'Other' field title or label

Dubs’s picture

This patch looks good and works for me. Thanks @cgoffin for the patch :-)

Sutharsan’s picture

@legolasbo, Two different approaches are now suggested. Can you choose a direction for this issue before we burn too much fuel of it.

tsmorisawa’s picture

I see that there is still some debate on which approach to take, but we also had success with the patch provided by @cgoffin. We did notice that with this patch, the value saved for "Other" was not filled in as a default value when the node was edited, so I have included a patch to resolve this. I have also updated the patch for the latest dev release.

Please let me know if there are issues with the patch, as it is my first attempt at creating one.

puddyglum’s picture

For us #21 was more successful than #18, and it appears to be working with #2894033: How can we change the 'Other' field title or label as well. The other values do save properly, but when you edit the page again the Other field is missing. I am in favor of going with #21 and fixing the issue with the Other field not being populated.

tsmorisawa’s picture

Updated patch to fix a few test failures and coding standard issues.

If my original post at #24 is still hidden, I mentioned that this patch should fix the issue mentioned in #25, along with updating it for the latest dev release.

EDIT: Thank you gisle for publishing the comment.

gisle’s picture

If my original post at #24 is still hidden.

It was mis-detected as spam unpublished by our too vigilant spam filter. I've published it. You now have the "Confirmed" user role so it is less likely that this will happen again.

puddyglum’s picture

I've tested the widget from #26 and it works for me. Attached patch fixes test failures and has the following changes:

- Added schema config for new "Add other value to allowed values" setting
- Changed default value for "Add other value to allowed values" to TRUE, to avoid impacting existing behavior
- Added a test for when this setting is FALSE
- Reverted code in function that saves values. Values were not saved properly during the tests when value was TRUE.

Question for consideration:
Should the "Add other value to allowed values" setting be FALSE going forward?

Andrew Answer’s picture

Status: Active » Needs review

I confirm what if "Add other value to allowed values" is unchecked, control saves the "other" state on submit, show textfield and fill it with stored value. But when checkbox is checked, I don't see adding newly entered value to select options.

puddyglum’s picture

@andrew-answer I re-tested this patch against the 8.x-1.x-dev branch with a new Drupal install.

Add to allowed values  Select element    # Values   Added to allowed values
Checked (TRUE)         Select list       1          Yes
Unchecked (FALSE)      Select list       1          No
Checked (TRUE)         Select list       Unlimited  Yes
Unchecked (FALSE)      Select list       Unlimited  No
Checked (TRUE)         Checkbox/radio    1          Yes
Unchecked (FALSE)      Checkbox/radio    1          No
Checked (TRUE)         Checkbox/radio    Unlimited  Yes
Unchecked (FALSE)      Checkbox/radio    Unlimited  No

It appears to be working correctly for me.

francoud’s picture

I wasn't able to apply the patch #28 with PHPStorm (didn't applied completely, don't know why) but finally I applied manually (!) and, yes, it seems working!

Gold’s picture

Status: Needs review » Needs work

The patch at #28 applies nicely. If you are just trying to prevent the user entered 'Other' value from being applied to the Select options then this will work well for you.

If you are *wanting* the user entered value to be added though the UX needs a little work. On a form with a form error the value is added to the select list and is left unticked, "Other" is still ticked and the user entered value is present in the Other text field. Once the value is added to the Options should the form not have that value checked, the Other checkbox/radio unchecked and the Other text field cleared?

tsmorisawa’s picture

@Gold, it looks like mentioned issue exists currently in both 8.x-1.0-alpha4 and the latest dev version without the patch. I suspect it might be a combination of Drupal saving the previously entered form field values as is when submitting a form with a validation error and Select or Other adding the value to the allowed options list on first submission. Notably, correcting the validation errors and submitting the form successfully will correctly set the field's value to the newly added option, although I agree that it is a little odd that it gets added before successful submission.

Maybe this can be split out to a separate issue? We have had no issues with the patch in #28.

francoud’s picture

as far as I can see, patch #28 is not working when the field is into a Paragraph. This makes the problem a little more complicated...

Will this problem ever be solved in a standard release?
Ignore this. #28 works perfectly also on Paragraphs.

tsmorisawa’s picture

Status: Needs work » Reviewed & tested by the community

It seems like there are no unresolved issues with the patch in #28 itself at this time. We have been using this patch successfully in production for a few months with no issues. As a result, I would like to mark this as RTBC.

  • legolasbo committed 3d5a078 on 8.x-1.x authored by jmonkfish
    Issue #2694251 by tsmorisawa, cgoffin, jmonkfish, chriscalip, legolasbo...
legolasbo’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x-1.x. Thanks!

Status: Fixed » Closed (fixed)

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

pub497’s picture

Component: Form elements » Code

Is this issue back? I'm seeing the same behaviour with version 4.0 ... entered values are being saved into the allowed values list, breaking any site configuration.

rootwork’s picture

@pub497 I'd open a new issue and reference this one; closed issues are unlikely to get much attention.