[meta] Make config schema checking something that can be ignored when testing contrib modules

In 10.0.0 and 9.5.x, this feature doesn't exist. At runtime, the system will just ignore the key it doesn't understand, and the text field will show all the formats. So the older sites don't get the benefit of the new feature, but also nothing breaks. This is fine.

IOW: it sort of works only A) by chance, B) because there is logic in the PHP that interprets this configuration to be missing to mean that it should fall back to some default value:

  public function defaultValuesFormValidate(array $element, array &$form, FormStateInterface $form_state) {
    if ($allowed_formats = $this->getSetting('allowed_formats')) {

treats the default value of [] and the absence (which would return NULL — see \Drupal\Core\TypedData\DataDefinition::getSetting()) as the same

Note that right now the absence of a key in a mapping does not cause a schema error!

Which means that at least the example in the issue summary would today never cause tests to fail! 🙈

Let's verify that claim:

1. Let's find a test that calls \Drupal\Tests\SchemaCheckTestTrait::assertConfigSchema() to check this.
2. For example \Drupal\Tests\standard\Functional\StandardTest does:
   

       // Now we have all configuration imported, test all of them for schema
       // conformance. Ensures all imported default configuration is valid when
       // standard profile modules are enabled.
       $names = $this->container->get('config.storage')->listAll();
       /** @var \Drupal\Core\Config\TypedConfigManagerInterface $typed_config */
       $typed_config = $this->container->get('config.typed');
       foreach ($names as $name) {
         $config = $this->config($name);
         $this->assertConfigSchema($typed_config, $name, $config->get());
       }
   

3. ✨ Allow text field to enforce a specific text format Fixed modified 4 files with default configuration to have the new allowed_formats key. Let's remove it from all of them and see what happens.

This is for the inverse: a new key appears in a mapping that was not known. #4 is when a key is absent that config schema says should be present.

 public function defaultValuesFormValidate(array $element, array &$form, FormStateInterface $form_state) {
    if ($allowed_formats = $this->getSetting('allowed_formats')) {

This code isn't in Drupal 10 so it would never run. The issue is Drupal 10.1-updated configuration on Drupal 10.0 sites. Note I didn't test this, just wrote down more or less what @alexpott talked about :)

00:02:53.788   ERROR: Unknown argument '--group'.
00:02:53.788 PHP Warning:  Trying to access array offset on value of type null in /var/www/html/core/scripts/run-tests.sh on line 1270

🙈

The last submitted patch, 8: 3361423-config-schema-checking-not-quite-strict-nor-when-adding-3.patch, failed testing. View results →

Getting a single test to run on DrupalCI is not the simplest thing. 😅 Sorry for all the noise!

Can you provide a patch that demonstrates the problem described in the issue summary? Because AFAICT these 2 patches disprove it? 😅

Aha, I think that y'all were expecting this to trigger:

    if ($element instanceof Undefined) {
      return [$error_key => 'missing schema'];
    }

in \Drupal\Core\Config\Schema\SchemaCheckTrait::checkValue().

That even has test coverage for the case reported in the issue summary: \Drupal\KernelTests\Core\Config\SchemaCheckTraitTest::testTrait() … except that it only works for top-level keys.

There is an important omission in \Drupal\Core\Config\Schema\SchemaCheckTrait::checkValue() that is AFAICT intentional: it validates only the data it is given, and does not at all check if keys are missing that should be present.

This omission has been present since day 1, i.e. since #2167623: Add test for all default configuration to ensure schema exists and is correct → .

It means that you could pass an empty array as the data for any piece of configuration and \Drupal\Core\Config\Schema\SchemaCheckTrait::checkConfigSchema() will consider it valid. Example attached.

The last submitted patch, 13: 3361423-schema-checking-considers-all-empty-config-valid2.patch, failed testing. View results →

Nice work figuring this out, that means at least the text field example is a non-issue then. I can't think of any other examples off-hand where we might run into things.

Based on #16 going to mark this. For the committer #15 does include a drupalci.yml change figured could be omitted when committing.

I don't think #15 is meant for commit.

Indeed it is not.

So how can we fix both #11 (i.e. detect new keys that are not known in the installed config schema AND detect missing keys) and #12 (detect missing keys at EVERY level)?

I think I see a way! 😄

(Been working on it for >12 of the past 24 hours — I've been nerdsniped by @catch & @lauriii 🙈)

4 commits, 4 roughly matching bullets:

1. Update \Drupal\Core\Config\Schema\SchemaCheckTrait::checkConfigSchema() to also validate the validation constraints specified in the config schema — 📌 KernelTestBase::$strictConfigSchema = TRUE and BrowserTestBase::$strictConfigSchema = TRUE do not actually strictly validate Fixed does that.
2. This still won't do anything for the originally reported problem because type: mapping does not have any validation constraints.
3. Note that there is one mapping in Drupal core's config schema that already does this: config_test.validation, which has:
   

     constraints:
       Callback:
         callback: [\Drupal\config_test\ConfigValidation, validateMapping]
   

4. Write a validation constraint that verifies that all keys defined for a mapping are present in the config schema. Already done: ✨ Add validation constraints to config_entity.dependencies Fixed allows us to add:
   

     constraints:
       ValidKeys: '<infer>'
   

   This would get us one of the things we need: .

Next batch of commits will make it more interesting.

So in the last of the aforementioned commits, we enabled it by default. The test coverage that we added proved that unknown keys trigger errors.

But what about required keys? There are plenty of examples in Drupal core where code expects certain config keys to be present. As demonstrated in #11 and #12

So let's make that possible!

1. So let's add a new constraint, similar to ValidKeys, but doing the opposite: not just allowing keys, but requiring keys: RequiredKeys. This too should support
   

     constraints:
       RequiredKeys: '<infer>'
   

   i.e. by default all keys should be required.

I realized (after a LOT of debugging 🙈) that this cannot work, because we still need to be able to distinguish between optional keys vs optional values. There are plenty of examples where some piece of configuration (some key-value pair) may be required but have an optional value — in fact, those already exist!

For example: editor.editor.*:image_upload.max_dimensions.width MUST be present in all Text Editor config entities, but it may be set to null to indicate there is no maximum image width.

So we need a new key. This would be specific to type: mapping. I think requiredKey: false would make sense: the absence of this in the config schema would imply requiredKey: true 👍

Comes with explicit test coverage in \Drupal\KernelTests\Core\Config\SchemaCheckTraitTest::testTrait().

#21 handled new/unknown keys.

#22 handled missing/required keys.

#22 then is closely related to the concept of "required data". But it applies it only for a subset of things: keys in mappings. That's not very consistent. It should be applied to everything. If configuration schema validation historically only checks the storage type, then the absence of a value is equivalent to NULL.

In other words: if nullable: true is not present, we should assume that type: string maps to VARCHAR NOT NULL, type: int maps to INTEGER NOT NULL et cetera in the database.

Sure enough, Typed Data already supports this: \Drupal\Core\TypedData\DataDefinition::isRequired() and \Drupal\Core\TypedData\DataDefinition::setRequired(). We simply don't use it for config yet! But we can:

1. First, let's call ::setRequired(): everything is required unless nullable: true is present.
2. … which led to discovering that NotNullConstraintValidator does not yet work correctly for sequences and mappings! 😱

Rather than tell you, let me show you, with a failing test. This will show

Exception: Exception when installing config for module config_test, message was: Schema errors for config_test.dynamic.dotted.default with the following errors: 0 [style] This value should not be null., 1 [size] This value should not be null., 2 [size_value] This value should not be null.

for core/modules/config/tests/config_test/config/install/config_test.dynamic.dotted.default.yml, which looks like this:

id: dotted.default
label: Default
weight: 0
protected_property: Default
# Intentionally commented out to verify default status behavior.
# status: 1

… and setting nullable: true in the commit I just pushed makes it pass 👍

#23 explained the problem shown in the 2 commits preceding it.

#24 then proved it can work with one commit.

The 3 commits I just pushed add test coverage (and a small fix), proving it now works for all types.

1. What's crucial: the key may be required while the value is optional!
2. The whole nullable: true and "required value" aspect explained in #23 may seem out of scope, but after having spent hours debugging things I realized I was conflating the two concepts 🙈 So while we definitely can (and should!) independently land those aspects, I felt it was important for clarity and confidence to ensure the difference is clear.
3. 👆 The test coverage in \Drupal\KernelTests\Core\Config\SchemaCheckTraitTest::testTrait() proves that this is true for every type. 🚀

That means that now we're finally in a place where we can truly start to solve the problem this issue was originally created for! 😄

That is: gracefully handling keys in configuration that ships with module FOO, but which are specified in the config schema only in Drupal core 10.1 while the module still wants to be compatible with (and tested against) 10.0 and 9.5.

In the 3 commits I just pushed, I added the infrastructure to make that happen, plus test coverage to prove it works.

I've now proven it can all work in SchemaCheckTraitTest.

But if you're anything like me, you're skeptical. "Will this work for all of core?"

Well, let's find out. Let's run \Drupal\Tests\standard\Functional\StandardTest too.

I've now proven it can all work in SchemaCheckTraitTest. And I show some output for the Standard install profile showing deprecation errors.

But if you're anything like me, you're skeptical. "Will this work for all of core?"

Well, let's find out. Let's run \Drupal\Tests\standard\Functional\StandardTest too. That's the commit I just pushed.

There was 1 error:

1) Drupal\Tests\standard\Functional\StandardTest::testStandard
Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for system.file with the following errors: 0 [] 'path' is a required key.

😬

Pushed commit that fixes default config.

No more default config errors, but just config schema problems now, because surely third party settings should not be required:

There was 1 error:

1) Drupal\Tests\standard\Functional\StandardTest::testStandard
Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for system.theme.global with the following errors: 0 [] 'third_party_settings' is a required key., 1 [logo.url] This value should not be null.

😬

Pushed commit that fixes config schema: lots of requiredKey: false additions, some nullable: true additions, a few plain bugfixes.

Now fast-forwarding a bit: shortcut_themes_installed() generates invalid config and views.* config … is not something we'll get squeaky clean in this issue. Pushed 2 commits for those.

Testing Drupal\Tests\standard\Functional\StandardTest
E                                                                   1 / 1 (100%)R

Time: 00:07.143, Memory: 4.00 MB

There was 1 error:

1) Drupal\Tests\standard\Functional\StandardTest::testStandard
Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for core.entity_view_display.comment.comment.default with the following errors: 0 [content.links] 'type' is a required key., 1 [content.links] 'label' is a required key., 2 [content.links] 'settings' is a required key.

The RequiredKeys constraint needs one more capability to support this kind of nuanced reuse of config schema types. For entity view displays, links is a special case, and for entity form displays, author is.

Pushed commit for that.

Testing Drupal\Tests\standard\Functional\StandardTest
E                                                                   1 / 1 (100%)R

Time: 00:07.143, Memory: 4.00 MB

There was 1 error:

1) Drupal\Tests\standard\Functional\StandardTest::testStandard
Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for core.entity_view_display.comment.comment.default with the following errors: 0 [content.links] 'type' is a required key., 1 [content.links] 'label' is a required key., 2 [content.links] 'settings' is a required key.

