[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"

Wim Leers → credited alexpott → .

Wim Leers → credited bircher → .

Wim Leers → credited borisson_ → .

Wim Leers → credited effulgentsia → .

@effulgentsia proposed the following:

The idea I have that's only partially formulated is introducing a config schema property: strict, that when true would turn on validation for null/missing keys/values. So, the MR wouldn't need to add all those suppressions in SchemaCheckTrait and instead we could just start adding strict: true to the schema of the config objects we're ready to make strict. That would also give a way to let contrib/custom start opting into it gradually. The trick, however, would be how to cascade strict: true down: for example, if it should reset to false by default for plugins and third_party_settings.

That's an interesting proposal I think. Thoughts:

1. I don't think it makes sense to use this only for turning on/off "null/missing keys/values", but for all validation constraints specified. But maybe that's already what you meant?
2. WRT cascading: I think that we may not even need special casing of certain cases. I think it can be as simple as relying on types: we'd mark type: config_entity as strict: true, but config_entity:third_party_settings looks like this:
   

       third_party_settings:
         type: sequence
         label: 'Third party settings'
         sequence:
           type: '[%parent.%parent.%type].third_party.[%key]'
   

   a new type is present inside the sequence, and that type again has the opportunity to declare itself as strict: true or not

IOW, I propose:

1. each type (a top-level key in a *.schema.yml file) can specify strict: true, and nothing else can
2. for a given node in a config object's node tree, we only run validation constraints if its type has strict: true
3. a config object (simple config or config entity) is only fully validatable if all of the config types present inside it (directly or indirectly), have strict: true
4. Later, for example in Drupal 11 or 12, we could then choose to start triggering a deprecation error for non-strict types corresponding to simple config & config entities, to inform that this will become required in Drupal 12 or 13. This would lead to far fewer deprecation errors, which would be less noisy.

@Berdir 6 days ago:

the new stricter config validation in kernel tests does not like install profile config overrides  😕

I have a kernel tests that installs config from contrib and my own modules, to verify that this is valid. some of those configs in contrib modules have config overrides in my install profile. the config install logic picks that up just fine. but unlike a proper install, it uses those overrides right now. of course, those overrides have dependencies on other config entities that will only be installed later on

the actual installer pushes overridden config back and installs it later on, kernel tests don't do that

I can't install those other things earlier on because the config in said module depend on yet other config to be installed first, including config from the contrib module whose config is overridden

also found this: https://www.drupal.org/project/webform/issues/3400868

— https://drupal.slack.com/archives/C1BMUQ9U6/p1699629962549669

I see I never stated explicitly in the issue summary what the dual problem is that this is trying to solve. Updated it.

Discussed proposal #1 at DrupalCon Lille

I discussed

each type (a top-level key in a *.schema.yml file) can specify strict: true, and nothing else can

with @alexpott at DrupalCon Lille.

He had VERY strong concerns about this. In a nutshell: config schema was always intended to contain only relevant information for code interpreting the config schemas (i.e. YAML files) to be able to validate the actual config (i.e. different YAML files). The addition of strict: true would be irrelevant noise as far as software other than Drupal is concerned.

Proposal #2: a no-op FullyValidatable constraint

So if we should not introduce a new top-level key/concept in config schema, we must use one of the existing keys. Which one is the closest match to what we need?

The constraints key. Anything config schema type that wants to apply validation logic specific to its semantics/needs would already need to add that key anyway.

So we need:

1. to not introduce new keys
2. to signal that some config schema type definition has been made fully validatable by its maintainer
3. to make it possible to deprecate this signal X years from now, e.g. when Drupal 13 requires all config schema definition to have validation constraints (maybe this will never happen, that's fine too, but it MUST be at least possible)

Proposal:

1. each type (a top-level key in a *.schema.yml file) can specify
   

   constraints:
     FullyValidatable: true
   

   , and nothing else can

2. for a given node in a config object's node tree, we only run validation constraints if its type has the FullyValidatable: true validation constraint
3. a config object (simple config or config entity) is only fully validatable if all of the config types present inside it (directly or indirectly), have the FullyValidatable: true validation constraint
4. Later, for example in Drupal 11 or 12, we could then choose to start triggering a deprecation error for corresponding to simple config & config entities, to inform that this will become required in Drupal 12 or 13. This would lead to far fewer deprecation errors, which would be less noisy.

HTML fixes.

#7 turns out to have uncovered a massive contrib disruption, which merits its separate critical issue: 🐛 Follow-up for #3361534: config validation errors can still occur for contrib modules, disrupting contrib Active .

Removing

1. Confusion about the intended problem to solve

Had a long chat with @effulgentsia last night. Turns out that he was referring to something quite different than what I've been referring to!

2. Did the type: mapping semantics change in the past year?

Timeline:

1. ✨ Add validation constraints to config_entity.dependencies Fixed introduced the ValidKeys constraint, and used it only for config dependencies — shipped in 10.1.x
2. 📌 Add validation constraints to _core_config_info Fixed adopted it for type: _core_config_info — about to ship in 10.2.x
3. 📌 KernelTestBase::$strictConfigSchema = TRUE and BrowserTestBase::$strictConfigSchema = TRUE do not actually strictly validate Fixed enabled running associated validation constraints automatically — and follow-ups restricted this to ONLY run for core tests, not contrib
4. 📌 `type: mapping` should use `ValidKeys: ` constraint by default Fixed enabled it for type: mapping — about to ship in 10.2.x

The config schema cheatsheet → says this literally:

If all keys can be predefined in the schema structure, then you should use a mapping, otherwise you should use a sequence (even if the list’s keys are strings).

Or as a decision tree:

              YES  ┌──────────────┐
               ┌──►│type: sequence│
 ┌───────────┐ │   └──────────────┘
 │keys known?├─┤
 └───────────┘ │   ┌──────────────┐
               └──►│type: mapping │
              NO   └──────────────┘

In my experience, the author of a type: mapping always intends all keys to be specified. It's almost always an oversight to not do that. That oversight almost always occurs because:

1. the DX of writing config schemas is … not great. (For an example, see 📌 Provide guidance to config schema developers: detect broken config schema types Needs work .)
2. the config schema is not really used anywhere in Drupal — I always expected config to be validated against config schema and be told which values did not match, so that it provides tangible value for me as a module maintainer — but the only things Drupal core uses it for:
   1. verifying that the high-level structure (string/bool/int and array or not) is correct
   2. Config Translation

Where does that primitive type checking happen? Tests are using SchemaCheckTrait by default in tests (i.e. module maintainers already are being informed in the context of a test):

      'config_test.types:new_key' => 'missing schema',
      'config_test.types:new_array' => 'missing schema',
      'config_test.types:boolean' => 'non-scalar value but not defined as an array (such as mapping or sequence)',

Since the 3rd point + 4th point in the timeline, ValidKeysConstraintValidator on type: mapping surfaces those same problems (again, Drupal core only):

      '0' => "[new_key] 'new_key' is not a supported key.",
      '1' => "[new_array] 'new_array' is not a supported key.",
      '2' => '[boolean] This value should be of the correct primitive type.',

That's why IMHO the 4-point timeline above did not actually change anything semantics yet.

3. Would the type: mapping semantics change if we introduced the "required values" and "required keys" issues?

Would

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

change the semantics?

If we go back to our beloved config schema cheatsheet → says this literally:

Advanced properties

Config schema elements can take some advanced properties that are rarely used.

• Set nullable: true to explicitly define that a key is optional and may not be present or have value. Practically used for mappings and sequences to accept a NULL value in their place, see SchemaCheckTrait::checkValue().

Translation: nullable: true in HEAD A) can only be set on mappings and sequences, B) when set, it means that BOTH the key AND the value are optional (without distinction).

