Configuration schema & required keys

Wim Leers → closed merge request !4093

This should land after 📌 Configuration schema & required values: add test coverage for `nullable: true` validation support Fixed , because that requires fewer config schema infrastructure additions and changes.

📌 Configuration schema & required values: add test coverage for `nullable: true` validation support Fixed is almost ready, expect progress here next week!

The blocker was just RTBC'd at #3364109-19: Configuration schema & required values: add test coverage for `nullable: true` validation support → , time to get this started!

Commits explained:

1. Added test expectations to the most low-level config system/config schema infrastructure test.
   
   
   
   👆This causes tests to fail 👍 Subsequent commits aim to get this to green again. Current failure:

   1) Drupal\KernelTests\Core\Config\SchemaCheckTraitTest::testTrait
   true does not match expected type "array".
   

2. Added a new RequiredKeysConstraint(Validator), modeled after ValidKeysConstraint(Validator) which was added in ✨ Add validation constraints to config_entity.dependencies Fixed .
3. Enabled the use of RequiredKeys: <infer> by default for all type: mapping, just like we did for ValidKeys in 📌 `type: mapping` should use `ValidKeys: ` constraint by default Fixed .

   Expected test output at this point:

   1) Drupal\KernelTests\Core\Config\SchemaCheckTraitTest::testTrait
   Exception: Exception when installing config for module config_test, message was: Schema errors for config_test.dynamic.dotted.default with the following errors: 0 [] 'third_party_settings' is a required key., 1 [dependencies] 'config' is a required key., 2 [dependencies] 'content' is a required key., 3 [dependencies] 'module' is a required key., 4 [dependencies] 'theme' is a required key., 5 [dependencies] 'enforced' is a required key.
   

   👆 No more hard crash, but an actual config validation error 👍

4. It's reasonable to require the top-level dependencies, but we shouldn't require every config entity to list very possible dependency type. So let's mark those keys optional.

   Expected test output at this point:

   1) Drupal\KernelTests\Core\Config\SchemaCheckTraitTest::testTrait
   Exception: Exception when installing config for module config_test, message was: Schema errors for config_test.no_status.default with the following errors: 0 [] '_core' is a required key.
   

   👆 Previous validation errors gone, new validation error instead 👍

5. The top-level _core is necessary for Drupal core only, it's an internal concern. Config is not REQUIRED to contain it for it to be valid. So let's mark this key optional.

   Expected test output at this point:

   1) Drupal\KernelTests\Core\Config\SchemaCheckTraitTest::testTrait
   Exception: Exception when installing config for module config_test, message was: Schema errors for config_test.system with the following errors: 0 [] 'baz' is a required key.
   

   👆 Previous validation error gone, new validation error instead. Now we've left the realm of "generic simple config & generic config entity validation config schema errors", and have entered the realm of the config schema specific to the config at hand: config_test.system👍

6. There are strong reasons to mark system.test:baz as optional. So did that, and documented why.

   Expected test output at this point:

   1) Drupal\KernelTests\Core\Config\SchemaCheckTraitTest::testTrait
   Exception: Exception when installing config for module config_test, message was: Schema errors for config_test.types with the following errors: 0 [] 'octal' is a required key.
   

   👆 Previous validation error gone, new validation error instead. We're running into a long-standing bug with PECL YAML, for which 📌 Drop PECL YAML library support in favor of only Symfony YAML Fixed has been open for a few years.

7. This issue cannot solve (plus, it'd be out of scope) 📌 Drop PECL YAML library support in favor of only Symfony YAML Fixed . That's why core/modules/config/tests/config_test/config/install/config_test.types.yml has the octal key-value pair commented out, and hence we have no choice but to mark this key as optional.

   Expected test output at this point:

   OK (1 test, 4 assertions)
   

   ✅ Tests are green again!

This concludes the basic infrastructure required for this issue.

Next steps:

1. running all Unit & Kernel tests, which will result in test failures
2. get to green
3. running all tests again, which likely will also result in test failures
4. get to green

For now, I'd like to get a review of this part already 🤓 Just a review of the general pattern and approach. Doing all the remaining work will have to wait for 📌 Configuration schema & required values: add test coverage for `nullable: true` validation support Fixed , because doing all the next steps now will likely cause painful conflicts.

I like the approach.
I didn't know that $definition->toArray(); and then reading the keys is the way to do it. But since this is core this is allowed.

To me it makes sense. And I am curious to see how many things fail when this is turned on for everything.

The documentation in #9 is great, this is super clear. Just like bircher, I'm also very curious to see what breaks when this is turned on for everything.

Now that I've got A) confirmation for the approach, B) 2 curious people besides me … pushed commit that runs all Unit & Kernel tests! 👍🤓🤓

Okay, I'm intrigued, that's not as bad as I thought it would be! 21.5K passes, 1.6K fails 🤩

Let's transplant the ::$ignoredPropertyPaths infrastructure from 📌 Configuration schema & required values: add test coverage for `nullable: true` validation support Fixed and see how many things we'd need to ignore or fix! 🤓

Infra lifted out of 📌 Configuration schema & required values: add test coverage for `nullable: true` validation support Fixed . Should have the exact same test results.

Added things to ignore to ::$ignoredPropertyPaths to make CommentFieldAccessTest and \Drupal\Tests\system\Kernel\Migrate\d7\MigrateActionsTest pass tests.

To be continued. But it's looking promising! 😊

The number of potential config schema property in Views is mind-boggling! 😳

I've already accumulated some 460 lines of ignored property paths!

On the bright side, I think there's a lot of of these ignored property paths for views.view.* that would be conditionally required: AFAICT many of these would be inherited from the default display to all other displays. Dealing with that will require issues of its own — we always knew sanitizing Views' configuration would be a big undertaking.

Then again: it's by far the most complex, most pluggable, most configurable thing we have in Drupal core, so it only makes sense! 😊

Very big push today. The end is near! 🕵️‍♀️

Down to 18 failures!

And 💯% of those 18 failures look like this:

Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for editor.editor.test with the following errors: 0 [image_upload] 'status' is a required key., 1 [image_upload] 'scheme' is a required key., 2 [image_upload] 'directory' is a required key., 3 [image_upload] 'max_size' is a required key., 4 [image_upload] 'max_dimensions' is a required key.

Analysis

editor.editor.*:image_upload has 5 keys:

1. status
2. scheme
3. directory
4. max_size
5. max_dimensions

However, when status === FALSE, the other 4 keys clearly do not make sense to exist!

this indicates the RequiredKeys validation constraint needs to support conditionally required keys.

I'm sure there's many more examples of this all across Drupal core. But because:

1. this is the Editor (Text Editor) config entity, which is already the most validatable config entity type ever since ✨ Add CKEditor 5 module to Drupal core Fixed
2. it's also the only one to actually use config validation in Drupal core
3. I'm the subsystem maintainer for both Text Editor and CKEditor 5

I'd like to use this one as the single concrete example of how to adopt conditionally required keys. A single concrete example + abstract test coverage is enough to prove it works reliably; we don't want to burden this issue with adopting it all over the place. (SchemaCheckTrait::$ignoredPropertyPaths is effectively already our todo-list for things to re-assess, where in some places conditionally required keys will make sense. But each of those need sign-off from the relevant subsystem maintainer(s).)

🚀 Just pushed 2 commits that introduce the necessary infrastructure + test coverage.

Issue summary updated for #18.

🥳

Now we only need to add

+      'image_upload' => [
+        'status' => FALSE,
+      ],

in a bunch of places! 👍

Just pushed that :)

