D8 issue: #1180992: [D8] list_field_validate() doesn't seem to accommodate select lists with <optgroup>

This is an interesting bug.
I'm using the Inline Entity Form to edit line items.
So I'm editing a shipping line item, and the Shipping service field always falls validation with an "invalid value" error message.
The problem is that "allowed_values_function" returns an array with optgroups, so it has an extra level.
However, list_allowed_values() doesn't take that into account, and just does an isset on the first level, so of course it fails.
Now that I've written this, it sounds like a core bug, but it's late and I'm content with just documenting this in an issue for now.

CommentFileSizeAuthor
#13 1530486-13.patch1.62 KBpoker10
#13 1530486-13_test-only.patch767 bytespoker10
#2 core-fix_for_optgroups_in_options_list-1530486-2.patch886 byteschimericdream

Comments

bojanz’s picture

bojanz commented
Title: Can't change shipping service field value » List fields with optgroups and "allowed_values_function" fail validation
Project: Commerce Shipping » Drupal core
Version: 7.x-2.x-dev » 7.x-dev
Component: Code » field system

Let's see what the core contributors think.

chimericdream’s picture

chimericdream commented
Title: List fields with optgroups and "allowed_values_function" fail validation » list_field_validate() doesn't seem to accommodate select lists with <optgroup>
Status: Active » Needs review
StatusFileSize
new core-fix_for_optgroups_in_options_list-1530486-2.patch886 bytes

Changing title to match 8.x-dev bug report of this issue and backporting the patch from 8.x-dev.
See: https://drupal.org/node/1180992

swentel’s picture

swentel commented
Status: Needs review » Closed (duplicate)

No need to have 2 issues with the same name. It needs to be fixed first in D8 anyway. You can post this patch over there, but leave the issue itself on 8.x because it needs to be fixed in that branch first.

swentel’s picture

swentel commented

Sorry forget the link to the other issue: #1180992: [D8] list_field_validate() doesn't seem to accommodate select lists with <optgroup>

chimericdream’s picture

chimericdream commented

I think it makes more sense to make bug fixes in the current version first, then incorporate them into future releases. What is the rationale behind fixing next year's version while letting known bugs (especially ones with working patches) sit in the version people are using now?

chimericdream’s picture

chimericdream commented
Status: Closed (duplicate) » Needs review

#2: core-fix_for_optgroups_in_options_list-1530486-2.patch queued for re-testing.

simon georges’s picture

simon georges commented

Closed #1012640: Support grouping in allowed values for select list as a duplicate of this one.

m.stenta’s picture

m.stenta
he/him
Primary language English
commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

This patch has been working for me in production for over a year. Marking this as RTBC...

stefan.r’s picture

stefan.r commented
Status: Reviewed & tested by the community » Needs review

Is this in Drupal 8 yet? If not it needs to go in there first

drumm’s picture

drumm
he/him
Location NY, US
commented
Issue summary: View changes
Issue tags: +affects drupal.org
Related issues: +#1180992: [D8] list_field_validate() doesn't seem to accommodate select lists with <optgroup>

It is not in D8 yet, #1180992: [D8] list_field_validate() doesn't seem to accommodate select lists with <optgroup>

dww’s picture

dww
we/he/they
commented
Title: list_field_validate() doesn't seem to accommodate select lists with <optgroup> » [D7] list_field_validate() doesn't seem to accommodate select lists with <optgroup>

Not clear why this and #1180992: [D8] list_field_validate() doesn't seem to accommodate select lists with <optgroup> are separate issues. Seems like this one is duplicate. But for now, at least prefixing them with core major version so we can tell them apart.

dww’s picture

dww
we/he/they
commented
Status: Needs review » Reviewed & tested by the community

Been testing this more locally, too. Solves the problems. Doesn't seem to cause any regressions.

I'm not sure how D7 handles test coverage for bug fixes like this. Tentatively setting to RTBC in the hopes this can make it into the next D7 core release. ;) If we need tests, please add the 'Needs tests' tag.

Thanks!
-Derek

poker10’s picture

poker10 commented
StatusFileSize
new 1530486-13_test-only.patch767 bytes
new 1530486-13.patch1.62 KB

Patch #2 looks good, we are also using it.

I think that adding a test for this is simple, as the allowed_values_function is already being tested with the list_test_allowed_values_callback(), which uses optgroups. Unfortunately only the first level key is being tested, so simpletest did not fails in that test. I have added additional test case to the patch from #2 and reuploading it with test only version. Patch itself is unchanged.

Also we do not need to wait for D9 issue anymore, as it would not be commited. Similar code is already in D9, see: https://git.drupalcode.org/project/drupal/-/blob/9.5.x/core/modules/options/src/Plugin/Field/FieldType/ListItemBase.php#L50.

The last submitted patch, 13: 1530486-13_test-only.patch, failed testing. View results

mcdruid’s picture

mcdruid commented
Issue tags: +RTBM

Tests confirm that this looks good, thanks!

  • poker10 committed bf07341 on 7.x 
    Issue #1530486 by poker10, chimericdream: [D7] list_field_validate()...

poker10 credited arski.

poker10 credited carlmcdade.

poker10 credited KarenS.

poker10 credited marcvangend.

poker10 credited yched.

poker10’s picture

poker10 commented
Status: Reviewed & tested by the community » Fixed
Issue tags: -RTBM

Thanks all!

Added also credits from the duplicate issue: #1012640: Support grouping in allowed values for select list

Status: Fixed » Closed (fixed)

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