Problem/Motivation

As part of #2164373: [META] Untie config validation from form validation — enables validatable Recipes, decoupled admin UIs …, we are going to want to validate language codes as language codes, with their own constraints, rather than as plain strings.

Steps to reproduce

N/A

Proposed resolution

Add a new langcode data type to config schema, which implements certain validation constraints that are specific to language codes.

Remaining tasks

Figure out how to implement the feature and add robust test coverage.

User interface changes

None.

API changes

TBD, but there will probably be at least one new config data type (langcode) and possibly a new validation constraint or two.

Release notes snippet

TBD

Issue fork drupal-3373653

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:

Comments

phenaproxima created an issue. See original summary.

andypost’s picture

Status: Active » Needs review

Added suggestion for one more place

wim leers’s picture

🥳 Posted a first review, but this is looking excellent already!

wim leers’s picture

I just have a single request for clarification … other than that, this is IMHO RTBC already! 🤩

Definitely missing a change record to announce this new config schema type though! 🤓

phenaproxima’s picture

Assigned: Unassigned » wim leers
Issue tags: -Needs change record

Change record! https://www.drupal.org/node/3373721

Assigning to Wim for final review.

phenaproxima’s picture

Adjusting credit.

wim leers’s picture

Assigned: wim leers » Unassigned

Tweaked the change record to be more similar to the one for #2920678: Add config validation for the allowed characters of machine names.

Tightened test coverage in 2 ways (which were the sole remaining concerns I raised in my review). This required adding a single additional line to the config schema definition.

IMHO this is now RTBC 😊

borisson_’s picture

Status: Needs review » Reviewed & tested by the community

This new data type for config schemas is a good addition to make configuration schema's more strict. I had one remark on the pull request but @Wim Leers pointed out that should not be an issue. I'm in the hopes that the last pull request will pass tests so already setting this to rtbc.

lauriii’s picture

Status: Reviewed & tested by the community » Needs work

There's 31 test fails with the MR 🤔

wim leers’s picture

Status: Needs work » Needs review

Mea culpa — forgot a single period! 🙈

borisson_’s picture

Status: Needs review » Reviewed & tested by the community

This time I waited for the testbot to come back green. Back to RTBC.

wim leers’s picture

Priority: Normal » Major
Issue tags: +blocker
Related issues: +#2443991: Allow default_langcode field value to be changed

This blocks #2443991: Allow default_langcode field value to be changed, which is marked Major. Matching priority.

bnjmnm’s picture

Status: Reviewed & tested by the community » Needs work

Setting to NW based on a few items I added to the MR. What I'm asking about is likely pretty trivial so if that proves to be the case, whoever addresses this can switch it back to RTBC without waiting for another NR cycle.

An additional question: are there contrib/custom cases that might result in errors on update? It seems possible since this constraint is applied to the langcode: property of the root configuration object. It gets a "The value you selected is not a valid choice." error, but perhaps at least the CR could include a section on how to most easily address the error if a site encounters it? Ideally it's phrased in a search-engine friendly way for those who don't read change records but get the error.

phenaproxima’s picture

Status: Needs work » Needs review
smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Applied the MR to 11.x
Searched for "label: 'Language code' and all the instances appear to have been captured.
CR makes sense to me.

Think this is good.

penyaskito’s picture

+1 I reviewed that this took into account the custom langcode configuration languages.

larowlan’s picture

Status: Reviewed & tested by the community » Needs work

Asked some questions on the merge request

phenaproxima’s picture

Status: Needs work » Needs review

Back to review!

penyaskito’s picture

Status: Needs review » Reviewed & tested by the community

I'd use reset() instead of key(), but that's too nitpicky for not RTBCing this. All feedback so far has been considered already.

Do we need release notes for this? I don't think so.
Change record? Exists at https://www.drupal.org/node/3373721.

So IMHO RTBC

borisson_’s picture

Issue tags: +ddd2023

+1 on the rtbc.

bnjmnm’s picture

Status: Reviewed & tested by the community » Needs work

Bad news: Back to NW because of a test failure
Good news: looks like the failure might be because this validation works/

phenaproxima’s picture

Status: Needs work » Needs review

Huh, go figure. I guess I forgot to add a couple of special internal language codes.

borisson_’s picture

Status: Needs review » Reviewed & tested by the community

Back to rtbc now that the test coverage looks complete.

bnjmnm’s picture

Status: Reviewed & tested by the community » Fixed

Love to see schemas get smarter and more leveraging of those handy Symfony validation constraints. Committed to 11.x

Status: Fixed » Closed (fixed)

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