Now most of the failures are due to

--- Expected
+++ Actual
@@ @@
-Array &0 ()
+Array &0 (
+    0 => ''id' is a required key.'
+    1 => ''provider' is a required key.'
+    2 => ''status' is a required key.'
+    3 => ''weight' is a required key.'
+    4 => ''settings' is a required key.'
+)

… turns out this is because of bugs in the config schema: some use type: filter instead of type: mapping — which actually results in a recursive schema definition! 😳 Config schema does not care nor help detect this 🫠 That's a fight for another day. For now, let's fix this.

And with one final commit, this should now be green!

I know that as a next step, we need to run all tests. But, before doing that, I'd like to get a review from @bircher especially, but ideally more people, such as @borisson_ and others 😇

Aha!

-    'settings.plugins.ckeditor5_language' => 'Configuration for the enabled plugin "Language" (ckeditor5_language) is missing.'
+    'settings.plugins.ckeditor5_language' => ''language_list' is a required key.'

Looks like I need to tweak things a little bit further, because CKEditor 5's existing validation constraints already are generating a better (more specific, more context-aware, more humane) error message.

Will do that. But I think the two remaining CKEditor 5-only test failures prove that this overall system works! 😊

1. For #23, CKEditor5EnabledConfigurablePlugins was causing that.
2. Green now!
3. Addressed all feedback on the MR, either by pushing updated code or responding by questions.

Ready for round two! 😄

The conversation from slack to put it more in context:
I meant you can have optional types this way:

# Schema for the configuration files of the Editor module.

editor.editor.*:
  type: config_entity
  label: 'Text editor'
  mapping:
    format:
      type: string
      label: 'Name'
    editor:
      type: string
      label: 'Text editor'
      constraints:
        PluginExists:
          manager: plugin.manager.editor
          interface: Drupal\editor\Plugin\EditorPluginInterface
    settings:
      type: editor.settings.[%parent.editor]
    image_upload:
      type: mapping
      label: 'Image upload settings'
      mapping:
        status:
          type: boolean
          label: 'Status'
        scheme:
          type: optional_type.string.[%parent.status]
          label: 'File storage'
        directory:
          type: optional_type.string.[%parent.status]
          label: 'Upload directory'
        max_size:
          type: optional_type.string.[%parent.status]
          label: 'Maximum file size'
        max_dimensions:
          type: optional_type.mapping.[%parent.status]
          label: 'Maximum dimensions'
          mapping:
            width:
              type: integer
              nullable: true
              label: 'Maximum width'
            height:
              type: integer
              nullable: true
              label: 'Maximum height'

optional_type.string.0:
  type: null
  requiredKey: false

optional_type.string.1:
  type: string
  
optional_type.mapping.0:
  type: null
  requiredKey: false   

optional_type.mapping.1:
  type: mapping

Of course type Null is not a schema type, but we could make one, ie a value that is always null, ignoring everything else.

also of course the optional_type definition doesn't go in that file.. but this is how we make dynamic schemas and I think then the logic of whether a key is required or not can already be described this way. Similarly to how we also don't have more of a conditional validation on whether the value is required or follow some rules based on other values outside of dedicated validation constraints.
So we could say requiredKey: false and it will mostly work since it will also not have null values when the status is true.

In the end what I would be mostly interested in when analysing the schema is to know if the resulting array needs to have the key or not..

One piece of important context is missing — @bircher's original concern as stated in Slack:

Hmmm I will have to give this a more proper look, but I am not a huge fan of making the constraint have non boolean values. it is either required or can be omitted. One can already have a different schema based on values of other fields.. But I would have to check it with the codebase open to see if that is as easy to do as I make it out to be

AFAICT this means that he's not a fan of requiredKeyIf, which accepts not true or false, but {path: 'some_key', value: 'some value'}. That'd be a big difference compared to all other config schema properties → , which all are either a string or a boolean (with the exception of mapping and sequence).

@bircher, can you confirm that's indeed your main concern? 🙏

Concerns about @bircher's proposal for conditionally required keys in #25

I had written in Slack:

But that would imply that we’d have to change the structure of LOTS of existing config! 😨😨 That upgrade path would be a complete nightmare.

In #25, @bircher shows what he meant with a concrete example. 👏 Thank you! 😊 That helps a lot!

So turns out that the concrete config wouldn't have to change (good), but the config schema would have to (bad, but less bad). It does make the schema much more verbose though 😳

I also worry that the already very abstract config schema will become even more abstract/complex to create. There currently is no way to verify it's a valid/sensible config schema, which is why for example a circular schema does not trigger any complaints and has hence been broken in core for years 🙈

There is one important bug in the concrete example in #25:

…
        max_dimensions:
          type: optional_type.mapping.[%parent.status]
          label: 'Maximum dimensions'
          mapping:
            width:
              type: integer
              nullable: true
              label: 'Maximum width'
            height:
              type: integer
              nullable: true
              label: 'Maximum height'

optional_type.mapping.0:
  type: null
  requiredKey: false   

optional_type.mapping.1:
  type: mapping

It would not be valid to define the mapping property in max_dimensions in the case of optional_type.mapping.0. It would actually need to be:

…
        max_dimensions:
          type: optional_type.mapping.editor_image_upload_max_dimensions.[%parent.status]
          label: 'Maximum dimensions'

optional_type.mapping.editor_image_upload_max_dimensions.0:
  type: null
  requiredKey: false   

optional_type.mapping.editor_image_upload_max_dimensions.1:
  type: mapping
  mapping:
    width:
      type: integer
      nullable: true
      label: 'Maximum width'
    height:
      type: integer
      nullable: true
      label: 'Maximum height'

… which implies this would lead to a significant increase in the number of config schema types that TypedConfigManager would need to know about — from 1 to 5 in this single example:

Now, some of those would be reusable (optional_type.string.*), but many wouldn't (none of the conditional mappings ones).