The RequiredKeys constraint needs one more capability to support this kind of nuanced reuse of config schema types. For entity view displays, links is a special case, and for entity form displays, author is.

Pushed commit for that.

And … finish! 🏁

Linking related issues.

Posted a review on gitlab, back to needs work to at least answer them

Thanks for the early review!

Quoting myself from Drupal Slack:

I couldn’t get this out of my head and kept working on it — the vision you originally had, @catch & @alexpott, is now proven to be feasible, and it’s passing tests for all of Standard 🤓😄

I propose to change this to a meta and split it off into child issues. Roughly one patch stack with matching comment should become one child issue.

I spent many, many hours making this as clear and cohesive as possible. I’d very much appreciate your input :pray: And I hope y’all will be excited about this 😊
https://www.drupal.org/project/drupal/issues/3361423#comment-15080360 🌱 [meta] Make config schema checking something that can be ignored when testing contrib modules Active

P.S.: excellent nerdsniping 👏

— https://drupal.slack.com/archives/C1BMUQ9U6/p1685219644013669?thread_ts=...

Created child issues:

1. 📌 Configuration schema & required values: add test coverage for `nullable: true` validation support Fixed (corresponds roughly to #23 through #25), which blocks:
2. 📌 Configuration schema & required keys Fixed

The first one extracts already ~20% from this MR into an isolated issue. The second one should extract even more. First getting those to green, will then continue splitting this up into child issues 👍

Assigning to @catch to hopefully get a high-level +1 on this direction 🤓😇

I'm mostly afk this week but I feel bad about the nerdsniping ;)

I don't think when @alexpott mentioned this issue or when I typed it up, either of us had fully considered the implications - i.e. we (or at least I) didn't realise there was no validation error on the 'extra' keys in the first place.

The combination of much more precise validation but also being able to ignore some of these on older versions seems good. Agreed on making as many bugfixes prerequisites of this as possible - it would be good to commit it earlier than later in a minor release so that contrib has time to catch up.

Unassigning me because I think this is a good idea, but tagging for subsystem maintainer review because config schema isn't really my area.

Clarifying title. The behavior per core branch is owned by the code in that older core branch/version. Which is always true.

The goal of this issue is to minimize contrib disruption.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Two times in two days now that I hit the "concurrent node edit causes node to get unpublished" bug on d.o 😳

Since #42, a lot has changed. Most notably, 📌 Follow-up for #3361534: config validation errors in contrib modules should cause deprecation notices, not test failures Fixed and 🐛 Follow-up for #3361534: config validation errors can still occur for contrib modules, disrupting contrib Active landed, which made it so that config validation using schema truly is ignored.

Drupal 10.2 has been out for 3 weeks, and there are no reports to my knowledge of contrib modules breaking (there were during the RC phase, which is why #3402168 happened). 👍

We now need 📌 Follow-up for #3361534: config validation errors in contrib modules should not cause deprecation notices, unless they opt in Active to enable contrib/custom modules to opt in.

AFAICT that means this issue is … de facto done? 😄

Per the research I did in #11 through #15, it appears that I was able to prove that the original reported issue is a non-issue. This is from May 2023 though, so ~7 months ago. We should double-check. At least I was able to convince @catch 🤓

What I subsequently did was prove this through actual code + tests in #19 through #32. Back in May.

~12 hours ago, 📌 Configuration schema & required keys Fixed finally landed, a precursor of which was in the #19–#32 PoC. The other part of the PoC landed in 📌 Configuration schema & required values: add test coverage for `nullable: true` validation support Fixed .

So … AFAICT this is done. See also 📌 Follow-up for #3361534: config validation errors in contrib modules should not cause deprecation notices, unless they opt in Active for more detail.

Leaving this to @catch or @alexpott to confirm and mark this .

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

I'm not sure how related or unrelated this is, but search_api_solr in 10.2 is throwing fits with the new config validation. 🐛 ValidKeysConstraintValidator thrown by config inspector Active has some details on what I'm seeing.

#48: I suspect that requires a development module to be installed: #3415861-3: ValidKeysConstraintValidator thrown by config inspector → . Let's figure it out there, I'll report back our findings here 😊