Configuration schema & required keys

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

#3364109: Configuration schema & required values: add test coverage for `nullable: true` validation support 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 #3324150: Add validation constraints to config_entity.dependencies.
3. Enabled the use of RequiredKeys: <infer> by default for all type: mapping, just like we did for ValidKeys in #3376794: `type: mapping` should use `ValidKeys: <infer>` constraint by default.

   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 #3205480: Drop PECL YAML library support in favor of only Symfony YAML has been open for a few years.

7. This issue cannot solve (plus, it'd be out of scope) #3205480: Drop PECL YAML library support in favor of only Symfony YAML. 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 #3364109: Configuration schema & required values: add test coverage for `nullable: true` validation support, because doing all the next steps now will likely cause painful conflicts.

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 #3364109: Configuration schema & required values: add test coverage for `nullable: true` validation support and see how many things we'd need to ignore or fix! 🤓

Infra lifted out of #3364109: Configuration schema & required values: add test coverage for `nullable: true` validation support. 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!

Conclusion: 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 #3231364: Add CKEditor 5 module to Drupal core
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.

Before https://git.drupalcode.org/project/drupal/-/merge_requests/4094/diffs?co...

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.

After https://git.drupalcode.org/project/drupal/-/merge_requests/4094/diffs?co...

Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for editor.editor.test with the following errors: 0 [image_upload] 'status' is a required key.

🥳

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

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:

Before

editor.editor.*

After

editor.editor.*, optional_type.string.0, optional_type.string.1, optional_type.mapping.editor_image_upload_max_dimensions.0, optional_type.mapping.editor_image_upload_max_dimensions.1

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. It failed tests 👍

Fresh look

1. Alternatively, #2663410: TypedConfigManager::_getDefinitionWithReplacements() incorrectly replaces generic definition 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 #3364108: Configuration schema & required keys 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: […] but that way if you check the schema you already know if the key is to be omitted and you don't have to duplicate the logic to look up the value when saving the yml

   
   
   👎 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

   
   
   👍 This seems the only viaable path forward: a new ConditionallyRequiredKeys validation constraint, which removes the need to add new properties (see https://www.drupal.org/node/1905070#properties) to config schema.
   

Implemented fresh look point 3 in 5a2ad02d1368de65f8d9bd1e010f640da4db8f9b. I hope that addresses your concerns, @bircher 🤞

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

💡

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'

👍

RE: blocker for #30

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

To ensure the correctness of it all, and to simplify future maintainers' lives, I added a lot of assertions to keep things straight. (It's almost a hard requirement for anything config schema-related IMO 😅 — at least I cannot keep track of things otherwise 🙈)

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 #3361034: Make 'sequence' Typed Data instances return type of 'sequence' instead of 'list' 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'];.
(Needs change record updates is already present 👍).

Just pushed that, which should make this 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.

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):

👆 The axes are:

1. static 🆚 dynamic
2. requiredKey: false: yes 🆚 no
3. and all possible matches for the original type either have the same requiredness: yes 🆚 no