IMHO this is infeasibly complex. Ironically, it'd be infeasibly complex in an effort to simplify the cumulative config schema infrastructure — but as I hope the above shows, it'd actually make things far more complex for the actual developer 😞 (Hence why I use the word "infeasibly".)

🙈 The current MR fails to catch the inverse: conditionally required keys that should NOT be present

Your proposal made me realize I had forgotten about the inverse side of required keys: when image_upload.status is false, we should indeed not require image_upload.scheme (the MR is already doing that part 👍) … but we should actually forbid image_upload.scheme (the MR is not yet doing that part 🐛)!

👍 Added test coverage for that.

Fresh look

1. Alternatively, 🐛 TypedConfigManager::_getDefinitionWithReplacements() incorrectly replaces generic definition Fixed introduced the type: some_type||some_subtype notation — perhaps using that could make sense.

   👎 No, that's only designed for reading the resulting type as computed by TypedConfigManager::getDefinition() handling type inheritance. Introducing this would make the config schema infrastructure even more complex.

2. Looking at the official documentation for config schema properties → , we already have nullable and translatable that are supported for every type, and then there are some type-specific properties listed. For type: mapping, only mapping is currently a type-specific property. The current MR is essentially making requiredKey and requiredKeyIf additional type-specific properties.

   That naming is perhaps confusingly similar to "required values" (which is what 📌 Configuration schema & required keys Fixed is handling), which is solely about nullable: true being set, and hence works for all types.

   But this issue is about "required keys", which applies only to type: mapping. So perhaps omittable and omittableIf would be better names?

   You even mentioned that in Slack:

   👎 That still wouldn't address your concern that we'd be adding a new config schema property → with a complex structure: it's neither a boolean nor a string.

3. Alternative solution: only allow the unconditional requiredKey: false, remove support for conditionally required keys, and instead make conditionally required keys a responsibility of validation constraints?

   CKEditor 5 already has something like this:

   • \Drupal\ckeditor5\Plugin\Validation\Constraint\EnabledConfigurablePluginsConstraint, which verifies that a plugin that is enabled and is configurable in fact has CKE5 plugin configuration present in the Config ⇒ configuration guaranteed to be present when needed
   • \Drupal\ckeditor5\Plugin\Validation\Constraint\ToolbarItemDependencyConstraint, which verifies that if configuration is present, that the corresponding toolbar item is also present ⇒ configuration guaranteed to be absent when not needed

Implemented fresh look point 3 in 5a2ad02d1368de65f8d9bd1e010f640da4db8f9b. I hope that addresses your concern 🤞

Wim Leers → credited phenaproxima → .

IS updated. It now specifically references @bircher's #25 and @phenaproxima's MR comment with similar concerns to explain the current architecture. 👍

212 test failures in the full test run. Many are in update path tests, as well as in CKEditor 5/Text Editor tests (which is to be expected given I used editor.editor.*:image_upload as the sole concrete example for ConditionallyRequiredKeys).

This seems very doable! 👍😊

Ok so my idea from 25 didn't work straight away because it would require a new type that wants its value to be empty/null.
But we can still do this conditional magic without anything new with the following example:

# Schema for the configuration files of the Editor module.

editor.editor.*:
  type: config_entity
  label: 'Text editor'
  mapping:
    format:
      type: string
      label: 'Name'
    editor:
      type: string
      label: 'Text editor'
      constraints:
        PluginExists:
          manager: plugin.manager.editor
          interface: Drupal\editor\Plugin\EditorPluginInterface
    settings:
      type: editor.settings.[%parent.editor]
    image_upload:
      type: mapping
      label: 'Image upload settings'
      mapping:
        status:
          type: boolean
          label: 'Status'
        scheme:
          type: optional_type.string.image_upload_status_[%parent.status]
          label: 'File storage'
        directory:
          type: optional_type.string.image_upload_status_[%parent.status]
          label: 'Upload directory'
        max_size:
          type: optional_type.string.image_upload_status_[%parent.status]
          label: 'Maximum file size'
        max_dimensions:
          type: optional_type.mapping.image_upload_status_[%parent.status]
          label: 'Maximum dimensions'
          mapping:
            width:
              type: integer
              nullable: true
              label: 'Maximum width'
            height:
              type: integer
              nullable: true
              label: 'Maximum height'

# This would go to some other file
optional_type.string.*:
  type: string

optional_type.mapping.*:
  type: mapping

# Set the 0 at the end to 1 in order to debug it on vanilla core and see that the requiredKey is set.
optional_type.string.image_upload_status_0:
  type: string
  requiredKey: false
optional_type.mapping.image_upload_status_0:
  type: mapping
  requiredKey: false

The reason this works is because the mapping information is inherited from the definition it is used on.
So we can add a new validation constraint.. but we don't have to necessarily as far as I can tell.

