Problem/Motivation

#2521800: List key|label entry field is textarea, which doesn't give guidance towards the expected input introduced validation on list fields which requires that the keys conform to machine name format, this wasn't previously a requirement. Since list fields are stored as text in the database (i.e. not a reference to the list item definition or anything), you can't just change the keys without introducing data integrity issues with existing data.

We should revert the validation, if not the whole issue.

See https://kevinquillen.com/fixing-list-text-field-data-integrity-issues-dr... for a longer explanation.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3411419

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:

About issue forks

Comments

catch created an issue. See original summary.

catch’s picture

catch
he/him
Primary language English
commented
Title: Revert (some of) #2521800 » Regression from #2521800: using machine name element for ListStringItem breaks with existing data
Status: Active » Needs review

Added an MR, no idea if it's this easy but it might be.

catch opened merge request !5982

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

Can we retain the code and just change the pattern on the #type machine name field?

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Needs work

That's a better idea, but what to? Spaces is the example given in the blog post, but presumably existing content could use more or less anything.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review

Actually thinking about #4/#5 more, if we set replace_pattern to .* or whatever we come up with that will accept any existing data in the field, then the machine name widget essentially won't work - because it's just going to leave whatever input as is.

To make that work, we'd have to leave the regexp as is (or as is plus spaces maybe), but also remove validation for the machine name pattern when there's existing data in the field or something. Maybe that's doable but feels like follow-up material.

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

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

OK @lauriii's changes are implemething the last paragraph of #6 but prove it's actually easy to do whereas I thought it would be a pain. I can't see any drawbacks to that approach, so RTBC from me (and none of my code is in the MR now).

catch’s picture

catch
he/him
Primary language English
commented

Pushed one commit to add an explanatory comment, leaving RTBC since it's just a comment addition.

alexpott credited kevinquillen.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: 11.x-dev » 10.2.x-dev
Status: Reviewed & tested by the community » Fixed

This looks like a good fix.

Committed and pushed 7ca3f9e2e0 to 11.x and 35b420b9ee to 10.2.x. Thanks!

  • alexpott committed 35b420b9 on 10.2.x 
    Issue #3411419 by catch, lauriii, larowlan, kevinquillen: Regression...

  • alexpott committed 7ca3f9e2 on 11.x 
    Issue #3411419 by catch, lauriii, larowlan, kevinquillen: Regression...
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

Neat solution 😍

Status: Fixed » Closed (fixed)

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

cilefen’s picture

cilefen commented

#3416819: Not able to add option with key and label '0' to List string field has a comment mentioning this issue.