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

@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

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

   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.

2. I want to make sure I understand correctly why that is even triggering validation errors for you? Is it because it's technically installing core config, so \Drupal\Core\Config\Schema\SchemaCheckTrait::isContribViolation() returns FALSE (since it only looks at the source of the original config, without the overrides), and that core config is being dynamically overridden?

I think the best solution for that particular problem (great catch!) would be to fix the KernelTestBase config install logic.

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.

with

**
 * @Constraint(
 *   id = "FullyValidatable",
 *   label = @Translation("Whether this config schema type is fully validatable", context = "Validation"),
 * )
 */
class FullyValidatableConstraint extends Constraint {}

and

class FullyValidatableConstraintValidator extends ConstraintValidator {

  /**
   * {@inheritdoc}
   */
  public function validate(mixed $value, Constraint $constraint) {
    // No-op.
    return;
  }

}

HTML fixes.

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

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!

Wim

Problem to solve: developers need some way to declare a config schema type as having been made fully validatable

Alex

Perceives type: mapping is getting new semantics.

Problem to solve: developers defining config schema types need to be able to opt in to new semantics.

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

Timeline:

1. #3324150: Add validation constraints to config_entity.dependencies introduced the ValidKeys constraint, and used it only for config dependencies — shipped in 10.1.x
2. #3374394: Add validation constraints to _core_config_info adopted it for type: _core_config_info — about to ship in 10.2.x
3. #3361534: KernelTestBase::$strictConfigSchema = TRUE and BrowserTestBase::$strictConfigSchema = TRUE do not actually strictly validate enabled running associated validation constraints automatically — and follow-ups restricted this to ONLY run for core tests, not contrib
4. #3376794: `type: mapping` should use `ValidKeys: <infer>` constraint by default 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 #3401837: Add basic validation to config schema definitions.)
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

• #3364109: Configuration schema & required values: add test coverage for `nullable: true` validation support
• #3364108: Configuration schema & required keys

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 Needs review for #11. 🙏

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

Clarified issue title.

@effulgentsia in #17:

Small rectification

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) → #3364108: Configuration schema & required keys
2. required values by default (opt out: nullable: true, matching what already exists in HEAD) → #3364109: Configuration schema & required values: add test coverage for `nullable: true` validation support

The actual review

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

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: Configuration schema & required keys — scheduled for tomorrow

What do you think, @effulgentsia? 🤓

As of #3364109-49: 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: 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: #3408165: Fully validatable config schema types: support dynamic types.

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.

🥳

With #3364108: Configuration schema & required keys having landed, this is fully complete in core, and now just needs #3408158: Run config validation for config schema types opted in to be fully validatable to allow contrib to opt in.

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

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 🚀 🤯