Where in Drupal core was nullable: true used? In 7 places in HEAD, 5 of which were added in the past year, the oldest one was introduced in 2016 in #1978714-73: Entity reference doesn't update its field settings when referenced entity bundles are deleted → .

That's the theory. In practice, I can modify SchemaCheckTraitTest to

1. remove a key (unset($config_data['float']);)
2. remove a value ($config_data['float'] = NULL;)

and in both cases, the test will still happily pass 🥲

This is what would change (and indeed both #3364109 and #3364108 modify that test coverage). This is where @effulgentsia is right that it changes the semantics … if and only if validation constraints are executed. And that's the key part: they would NOT be executed under the proposal I have here.

4. Conclusion

That's why I still believe my proposal #2 actually addresses @effulgentsia's concerns too:

1. constraints: { FullyValidatable: true } would be a signal to Drupal core that "this is now ready to have its validation constraints executed"
2. if #3364109 and #3364108 are only allowed to land if and only if point 1 is complete, then we SHOULD be able to remove the enormous accumulation in \Drupal\Core\Config\Schema\SchemaCheckTrait::$ignoredPropertyPaths in both of those issues' MRs, because then only those simple config & config entities that declare themselves fully validatable would actually be executing those validation constraints

On Monday I will implement proposal 2, and will verify that it allows me to remove ALL of the ignored property path additions in #3364109 and #3364108.

Unless on Monday I see that comments are awaiting me telling me why that will not work.

For now, marking for #11. 🙏

🐛 Follow-up for #3361534: config validation errors can still occur for contrib modules, disrupting contrib Active just landed 👍 Disruption disaster for contrib averted 🥳

Clarified issue title.

I think it makes perfect sense to allow config to explicitly opt into validation, if there are semantic changes that come about as a result of doing that. Having read Wim's write-up in #11, it sounds like his proposal would address @effulgentsia's concerns, although I was not part of their discussion so I can't say for certain. But what I can say is, based on what Wim wrote there, I support his proposal.

But what I can say is, based on what Wim wrote there, I support his proposal. I don't adore the idea of having to explicitly mark all schema types as "you can validate this" - it feels a little clunky - but it might be a necessary crutch for now, and as Wim points out, we can deprecate and remove it later.

I understand this, it also gives us a less clear list of changes to go trough to get config and tests in a better shape, as we don't simply have to follow one specific list. I guess we can still go after adding the constraint to all mappings that exist.

> That sounds like a bug in the config install logic in KernelTestBase (for install profiles specifically? 🤔) then, because the order of the installation does matter? Because "those overrides have dependencies on other config entities that will only be installed later on" sounds wrong for sure.

The thing is, kernel tests don't really have a concept of install profiles and due to dependency issues, my test-all-schema kernel test in two big install profiles keep getting more fragile/tricky to handle, due to dependencies. I usually need to install some modules, then their config, then some more modules, then more config.

Kernel tests by design are not a fully functional system and don't do a full install. I expect I'll just have to convert them to functional tests and do a full, complete install. Or just enable strict config schema checks/validation in the regular install on CI? Is there a settings.php flag that I could enable do do this?

#11 is a good summary of my concerns. To summarize even more, I'm concerned that it's not at all clear to module authors who write config schemas that the semantics of the schema is that every key that's in it is considered required-by-default unless it explicitly says nullable: true.

On the one hand:

• https://www.drupal.org/node/1905070 → has said exactly that since Oct. 2016.

On the other hand:

• It did not say that when Drupal 8.0.0 was released in 2015.
• I can't find any change record that says it, other than https://www.drupal.org/node/3362879 → which a) is for 10.2 which hasn't been released yet, and b) it only indirectly mentions it.
• A code comment in HEAD still explicitly says that Null values are allowed for all primitive types.

