`type: mapping` should use `ValidKeys: <infer>` constraint by default

Clarifying the current status of Drupal core.

At this point, we could choose to remove the test-only validation constraint:

diff --git a/core/modules/config/tests/config_test/config/schema/config_test.schema.yml b/core/modules/config/tests/config_test/config/schema/config_test.schema.yml
index 2a707facb0..87f84bb9e3 100644
--- a/core/modules/config/tests/config_test/config/schema/config_test.schema.yml
+++ b/core/modules/config/tests/config_test/config/schema/config_test.schema.yml
@@ -172,9 +172,6 @@ system.action.*.third_party.config_test:
 config_test.validation:
   type: config_object
   label: 'Configuration type'
-  constraints:
-    Callback:
-      callback: [\Drupal\config_test\ConfigValidation, validateMapping]
   mapping:
     llama:
       type: string

… because it literally is surfacing the same problem that ValidKeys: '<infer>' surfaces, but with more precision. Applying the above diff would result in

    // 2 constraint violations triggered by the default validation constraint
    // for `type: mapping`
    // @see \Drupal\Core\Validation\Plugin\Validation\Constraint\ValidKeysConstraint
    $this->assertSame('', $result->get(0)->getPropertyPath());
    $this->assertEquals("'elephant' is not a supported key.", $result->get(0)->getMessage());
    $this->assertSame('', $result->get(1)->getPropertyPath());
    $this->assertEquals("'zebra' is not a supported key.", $result->get(1)->getMessage());

keeping this 👆 and removing this 👇

    // 1 additional constraint violation triggered by the custom
    // constraint for the `config_test.validation` type, which indirectly
    // extends `type: mapping` (via `type: config_object`).
    // @see \Drupal\config_test\ConfigValidation::validateMapping()
    $this->assertEquals('', $result->get(2)->getPropertyPath());
    $this->assertEquals('Unexpected keys: elephant, zebra', $result->get(2)->getMessage());

However, it's harmless to keep both. And in a way, it serves as a nice example to show that it's always possible to add additional validation constraints. So I vote to keep it as-is.

Thanks to \Drupal\Core\Config\TypedConfigManager::determineType() and ::getFallbackName(), e.g. type: field.storage_settings.[%parent.type] automatically resolves to field.storage_settings.boolean, but then falls back to field.storage_settings.* (because there's no specific settings for the boolean field type).

However, type: field.storage_settings.* does not specify a concrete mapping … and nor does type: mapping! That's what this was crashing on.

So either we need to re-create all of the inheritance/extension logic in \Drupal\Core\Validation\Plugin\Validation\Constraint\ValidKeysConstraint::inferKeys() … or we just rely on \Drupal\Core\Config\TypedConfigManager::getDefinitionWithReplacements() doing that for us like it has done for the past ~decade. Then all we need to is ensure that even the root type (type: mapping) has a list of allowed mapping keys specified, even if that list is empty. That way, it is nicely inherited down the chain (type: mapping → type: field.storage_settings.* →

Then everything starts working! 👍

In comment #4 Wim wasn't sure about keeping the ValidMapping constraint. It is a different implementation, so not completely the same and I agree that it is valuable to keep both. It is a good example to keep.

I'm not sure I completely understand the comment in #5, but I can see why setting the ValidKeys: '<infer>' on the root mapping type bubbles up, so that makes a lot of sense.

This is probably the simplest of all the config schema validation mapping issues that was committed over the last few weeks and it provides some additional strictness. It is weird to see nothing in core actually have an error against this, as opposed to some of the other issues, but the test coverage proves this works.

It is weird to see nothing in core actually have an error against this, as opposed to some of the other issues, but the test coverage proves this works.

It's not that weird, because we're not validating all config in Drupal core yet. 📌 KernelTestBase::$strictConfigSchema = TRUE and BrowserTestBase::$strictConfigSchema = TRUE do not actually strictly validate Fixed would change that, and would likely trigger additional failures.

Regarding #5: see the test failures for the commits prior to the last, they contain output like this:

assert(array_key_exists('mapping', $definition)) in /var/www/html/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/ValidKeysConstraint.php:93

That means the mapping key-value pair was not defined on the given config schema type definition. So, for example, field.storage_settings.* does not contain it. And hence the assertion failed.

By adding mapping: {} to type: mapping's definition, we ensure that the mapping key-value pair is always defined for all mappings (and child types such as config_object, as well as grandchild types such as field.storage_settings.*). That's what #5 tried to explain, probably not concretely enough 🙈

Now that I am more confident I'll RTBC this one, let's break 📌 KernelTestBase::$strictConfigSchema = TRUE and BrowserTestBase::$strictConfigSchema = TRUE do not actually strictly validate Fixed even harder.

🤣 Thanks… I guess? :P

@Wim, can you reroll this issue now that 📌 KernelTestBase::$strictConfigSchema = TRUE and BrowserTestBase::$strictConfigSchema = TRUE do not actually strictly validate Fixed is in?

Did that!

Tests are still running, but I already see several failures. Yay for 📌 KernelTestBase::$strictConfigSchema = TRUE and BrowserTestBase::$strictConfigSchema = TRUE do not actually strictly validate Fixed keeping us more honest going forward! 😊

Will investigate the failures as soon as the full test suite finishes running.

Well that turned out to be very simple! 🤩 Barely any violations in Drupal core! 👍

This is great, super happy to see that the strict config schema checking found 5 more fails.

Committed b90b2e3 and pushed to 11.x. Thanks!

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