Oh just a comment on my code comment...
it only works for the happy path :(

if the data is not there it won't parse the type.

After back-and-forth, I'm now starting to see the light in @bircher's proposal 🤓😄

Also, there's a pre-existing example of this in core, which I noticed while running the tests after I got it working:

core.date_format.*:
  type: config_entity
  label: 'Date format'
  mapping:
    id:
      type: string
      label: 'ID'
    label:
      type: required_label
      label: 'Label'
    locked:
      type: boolean
      label: 'Locked'
    pattern:
      type: core_date_format_pattern.[%parent.locked]
      label: 'PHP date format'

# Unlocked date formats should use the translatable type.
core_date_format_pattern.0:
  type: date_format
  label: 'Date format'

# Locked date formats are just used to transport the value.
core_date_format_pattern.1:
  type: string
  label: 'Date format'

👍

#31: Worked around that in \Drupal\Core\Validation\Plugin\Validation\Constraint\RequiredKeysConstraint::resolveMapping() 👍 😄 We can choose to move that into a new Mapping::getSchemaElements() or something like that later if we like. Detailed comments in place.

I worked the entire day to figure out how to rely solely on schema to figure out which keys are required vs optional vs extraneous, so that the test coverage in \Drupal\Tests\editor\Kernel\EditorValidationTest::testImageUploadSettingsAreConditionallyRequired() would not need to change at all! What a journey that was! 🤯

But it WORKS!

It's rather complicated … but that's only because config schema is complicated, and because the whole point of #30 is to rely solely on the config schema's existing support for dynamic types to determine when which type is actually used: for example type: core_date_format_pattern.[%parent.locked] in HEAD resolves to either core_date_format_pattern.0 or core_date_format_pattern.1 already too — this uses that exact same mechanism.

I'll once again need to update the issue summary for this approach.

I HOPE that this is the one that gets @bircher's firm approval 🙏😇

Oh, wow! Some of the test failures are actually proving that this is totally worth it, for example:

Exception: Exception when installing config for module node, message was: Schema errors for core.entity_view_display.node.book.default with the following errors: 0 [content.links] 'settings' is a conditionally required key.

👆 The logic I added figured out that settings is not just required for content.links, it's conditionally required. That's much more helpful of an error message.

Just pushed fe3aa1871378de6972d978f1d6b7a1657f78db60, which will make some additional tests pass.

The fact that this just made it work automatically for many existing types is just definitive proof that the approach proposed by @bircher is the right one.

Turns out that editor.editor.*:image_upload was just a bad example, because it doesn't follow Config Schema best practices. #30 makes it use best practices … and then it Just Works 👍😊

Tweaked the ignored property paths to expect the conditionally required key message when appropriate (see git diff cd045a0e6c08a41c42e7d6ad5d0b959d051c6c87 0c745c33ecac926a4bba4f035f7935efd7b771c5 to the full set of changes).

Now hitting a new core bug, which 🐛 Make 'sequence' Typed Data instances return type of 'sequence' instead of 'list' Fixed recently almost fixed, but apparently not quite: \Drupal\Core\Config\Schema\SequenceDataDefinition::getDataType() was wrong before (it hardcoded return 'list';) but is still wrong now (return 'sequence';), it must instead return the current sequence type or subtype: return $definition['type'];.
( is already present 👍).

Once I push that, this should be green again…

That was green, yay!

Time to break it again, but for good reasons 🤓

Over the weekend, I was thinking about this. I was not quite satisfied with the 'SOMETHING' is a conditionally required key. message: it lacks precision. WHY is it conditionally required? What does that even mean?!? 🤔 🙈🤷‍♂️

Thanks to the massive rewrite that I did based on @bircher's proposal (see #32 and related commits), I actually should have everything I need to generate a very precise message. That should allow us to make a big leap forward in having Drupal generate messages that actually help make config schema less mysterious. 🥳👍

So, here's the logic, plus a first round of updated tests that should make the before vs after crystal clear:

-        "'scheme' is an extraneous key.",
+       "'scheme' is an extraneous key because image_upload.status is set to 0 (see config schema type optional_type.string.image_upload_status_0).",

and

-        "'scheme' is a conditionally required key.",
+        "'scheme' is a conditionally required key because image_upload.status is set to 1 (see config schema type optional_type.string.*).",

Much better, right?! 😃

Issue summary updated for #32.

I have a few high level remarks. I thought I had posted a review to this issue already, but I guess it got corss posted with #33 and I didn't notice.

First and foremost I don't like that the boolean requiredKey on the schema does now more than indicating whether a key is required or not. I am not against being able to express this information, after all this is what my "dummy null type" suggestion from #25 was supposed to do.
I also don't like that you can only set it to false even though true is redundant.
As it is now actually there are three states:

• key required
• key optional (if there is a non-null/empty value the key is there otherwise not)
• key extraneous (ie empty/null value is enforced.)

You have it spelled out in the code already.

Secondly and related to the first is that this information is not accessible from a resolved schema but only if you analyse what other schema types a particular type could have resolved to. This is highly unpractical for modules that try to use the schema to do something to arbitrary config. It would be much better if the logic to decide was either more explicit in the schema or in class other than the validation constraint so that the validation constraint can just check if the condition is met and fail the validation with a pretty message.

For example we could add a method to \Drupal\Core\Config\Schema\Mapping to return which keys are in this resolved instance to be used and which to be omitted or set to empty.

Thanks for the review, @bircher! 🙏😊 Let's get this DONE! 😄

First and foremost I don't like that the boolean requiredKey on the schema does now more than indicating whether a key is required or not.

Can you elaborate why you write this now "does more"? Because in my reading, that still is the only thing it does. 🤔

I also don't like that you can only set it to false even though true is redundant.

Can you elaborate why you do not like this? I'm open to allowing requiredKey: true. But it's confusing and pointlessly verbose to allow this because the very purpose of this issue is to make all keys defined for a mapping in its schema required. If it's the default, then there's no reason to state that explicitly in the schema. That's why I specifically added explicit validation for this (config schema is sorely lacking validation: the learning curve of writing config schema is steep, the DX is essentially "use a debugger"): to not make config schema even harder to write! 😅

As it is now actually there are three states:

• key required
• key optional (if there is a non-null/empty value the key is there otherwise not)
• key extraneous (ie empty/null value is enforced.)

This is inaccurate. A key being optional or extraneous is unrelated to the value for that key.

Accurate imprecise version:

• key required — if requiredKey: false IS NOT specified for it
• key optional — if requiredKey: false IS specified for it
• key conditionally required — if the key is in a mapping inside a dynamic type AND requiredKey: false IS NOT specified for that key in its eventual (resolved) type
• key extraneous — if the key is in a mapping inside a dynamic type AND requiredKey: false IS specified for that key in its eventual (resolved) type

(Dynamic type: most commonly: [%parent.type])

Accurate precise version (yay truth tables):

Secondly and related to the first is that this information is not accessible from a resolved schema but only if you analyse what other schema types a particular type could have resolved to. This is highly unpractical for modules that try to use the schema to do something to arbitrary config. It would be much better if the logic to decide was either more explicit in the schema or in class other than the validation constraint so that the validation constraint can just check if the condition is met and fail the validation with a pretty message.

For example we could add a method to \Drupal\Core\Config\Schema\Mapping to return which keys are in this resolved instance to be used and which to be omitted or set to empty.

Will make this happen! 👍💪

(This is why we have the issue tag 👍😅)

I think I don't understand what you mean by "optional".

For all practical purposes a mapping key is either there or not, there are only two states. Now if the key is required then the mapping needs to have it even with an empty value. so far so good.
When the key, however, is not required then it is therefore optional. Now the question is if you should set the key with an empty value or not set it at all. Being optional, I would understand both to be valid.
An example here is a role which depends on a module but then a permission is removed and the role no longer depends on the module. should it still have an empty module key under the dependencies? are both options valid? will they result in a unnecessary diff if the code that made this transition is not the same for the active config and the sync config?

For me this means optional, or not required. And by looking at how dependencies or third party settings work, I would say not required means unset it if empty.

Now the extraneous property effectively means that no value is allowed. So this is definitely a quality that I would not associate with being optional.
But more importantly in the current approach the extraneousnes depends on other branches of types. And one can very well make one of the branches be dynamic again and maybe circle to a previous one, or have a type more depending on other modules that may or may not be installed.
My argument is that because this is not immediately visible from the schema it is hard to grasp. Both from config schema consumer perspective and a module author that struggles to make a schema that validates their config.
In other words if you need a table to document how it works it is not simple enough.

I think I don't understand what you mean by "optional".

Uh oh! 🫣

Reading #40, it's clear that the one thing you're worried about is the "extraneous" validation error message.

I explained the reason for this in #18 (and the issue summary also specifically links to that comment):

editor.editor.*:image_upload has 5 keys:

1. status
2. scheme
3. directory
4. max_size
5. max_dimensions

However, when status === FALSE, the other 4 keys clearly do not make sense to exist!

If those 4 keys besides status do not make sense to exist, then keeping them around would be:

• confusing ("what is the of these things if uploads are disabled?!")
• pointless ("they are , why are they here?")

That's why I worked hard to add support for that: so that somebody auditing a site's config or looking at the diffs for a site's config can make sense of it.

👆 @bircher Do you agree with that rationale? If not: why not?

Yes I understand that these keys should not be there and are therefore extraneous.
But they are not optional! they are forbidden! they are required to not exist! That is a different thing than being optional.
And I think it would be much more expressive for the schema to say that explicitly.
For example like in my suggestion from #25 with a new type which can not contain any value. But other options are also possible, for example adding yet another flag requiredEmpty: true

The flaw with putting too much information into the boolean flag is that it limits what the conditional config can express. Bear in mind that the example here uses a conditional schema of [%parent.boolean_state] there are usually more than two options often a string. the way the current MR implements it you can not make the key optional in one branch,

Bear in mind that the example here uses a conditional schema of [%parent.boolean_state] there are usually more than two options often a string. the way the current MR implements it you can not make the key optional in one branch,

Do you have an example of this kind of optional states?

RE #43: Just search [% in all files ending in .yml and you will see lots of config schema.
Just a few examples in the general area of editors and filters ie the what you would be familiar with:

• The type of filter settings is filter_settings.[%parent.id] where the id is a string.
• The editor.settings.ckeditor5 plugins are a sequence with type ckeditor5.plugin.[%key]
• In editor.editor.* the settings is a mapping with type editor.settings.[%parent.editor] where editor is a plugin name. (plugin ids is a very commonway to have a dynamic schema)
• Or the layout_builder.component which is a mapping with a key of configuration that has the type block.settings.[id] (so the type is inferred from the value itself. (there are a handful of examples like that)
• And of course third party settings have the type [%parent.%parent.%type].third_party.[%key]

Finished #39 — API additions:

1. Mapping::getResolvedDataDefinitions(): Resolves (dynamic) subtypes of `type: mapping`, to determine the exact data definitions for each key in the mapping, as prescribed by config schema.
2. Mapping::getValidKeys()
3. Mapping::getRequiredKeys()
4. Mapping::getOptionalKeys()
5. Mapping::getConditionallyOptionalKeys()

The first one is for fairly advanced use cases, the last 4 are what you were really hoping for I think, @bircher? 😊

#42:

Yes I understand that these keys should not be there and are therefore extraneous.
But they are not optional! they are forbidden! they are required to not exist! That is a different thing than being optional.

Hm … 🤔🤔🤔

I see your point! 👍

So actually the problem here is that I am conflating "required/optional" (RequiredKeys constraint, being added here) 🆚 "allowed/forbidden" (ValidKeys constraint). This is how it should really work:

                                 ┌──────────┐
                           ┌────►│ required?│
            ┌────────┐     │     └──────────┘
      ┌────►│ valid? ├─────┤
      │     └────────┘     │     ┌──────────┐
──────┤                    └────►│ optional?│
      │     ┌─────────┐          └──────────┘
      └────►│ invalid?│
            └─────────┘

👇

1. Step 1: assess the keys that are present using the ValidKeys constraint (a key-value pair in a mapping either has either a valid key or an invalid key: it's valid if it's specified for this mapping type)
2. Step 2: assess whether keys are missing that should be present using the RequiredKeys constraint

… which means that I'm incorrectly putting this logic for "extraneous keys" in the RequiredKeys constraint validator instead of in the ValidKeys validator, where it belongs: a key is extraneous if some types for a given dynamic mapping type allow that key, but not the current type!

Now that I've implemented all that, look at we get magnificently beautiful and helpful errors like this:

[display.block_1.display_options] 'block_category' is a conditionally required key because display.block_1.display_plugin is block (see config schema type views.display.block).

or

[display.block_2.display_options.arguments.created] 'date' is a conditionally required key because display.block_2.display_options.arguments.created.plugin_id is date (see config schema type views.argument.date).

🤩

So now it finally is fully using config schema as intended 😊🚀 That's what I was hoping for all along: to make virtually no changes to the existing config schema (nor its infrastructure), and just surface actually helpful messages! 👍

Stay tuned while I get this to green …  hopefully the last iteration! 🤓 The revised API additions:

1. Mapping::getValidKeys()
2. Mapping::getRequiredKeys()
3. Mapping::getOptionalKeys()
4. Mapping::getConditionallyValidKeys()

Yes! I think we are converging to a great solution.

I didn't check the latest code, but I hope it doesn't use the requiredKey for deciding that in the other branch the key is not optional and therefore in this branch it is forbidden.
But I had an idea last night that would solve your problem of making keys forbidden without anything new. ie this schema works even in Drupal 9 and the ValidKeys constraint would take care of it. Since nothing here is optional.

editor.editor.*:
  type: config_entity
  label: 'Text editor'
  mapping:
    format:
      type: string
      label: 'Name'
    editor:
      type: string
      label: 'Text editor'
      constraints:
        PluginExists:
          manager: plugin.manager.editor
          interface: Drupal\editor\Plugin\EditorPluginInterface
    settings:
      type: editor.settings.[%parent.editor]
    image_upload:
      type: editor_editor_image_upload.[status]
      label: 'Image upload settings'

# This is the fallback type so that it works even with unpatched config, ie the one where the status is also omitted entirely.
editor_editor_image_upload.*:
  type: mapping
  mapping:
    status:
      type: boolean
      label: 'Status'
      requiredKey: false

editor_editor_image_upload.1:
  type: mapping
  mapping:
    status:
      type: boolean
      label: 'Status'
    scheme:
      type: string
      label: 'File storage'
    directory:
      type: string
      label: 'Upload directory'
    max_size:
      type: string
      label: 'Maximum file size'
    max_dimensions:
      type: mapping
      label: 'Maximum dimensions'
      mapping:
        width:
          type: integer
          nullable: true
          label: 'Maximum width'
        height:
          type: integer
          nullable: true
          label: 'Maximum height'

hahaha lol..
I was curious and took a peek at the MR anyway and looking at the code now I see that you had the same idea!! excellent!
I will review it more in detail later when I have some time for it.

The mysterious failures I couldn't reproduce: due to ✨ Provide options to sanitize filenames (transliterate, lowercase, replace whitespace, etc) Fixed having landed in the mean time 🙈 Extracted the migration test improvement to #3197324-58: Exception trace cannot be serialized because of closure → .

Green again!

I need to shift attention to other things and I'll be on vacation starting next Wed until the Fri the week after it. I'm hoping that while I'm out:

1. @bircher does a thorough review of everything in Mapping 🤓🧐
2. @alexpott is back from vacation and can hopefully do a high-level review of the approach — by starting in the issue summary and then only looking at the final set of changes, not the >100 commits 🤞

(and maybe — a man can dream — I'll come back to this issue being unblocked because 📌 Configuration schema & required values: add test coverage for `nullable: true` validation support Fixed landed? 😇)

I reviewed the pull request again, this is looking super solid and is well documented. I think I understand most of what is going on, and based on the conversations here, and in slack, with @bircher it looks like it catches all the special cases we currently have. I am curious to see what happens if we let this loose on contrib, but since the should not be doing any schema checking yet, there are no issues with that either.

I think the state this has ended up in is a lot more solid than what we started with, kudos to @Wim Leers to keep up with all the comments on this issue.

@borisson_ should this be RTBC?

The MR needs to be "re-rolled". There are two merge conflicts.

	both modified:   core/modules/ckeditor5/tests/src/Kernel/ValidatorsTest.php
	both modified:   core/tests/Drupal/KernelTests/Core/Config/ConfigEntityValidationTestBase.php

The second one I think is pretty trivial to solve, the first one I am less sure.

I tried to use this new API in config split on the line mentioned in the issue summary here. And I haven't done manual checks but the relevant test passes when core is on this MR branch. See 📌 Use required keys of mappings instead of special casing Needs review

I did have to change some things to make the config validation happy and I haven't finished that yet because I am using config from a core test module and it makes my test fail.. so that is on me and I need to fix that. But I just wanted to note that this is much more strict now. And is probably more disruptive than we would like. but there is another issue to address that if my memory is right.

But over all I am happy with where this is going! the API addition is useful and solves the problem it is set out to solve.

🐛 \Drupal\Tests\ckeditor5\Kernel\ValidatorsTest::test() obscures 2nd, 3rd etc violation message for the same property path Fixed was extracted from this issue and landed. That explains 1 of the 2 conflicts. The other conflict is 📌 Add new `ImmutableProperties` constraint Fixed , which tightened the existing test infra. MR updated! 👍

I tried to use this new API in config split […] See #3390285: Use required keys of mappings instead of special casing

Thank you! (Linking the issue.)

And is probably more disruptive than we would like.

Nothing enforces config to be valid in Drupal core today.

Modules can choose when to opt in. Site owners can optionally choose to validate all their config, by using https://www.drupal.org/project/config_inspector → 's Drush command.

Nobody is being forced into disruption. Everybody is given the choice to not change anything, or to reduce the pain of configuration-triggered bugs (module maintainers) or managing configuration (site maintainers).

Module maintainers know what the expected structure is. So module maintainers can provide automatic update paths, just like core has already done in a whole range of issues — most recently: 📌 Views should require a label, rather than falling back to an unhelpful ID RTBC . Even if only core writes upgrade paths for all of the invalid config, then this is still worth doing. But hopefully, eventually, by Drupal 11 or maybe only by Drupal 12, we'd be able to do this for all config.

But over all I am happy with where this is going! the API addition is useful and solves the problem it is set out to solve.

🥳🥳🥳🥳 So … what's next? 🤓😊 Besides cleaning up this MR, what is blocking an RTBC from you?

Have not reviewed but see it has some test failures.

Also do CR updates and the follow up still need to happen?

Indeed, did not yet find the time to solve those. Will get that done in the next 2 days.

All done here now!

Test coverage has been finished, the obsolete ConditionallyRequiredKeys stuff is gone.

@smustgrave I think this should update the change record at https://www.drupal.org/node/3362879 → , rather than a separate change record, because now the validation is actually behaving as people expected (similar to what we did at 📌 `type: mapping` should use `ValidKeys: ` constraint by default Fixed , which also did not get its own change record).

However, a separate change record just for marking a key as optional would probably make sense?

Walked @alexpott through this yesterday, together with @borisson_ and @bircher.

Conclusions:

1. "conditionally required key" is not just confusing to interpret, it's actually wrong: it should be "required key"
2. "extraneous key" has a similar problem, "unknown key" would be more appropriate
3. 📌 Follow-up for #3361534: config validation errors in contrib modules should cause deprecation notices, not test failures Fixed did not go far enough: even deprecation notices/errors would be too disruptive for the contributed module ecosystem. No errors/notices whatsoever should be triggered, unless a module opts in to strict config schema compliance & testing. Opened 📌 Follow-up for #3361534: config validation errors in contrib modules should not cause deprecation notices, unless they opt in Active for this.
4. @alexpott is +1 to the overall approach

@alexpott Wouldn't it be easier to understand the concrete example in this issue (image uploads) if it were done in a separate merge request, so that the consequences of those schema changes can be seen independent of the infrastructure we're adding here?

(It'd mean extra work for me, but I think it might be better for Drupal users looking for an example.)

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

Merged 11.x back into this branch, also hopefully the MR should be green now.

I'm not sure if I can rtbc this issue now that I did some work on it, but to me it looks like it is done.
I'll start creating followups when this lands, so leaving that tag in place.

Created (and fixed) an example followup already: 📌 Remove system.mail from SchemaCheckTrait ignored property paths Postponed .

I’m working on the missing test coverage.

See https://git.drupalcode.org/issue/drupal-3364108/-/pipelines/43969/test_r....

It is impossible to test the new RequiredKeys constraint in isolation.

Combine that with the observation that it doesn’t really make sense to separate it from the ValidKeys constraint (they are complementary in what they do, but RequiredKeys must currently be made aware of what options are passed to ValidKeys to ensure all edge cases are handled correctly), and the logical conclusion is: we should have ONLY ValidKeys and just make it more capable.

It is already impossible anyway for any other type to reuse this validator, so it simplifies everything: fewer edge cases to test, simpler logic, one less TODO, simpler test coverage.

Will do that and update the issue summary. 👍

we should have ONLY ValidKeys and just make it more capable.

It is already impossible anyway for any other type to reuse this validator, so it simplifies everything: fewer edge cases to test, simpler logic, one less TODO, simpler test coverage.

Done. ✅

Next up: explicit test coverage for the new methods on \Drupal\Core\Config\Schema\Mapping.

Bug deep in core, discovered here, but with a work-around in place: 🐛 Follow-up for #2663410: calling TypedConfigManager::getDefinition() causes cache pollution Active , work-around: https://git.drupalcode.org/project/drupal/-/merge_requests/4094/diffs?co....

Whew! 🤓

1. Self-reviewed the entire MR after having let it rest for ~2 weeks.
2. Wrote the missing test coverage
3. Cleaned up

DONE 🥳

Removing , because at this point it does not matter anymore which lands first: this one or 📌 Configuration schema & required values: add test coverage for `nullable: true` validation support Fixed . In fact, this one is much higher impact and should probably land first at this point!

IMHO the best way to observe this in action is through \Drupal\Tests\editor\Kernel\EditorValidationTest::testImageUploadSettingsAreConditionallyRequired(), because it most clearly demonstrates how this issue/MR will both improve the UX (precise error messages) and the DX (no more "garbage" or "noise" in config).

Recommended review plan

1. Understand the real-world impact
   1. new EditorValidationTest::testImageUploadSettingsAreConditionallyRequired() test
   2. editor.schema.yml changes
   3. consequences of this throughout the codebase (grep for image_upload and setImageUploadSettings in the diff): many tests were required to be updated, because test fixtures must now be strictly valid (in core — contrib tests are unaffected thanks to 📌 Follow-up for #3361534: config validation errors in contrib modules should cause deprecation notices, not test failures Fixed and 📌 Follow-up for #3361534: config validation errors in contrib modules should not cause deprecation notices, unless they opt in Active
2. Understand how we achieve this without adding any new APIs, and are just inspecting the existing config schema
   1. read the new \Drupal\KernelTests\Config\Schema\MappingTest test, and make sure to understand all the @see-referenced things
   2. once you agree that these things make sense, move on to the new methods on \Drupal\Core\Config\Schema\Mapping ← this makes up the bulk of the complexity. But it's not new complexity: it's "just" automatically making sense of all of the existing config schemas!
   3. Now move on to ValidKeysConstraintValidator and observe how it uses these methods.
   4. Note: these new methods are also what config_split and other modules can use — see 📌 Use required keys of mappings instead of special casing Needs review

As you can tell, I reviewed this.

A lot of it was relatively straightforward and makes sense. The stuff that really ties my brain into knots is the stuff that was added to Mapping. That's really complicated. The inline comments are very helpful, but there is a terminology problem that, IMHO, would be improved by having a lot of examples to illustrate what is being transformed into what (if that makes sense). I just found it quite difficult to visualize the stages of resolving these dynamic types, and what their possible keys are, from comments alone.

Having said all that...this is extraordinarily complicated stuff and I don't know how much more heroic of an effort could have been made here. But I fear that this will be too intimidating for anyone else to maintain. The priority is getting this functionality into core, though; refactoring, if there's any useful refactoring to be done, can come later.

The inline comments are very helpful, but IMHO, they would be improved by having a lot of examples to illustrate what is being transformed into what (if that makes sense), at every stage of the logic.

I just found it quite difficult to visualize the stages of resolving these dynamic types, and what their possible keys are, from comments alone.

+1, will do! I personally really like having those kinds of comments too 😊

Turns out that because this was rebased on top of the recently landed 📌 Introduce a new #config_target Form API property to make it super simple to use validation constraints on simple config forms, and adopt it in several core config forms Fixed , which made FileSystemForm use validation constraints, that we run into path being a required key. But that was deprecated into #3039026: Deprecate file_directory_temp() and move to FileSystem service → , so it makes sense that that key is not set.

So why is it still in the config schema? Because it was necessary for tests. Why was it not deprecated? Because #2997100: Introduce a way to deprecate config schemas → landed a year later.

Conclusion:

1. Separate issue needed to fix the config schema for system.file
2. This issue must be updated to automatically treat deprecated keys as optional, and test coverage must be updated

• Added kernel test coverage + fix for treating deprecated property paths as optional keys.
• @borisson_ created 📌 Deprecate path.temporary in system.file configuration schema Fixed for #69.1. I created an MR. Unfortunately, turns out that it is forced to fix a small bug in both the 9.4.0 "bare" and "phpass" update path fixtures 🥲, so I'm forced to bring that into this MR too.
• First batch of feedback from @borisson_ and @phenaproxima on the MR addressed. Follow-ups created: (out-of-scope) 📌 "type" in Constraint plugin definitions not used nor validated, should be enforced Active .

Down to only 4 failures, Continuing to get this back to green and in a more reviewable state.

This MR is likely too impractical to land in one piece (even if #3400368 were to land today), so I will extract issues from this one, once all feedback has been addressed and the tests are green.

New issues:

1. BLOCKER: 📌 Deprecate path.temporary in system.file configuration schema Fixed
2. non-blocker systemic fix for an out-of-scope decade old bug: 📌 Provide guidance to config schema developers: detect broken config schema types Needs work (to justify fixing the incorrect schema for filter_settings.filter_html here)
3. non-blocking follow-up to remove a @todo discovered here: 🐛 Fix bugs in update path surfaced by config validation Active
4. BLOCKER (would land a subset): 📌 Introduce Mapping::getValidKeys(), ::getRequiredKeys() etc. Active (unblocks both this issue + config_split in contrib)

PP-2 for 📌 Deprecate path.temporary in system.file configuration schema Fixed + 📌 Introduce Mapping::getValidKeys(), ::getRequiredKeys() etc. Active .

This can still be reviewed to see how this builds on top of #3401883 though!

Well well, https://git.drupalcode.org/project/drupal/-/merge_requests/4094/diffs?co... in response to @phenaproxima's review uncovered a bug: %key-based dynamism did not yet generate appropriate validation error messages!

This also means the test coverage in \Drupal\KernelTests\Config\Schema\MappingTest is insufficient.

📌 Deprecate path.temporary in system.file configuration schema Fixed landed 🚢

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

Making this opt-in using the mechanism proposed at

• #3395099-11: Allow config types to opt in to config validation, use as signal to opt in to "keys/values are required by default" →
• #3395099-19: Allow config types to opt in to config validation, use as signal to opt in to "keys/values are required by default" →

Already proven to be successful over at #3364109-49: Configuration schema & required values: add test coverage for `nullable: true` validation support → 🕺

Turns out the hardening I just did here turned up over at 📌 Introduce Mapping::getValidKeys(), ::getRequiredKeys() etc. Active too after I addressed @alexpott's feedback, because thanks to that feedback the validation of the schema now happens earlier 👍

📌 Introduce Mapping::getValidKeys(), ::getRequiredKeys() etc. Active landed! I didn't get to finish #77 yet, but that having landed will definitely make this MR infinitely easier to review & read 😄

Found a bug in TypedConfigManager: 🐛 Calling TypedConfigManager::create() does not use the definitions of TypedConfigManager (config schema), but of TypedDataManager (field types) Active .

Similar to #3364108-49: Configuration schema & required keys → , I reverted all changes, got tests to pass on HEAD, and then introduced the opt-in infrastructure proposed at #3395099-19: Allow config types to opt in to config validation, use as signal to opt in to "keys/values are required by default" → .

Similar to #3364108-51: Configuration schema & required keys → , the MR was nice and simple until a702b532 (no more >1000 lines being added to $ignoredPropertyPaths!).

Then I opted in the Editor config entity type by adding the FullyValidatable constraint. Which then required schema changes and test adjustments. I'm fine with omitting all of that from this MR, but @alexpott indicated at DrupalCon Lille he'd prefer to demonstrate how this can be used. I think that could happen in a separate issue too, but I'm not opposed to adding it here.

Finally, I added test coverage similar to #3364108-51, to prove that config entity types are not affected by this change until they opt in: setting type: mapping properties on config entities that have not opted in now are proven to not trigger any validation errors, which also proves the absence of disruption.

Change record created: https://www.drupal.org/node/3404425 → , one change record updated by creating a draft revision → and updated the issue summary.

Reviewed this and found some relatively minor stuff, but in general it seems pretty good.

It's really complicated, but I also understand that other MRs are merged into this one right now and will be stripped out when they land in other issues.

@claudiu.cristea independently found the same "invalid filter settings schema" problem and worked on a tightly scoped fix in 🐛 Filter settings schema types are incorrect Active . That issue is now RTBC, and much preferable over the far bigger undertaking to prevent this bug pattern from ever getting reintroduced, for which we still have 📌 Provide guidance to config schema developers: detect broken config schema types Needs work .

All feedback from @phenaproxima has been addressed.

All feedback from @phenaproxima once again addressed.

Extracted 📌 Refine ValidKeysConstraintValidator's messages: use #3401883's new Mapping infrastructure Needs review from this issue, which does NOT do the more complex/harder to understand "required keys" change, but only the refinement of the existing message of the ValidKeys validation constraint.

Once that lands, this MR becomes 30% smaller.

📌 Configuration schema & required values: add test coverage for `nullable: true` validation support Fixed landed!

That means the changes to be merged. It was the most complex merge conflict of the year for sure 😳 I think I got it! 🤞

This is still soft-postponed on 📌 Refine ValidKeysConstraintValidator's messages: use #3401883's new Mapping infrastructure Needs review , that would remove 4 files, +361, -60 from this MR.

Merged in upstream. This caused a failure in \Drupal\Tests\block\Kernel\Migrate\d6\MigrateBlockTest due to 📌 Remove forum from block migration tests Fixed (landed 2 days ago), whose expected messages become more precise thanks to this issue. 👍 Trivial update!

Merged in upstream because 📌 [PP-1] Make NodeType config entities fully validatable Needs review landed! 🚀

\Drupal\Tests\node\Kernel\NodeTypeValidationTest passes. Let's find out if everything else does too. 👀

Now that 📌 Refine ValidKeysConstraintValidator's messages: use #3401883's new Mapping infrastructure Needs review landed, I'm removing the postponed status from this issue.

Merged in upstream; resolved conflicts with 📌 Refine ValidKeysConstraintValidator's messages: use #3401883's new Mapping infrastructure Needs review . Before: 45 files, +1053, -151. After: 44 files, +761, -91. 🥳

Due to 📌 [PP-1] Make NodeType config entities fully validatable Needs review having landed, we have three test failures (also already occurring at #88).

3 test failures:

1. \Drupal\Tests\forum\Kernel\Migrate\d7\MigrateTaxonomyTermTest
2. \Drupal\Tests\forum\Kernel\Migrate\d7\MigrateTaxonomyTermTranslationTest
3. \Drupal\Tests\menu_ui\Functional\MenuUiNodeTest::testMainMenuIsPrioritized()

The first 2 are occurring because they contain a NodeType config entity that specifies third_party_settings.menu_ui.parent, but node.type.*.third_party.menu_ui in menu_ui.schema.yml dictates third_party_settings.menu_ui.allowed_menus also is required.

For the third it's the other way around: allowed_menus is defined, but parent is not.

We can make tests pass by doing

diff --git a/core/lib/Drupal/Core/Config/Schema/SchemaCheckTrait.php b/core/lib/Drupal/Core/Config/Schema/SchemaCheckTrait.php
index 88682009ae2..a130d0b3cb2 100644
--- a/core/lib/Drupal/Core/Config/Schema/SchemaCheckTrait.php
+++ b/core/lib/Drupal/Core/Config/Schema/SchemaCheckTrait.php
@@ -52,6 +52,13 @@ trait SchemaCheckTrait {
         'This value should not be blank.',
       ],
     ],
+    'node.type.*' => [
+      // @todo Fix config or tweak schema of `node.type.*.third_party.menu_ui`
+      // @see menu_ui.schema.yml
+      'third_party_settings.menu_ui' => [
+        '.* is a required key.',
+      ],
+    ],
   ];
 
   /**

… but it would be better to fix 📌 [PP-2] Fully validatable config schema types: support dynamic types Postponed instead.

On top of that: this issue is modifying editor.editor.*:image_upload to demonstrate how to evolve config schema to take advantage of dynamically required keys, which means there's now two reasons to fix 📌 [PP-2] Fully validatable config schema types: support dynamic types Postponed .

I think it probably would be easier to fix 📌 [PP-2] Fully validatable config schema types: support dynamic types Postponed here.

Expanded test coverage. It failed 👍

Then pushed the solution to handle the dynamic type boundaries as suggested by @effulgentsia at #3364109-60: Configuration schema & required values: add test coverage for `nullable: true` validation support → .

There were failures in EditorValidationTest and \Drupal\Tests\ckeditor5\Kernel\ValidatorsTest.

Marking all of core's text editor plugin configuration as well as the CKEditor 5 plugin configuration as fully validatable made all tests green again — because the test coverage specific to Text Editor + CKEditor 5 was already expecting validation errors that this issue was introducing.

(Complying with @effulgentsia's observation at #3364109-60: Configuration schema & required values: add test coverage for `nullable: true` validation support → that this should be opt-in to prevent disruption was proven in practice by that. 😄)

We can probably extract more from this into smaller issues, but I think it is now in a very reviewable state, the Change Request is still up to date with the changes in the MR.

This MR looks really great. My only hesitation with committing it is the various changes in here related to enforcing the lack of any other keys when an editor's image_upload.status is FALSE. For example, the need to remove those other keys from Umami's editor.editor.basic_html.yml. I think that should all be removed from this MR and moved to a follow-up that gets its own commit and its own change record.

I don't think separating that out needs to affect this MR too much though. For example, could we accomplish that by adding a editor.image_upload_settings.0
config schema object that doesn't add FullyValidatable? We can still leave FullyValidatable in place on editor.image_upload_settings.1.

The reason I want to punt on enforcing FullyValidatable on editor.image_upload_settings.0 is that I think it could be useful in some situations to have default config that adds the upload configuration even with a status=false, so that someone could change the status to true and have the other stuff already properly configured. Maybe I'm wrong and that's not useful, but in either case, I'd like us to discuss that in its own issue without holding up this one.

Other than that, I think this is good to go.