So, my biggest concerns with the MR in 📌 Configuration schema & required values: add test coverage for `nullable: true` validation support Fixed is that:

1. It's adding setRequired() to every non-nullable element, which means anything that calls isRequired() for those definition objects will start seeing TRUE where it used to see FALSE. Is that fixing a bug (by making the code consistent with what https://www.drupal.org/node/1905070 → has said for the last 7 years) or is it adding a BC break to a reasonable expectation that things are not required by default? It's hard to say.
2. By virtue of adding the setRequired(), it's also adding the NotNull constraint to enforce that, which makes sense if we agree that these should all be required, but for people who did not know that all config elements are required by default, it means that not only will calls to isRequired() return a different value than they did before, but that also new violations will get returned that weren't returned before.

The FullyValidatable constraint proposed in #11 addresses my 2nd concern in the above list.

I'm not sure what we want to do about the first concern.

Should we make the setRequired() change regardless of the FullyValidatable constraint? If we name the constraint "fully validatable", then I think we should, since I think it would be weird for the presence of a constraint to change whether or not something is semantically required, it should only affect whether or not we run other constraints or how we handle their violations (or whether we perform validation at all).

Or, in addition to the FullyValidatable constraint, should we also add a new schema property to mappings, such as allDescendantsRequiredByDefault, which is what I also proposed simply calling strict. That way, we don't change the return value of isRequired() except within structures that people opt into. The downside of this though, is that we'd be introducing a new schema property to do what https://www.drupal.org/node/1905070 → (but not our code) has already said we've been doing for 7 years.

