New config schema data type: `required_label`

Taking this on because nobody seems interested 😅

Tests are pesent since 8fc9f0b7.

I just pushed 537e40d78cb8457979a12169f4bcfe823ede4d06.

Well well now we find ourselves in a pretty much impossible position:

1. According to \Drupal\menu_ui\MenuForm::form(), description is optional.
2. But according to \Drupal\system\MenuInterface::getDescription(), the return type is always string, not string|null, so that implies it's not optional.

This means that the reality is that the empty string is in fact treated as an acceptable value 🙈

The same pattern applies to:

1. ContactForm's \Drupal\contact\ContactFormInterface::getMessage()
2. Vocabulary's \Drupal\taxonomy\VocabularyInterface::getDescription()

This leaves only 3 simple solutions (more complex solutions are always possible!):

1. ❌ truly make it an optional value, but this requires a BC break for MenuInterface::getDescription() (and a number of other classes) → not acceptable
2. introduce an additional config schema type because a description/message is conceptually not at all a "label", but just an optional translatable long string/piece of text
3. switch to the Length constraint.

The second choice is IMHO the best one. But it technically constitutes a data model change, even though virtually nobody is using config schema. The third choice is arguably the least disruptive, but also makes config schema validation less meaningful. The second choice is theoretically disruptive but clearly moves the entire config/config schema system to a place where all data & metadata becomes more meaningful.

Because the disruption is so highly theoretical, I'm for now going with option 2. And actually … the needed config schema type already exists:

# Human readable string that can contain multiple lines of text or HTML.
text:
  type: string
  label: 'Text'
  translatable: true

Did that in 5630efacd75a70cb115bd3feae4b198a7e374bf2.

The failures in \ValidKeysConstraintValidatorTest, are failing because the
default system.site configuration has slogan: '' and name: '', but both are of type: label, so those trigger validation errors too.

In the case of name, it's only triggering a validation error because kernel tests bypass the installer (which would require the user to enter a site name).

In the case of slogan, the use of type: label is simply inappropriate.

Fixed in eb066c37002c7010e5157271056ad99b4b1c2461.

This should now be green! 🥳

Will update the issue summary tomorrow, calling it a day.

Root cause for remaining failures: config_translation config schema integration

Determined the root cause: #2144413: Config translation does not support text elements with a format's logic combined with config_translation_config_schema_info_alter() not documenting reality 😬

This MR indeed makes the site slogan not show up in the translation UI:

(Left = MR, right = HEAD.)

