Configuration schema & required values: add test coverage for `nullable: true` validation support

The commits above:

1. enable the smallest possible config schema checking test, which only validates config (config_test.*) that exists precisely for testing purposes → ✅
2. enables execution of validation constraints for that config_test.* config → ❌ fails due to a bug in the existing validation logic for config_test.validation that was introduced in #1928868: Typed config incorrectly implements Typed Data interfaces:
   

   Exception: Exception when installing config for module config_test, message was: Schema errors for config_test.validation with the following errors: 0 [] Unexpected keys: langcode
   

3. commit that fixes the validation logic → ❌ still fails because the test explicitly sets $config_data['boolean'] = []; and an array is obviously not a boolean, so now \Drupal\Core\Validation\Plugin\Validation\Constraint\PrimitiveTypeConstraintValidator fails, because validation is actually enabled! That looks like this:
   

   1) Drupal\KernelTests\Core\Config\SchemaCheckTraitTest::testTrait
   Failed asserting that two arrays are equal.
   --- Expected
   +++ Actual
   @@ @@
        'config_test.types:new_key' => 'missing schema'
        'config_test.types:new_array' => 'missing schema'
        'config_test.types:boolean' => 'non-scalar value but not defi...uence)'
   +    0 => '[boolean] This value should b... type.'
    )
   

4. expect that PrimitiveType constraint violation → ✅

Alright, now we have a working baseline to continue our work, to actually implement the proposed plan 👍

The commits above:

1. 🥳🚀 The actual proposed change!
   

   $data_definition->setRequired(!isset($data_definition['nullable']) || $data_definition['nullable'] === FALSE);
   

   → ❌ this should cause a test failure:

   1) Drupal\KernelTests\Core\Config\SchemaCheckTraitTest::testTrait
   Exception: Exception when installing config for module config_test, message was: Schema errors for config_test.dynamic.dotted.default with the following errors: 0 [dependencies] This value should not be null., 1 [style] This value should not be null., 2 [size] This value should not be null., 3 [size_value] This value should not be null.
   

2. … but one of those errors is not like the others: [dependencies] This value should not be null. is something that config_test.dynamic.dotted.default (which is type: config_test.dynamic.*.*, which extends type: config_entity) inherited from type: config_entity.

   In other words: dependencies has been missing all along from that configuration entity, because configuration entities must ALWAYS have their dependencies defined. This is easily remedied though: we just update core/modules/config/tests/config_test/config/install/config_test.dynamic.dotted.default.yml to contain dependencies: {}. → ❌ still, with NO effect whatsoever, still the exact same exception!? 🤪🤯

3. Turns out that NotNullConstraintValidator does not work correctly for config schema sequences and mappings. 😱 It does:
   

       if (($typed_data instanceof ListInterface || $typed_data instanceof ComplexDataInterface) && $typed_data->isEmpty()) {
         $value = NULL;
       }
   

   … which makes sense for lists & complex data in a entity fields context, but not in a config schema context. Sequence and Mapping extend ArrayElement, which in turns implements ComplexDataInterface. That's why this code is causing dependencies: {} to be remapped to dependencies: null.

   A tweak to the if-test and a helpful comment later → ❌ as expected, but now only for the top-level keys that are defined by config_test.schema.yml:

   1) Drupal\KernelTests\Core\Config\SchemaCheckTraitTest::testTrait
   Exception: Exception when installing config for module config_test, message was: Schema errors for config_test.dynamic.dotted.default with the following errors: 0 [dependencies] This value should not be null., 1 [style] This value should not be null., 2 [size] This value should not be null., 3 [size_value] This value should not be null.
   

   👍

So it's not yet working, but we discovered a hard-blocking bug related to validation of required values and that's now fixed. 👍

The commits above:

1. Explicit test coverage for optionality/requiredness of values! → ❌ fails:
   

   1) Drupal\KernelTests\Core\Config\SchemaCheckTraitTest::testTrait
   Failed asserting that two arrays are equal.
   --- Expected
   +++ Actual
   @@ @@
        7 => '[int] This value should not be null.'
        8 => '[string] This value should no... null.'
        9 => '[string_int] This value shoul... null.'
   +    'config_test.types:_core' => 'variable type is NULL but applied schema class is Drupal\Core\Config\Schema\Mapping'
   +    'config_test.types:array' => 'variable type is NULL but applied schema class is Drupal\Core\Config\Schema\Sequence'
    )
   

   Ah, yes, \Drupal\Core\Config\Schema\SchemaCheckTrait::checkValue() also still generates an error for the mismatch. If/when we eventually reach 100% config validation, we'll be able to remove these. But for now, we should expect these too.

   (Every piece of configuration that reaches 100% validatability will be able to adopt config validation for use in its admin UI; schema checking's storage type checking alone is not enough for those UIs: that's where validation comes in.)

The failures in BaseFieldOverrideValidationTest, ContactFormValidationTest, et cetera are all in ConfigEntityValidationTestBase subclasses, which were all added in #3324150: Add validation constraints to config_entity.dependencies. They explicitly perform validation and are the only things in Drupal core to do so, precisely to help Drupal core get to a place where all config entities are 100% validatable.

But now more things are marked as required, so we get a lot more This value should not be null. errors. Unfortunately, those failures are currently a PITA to debug … and to fix that, #3349293: Make assertions using ConfigEntityValidationTestBase::assertValidationErrors() clearer already exists (and has been RTBC for 3 weeks). I've applied that patch locally to be able to make sense of the otherwise impossible to decypher failing assertions.