I think it makes sense to proceed with FullyValidatable and not with strict for now and discuss whether or not we need/want a new strict property in a followup, but I think splitting it up that way would mean committing all this to 10.3.x only (and not to 10.2) so that we don't end up releasing a partial behavior change without having had time to work out the full impact of it.

It's adding setRequired() to every non-nullable element, which means anything that calls isRequired() for those definition objects will start seeing TRUE where it used to see FALSE. Is that fixing a bug (by making the code consistent with what https://www.drupal.org/node/1905070 → has said for the last 7 years) or is it adding a BC break to a reasonable expectation that things are not required by default? It's hard to say ... I'm not sure what we want to do.

Well, I'm not as familiar with this issue or problem space as either you or Wim, but...when we change behavior like this, even if making it match what was (mistakenly) documented...isn't it the convention to somehow issue a deprecation notice, in order to prevent hard BC breaks? Is there some way we could do that? Maybe if $definition['nullable'] is not set, we could issue a deprecation and warn that in Drupal 11, not setting that key will mean the property is treated as required?

@effulgentsia in #17:

semantics of the schema is that every key that's in it is considered required-by-default unless it explicitly says nullable: true.

This not exactly what is being proposed. I explained this previously in response to a question of yours, on September 2: #3364109-44: Configuration schema & required values: add test coverage for `nullable: true` validation support → . The proposal across the two issues in a nutshell:

1. required keys by default (opt out: requiredKey: false — alternatively optionalKey: true) → 📌 Configuration schema & required keys Fixed
2. required values by default (opt out: nullable: true, matching what already exists in HEAD) → 📌 Configuration schema & required values: add test coverage for `nullable: true` validation support Fixed

On the one hand: […] On the other hand: […]

🥲 This is indeed one of the gordian knots we find ourselves in with the config system today!

The FullyValidatable constraint proposed in #11 addresses my 2nd concern in the above list.

👍

I'm not sure what we want to do about the first concern. […] Should we […]

We can solve this using that same mechanism: the root of any data definition being processed is guaranteed to be a config schema type (i.e. a definition in a *.schema.yml file).
In simpler terms: we can make the setRequired() call in #3364109 that you're (rightfully!) concerned about we can make it only get applied when the containing config schema type definition has the FullyValidatable constraint specified.

This would then work exactly like the allDescendantsRequiredByDefault you proposed.

Working on this right now, my quick experiment shows this works 👍😊 I will update 📌 Configuration schema & required values: add test coverage for `nullable: true` validation support Fixed .

