[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. #784672: Allow text field to enforce a specific text format 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.

EDIT: oh wait, you were referring to the opposite problem! 😅 I'll upload a new patch to see how that fares. I don't think it'll do anything either.

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.

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

🙈

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

⚠️ 3361423-config-schema-checking-not-quite-strict-nor-when-adding-4.patch passed

This means that keys that DO appear and are ABSENT in the config schema would not trigger config schema errors.

→ the "new keys" case

⚠️ 3361423-config-schema-checking-not-quite-strict-4.patch passed

This means that keys that DO NOT appear and are PRESENT in the config schema would not trigger config schema errors.

→ the "missing keys" case

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

What am I missing? 🙈

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.

SchemaCheckTrait::checkValue()'s behavior: validate available data against schema

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.

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

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, here's the thought process that goes with them:

1. Update \Drupal\Core\Config\Schema\SchemaCheckTrait::checkConfigSchema() to also validate the validation constraints specified in the config schema — #3361534: KernelTestBase::$strictConfigSchema = TRUE and BrowserTestBase::$strictConfigSchema = TRUE do not actually strictly validate 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: #3324150: Add validation constraints to config_entity.dependencies allows us to add:
   

     constraints:
       ValidKeys: '<infer>'
   

   This would get us one of the things we need: detect new keys that are not known in the installed config schema.

5. Next: enable it by default, by adding it to type: mapping …  but this will introduce the very problem that this issue was originally created for!
6. 🚀 This would result in the following exception (but I can't run StandardTest yet, there's too many bugs in core's config schema for now — stay tuned! 🤓):
   

   Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for field.field.node.page.body with the following errors: field.field.node.page.body:settings.allowed_formats missing schema, 0 [settings] 'allowed_formats' is not a supported key.
   

7. Test coverage was added that proves 👆 happens — see \Drupal\KernelTests\Core\Config\SchemaCheckTraitTest::testTrait()

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.

2. Only keys that have nullable: true specified should be allowed to be omitted, and that way we reuse that existing config schema property! 👍 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

For example style is not present, so it falls back to the default value defined in the config entity class, which is NULL:

  /**
   * The image style to use.
   *
   * @var string
   */
  public $style;

… 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 — check out the commit before the fix to observe the failure you'd get), 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.

1. We need some facility that allows specifying that some keys are new. This requires knowing either:
   • if the current version of Drupal core and every contrib module/theme is the latest release (i.e. with the most complete schema)
   • or whether the configuration being validated is shipped with a profile, distribution, module or recipe.

   I think the latter is feasible: the absence of _core.. Change record: https://www.drupal.org/node/2653358.

2. Distributions/installation profiles do not include this in their config — see for example Thunder and Open Social's config. Even if they currently include it, that'd be a bug and cause problems with all sorts of config management — see #3127915: Remove default config hashes from exported config for an example.

   Conclusion: AFAICT this is a viable heuristic! 🚀

All wrong. Turns out that ConfigInstaller sets this very early on, and so it'll already be set by the time config schema checking/validation run 😬 And even recomputing the hash is impossible, because the key order is not stable… 🤪

3. Fortunately that led me down a path that does seem to work:

              $config_to_create[$name] = [
                '_core' => [
                  'default_config_hash' => Crypt::hashBase64(serialize($config_to_create[$name])),
   +              'test' => drupal_valid_test_ua() !== FALSE,
                ],
              ] + $config_to_create[$name];
            }
   

   Together with some logic in ValidKeysConstraintValidator to trigger a deprecation error in case we're inside tests instead of a validation error (plus equivalent logic in to avoid complaining about missing schema), we get this (after commenting out all the <code>allowed_formats occurrences in the config schema, hence simulating testing on an older version):

   /opt/homebrew/bin/php /private/var/folders/x1/mlh998q15c70vs7_y8s1mgqm0000gp/T/ide-phpunit.php --configuration /Users/wim.leers/core/core/phpunit.xml --filter Drupal\\Tests\\standard\\Functional\\StandardTest --test-suffix StandardTest.php /Users/wim.leers/core/core/profiles/standard/tests/src/Functional
   Testing started at 1:39 PM ...
   PHPUnit 9.5.28 by Sebastian Bergmann and contributors.
   
   Testing /Users/wim.leers/core/core/profiles/standard/tests/src/Functional
   .                                                                   1 / 1 (100%)
   
   Time: 00:26.700, Memory: 10.00 MB
   
   OK (1 test, 245 assertions)
   
   Unsilenced deprecation notices (8)
   
     2x: The 'field.field.node.page.body' configuration contains invalid keys at the property path 'settings'. The following keys are either invalid or only exist in newer versions of the config schema: 'allowed_formats'.
       2x in StandardTest::testStandard from Drupal\Tests\standard\Functional
   
     2x: The 'field.field.node.article.body' configuration contains invalid keys at the property path 'settings'. The following keys are either invalid or only exist in newer versions of the config schema: 'allowed_formats'.
       2x in StandardTest::testStandard from Drupal\Tests\standard\Functional
   
     2x: The 'field.field.comment.comment.comment_body' configuration contains invalid keys at the property path 'settings'. The following keys are either invalid or only exist in newer versions of the config schema: 'allowed_formats'.
       2x in StandardTest::testStandard from Drupal\Tests\standard\Functional
   
     2x: The 'field.field.block_content.basic.body' configuration contains invalid keys at the property path 'settings'. The following keys are either invalid or only exist in newer versions of the config schema: 'allowed_formats'.
       2x in StandardTest::testStandard from Drupal\Tests\standard\Functional
   
   Process finished with exit code 1
   

   Comes with explicit test coverage in \Drupal\KernelTests\Core\Config\SchemaCheckTraitTest::testDeprecationForNewKeysInTests() 👍

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

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

P.S.: excellent nerdsniping 👏

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

Created child issues:

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

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 🤓😇

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.

Time for an update!

Since:

1. #3361534: KernelTestBase::$strictConfigSchema = TRUE and BrowserTestBase::$strictConfigSchema = TRUE do not actually strictly validate landed
2. #3364109: Configuration schema & required values: add test coverage for `nullable: true` validation support is RTBC
3. #3364109 is the blocker for the next step, #3364108: Configuration schema & required keys, which I'm working on right now

… everything is moving forward as intended 👍 The MR here served as a PoC, doing a subset of the work in all 3 of those issues. So I've just closed that MR, since the first part is already in core, the second part is RTBC and the third part is being worked on 👍

The one caveat: I had intended this issue to handle the "ignore config validation errors in contrib testing" part. But since @jibran over at #3361534-89: KernelTestBase::$strictConfigSchema = TRUE and BrowserTestBase::$strictConfigSchema = TRUE do not actually strictly validate reported that his https://www.drupal.org/project/dynamic_entity_reference contrib module started failing tests (even though there's not yet a 10.2.x branch! 😳👏), I've spun that out into its own issue: #3379899: Follow-up for #3361534: config validation errors in contrib modules should cause deprecation notices, not test failures.

That means we can keep this meta as a catch-all to ensure that contrib is not going to be disrupted by the introduction of config validation in core.

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

Republished this node. 👍

Since #42, a lot has changed. Most notably, #3379899: Follow-up for #3361534: config validation errors in contrib modules should cause deprecation notices, not test failures and #3402168: Follow-up for #3361534: Config validation errors can still occur for contrib modules, disrupting contrib 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 #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" 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, #3364108: Configuration schema & required keys finally landed, a precursor of which was in the #19–#32 PoC. The other part of the PoC landed in #3364109: Configuration schema & required values: add test coverage for `nullable: true` validation support.

So … AFAICT this is done. See also #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 more detail.

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

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. #3415861: ValidKeysConstraintValidator thrown by Config Inspector on Drupal 10.1.x and 10.2.x due to core bug triggered only by Config Inspector 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 on Drupal 10.1.x and 10.2.x due to core bug triggered only by Config Inspector. Let's figure it out there, I'll report back our findings here 😊

This is still causing troubles in contriblandia. See #3465501: Fix Deprecation warning for profiles view for Drupal 10.3. We're trying to fix a deprecation that requires changing default views. Due to #3458099: Views handler loading should respect configuration, it's "safe" to update default views provided by contrib modules to use the new handler IDs, since the actual handler used is determined by the Views Data API, not the views config (until 11.2.0 when #3458099 will be released). However, if you change the config, and run tests against 10.2.x core, the plugins don't exist, and the whole test run explodes due to strict config schema errors. See https://git.drupalcode.org/issue/profile-3465501/-/jobs/2317666

I'd love it if there was an easy way to globally set $strictConfigSchema = FALSE for an entire pipeline. Or whatever this issue proposes to allow contrib to ignore these errors that are only triggered during automated testing and have no impact on runtime behavior.

Haven't closely read every comment, but this seems like the right issue to raise this concern. 😅 Please let me know if I should be barking up another tree, instead.

Thanks!
-Derek

Related but slightly different problem: shipped config in modules, where schema is defined in another dependency (e.g. core) means that we cannot test with different versions of that dependency with strict config schema validation.