--- a/core/modules/config_translation/config_translation.module
+++ b/core/modules/config_translation/config_translation.module
@@ -181,6 +181,7 @@ function config_translation_entity_operation(EntityInterface $entity) {
 function config_translation_config_schema_info_alter(&$definitions) {
   $map = [
     'label' => '\Drupal\config_translation\FormElement\Textfield',
+    'string' => '\Drupal\config_translation\FormElement\Textfield',
     'text' => '\Drupal\config_translation\FormElement\Textarea',
     'date_format' => '\Drupal\config_translation\FormElement\DateFormat',
     'text_format' => '\Drupal\config_translation\FormElement\TextFormat',

makes the test pass but actually makes things worse:

(Left = MR, middle = HEAD, right = MR + aforementioned one line change.)

But config_translation_config_schema_info_alter() said:

Other translatable types will appear as a one line textfield.

… apparently that is really to mean that only config schema types can be marked translatable: true; a concrete config schema apparently is not allowed to specify it! Or rather, it's allowed (no validation error occurs), but it's not supported by Config Translation. Whether that's intentional or accidental is unclear. I suspect it's intentional.

Turns out that there's at least two crucial follow-ups that never happened:

1. #2346195: Document configuration schema with a dedicated @config_schema_api @defgroup
2. #2275865: Per language settings (vs translated settings) are not directly supported

Proposed solution

Given that it appears intentional, the solution is actually fairly simple:

1. Introduce type: translatable_string, in addition to type: label and type: string. Change record created: https://www.drupal.org/node/3349638
2. Update the site slogan to use this type instead.
3. Follow-up to trigger validation error for config schema declaring translatable in concrete config schema, since it is only allowed in type definitions.

Just did that in d394d6f09f1705b925da689970e3783f44d85680 👍

Follow-up created, including patch: #3349656: Config Translation only supports `translatable: true` at the top level, but has no validation to indicate this.

We will need a change record to announce that new type, so adding the tag for that.

Did that yesterday already: https://www.drupal.org/node/3349638.

Will update the issue summary later today.

While working on the issue summary update, I noticed/realized that #11 should be applied to all places where { type: label } was being changed to { type: string, translatable: true }, not only for the site slogan (which happens to be the one thing with test coverage).

So: applying it to those too.

• Merged in all upstream 10.1.x changes
• Updated issue summary

Fixed markup.

#19: thank you! 😊 🙏

Agreed. Created #3357126: Test to assert every config property path that has `translatable: true` can be translated using the UI for this.

This is a great feature, but I'm confused -- ConfigEntityBase has no validate(), so where will config entities get validated?

Correct. Nothing is using it yet. Because not a single piece of configuration is 100% validatable by config.

See #2164373: [META] Untie config validation from form validation — enables validatable Recipes, decoupled admin UIs ….

Had a long discussion with @alexpott in Drupal Slack about this.

His initial reaction:

That is one disruptive change :)
I feel that we should be deprecating label and introducing two new schema types
I agree that empty labels are a big issue

My initial reaction: Why is it disruptive? nobody is validating things today! — sad laughter followed.

His follow-up question was So how to we tell people to move from label to translatable_string … another great question. I started responding. And that's when I realized that telling people to move from type: label to type: translatable_string is difficult to convey. Plus, there are cases where things are labels, but they are optional: the prefix and suffix settings on e.g. integer fields. They both default to the empty string (see \Drupal\Core\Field\Plugin\Field\FieldType\IntegerItem::defaultFieldSettings()).

That's why the MR changes their config type:

@@ -705,10 +713,10 @@ field.field_settings.integer:
       type: integer
       label: 'Maximum'
     prefix:
-      type: label
+      type: translatable_string
       label: 'Prefix'
     suffix:
-      type: label
+      type: translatable_string
       label: 'Suffix'

But … that's where I realized @alexpott was right from the very beginning: it’s easier to flip things around:

• keep label unchanged: do not add validation constraints to them
• instead, add a new required_label, which all code is then free to adopt

So then we end up with:

1. string = string, NOT translatable
2. label = string, translatable
3. required_label = string, translatable, REQUIRED

(Because optional labels still have a use case, for example this prefix/suffix thing.)

@alexpott on that conclusion: That looks good

Let's do this! 💪

Wow, that was a lot more change. But likely reduces future disruption. 👍

(None of the disruption would have happened immediately because as @joachim asked: "what would the impact be?" → none for now, since config validation is not used anywhere, because not a single piece of configuration has 100% validation constraint coverage.)

Having done this …I now do wonder if @alexpott was on to something when he pondered whether we should deprecate type: label: Would it be clearer/simpler if we deprecated type: label and forced everyone to choose between a new type: optional_label vs type: required_label? 🤔

I also need to update the CR.

Turns out the test failures were due to a stupid mistake I made in the commit in which I changed from the type: translatable_string approach to the type: required_label approach that @alexpott recommended: I forgot to restore translatable: true 🙈😬

Also, #3349293: Make assertions using ConfigEntityValidationTestBase::assertValidationErrors() clearer landed since this merge request was last updated, and that changed ::assertValidationErrors() slightly, adjusting for that too.

To update the impact measurement, I applied #3324984-29: Create test that reports % of config entity types (and config schema types) that is validatable, but then I had to apply a relative patch to update expectations. That's attached. Issue summary updated.

Change record updated.

Addressed @phenaproxima's concern from ~4 months ago on the MR thanks to the pivot in #25 + #26. Opened #3377030: Add validation constraint to `type: label`: disallow multiple lines to address that instead, to keep the scope of this issue clear.

Merged in origin/11.x now that #3361534: KernelTestBase::$strictConfigSchema = TRUE and BrowserTestBase::$strictConfigSchema = TRUE do not actually strictly validate landed: very curious to see if this triggers new test failures, just like @borisson_ said in #35… 🤓

Actually, this triggered many hundreds of kernel test failures. Yay for #3361534: KernelTestBase::$strictConfigSchema = TRUE and BrowserTestBase::$strictConfigSchema = TRUE do not actually strictly validate doing what it was intended to do!

Patterns:

1. NodeType::create(['type' => 'someting'])->save() — i.e. without the label key ('name')
2. Same for CommentType ('label')
3. Same for ImageStyle ('label')
4. Same for View ('label')
5. Same for EntityViewMode ('label')
6. Same for DateFormat ('label')
7. et cetera.

This is not surprising, given that we've specifically made config entity labels required. Which is the correct thing to do because config entities' annotations specifically indicate that the label is required.

I've done a first (very big) batch of fixing these. This is an example of how config validation helps makes test A) more reliable, B) more representative of real sites.

This also makes it clear that this would be fairly disruptive for contrib/custom modules' test coverage: NodeType::create() is likely to be fairly common there too. The fix is obviously trivial though … but still, it has a wide (but simple) impact. So tagging Needs release manager review. This sort of touches upon #3361423: [meta] Make config schema checking something that can be ignored when testing contrib modules — see the if (self::convertViolationsToDeprecation($this->schema->getRoot())) { snippet in that PoC MR to see how that could work based on any combination of conditions: core vs contrib/custom, test vs non-test, Drupal core version-dependent, …

Unassigning, because switching to other work before starting the weekend 🤓 This is definitely available for others to continue though! 😊 Otherwise, will continue this next week!

EDIT: those commits made test results go from 639 failures to 531 👍

Fixing tags.

😬

Great progress here by @phenaproxima! 🤩

In Slack, he raised:

phenaproxima

We need to talk about the required_label thing. Tests are failing because certain things are set as required_labels which probably shouldn’t be. As an example — block labels. It makes sense for these to be optional. Same with layout section labels.

I can easily revert those changes to make tests pass, but my point is that we have to apply required_label somewhat judiciously. :slightly_smiling_face:

wimleers
@phenaproxima That’s fair. I just wanted to make it a nice & simple rule. But for blocks and layout sections it probably makes far less sense indeed :+1:

I can add one more to that list: Views. Because it has this logic in View::label():

  public function label() {
    if (!$label = $this->get('label')) {
      $label = $this->id();
    }
    return $label;
  }

That already ensures Views' admin UI remains usable. 👍

👍 That works for me! 😊

Then let's add some logic to \Drupal\views\Entity\View::label() that detects the absence of a label and triggers a deprecation error, and says that falling back to the ID as the label will be removed in Drupal 11?

https://git.drupalcode.org/project/drupal/-/merge_requests/3690/diffs?co... changes the update path fixture. Let's not do that. Let's revert the config schema change for field.value.string:value instead.

It kinda doesn't make sense that '' (the empty string) is a valid default value because that's meaningless … but it's also just harmless 😊

That also means less disruption and more focus in this issue. In the future, every single field type can choose to make things more strict! 🤓

268 files changed. 😳😳😳 🤯 I think that this means it's nearly impossible to land this issue… 😅

Now, don't get me wrong: every single one of the view.*.yml updates you made is valuable and not in vain. But I think that it's unreasonable to expect a core committer will sit through and read all of this in detail.

IMHO the more reasonable approach is to ignore certain patterns, just like we did in #3361534: KernelTestBase::$strictConfigSchema = TRUE and BrowserTestBase::$strictConfigSchema = TRUE do not actually strictly validate with validation constraint messages, and to then defer the concrete fixes to follow-up issues (i.e. #3362457: Fix all PluginExistsConstraint constraint violations in tests, #3362456: Fix all ExtensionExistsConstraint constraint violations in tests etc.)

That way, this issue would be able to land much sooner. However, that infrastructure does not yet exist. I'm introducing it over at #3364109: Configuration schema & required values: add test coverage for `nullable: true` validation support, see https://git.drupalcode.org/project/drupal/-/merge_requests/4092/diffs?co... and some subsequent commits.

Using git diff 5052fcddde7744bd1f78f21bbcfe0a502bd44cd0 HEAD -- **/views.view.*.yml it's easy to revert (and extract!) virtually all the Views changes programmatically. The remainder I checked manually. That brings it from 268 changed files to only 166. 👍

So let's bring that infrastructure here, so that I can defer the gazillion View config entity updates to a separate issue. Also because the upgrade path is an important question, as @borisson_ points out in #47. That'd be another can of wriggly, scary worms 🪱🥫😅 Nope, this is not really a concern, since 100% of the modified config entities are test config entities only: if they had been created through the UI, they would have had labels. Verify that ONLY test config is being modified by grepping the patch for config/install and config/optional and … 0 matches are found! 👍😊

One more non-trivial change is being made: to the Search module's migration path. So, extracting that next.

We definitely needed to revert the Search-related changes because MigrateSearchPageTest was not receiving updated test coverage as part of this — which means the existing test coverage is inadequate 😅

(Well, SchemaCheckTrait was triggering a validation error, but that's not explicit test coverage for the now more complicated migration logic. Which is exactly what made this hard to review, and what will be simple to review in a follow-up issue!)

Done.

As far as I'm concerned, this is now RTBC. But I worked too much on this issue to RTBC it 😇 Will create the follow-ups for Views & Search as soon as @phenaproxima +1s what I'm proposing in #48 and here.

If you have the bandwidth, could you create those follow-ups? 🙏 I'm dealing with a lot of other config schema validation issues 😇

100% agreed that we should do #3379899: Follow-up for #3361534: config validation errors in contrib modules should cause deprecation notices, not test failures.

I don't think this needs to be blocked on #3379899 though, because that one is trivial to add, and does not conflict with this at all. But, from a release manager POV that sequencing makes things simpler, so let's postpone this on that one. 👍

Thanks, @catch! 😊

#59: yay! 😊 Merged in upstream, only a trivial conflict in SchemaCheckTrait. 👍

#58:

• Yep, I hear you! That's what the Data model changes section in the issue summary covers. Expanded that part to refer to your comment + example 👍
• That's a great question! The answer is multi-faceted:
  1. YES, every config entity in Drupal core that supports labels, will now be required to have a label. The sole exceptions are listed in SchemaCheckTrait::$ignoredPropertyPaths, i.e. View and SearchPage config entities and they have follow-up issues: #3380475: SearchPage entities implicitly require labels: make explicit, and fix migration from D7 and #3380480: Views should require a label, rather than falling back to an unhelpful ID. There, we will determine what the appropriate course of action is. Rationale: every config entity type in Drupal core that has a UI supports labels, to be able to provide a good admin UX. Which in turn implies the label should be required for that good admin UX to actually be possible. (Note that some config entity types have never supported labels, for example RestResourceConfig.)
  2. Because that will now be the expectation for all core config entity types except for 2, the disruption to contrib modules would be enormous if #3341682: New config schema data type: `required_label` hadn't landed first (thanks @lauriii!). But thanks to that landing first, every contrib module creating any config entity in any test will now get a deprecation notice triggered (except for View and SearchPage, due to the above).
  3. This issue is not changing \Drupal\Core\Config\Entity\ConfigEntityBase, which means the inherited \Drupal\Core\Entity\EntityBase::label() still applies, which means that according to the type hint, a return value of null is still allowed. We cannot change this, that would be a BC break. We can consider changing that in the future, but it'd be a significant change. And would break many config entity types.
  4. For comments 10–13 in #3324506: Group relationship type no longer has a label, leading to PHP warnings all over the place, I'd say the proper solution is to modify ConfigEntityBundleBase (introduced in #2261369: Introduce a common config entity base class for all bundle config entity types) and make labels required there. Because those are not just any config entity types, they're used as bundles for some entity types, and then it indeed makes sense to force implementors to provide a label, otherwise anything entity-reference-related is unable to provide a good admin UX. Also see \Drupal\Core\Entity\EntityType::getBundleLabel(), which makes it even more explicit that it's a built-in assumption that entity bundles need to be able to have human-readable labels. Furthermore, there already is a no_ui key-value pair for the @FieldType plugin annotation, which is dealing with exactly this use case, and it's also in the Entity/Field API space. So I suggest creating a new issue, propose allowing entity bundle config entity types to have a no_ui annotation, and reference these related issues 😊 But all of that is for sure out of scope, because:
  5. This issue is not changing ANYTHING wrt infrastructure in Drupal core assuming the presence or absence of labels on config entity types! See the first point in this list: this is only making it required for config entities that explicitly already support it 😊 There is an easy way to see this: this MR is NOT modifying the type: config_entity config schema data type in core.data_types.schema.yml, it's only modifying concrete config entity types' schema definitions.

Thanks for adding the links, @phenaproxima, but you seem to have accidentally merged in an older version of this branch/MR, causing all the views.view.*.yml changes to be reinstated 😅 And also a merge of origin/11.x that is identical to what I had pushed a day prior. I went back to 0cd5b9a7 and cherry-picked the todo-linking commit to get back to a simpler history.

This is one of the many symptoms why I am not so sure that this GitLab issue fork + MR workflow truly is simpler than patches. 🙈 I've made similar mistakes in the past, because git sometimes just gets in the way. If we'd be able to rebase on upstream instead of merge in upstream and GitLab's UI would suggest to go to the newly pushed version of the same commit, then it'd be a lot simpler.

Next: addressing @lauriii's other comments.

@lauriii asked some excellent questions, but my git archeology shows that the changes are correct 😇

I would be totally fine moving these changes out of this issue, but at the same time I think these are just a few good examples for others to follow. It also demonstrates how (frighteningly) widespread the problem is 😱

Cross-posted with @kristiaanvandeneynde, resulted in the node getting unpublished — republishing this issue.

So basically, I think the 2 examples @lauriii questioned and for which I did the necessary research … show how incredibly pervasive the problem is … and that probably most admin UIs in Drupal core contain bugs like this 😅🙈
EDIT: to help improve this ecosystem-wide, we could do something like #3364506-68: Add optional validation constraint support to ConfigFormBase 🤓

But there's a grand total of 5 more config schema changes beyond making labels required for all config entities that actually store labels, so updated the issue summary to list all 5.

Restoring RTBC state, because I believe all of @lauriii's feedback has been addressed, and per #64, so have @kristiaanvandeneynde's.