so that we don't end up releasing a partial behavior change without having had time to work out the full impact of it.

💯! 😊

Working on this right now, my quick experiment shows this works 👍😊

• required values now is opt-in #3364109-49: Configuration schema & required values: add test coverage for `nullable: true` validation support → and much smaller! 🥳
• required keys now is about to be opt-in #3364108-77: [PP-1] Configuration schema & required keys → — scheduled for tomorrow

What do you think, @effulgentsia? 🤓

As of #3364109-49: [PP-3] Configuration schema & required values: add test coverage for `nullable: true` validation support → and #3364108-81: Configuration schema & required keys → , both of thos issues have fully implemented #19. Their MRs show (the opt-in pattern drastically reduced their size + new tests now prove) that there is no change at all until a config schema type opts in 😊

I was originally hoping to change all of Drupal core at once, but I think this is a much better pattern — thanks to you, @effulgentsia.

Ready for review, @effulgentsia! 🏓

(Attempt to make this issue as clear and actionable as possible.)

This is hard-blocked on @effulgentsia for 15 days now (since #20). Pretty much all config validation progress is hard-blocked on "required values" (#3364109) and "required keys" (#3364108), both of which are blocked on this.

Status:

1. This issue needs sign-off from @effulgentsia to allow #3364109 + #3364108 to land.
2. Proposal 2 (see #11 + #19 for additional nuance) has been implemented in both of those issues, to prove its viability.

Why is this issue necessary and why do we need those "required values" and "required keys" issues?

• Because #3364109 removes the need for a lot of NotNull-sprinkling
• Because #3364108 adds validation that is otherwise impossible to add today.
• Both are crucial for making Recipes reliable too: #3405328: [meta] Make recipes safer to use in the real world by supporting config validation and rolling back a broken recipe → .

Both of those issues have had all their feedback addressed, have blocking child issues for all parts that can be extracted (and most of those have landed already).

@effulgentsia has approved this approach at #3364109-60: [PP-1] Configuration schema & required values: add test coverage for `nullable: true` validation support → 🙏🚀

He surfaced one remaining concern but proposed to do that in a follow-up, which I created: 📌 [PP-2] Fully validatable config schema types: support dynamic types Postponed .

Given that this is a meta issue, the above is almost enough to close this issue, so moving to RTBC. What is still necessary both on #3364109 and here is approval from a subsystem maintainer. And in this case, there's only one: Alex Pott. So assigning to him.

I've reviewed the underlying MRs and I think the opt-in approach via the FullyValidatable constraint is a great plan for contrib, a little bit painful for core, but the balance feels about right.

+1 for the approach and I agree that this plan is ready.

🥳

I'm triaging RTBC issues → . I skimmed the IS and comments and see in #24 that the maintainer has approved "Proposal 2: FullyValidatableConstraint". The IssueSummary should be updated to make the clear, adding tag.

I also skimmed the Change Record. I see only two things, I think it needs a better title (it does not read well) and the first line is a header with "Fully validatable" which isn't needed. It should just start with a 'nice' introductory sentence. I don't think the title needs to include the 'opt in' information so how about, "Stricter validation for config schema types is available"?

I have added tags for the above. I am also leaving at RTBC as I reckon someone here will notice the tags.

Cheers

Updated IS, changed the CR title and removed the first title. Writing a 'nice' introductory sentence would be helpful, but I don't have an idea on what that could look like.

Thanks for updating the issue summary, @borisson_. Removing the tag.

I went into the change record and wordsmithed it a bit. Tried to streamline some things and rephrase for clarity. So, removing the tag.

With 📌 Configuration schema & required keys Fixed having landed, this is fully complete in core, and now just needs 📌 [PP-1] Run config validation for config schema types opted in to be fully validatable Postponed to allow contrib to opt in.

Since only one issue remains, closing this meta: it has fulfilled its purpose 👍

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