`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.* → type: field.storage_settings.boolean) for us 🤓

Then everything starts working! 👍

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. #3361534: KernelTestBase::$strictConfigSchema = TRUE and BrowserTestBase::$strictConfigSchema = TRUE do not actually strictly validate 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 🙈

🤣 Thanks… I guess? :P

Did that!

Tests are still running, but I already see several failures. Yay for #3361534: KernelTestBase::$strictConfigSchema = TRUE and BrowserTestBase::$strictConfigSchema = TRUE do not actually strictly validate 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! 👍