Problem/Motivation
While working on #3324150: Add validation constraints to config_entity.dependencies and #3324140: Convert field_storage_config and field_config's form validation logic to validation constraints, we observed that it's very difficult to get a sense of how much of a particular config schema tree (for example, filter.format.*, user.settings or field.storage.*.*), or subtree (for example, filter.format.*.dependencies) is covered by validation constraints.
Steps to reproduce
N/A
Proposed resolution
Add test that allows us to track which config schema subtrees are 100% validatable. This would enable us to avoid regressing until we finish #2164373: [META] Untie config validation from form validation — enables validatable Recipes, decoupled admin UIs ….
IOW: add this test coverage until #2164373: [META] Untie config validation from form validation — enables validatable Recipes, decoupled admin UIs … is done, and then we remove it again. It'd be both a progress tracker and a regression preventer.
Remaining tasks
- ✅
Working test with visual output that makes it easy to see where Drupal core's config schema validatability stands — both at the high level (config entity types + config schema types) and the nitty gritty detail (the property paths underneath each type) with corresponding helpful output (list and tree, respectively).→ #3 - ✅
Validate (pun intended) this in at least one other core issue where validation constraints are being added to config.→ #3324140-8: Convert field_storage_config and field_config's form validation logic to validation constraints - Get to green
- Get core committer buy-in to add this test coverage to Drupal core.
User interface changes
None.
API changes
None.
Data model changes
None.
Release notes snippet
N/A
| Comment | File | Size | Author |
|---|---|---|---|
| #37 | validatability.png | 218.07 KB | wim leers |
| #34 | 3324984-34.patch | 29.32 KB | wim leers |
| #34 | interdiff.txt | 4.37 KB | wim leers |
| #29 | 3324984-29.patch | 28.34 KB | wim leers |
| #29 | interdiff.txt | 1.77 KB | wim leers |
Comments
Comment #2
wim leersImproved proposal.
Once
testConfigEntityTypes()proves that all config entity types are 100% validatable (or just one… we have to start somewhere!), we can start enablingPATCH,POSTandDELETEvia JSON:API! 🚀An interesting caveat there is that we'd have to make sure that:
third_party_settingsmade by contributed or custom modules do not reduce the validability to less than 100%. So chances are that parts of the testing infrastructure I'm proposing here will continue to live on in the actual validation logic (we may want to verify that the unvalidatable subtrees do not intersect with the data received for a config entity in a
PATCHorPOSTrequest).Comment #3
wim leersThis was mind-boggling to write. But it now exists! Will follow up with issue summary update and detailed comments later. But the results speak for themselves:
Full detail:
Comment #4
wim leersDrupal CI fails completely if you don't specify
@groupapparently, even though the commit hooks don't complain 🤷Comment #5
wim leersI forgot that
Choicewas not registered by core; that the CKEditor 5 module had to work around it.Fixing that seems very reasonable, but should happen elsewhere.
Comment #6
wim leersIssue summary updated 👍
Patch overview
Why a patch? Because A) I doubt this will land in Drupal
10.1.x, B) I want to be able to test this against multiple core branches to observe if there are differences wrt config schema.Also, if this were a merge request, then changing it to target a next minor branch in the future would be impossible — GitLab is unfortunately not able to do that 🤷
booleanis one of the dozen or so "basic types" (that's the term thatcore.data_types.schema.ymluses).For
stringand others we cannot add validation, because we don't know what shape of string is expected.But for boolean, it's clear and consistent: it's either
falseortrue, it can't be anything else.Note: what's tricky is that this still technically allows ~ (
NULL) … unless we addNotNull. But IMHO the whole point of all of these basic types is that they do contain some value, not NULL, so they should all get it.Fortunately,
\Drupal\Core\TypedData\TypedDataManager::getDefaultConstraints()handles that for us. IMHO that should be moved into the config schema for explicitness, but that's subjective. 🤓This is by far the most important change in this patch.
It uses the
ValidKeysvalidation constraint's<infer>setting, which was introduced recently, in #3324150: Add validation constraints to config_entity.dependencies, and comes with strong test coverage (see\Drupal\KernelTests\Core\TypedData\ValidKeysConstraintValidatorTest).The comment captures what it does. It makes
type: mappingall across Drupal core actually sanely validatable by default! 🚀The whole point of
type: mappingis that you have known keys. If you don't know, you have to usetype: sequence. The thing is though that there is no validation happening on this today. By adding this constraint, we allow future code that validates config to actually discover the keys in mappings that are not known to the config schema, i.e. that violate the config schema.This has zero consequences today because no configuration is validated. (It's only checked if it matches the structure of the config schema, but only fairly loosely.)
These are bugs in the config schema introduced many years ago (these types literally don't exist!), but the absence of test coverage means this was never noticed.
🐛 This is a more complex bug in core's config schema. The
filtertype definition looks like this:That dynamic type
filter_settings.[%parent.id]matches this as well as several other types. Note how this means there's a circular dependency:filter→filter_settings→filteretc.👍 These are the dozen or so basic types, which have no
typespecified and (mostly) have noconstraintsbecause they're so low level.ℹ️ Speaks for itself, eventually we'll remove this and instead flip this around: once the majority of config schema definitions/types actually is validatable.
ℹ️ The goal is to make this non-empty — see #3324140: Convert field_storage_config and field_config's form validation logic to validation constraints for the first ones.
ℹ️ These are the two test methods. One focuses on config entity types (which is probably the thing with a bigger ecosystem impact, so higher priority), the other checks literally every type in config schema!
This currently takes ~16 seconds to run on my laptop.
We may want to add more test methods in the future — for example to help ensure all configuration that core ships with is actually valid. (To gently ease into actually validating configuration in the future.)
🤓 Helper classes for this test — trying to avoid Arrays of Doom ™️.
Comment #7
wim leersThe
ValidKeys: ['<infer>']constraint made one reasonable assumption through anassert()that requires a little bit more nuance now that we're using it across all of core 😊Comment #8
wim leersCompleted : #3324140-8: Convert field_storage_config and field_config's form validation logic to validation constraints.
Comment #9
wim leersThis is causing most all the CKEditor 5 functional JS test failures. Because Drupal core allows
booleanis one of the dozen or so "basic types" (that's the term thatcore.data_types.schema.ymluses).For
stringand others we cannot add validation, because we don't know what shape of string is expected.But for boolean, it's clear and consistent: it's either
falseortrue, it can't be anything else.Note: what's tricky is that this still technically allows ~ (
NULL) … unless we addNotNull. But IMHO the whole point of all of these basic types is that they do contain some value, not NULL, so they should all get it.Fortunately,
\Drupal\Core\TypedData\TypedDataManager::getDefaultConstraints()handles that for us. IMHO that should be moved into the config schema for explicitness, but that's subjective. 🤓So let's go with the flow; stop using
Choice, and just explicitly assignPrimitiveTypeinstead.Comment #11
wim leersLooks like this issue's introduction of
ValidKeys: ['<infer>']is largely making the test-only validation of one specific mapping obsolete — excellent, that's exactly what the point is! Keeping that explicit test coverage around though — it was last refined in #2925689: ConfigValidation class contains code that is brittle and changing for every addition, but introduced in #1928868: Typed config incorrectly implements Typed Data interfaces.Comment #12
wim leers→ yay, the automatic addition of
constraintsto all subtypes ofmappingis causing a test to fail that tests exactly that 👍Now the only remaining test failure will be the test that this issue is adding, which is exactly the point 👍
Comment #13
wim leersIdentified 3 pre-existing (but stalled for years) issues that will move the needle a lot on the reported percentages:
(Made them children of #2164373: [META] Untie config validation from form validation — enables validatable Recipes, decoupled admin UIs ….)
Comment #16
wim leers#12's only failure is the new test, so … ready for review!
(I wish DrupalCI supported the concept of "allowed failure" because this would be the perfect use case for that.)
Comment #17
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #18
andypostThe
use_more_textshould belabelbut sometimes it saved asfalsebecause it should be translatableThank to @chx https://drupal.slack.com/archives/C1BMUQ9U6/p1683905207064219
Comment #19
andypostFiled child issue #3360973: Convert config schema to label for views use_more_text
Comment #20
wim leersI think that based on what I did at #3359418: Expose validation constraint violations in Config Inspector UI and drush command, I should be able to simplify things here 🤞
Comment #21
wim leersComment #23
borisson_@Wim Leers, can we mark this test as incomplete, so that we can allow this to run on d.o infrastructure and have output from it, without it having to sit in the queue?
https://docs.phpunit.de/en/10.2/writing-tests-for-phpunit.html#incomplet...
Comment #24
wim leersSince #12, a handful of crucial issues have landed:
type: machine_name: #2920678: Add config validation for the allowed characters of machine namestype: _core_config_info: #3374394: Add validation constraints to _core_config_infotype: langcode: #3373653: Add a `langcode` data type to config schemaThe last 2 both in the past 2 weeks!
These have made the following fully validatable:
type: config_objectcomment.settingsmedia_library.settingsmenu_ui.settingsnode.settingssystem.feature_flagsRerolled for all that, and compared to ~6 months ago in #3:
🚀 Some real progress! 🤩
#23: agreed, and looking into that 👍
Comment #25
wim leersSelf-review after ~6 months of absence and progress in Drupal core:
This was done because:
— #6.1
… but as we're trying to get this to a committable state, reverting it.
Comment #26
wim leersIn addition to #25, I still stand by the rationale in #6.2 to do
… but this is doing far more than just "create test". So let's remove that from the scope here and make this about only a test. Created #3376794: `type: mapping` should use `ValidKeys: <infer>` constraint by default for that.
Comment #29
wim leersApplying the trick that @borisson_ proposed. That indeed should make tests pass!
If committed, this would:
\Drupal\KernelTests\Core\Config\ConfigSchemaValidatabilityTest::FINISHED_CONFIG_SCHEMA_SUBTREESTo get the detailed output (as shown in e.g. #3), you'd have to run it locally and uncomment the two lines that contain this:
Comment #30
wim leerswas added in #5 and is obsolete thanks to #3373653: Add a `langcode` data type to config schema.
Marking for:
Comment #31
borisson_Mixing inheritdoc and documentation is not allowed, so we need to copy over the rest of the documentation from the base implementation
this needs docs
We need to file a followup or fix this in this issue as well.
I'd prefer a followup because this is already very helpful as is.
This can also be false when a complete schema is fixed; not sure if we need to change the message here.
I don't think we should, so can be ignored.
brackets are not needed here I think.
Needs docs
needs docs
needs docs
needs docs
Comment #32
wim leers#31: thanks, indeed all those things I still need to address 👍 #30.2 should actually address the majority of those 😊
Meanwhile, #3376794: `type: mapping` should use `ValidKeys: <infer>` constraint by default landed, and led to another big boost! Or … it should have, but the numbers remain identical 🤔 That must be a bug. I do remember working on #3359418: Expose validation constraint violations in Config Inspector UI and drush command and starting from the code I wrote here, but realizing I could massively simplify it and make it more accurate. I'm hopeful that doing #30.2 will also fix that. 🤞
Comment #33
wim leersUpdate after many more issues landed.
Update since ~1 month ago in #24:
🚀 Some real progress! 🤩
Tackling #30 + #31 tomorrow.
Comment #34
wim leersThis is how the numbers in #33 were computed.
Comment #35
wim leersThis is an unsustainable core patch until such a time that it lands in core. It'll eventually be useful to do something like this … but only when we actually reach 100%, to avoid regressing.
To be clear, we did make a ton of progress in the past few months:
Twelvefully validatable simple config objects right now!👆 That was wrong, thanks @effulgentsia for pointing that out! Looks like I simply did not realize that
system.menu.*(meaningMenuconfig entities) andshortcut.set.*(meaningShortcutSetconfig entities) are in fact fully validatable! 🤯So that means 6 simple config and 2 config entity types, of which there are 7 occurrences (1 shortcut set, 5 menus).
Comment #36
wim leersFYI: Over at #3391990: Automated report on core config validatability, I'm working to generate a nice automated report, which eventually should also include charts 🤓

The very first run is happening right now: https://git.drupalcode.org/project/config_inspector/-/jobs/147889 🤩
Comment #37
wim leersOn Dec 30, #3391990: Automated report on core config validatability landed. It now generates an up-to-date visualization of core config validatability daily.
It is currently underestimating because #3364109: Configuration schema & required values: add test coverage for `nullable: true` validation support did not update the ones listed/surfaced by #35. Fixing that in #3412084: Follow-up for #3364109: opt in already validatable simple config to FullyValidatable.
Comment #38
wim leersThanks to #3422641: Add a "Validatable config" tests job to GitLab CI to help core evolve towards 100% validatability having landed, we don't need this anymore! 🚀