As you can see, despite 6 possible combinations, there are only 4 possible outcomes.

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 Contributed project blocker issue tag 👍😅)

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 meaning of these things if uploads are disabled?!")
• pointless ("they are unused, 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?

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? 😊 2–4 return the concrete values for the given Mapping instance. The last one computes a result that will be the same for every instance of this particular mapping (i.e. at this relative property path — so for example it'll compute the same for the image_upload property path on every Editor config entity. It uses ::getResolvedDataDefinitions() to achieve this.

P.S.: 4 files changed, 333 insertions(+), 409 deletions(-) 👍😄

#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()

The mysterious failures I couldn't reproduce: due to #2492171: Provide options to sanitize filenames (transliterate, lowercase, replace whitespace, etc) 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 and @borisson_ do 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 #3364109: Configuration schema & required values: add test coverage for `nullable: true` validation support landed? 😇)

#3382453: \Drupal\Tests\ckeditor5\Kernel\ValidatorsTest::test() obscures 2nd, 3rd etc violation message for the same property path was extracted from this issue and landed. That explains 1 of the 2 conflicts. The other conflict is #3382580: Add new `ImmutableProperties` constraint, 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: #3380480: Views should require a label, rather than falling back to an unhelpful ID. 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?

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 #3376794: `type: mapping` should use `ValidKeys: <infer>` constraint by default, 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. #3379899: Follow-up for #3361534: config validation errors in contrib modules should cause deprecation notices, not test failures 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 #3395099: [meta] Allow config types to opt in to config validation, use "FullyValidatable" constraint at root as signal to opt in to "keys/values are required by default" for this.
4. @alexpott is +1 to modifying editor.editor.*:image_upload to provide at least one concrete example of how config schemas can be refined to more accurately capture the intent of what configurations make sense (in this case: when disabling image uploads it makes no sense to have configuration stored for the directory where to upload it into!) — he preferred doing it here rather than in a follow-up issue, to provide a concrete example
5. @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.)

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. (Due to hardcoded assumptions in DataDefinition that are definitely out of scope to fix.)

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: #3400181: Follow-up for #2663410: calling TypedConfigManager::getDefinition() causes cache pollution, 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 PP-1, because at this point it does not matter anymore which lands first: this one or #3364109: Configuration schema & required values: add test coverage for `nullable: true` validation support. 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 #3379899: Follow-up for #3361534: config validation errors in contrib modules should cause deprecation notices, not test failures and #3395099: [meta] Allow config types to opt in to config validation, use "FullyValidatable" constraint at root as signal to opt in to "keys/values are required by default"
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 #3390285: Use required keys of mappings instead of special casing

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 #3382510: 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, 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 #3400368: Deprecate path.temporary in system.file configuration schema 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) #3401592: "type" in Constraint plugin definitions not used nor validated, should be enforced.

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: #3400368: Deprecate path.temporary in system.file configuration schema
2. non-blocker systemic fix for an out-of-scope decade old bug: #3401837: Add basic validation to config schema definitions (to justify fixing the incorrect schema for filter_settings.filter_html here)
3. non-blocking follow-up to remove a @todo discovered here: #3401876: Fix bugs in update path surfaced by config validation
4. BLOCKER (would land a subset): #3401883: Introduce Mapping::getValidKeys(), ::getRequiredKeys() etc. (unblocks both this issue + config_split in contrib)

PP-2 for #3400368: Deprecate path.temporary in system.file configuration schema + #3401883: Introduce Mapping::getValidKeys(), ::getRequiredKeys() etc..

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 and \Drupal\KernelTests\Core\TypedData\ValidKeysConstraintValidatorTest is insufficient.

#3400368: Deprecate path.temporary in system.file configuration schema landed 🚢

Making this opt-in using the mechanism proposed at

• #3395099-11: [meta] Allow config types to opt in to config validation, use "FullyValidatable" constraint at root as signal to opt in to "keys/values are required by default"
• #3395099-19: [meta] Allow config types to opt in to config validation, use "FullyValidatable" constraint at root 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 #3401883: Introduce Mapping::getValidKeys(), ::getRequiredKeys() etc. too after I addressed @alexpott's feedback, because thanks to that feedback the validation of the schema now happens earlier 👍

#3401883: Introduce Mapping::getValidKeys(), ::getRequiredKeys() etc. 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: #3403782: Calling TypedConfigManager::create() does not use the definitions of TypedConfigManager (config schema), but of TypedDataManager (field types).

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: [meta] Allow config types to opt in to config validation, use "FullyValidatable" constraint at root 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.

@claudiu.cristea independently found the same "invalid filter settings schema" problem and worked on a tightly scoped fix in #3404431: Filter settings schema types are incorrect. 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 #3401837: Add basic validation to config schema definitions.

All feedback from @phenaproxima has been addressed. 12 open threads on the MR remain, most of which hopefully will be confirmed to be resolved by @phenaproxima.

Those remaining threads should not stop a subsystem maintainer review, because the review is in nitpicking territory already. (It's been ready for 13 days now, since #78.)

All feedback from @phenaproxima once again addressed.

Extracted #3406478: Refine ValidKeysConstraintValidator's messages: use #3401883's new Mapping infrastructure 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.

#3364109: Configuration schema & required values: add test coverage for `nullable: true` validation support 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! 🤞

Before

51 files, +1129, -151

After

45 files, +1049, -148

This is still soft-postponed on #3406478: Refine ValidKeysConstraintValidator's messages: use #3401883's new Mapping infrastructure, 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 #3371219: Remove forum from block migration tests (landed 2 days ago), whose expected messages become more precise thanks to this issue. 👍 Trivial fix!

Merged in upstream because #3379091: Make NodeType config entities fully validatable landed! 🚀

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

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

Due to #3379091: Make NodeType config entities fully validatable 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 #3408165: Fully validatable config schema types: support dynamic types 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 #3408165: Fully validatable config schema types: support dynamic types.

I think it probably would be easier to fix #3408165: Fully validatable config schema types: support dynamic types 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. 😄)

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've suggested that before, so I'm totally fine with that 😊 It's @alexpott who requested during DrupalCon Lille for this to stay part of this MR to showcase it here. But, a lot has changed then, so I doubt he'd be opposed.

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.

No, that is not enough. Observe it by applying this:

$ git diff
diff --git a/core/modules/editor/config/schema/editor.schema.yml b/core/modules/editor/config/schema/editor.schema.yml
index eeb9c6a363d..a8daf9f2b7f 100644
--- a/core/modules/editor/config/schema/editor.schema.yml
+++ b/core/modules/editor/config/schema/editor.schema.yml
@@ -24,8 +24,8 @@ editor.editor.*:
 editor.image_upload_settings.*:
   type: mapping
   label: 'Image upload settings'
-  constraints:
-    FullyValidatable: ~
+#  constraints:
+#    FullyValidatable: ~
   mapping:
     status:
       type: boolean
diff --git a/core/profiles/demo_umami/config/install/editor.editor.basic_html.yml b/core/profiles/demo_umami/config/install/editor.editor.basic_html.yml
index 60cd331cc87..eba91ebf2f8 100644
--- a/core/profiles/demo_umami/config/install/editor.editor.basic_html.yml
+++ b/core/profiles/demo_umami/config/install/editor.editor.basic_html.yml
@@ -59,3 +59,9 @@ settings:
       allow_view_mode_override: true
 image_upload:
   status: false
+  scheme: public
+  directory: inline-images
+  max_size: ''
+  max_dimensions:
+  width: null
+  height: null

and then running DemoUmamiProfileTest::testConfig().

You'll get:

1) Drupal\Tests\demo_umami\Functional\DemoUmamiProfileTest::testConfig
Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for editor.editor.basic_html with the following errors: editor.editor.basic_html:image_upload.scheme missing schema, editor.editor.basic_html:image_upload.directory missing schema, editor.editor.basic_html:image_upload.max_size missing schema, editor.editor.basic_html:image_upload.max_dimensions missing schema, editor.editor.basic_html:image_upload.width missing schema, editor.editor.basic_html:image_upload.height missing schema, 0 [image_upload.width] 'width' is not a supported key., 1 [image_upload.height] 'height' is not a supported key., 2 [image_upload] 'scheme' is an unknown key because image_upload.status is 0 (see config schema type editor.image_upload_settings.*)., 3 [image_upload] 'directory' is an unknown key because image_upload.status is 0 (see config schema type editor.image_upload_settings.*)., 4 [image_upload] 'max_size' is an unknown key because image_upload.status is 0 (see config schema type editor.image_upload_settings.*)., 5 [image_upload] 'max_dimensions' is an unknown key because image_upload.status is 0 (see config schema type editor.image_upload_settings.*).

/Users/wim.leers/core/core/lib/Drupal/Core/Config/Development/ConfigSchemaChecker.php:98
/Users/wim.leers/core/core/lib/Drupal/Component/EventDispatcher/ContainerAwareEventDispatcher.php:111
/Users/wim.leers/core/core/lib/Drupal/Core/Config/Config.php:230
…

… because those in the case of editor.image_upload_settings.0, none of those keys are defined.

[…] 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

🤔 Oh, I thought it was the mere additional complexity in this MR, I didn't realize it'd be this.

This I disagree with. 😅

I disagree because: where do we draw the line? Are we going to allow arbitrary "default optional values" everywhere in config where something is conditionally configured? For example, would we provide default configuration for some filter plugins in the Basic HTML text format just in case they get enabled at some point?

Note that the values in Demo Umami already have no meaning at all, they are just the materialized-to-config-YAML values that are actually defined in the logic at editor_image_upload_settings_form()!

And that is actually supporting the point I've been making (see #66 and earlier): this is usually just meaningless noise! Not only was this config not used, it's just values that happened to be the default values at the time the config was created 🙈

I'm fine with splitting off all the Editor-related changes to a separate issue. But then I'd prefer to extract the entirety of those changes, not just that one subset, because as I've shown above: it's neither trivially possible nor desirable. If the desirability in this particular case needs further discussion, just say so, and I'd be happy to split it off into a new issue 😊

Wonderful! 🥳

This allowed me to close the meta/plan issue at #3395099-33: [meta] Allow config types to opt in to config validation, use "FullyValidatable" constraint at root as signal to opt in to "keys/values are required by default", because only one issue remains in that plan: #3408158-6: Run config validation for config schema types opted in to be fully validatable, which is now unblocked 👍

This also means that we can truly start working on #2300677: JSON:API POST/PATCH support for fully validatable config entities, after 9.5 years of being blocked 🚀 🤯