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.

Some questions arise...

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. 📌 Document configuration schema with a dedicated @config_schema_api @defgroup Active
2. #2275865: Per language settings (vs translated settings) are not directly supported →

Given that it's 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: 🐛 Config Translation only supports `translatable: true` at the top level, but has no validation to indicate this Needs work .

I went trough this MR a couple of times and the code looks ready to me. I agree with the approach taken in #5 and I think that will not be an issue in practice regarding BC.
We will need a change record to announce that new type, so adding the tag for that.

This comment can be considered a +1 on rtbc already, but not setting it to that because of the change record and issue summary update that are needed. I also think that that someone with more insight on the BC implications would need to comment on the change as well.

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.

Overall changes looks good, so +1 for RTBC.

Only moving to NW for the issue summary update

Looking at MR 3690 it appears all threads have been answered.
Applied the patch locally to checkout the tests.
Reverted core/modules/taxonomy/config/schema/taxonomy.schema.yml and core/modules/contact/config/schema/contact.schema.yml
Ran testLabelValidation against those two sets and got Failed asserting that two arrays are identical.

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

With all tests now being green this looks like it is good to go. I had already reviewed in depth in #13 and the new change in #17 sounds like it makes sense, but also signals missing test coverage for those things. We should probably add that as a followup?

Fixed markup.

#19: thank you! 😊 🙏

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

> Virtually every configuration entity has a label. There is no validation on this at all. There is a lot of freedom in what this string contains, but one thing is certain: it should not be empty.

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

The validate() method is on FieldableEntityInterface, so only content entity types have it.

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 🌱 [META] Untie config validation from form validation — enables validatable Recipes, decoupled admin UIs … Active .

Wim Leers → credited alexpott → .

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: — sad laughter followed.

His follow-up question was … 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:

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? 🤔

Tests seem to indicate something is still broken.

I also need to update the CR.

Continuing this tomorrow.

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, 📌 Make assertions using ConfigEntityValidationTestBase::assertValidationErrors() clearer Fixed 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 📌 Add validation constraint to `type: label`: disallow multiple lines Fixed to address that instead, to keep the scope of this issue clear.

This new type is quite simple, and we are using it in a couple of places already. I would love to get 📌 KernelTestBase::$strictConfigSchema = TRUE and BrowserTestBase::$strictConfigSchema = TRUE do not actually strictly validate Fixed in first. I don't think that might expose new errors here, since we are only using this in a few new places but still think it makes sense to get that one in to ensure we don't introduce new schema errors here.
I don't see anything in this patch I don't understand anymore, and the new places this is used are all sensible.

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

There are a couple of failures in nightwatch tests.

Actually, this triggered many hundreds of kernel test failures. Yay for 📌 KernelTestBase::$strictConfigSchema = TRUE and BrowserTestBase::$strictConfigSchema = TRUE do not actually strictly validate Fixed 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 . This sort of touches upon 🌱 [meta] Make config schema checking something that can be ignored when testing contrib modules Active — 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!

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. 👍

Re #42:

I would ask that we don't make view labels optional. To me, these are entities that absolutely should be labeled - the ID may or may not be descriptive or useful. To me, Views is doing a strange (but very Views-like) thing here by repurposing the ID as the label. We should, in my opinion, rip out that logic in a follow-up, ensure labels are required in the UI, and add an update path.

Besides, selfish reasons: it took a long time of updating the well over 100 test views scattered throughout core to ensure they all had labels. This was responsible for a BIG chunk of the test failures. I guess we don't need to revert all that work, regardless of what we chose...but to me, it's worth the pain if we make views act a little more sanely.

👍 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! 🤓

Welp, it's passing tests! I think this is ready. Sorry for the massive change set -- a lot of tests were previously doing the "wrong" thing (creating certain entities without labels), which is now fixed. But it meant fixing a lot of tests, and especially a lot of test views. Whew!

This looks great, I found 2 nitpicks in the code, this looks very big but most of the changes are because of the change in views. I wonder if we need an update hook to change all existing views to give them the id as a label if no label has been set?

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 📌 KernelTestBase::$strictConfigSchema = TRUE and BrowserTestBase::$strictConfigSchema = TRUE do not actually strictly validate Fixed with validation constraint messages, and to then defer the concrete fixes to follow-up issues (i.e. 📌 Fix all PluginExistsConstraint constraint violations in tests Fixed , 📌 Fix all ExtensionExistsConstraint constraint violations in tests Fixed 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 📌 Configuration schema & required values: add test coverage for `nullable: true` validation support Fixed , 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. 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.

Good to know that the upgrade path is not needed, this makes this one indeed a lot smaller. I had already looked at the other issue and I think the infrastructure for partial ignores from there does indeed make this a lot easier to review.

RTBC, but we still need the follow-ups Wim mentioned in #49. Hopefully the tests agree with me here, because they haven't returned from the bot yet.

Will create the follow-ups for Views & Search as soon as @phenaproxima +1s what I'm proposing in #48 and here.

I'm okay with whatever gets this issue landed. Kicking back for the follow-ups.

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

Opened 🐛 SearchPage entities implicitly require labels: make explicit, and fix migration from D7 Active for the search stuff.

And 📌 Views should require a label, rather than falling back to an unhelpful ID RTBC to handle Views.

Restoring RTBC per #50, now that the follow-ups are created. Hopefully we can get a release manager to commit this.

I think this is fine if we do 📌 Follow-up for #3361534: config validation errors in contrib modules should cause deprecation notices, not test failures Fixed , so that it only becomes a deprecation error. I am not sure we should be adding more of these to 10.2.x before that lands though since 10.2.x branching/alpha is not all that far away at this point.

So I guess that means we could remove the RM review tag if this is PP-1, or keep it on if there's a reason to do this first?

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

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! 😊

One thing I don't see mentioned here is that core allows config entities to have no label but in other places expects there to be one. This problem hit Group particularly hard with the entire Views creation wizard breaking because I stopped providing a label for one of my bundle entities. ConfigEntityBase has no label method and uses EntityBase's, which allows to return NULL as label.

See 💬 Group relationship type no longer has a label, leading to PHP warnings all over the place Fixed , especially comments 10 through 13

Yeah. This definitely feels like a broken promise. But we don't want to fault contrib modules that are doing things the "right way" when it's broken.

From what I can tell, this issue is merely adding a new schema type and making sure large parts of core are updated to use that. But what's the expectation here? Can we still have config entities without labels (like Block) or is everything expected to follow suit at some point, like the spinoff Views issue shows?

Committed the blocker.

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

#58:

• Yep, I hear you! That's what the 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: 🐛 SearchPage entities implicitly require labels: make explicit, and fix migration from D7 Active and 📌 Views should require a label, rather than falling back to an unhelpful ID RTBC . 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 📌 New config schema data type: `required_label` Fixed 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 💬 Group relationship type no longer has a label, leading to PHP warnings all over the place Fixed , 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.

Asked few questions on the MR

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.

Thanks for addressing my concerns Wim.

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 😅🙈

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.

lauriii → closed merge request !3690

Committed 67360f2 and pushed to 11.x. Thanks!

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