For each of the values that were not allowed to be NULL (and actually, all of them were really just missing required keys, but that's for #3364108: Configuration schema & required keys to solve, not this issue!), I went with the most minimal value possible, because they would almost all benefit from explicit validation constraints in the future:

• type: integer → 0
• type: string → ''
• et cetera

This should now be green!

Yay, #3349293: Make assertions using ConfigEntityValidationTestBase::assertValidationErrors() clearer landed just now!

A lot has changed sinceI worked on this 2 months ago today!

1. #3349293: Make assertions using ConfigEntityValidationTestBase::assertValidationErrors() clearer landed, which addresses the painful test debugging I mentioned in #7 🚀
2. #3361534: KernelTestBase::$strictConfigSchema = TRUE and BrowserTestBase::$strictConfigSchema = TRUE do not actually strictly validate landed, which makes this MR a lot smaller and added a few more nullable: true cases.
3. #3376794: `type: mapping` should use `ValidKeys: <infer>` constraint by default landed and made type: mapping behave like everyone (including core committers!) already thought it worked.

I'll process and address all feedback here 👍

A gazillion failures as expected! 😄

Why now and not before? Because before, I was specifically only testing one piece of config. But with #3361534: KernelTestBase::$strictConfigSchema = TRUE and BrowserTestBase::$strictConfigSchema = TRUE do not actually strictly validate now committed, as well as several of its follow-ups, we're actively working towards getting all of Drupal core's config to comply with the validation constraints. So the same is true here now.

Turns out that a whole range of problems can be solved quite easily: for example \Drupal\taxonomy\Entity\Vocabulary::$weight is required (because it does NOT have nullable: true in taxonomy.schema.yml), but it has a default value of 0. That's why that does not trigger thousands of validation errors.

But … the same is not true for Role::$weight! 😅 So simply setting Role::$weight = 0; solves a lot of validation errors, plus it's entirely reasonable.

Another example of a pattern: NodeType::$description, MediaType::$description, Vocabulary::$description are all treated optional by the entirety of the current codebase, plus it's not essential for usability, so the pragmatic solution is to simply mark these as nullable: true.

The batch of commits I just pushed should bring us from 3,111 failures to hopefully far fewer.

Yay, #3362457: Fix all PluginExistsConstraint constraint violations in tests and #3377030: Add validation constraint to `type: label`: disallow multiple lines just landed — rebased! 👍

That got us from 3,111 to 1,450 👍

Here's another big push.

Next steps:

1. Switch to only running Unit & Kernel tests, to first get those to green — that should fix most of the functional tests too, because it'll result in a few more config entity type default value tweaks + config schema tweaks 👍
2. Once those are green, I will first work on test coverage for all those config entity modifications, in the respective ConfigEntityValidationTestBase subclasses. (Which are also kernel tests.)
3. Then I'll re-enable running the full test suite, and will work on addressing whatever comes up.

The sheer number of specific failures means that fixing all invalid config or inaccurate config schema in this issue would lead to an enormous MR.

So rather than doing it all here, I propose to adopt the same approach as #3361534: KernelTestBase::$strictConfigSchema = TRUE and BrowserTestBase::$strictConfigSchema = TRUE do not actually strictly validate: allow ignoring patterns (there: violation constraint messages, here: specific property paths in specific config), to enable the bulk of the work to be deferred to follow-up issues. We have a good track record on that front: all 4 follow-up issues of #3361534 have been fixed in the subsequent week! 😊

I focused on one specific test here to determine what the necessary infrastructure is to make that possible: ResolvedLibraryDefinitionsFilesMatchTest. It may cause some additional tests to pass, but that's the only one I was targeting here.

To be continued.

As I said in #15, making all necessary changes in this MR would lead to an untenable scope. So I'm going back to the roots of this issue:

Initially only do this for config_test.* configuration, to first get the infrastructure in place.

… which is now far more true than before, thanks to this MR adding \Drupal\Core\Config\Schema\SchemaCheckTrait::$ignoredPropertyPaths and ::isViolationForIgnoredPropertyPath(), which clearly prove that the infrastructure change is working because there were thousands upon thousands of test failures 😁🤓

Updated issue summary, and marking this as blocking #3361423: [meta] Make config schema checking something that can be ignored when testing contrib modules.

That's working … except that we're now ignoring all violation messages for any given property path, but multiple different ones may exist for the same property path!

For example right now: a label is required (so NULL is not allowed, only strings), but not multi-line strings.

So the SchemaCheckTrait::$ignoredPropertyPaths infrastructure should only ignore the specified validation error messages at the specified property paths in the specified config 😅🤓

Just pushed a commit that does that. Now we should finally be green! (At which point I'll run the entire test suite again, at last.)

Only 35 failures in functional tests + a few failing Nightwatch tests! 👍 This should bring it down to zero, or very close to zero.

… but this is now definitely in the Needs review territory! 😊

Actually, it's smaller thanks to that infrastructure 😅 If we'd have had to fix everything here, this would have been a megabyte-sized unreviewable monster I think 🙈

Anyway, glad you like it — I'll start creating follow-up issues 👍

Yep, the ability for contrib to opt out will happen in #3361423: [meta] Make config schema checking something that can be ignored when testing contrib modules!

I'll still:

1. create a change record for #21
2. create the necessary follow-up issues

1. Rather than creating a new change record, I've updated https://www.drupal.org/node/3362879. That will be better for Drupal developers: to have all the config schema-related test changes in a single change record. See https://www.drupal.org/node/3362879#consequences-ignored.
2. Follow-ups created:
   • #3379725: Make Block config entities fully validatable → not yet actionable because this careful assessment should probably only happen once per config entity type, and doing it now would require doing it again for other property paths on this config entity type that do not yet have validation constraints
   • #3379731: Fix NodeType config entity type config schema violations: name, description, help → actionable now because once this issue lands, 100% of node.type.* is validatable! 👍 nope, not even this one: #3379731-2: Fix NodeType config entity type config schema violations: name, description, help
   • #3379734: Fix system.date config schema violations: timezone.default → actionable because its probable solution results in a narrow enough scope 👍
   • [more to follow, I have dozens of issues to create here… 😬 does that REALLY make sense at this time? 😅🙈]

This is now also blocking #3379091 — see #3379091-13: Make NodeType config entities fully validatable and #3379091-14: Make NodeType config entities fully validatable.

Per @lauriii 👍😊

Merged in upstream, trivial conflict 👍

Fixed.

#34 Thanks for that careful consideration! Because I've experienced old links to MR commits breaking when rebasing (and those old links to commits are linked here on the d.o issue), I've been favoring merging in upstream rather than rebasing. At least for more complex issues like this one — if it's a simple commit history, then I'll just do a rebase since it won't be hard to find the right commit by hand when reviewing.

Thanks to #34 I knew I had to not just merge in upstream, but also to adjust the test that was just added in #3382580: Add new `ImmutableProperties` constraint ever so slightly. Since it's such a trivial change (just respecting the ignored property paths we've had in HEAD for a while), re-RTBC'ing.

Can't wait to read your review! 😊

#37:

Do we not validate config during a config import though?

Not really.

\Drupal\Core\Config\ConfigImporter::validate() triggers \Drupal\Core\Config\ConfigEvents::IMPORT_VALIDATE, but that does not run any actual validation at all. There's just a handful of things that are validated by custom event subscribers:

1. \Drupal\system\SystemConfigSubscriber::onConfigImporterValidateNotEmpty() (prevent empty config from deleting everything!)
2. \Drupal\system\SystemConfigSubscriber::onConfigImporterValidateSiteUUID() (site UUID must match)
3. \Drupal\Core\Entity\Event\BundleConfigImportValidate::onConfigImporterValidate() (bundles being deleted in this import must not be in use)
4. \Drupal\Core\EventSubscriber\ConfigImportSubscriber::onConfigImporterValidate() (config names must be valid)
5. \Drupal\config\ConfigSubscriber::onConfigImporterValidate() (prevent uninstalling the config module itself)
6. \Drupal\content_moderation\EventSubscriber\ConfigImportSubscriber::onConfigImporterValidate() (similar to the bundle one above)

That's literally it! Everything else is just blindly accepted by the configuration system.

None of the config import UIs nor any of those event subscribers are calling SchemaCheckTrait::checkConfigSchema(). And none of them are calling validate() on the config objects.

If we did do thorough validation \Drupal\Tests\config\Functional\ConfigImportUITest would have failed since [##3361534].

#38:

It also conceptually conflicts with SchemaCheckTrait's current logic to allow NULL for all primitive types.

The logic you refer to was written in #2245729: Add missing configuration schema in Color component, in May 2014. It was specifically written to help us find missing config schemas. Config schemas were not required initially, until they turned out to be essential for i18n support. It was even broader then:

          // Null values are allowed for all types.
          if ($value === NULL) {
            $success = TRUE;
          }

That's also a good reminder that just like I wrote in my previous comment, SchemaCheckTrait is used ONLY by tests. Its original purpose was to make sure that the config schema for Drupal core is complete & accurate. All we're doing here, is expanding further upon that.

Also, it took only a month to realize that this alone was too broad/not precise enough: #1978714: Entity reference doesn't update its field settings when referenced entity bundles are deleted already tightened that logic in June 2014, to:

      if ($element instanceof PrimitiveInterface) {
        $success =
…
          // Null values are allowed for all primitive types.
          ($value === NULL);
      }
      // Array elements can also opt-in for allowing a NULL value.
      elseif ($element instanceof ArrayElement && $element->isNullable() && $value === NULL) {
        $success = TRUE;
      }

This issue is just generalizing that very same pattern.

Yes, it is a change. But that was also true back then for type: sequence.

If we don't do this, then it is impossible for modules to assume a value will be set if the config is valid. We'd force every single module to specify additional validation constraints to guarantee the value is not NULL. That would still result in the NotNull constraint being used everywhere. The only difference is that you'd make the DX:

• more painful/verbose (rather than just setting nullable: true|false in the config schema)
• inconsistent: if we don't allow nullable: true|false on primitive types, it'd be inconsistent with how it works for sequences. If the default would not be nullable: false, then it'd again be inconsistent with how it works for sequences.

then this MR should also change that part of SchemaCheckTrait

Great point! There is a simple reason I have not done that yet: \Drupal\Core\Config\Schema\SchemaCheckTrait::checkValue() will become obsolete: we'll be able to eventually delete it. Otherwise we end up with the same logic twice. I'm just keeping ::checkValue() unchanged to minimize disruption. Changing it like you suggest would cause more disruption: contrib modules' tests do not get any validation errors (see the trait), but they do run this. So, added a comment instead. Hope that makes sense to you!

In addition to config validation potentially running during a config import per #37, config validation now also runs in ConfigFormBase thanks to #3364506: Add optional validation constraint support to ConfigFormBase, so tagging this for an issue summary update.

The first statement is wrong, the second is accurate. So: updated issue summary. 👍

#39

Something that makes this MR potentially not too disruptive is that the config validators, including the NotNull validator, only run for keys that exist in the data being validated. So the setRequired() referenced in #38 isn't actually making the keys required, it's only making sure that the values aren't explicitly set to NULL.

(Emphasis mine.)

Correct! And this is also one of the most confusing parts about the entire configuration system! There's a very important distinction between "required values" and "required keys". That's why this issue is specifically titled "required values". See #3364108: Configuration schema & required keys for the issue for required keys.

All of the following combos are possible:

Currently, config schema is not able to express these. And without knowing what is actually intended, it is impossible to fully validate config, which means it's impossible to reliably (without unintentionally breaking the site because the code assumes the presence of a key or value) or safely update config (security implications because of the code's assumptions and the user intentionally submitting certain config).

However, given that, I don't think this MR should call setRequired(). We already have isNullable() for ArrayElement; what about adding that to the other types that we need it for and base the NotNull constraint on that?

[…] and it's at the point when we do that that I think we should use setRequired().

👎 That would change the meaning of ::setRequired(). Its meaning is specifically for values. Look at \Drupal\Core\Field\FieldConfigInterface::setRequired() and friends: it's always meant to be for "this thing exists, but is its value optional or not?".

But this is a moot point already, because you wrote later:

#41:

Given that, the use of setRequired() in this MR makes sense.

👍

I see one important reason for this change is missing from the issue summary.

Quoting \Symfony\Component\Validator\Constraints\ChoiceValidator::validate():

        if (null === $value) {
            return;
        }

• \Symfony\Component\Validator\Constraints\RangeValidator::validate(): same thing.
• \Symfony\Component\Validator\Constraints\LengthValidator::validate(): same thing.
• …

… which makes sense: if the value is optional, it is NULL, and in that case, none of these validation constraints need to run anymore.

That means that:

machine_name:
  type: string
  label: 'Machine name'
  constraints:
    Regex:
      pattern: '/^[a-z0-9_]+$/'
      message: "The %value machine name is not valid."
    Length:
      # @see \Drupal\Core\Config\Entity\ConfigEntityStorage::MAX_ID_LENGTH
      max: 166

can easily be bypassed … by just specifying NULL! 😱

The only solution today is to specify NotNull explicitly in constraints too, as the first one. But that only works as long as a config schema does not extend/reuse a type, because it is not possible to unset constraints from a parent type.

Imagine we had hardened type: machine_name like this in Drupal core:

machine_name:
  …
  constraints:
    NotNull: …
    Regex: …
    Length: …

then nothing would be able to use type: machine_name and make it optional. They'd have to duplicate the entire type definition!

Making this opt-in using the mechanism proposed at

• #3395099-11: [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"
• #3395099-19: [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"

1. Merging in origin/11.x surfaced 3 new test failures, all in new test coverage added since this was RTBC'd
2. Then I reverted ALL of the test changes. The goal: to have tests pass WITHOUT changing them, because this would prove that @effulgentsia's concerns have been addressed — zero changes in behavior UNTIL config schema definitions opt in 😊
3. Dropping the ::setRequired() call in TypedConfigManager::buildDataDefinition() then made all existing tests pass, proving there's no more disruption: 0 failures.
4. THEN adding explicit test coverage that reuses the existing low-level testing config schema type, and uses the new FullyValidatable constraint introduces a test failure. The goal is to make this test pass without breaking any other. If that works, I've proven that my proposal to @effulgentsia at #3395099-19: [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" works.
5. Finally, restoring the ::setRequired() call, but this time running it only for those types that have opted in, should make the tests pass again.

A change record is needed to make core/contrib/custom maintainers aware of the availability of this mechanism. I first need to update #3364108: Configuration schema & required keys to use the same mechanism.

A follow-up is needed to refine #3402168: Follow-up for #3361534: Config validation errors can still occur for contrib modules, disrupting contrib, to allow contrib modules which have types that are explicitly marked as fully validatable, to run checkConfigSchema(validate_constraints: true), because today they're forced to use validate_constraints: false.

Although the MR was now delightfully simple until 1d7f8516, I figured that to get maximum confidence that this will not disrupt anything, we would need explicit test coverage to prove no disruption until opting in (by specifying the FullyValidatable no-op constraint):

• ✅ for existing simple config (already in the MR: SchemaCheckTraitTest)
• ❌ for existing config entities (missing!)

So, added ConfigEntityValidationTestBase::testRequiredPropertyValuesMissing() for that, which tests to all core config entity types.

This unfortunately surfaced a few more small bugs that are probably too out-of-scope to fix here:

• #3404023: DefaultSingleLazyPluginCollection::setConfiguration() accepts NULL but ConfigurableInterface::setConfiguration() does not
• #3404039: PluginExistsConstraintValidator should return early if given NULL
• #3404061: When setting `NotNull` constraint on type: required_label, two validation errors appear

Fixes for those are included in this MR, but will hopefully land independently to make this MR simpler again. The choice is to either include those fixes in the MR or to skip the tests of the affected config entity types. But since a whopping 26 config entity types (see the test results) are affected, I figured it was better to include those fixes.

Issue summary updated to reflect #51.

Change record created: https://www.drupal.org/node/3404425

#3404039: PluginExistsConstraintValidator should return early if given NULL just landed. MR updated. One less file being changed 👍 Two blockers to go!

EDIT: conflicted with #56 — I was already doing that obviously :D

Merged in upstream and removed the work-around that is now obsolete 👍

Last blocker: #3404023: DefaultSingleLazyPluginCollection::setConfiguration() accepts NULL but ConfigurableInterface::setConfiguration() does not — also RTBC already :) That will allow this MR to remove a mere 10 LoC additions and 2 files touched, but it will mean that this issue's MR is finally matching its intended scope exactly!

@effulgentsia in #60: I'm very glad to read that!

• Pushed 1 forgotten revert (yay, smaller MR!) after the rebase on top of #3404061: When setting `NotNull` constraint on type: required_label, two validation errors appear from #59. This change was obsolete since that fix landed 😄
• Pushed 1 tiny tweak to the test coverage, necessary after the rebase on top of #3404061: When setting `NotNull` constraint on type: required_label, two validation errors appear from #59.
• Change record: https://www.drupal.org/node/3404425
• #49 tagged the need for a follow-up. Created: #3408158: Run config validation for config schema types opted in to be fully validatable, postponed on this issue.
• #60:
  

  Since this MR only adds FullyValidatableConstraint to menus and shortcut sets, neither of which has dynamic types (plugins or third party settings), I think this is fine for now. However, before adding FullyValidatableConstraint to config roots that include any descendants with dynamic types, I think we need to work out how to handle those boundaries. It's impossible for a config root to know that all of the plugins within it are fully validatable, so I think the semantics of FullyValidatableConstraint must be limited to only its descendants with static types. For an element that has an ancestor with a dynamic type, we need to use that ancestor's presence or absence of FullyValidatableConstraint instead of checking ->getRoot(). But again, we can punt that to a follow-up since that's not surfaced for menus and shortcuts.

  💯 Very good point!

  I wrote about this in detail months ago but cannot find it right now. Created #3408165: Fully validatable config schema types: support dynamic types.

Merged in #3404023: DefaultSingleLazyPluginCollection::setConfiguration() accepts NULL but ConfigurableInterface::setConfiguration() does not 👍 2 fewer files touched by this MR now, and AFAICT zero out-of-scope changes left — all 3 issues listed in #51 have landed 🚢

Back to RTBC — the remark was on the one hunk in the MR that was necessary because apparently not a single test in core is doing $config_entity->set($property_with_a_plugin_collection, NULL). It's a tiny (but important!) aspect of the MR, and I just addressed @alexpott's concern. No material changes to the overall MR.

🚀

Rebased/updated:

• #3379091: #3379091-30: Make NodeType config entities fully validatable ← on the verge of RTBC now
• #3364108: #3364108-86: Configuration schema & required keys ← became a fair bit smaller/simpler!

I tracked down the root cause of what @2dareis2do ran into: #3416934-23: Config Inspector crashes on 10.1.x + 10.2.x for `type: mapping` with `nullable: true` due to core bug. No contrib disruption happens unless you were specifically using the Config Inspector module 👍