Require `langcode: …` only for simple config that contains translatable values

Wow, it took me ~1 hour to figure out why/how the hell mergedConfigExport was pre-set to some value — turns out I am to blame, due to #2949021: Deprecate schema fallback in ConfigEntityType::getPropertiesToExport → ~6 years ago 😅 Untangling that is out of scope here, so just updating that existing test fixture.

What about things like third party settings which may contain translatable strings even if the original config entity definition does not?

Finally found why I was under the impression that all config automatically got a langcode upon saving: \Drupal\locale\LocaleConfigManager::updateDefaultConfigLangcodes() does that!

And locale_config_batch_set_config_langcodes() means that all default configuration gets updated to have a langcode.

Interestingly, \Drupal\locale\LocaleConfigManager::getTranslatableData() already has logic similar to what LangcodeRequiredIfTranslatableValuesConstraintValidator is adding!

1. CR created: https://www.drupal.org/node/3427629 →
2. Per #5, we now need an update path for all config on a site. All config on a site will have to be checked against its config schema (if it has it) to ensure that langcode is present if and only if it it contains translatable data. For config without a config schema, we should accept either. The update path test should start from a fixture containing one config object that has translatable values but is missing langcode, and one config object that has langcode but has no translatable values. That test should then fail.

The MR is now 99% green. Unassigning to let somebody else push this to the finish line.

#4: very interesting question! 🤔 I think we can do something similar to what we have for \Drupal\locale\LocaleConfigManager::updateDefaultConfigLangcodes() but then performed by the system module whenever modules are installed or uninstalled, to automatically add/remove langcode as needed.

#7: Follow-up question then: Is that complexity really worth not needing a langcode key? ;)

@Berdir: I actually agree with you 😇 IMHO just adding langcode: … to all config is totally fine and reasonable.

But @alexpott disagreed over at #3426429-11: [PP-1] Mark layout_builder.settings fully validatable → 🙈

@Wim Leers can you resolve the phpcs issue, very curious to see the tests run.

@smustgrave See #6, I can't. But 99.9% of tests are green already, see the last full CI run 😄

RE #4 - third party settings are not for simple config and config entities always have a langcode.

I think this issue is valuable because adding a langcode to simple configuration when there's nothing translatable in it is confusing. What does it mean, what happens if I change it. I've already seen a site where people used language overrides to override things that are not translatable. The problem with doing this is the moment you save that config via the UI your overrides will disappear. (Now we're getting on to the subject of validating language config translation overrides - definitely one for the future!)

D'oh, right, I wish I'd thought of that, @alexpott! 😄 Clarified issue title.

Now we're getting on to the subject of validating language config translation overrides - definitely one for the future!

🤓🤓🤓 Is there an issue for this already? 😄

I don't have a ton of complaints here!

But we still need to get everything green, as well as that update path, per #6.

omkar.podey → made their first commit to this issue’s fork.

Alright - I've written an update path and a test for it.

So locally I added a langcode to a random settings config file that didn't have any translation.
Applied the MR and ran the update hook
Hook ran fine and exported config and see the langcode has been removed.

Question on the MR since we are going to 11.x should we not do the trigger_error first instead add the violation.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

I wrote the logic. @phenaproxima was happy with that.

I knew an update path (+ tests) was necessary, but I don't have the time to do that right now. @phenaproxima did 🥳

Then @phenaproxima wrote the update path + tests. I just reviewed it. It looks GREAT! I found only a few things:

1. One comment nit: https://git.drupalcode.org/project/drupal/-/merge_requests/7018#note_295315
2. I think rather than deleting the body of a post-update function we should use hook_removed_post_updates(): https://git.drupalcode.org/project/drupal/-/merge_requests/7018#note_295312
3. We still need the @todo @phenaproxima asked for at https://git.drupalcode.org/project/drupal/-/merge_requests/7018#note_281429

4. P.S.: I also responded to @smustgrave's and @phenaproxima's questions about historical config system oddities 👍🤓

Welp, removing the defunct (unshipped) post-update from dblog makes tests pass, so I think that's about it. All I gotta do is file a follow-up.

Added 📌 Simple config with a `langcode` element, but no translatable elements, should raise a validation error Active to convert the "superfluous langcode" deprecation error into a validation error, and linked it in the code.

Dear committer, see #20 for why it's okay for me to RTBC this — @phenaproxima reviewed and RTBC'd the logic I wrote, I reviewed and RTBC'd the update path+tests he wrote.

Left some comments on the MR

This is now blocking several config validation issues, so tagging accordingly.

I've addressed the feedback in the update path (which is the only part of this patch which I wrote); leaving the rest for Wim to do, so I can re-RTBC those parts.

I agree with all changes here so far.

I responded to all MR threads.

2 MR threads still left:

1. BC dance: https://git.drupalcode.org/project/drupal/-/merge_requests/7018#note_296154
2. one doubt about the batch update @phenaproxima added: https://git.drupalcode.org/project/drupal/-/merge_requests/7018#note_296160

IMHO after those 2 trivial things are fixed, @phenaproxima should re-RTBC. 😊

Okay, I think everything is addressed now. Restoring RTBC as per #26.

Adding issue credit.

Committed 83a53ba and pushed to 11.x. Thanks!
Committed 2c3cb5e and pushed to 10.3.x. Thanks!

I don't think we need release notes for this because the validation system will tell you what to do and the changes are not mandatory for contrib / custom yet.

Wonderful to see this in 🥳 Updated blocked issues.

::validate() should be return typehinted. Opened 📌 Fix return typehinting for LangcodeRequiredIfTranslatableValuesConstraintValidator::validate() Active .

@mondrake Whoops — sorry. That seems a trivial follow-up fortunately :) Reviewed it.

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