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 ✨ Add validation constraints to config_entity.dependencies Fixed . 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, 📌 Make assertions using ConfigEntityValidationTestBase::assertValidationErrors() clearer Fixed 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 📌 [PP-1] Configuration schema & required keys Needs review 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, 📌 Make assertions using ConfigEntityValidationTestBase::assertValidationErrors() clearer Fixed landed just now!

looks really nice already! due to the relatively low amount of nullables I didn't write any special code for it yet in config split and I will wait for the other issue to be done before changing it there.

btw I love your comments here. I am wondering is not more of this insight could be added as comments somewhere. i went through a similar process when working on config split and getting into the weeds of the config schema.

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

1. 📌 KernelTestBase::$strictConfigSchema = TRUE and BrowserTestBase::$strictConfigSchema = TRUE do not actually strictly validate Fixed landed, which makes this MR a lot smaller and added a few more nullable: true cases.
2. 📌 `type: mapping` should use `ValidKeys: ` constraint by default Fixed 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 📌 KernelTestBase::$strictConfigSchema = TRUE and BrowserTestBase::$strictConfigSchema = TRUE do not actually strictly validate Fixed 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, 📌 Fix all PluginExistsConstraint constraint violations in tests Fixed and 📌 Add validation constraint to `type: label`: disallow multiple lines Fixed just landed — rebased! 👍

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

Here's another big push. I will then 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 👍

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

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 🌱 [meta] Make config schema checking something that can be ignored when testing contrib modules Active .

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! 👍 This should bring it down to zero, or very close to zero. A fix for the failing Nightwatch test is coming…

… but this is now definitely in the territory! 😊

This issue is a lot bigger then it could be, because it also includes the setup for validating the config schema's in the tests and making sure only the known things are passing.
After this one is done, the follow up issues after this one should be a lot smaller.

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 👍

Beautiful!
RTBC+1

Yes we add a lot of things to ignore.. but that is much more manageable than fixing everything here. I am curious how much of the contrib tests will be affected by this. I would assume that contrib schema and test config are equally out of whack. So I think how to address this by expanding the ignore rule would be great to document in a change record. Of course fixing it would be preferable.. but maybe we can still make it easier for tests not to fail.

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

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:
   • 📌 Fix Block config entity type config schema violations: weight, property Postponed → 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
   • 📌 [PP-2] Fix NodeType config entity type config schema violations: name, description, help Postponed → nope, not even this one: #3379731-2: Fix NodeType config entity type config schema violations: name, description, help →
   • 📌 Fix system.date config schema violations: timezone.default Active → 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? 😅🙈]

[more to follow, I have dozens of issues to create here… 😬 does that REALLY make sense at this time? 😅🙈]

No I agree, creating all of these is useless if it means we have to postpone all of them because we need to add validation to more types still. I think we should keep this is as is for now.

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

Per @lauriii 👍😊

I have reviewed the changes earlier this week when I rebased the MR. I didn't have specific concerns on this change and it makes sense that we are marking all non-nullable elements as required. However, I felt that this should at least get at least one review from a subsystem maintainer or a framework manager, given that it is a fairly significant change. I think it would be fine for a framework manager to sign-off on this as well given that the sole Configuration API subsystem maintainer is on leave. 😊

Merged in upstream, trivial conflict 👍

"Custom Commands Failed" after the rebase 😞

Fixed.

back to rtbc.

This MR is quite a bit behind HEAD and needs a rebase.

I tried rebasing this MR, but ran into trouble because #29 was a merge rather than a rebase. Doing another merge of 11.x only turns up one conflict that's easy to resolve, but I'm hesitant to push that in case it would be better to redo the MR as a rebase.

In the meantime, here's a patch, which I'll use for reviewing until the MR is in a mergeable state. Setting to NR to run tests.

The last submitted patch, 34: 3364109-34.patch, failed testing. View results →

#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 📌 Add new `ImmutableProperties` constraint Fixed 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! 😊

I stared reviewing this MR but it's going to take me some time to wrap my head around it. However, the first thought that jumps out at me is that the proposed resolution says:

This has zero effect on running a Drupal site: nor on the rest of Drupal core, nor on contrib. Because config validation does not yet run in production, only on tests

Do we not validate config during a config import though? If we do, then I would consider this as affecting production (in the sense that people deploy config to production), not just tests.

I'm still processing the implications of this MR. In particular:

+    // All types are required by default: configuration is expected to be
+    // complete.
+    // @see \Drupal\Core\TypedData\TypedDataManager::getDefaultConstraints()
+    $data_definition->setRequired(!isset($data_definition['nullable']) || $data_definition['nullable'] === FALSE);
+

Seems like potentially quite a disruptive change. It also conceptually conflicts with SchemaCheckTrait's current logic to allow NULL for all primitive types.

If the goal of this MR is in fact to change Drupal's design to no longer allow NULL for primitive types unless they've explicitly set nullable: true in their schema, then this MR should also change that part of SchemaCheckTrait, so "Needs work" for that. Meanwhile, I'm still mulling over whether such a design change is appropriate. I do appreciate the benefit of more strictness, but I'm also concerned about how to manage the BC implications.

This has zero effect on running a Drupal site: nor on the rest of Drupal core, nor on contrib. Because config validation does not yet run in production, only on tests

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

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. I'd expect there to be fewer cases of sites setting values of config keys explicitly to NULL than there are of sites missing config keys entirely.

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?

At some point, we might also want to trigger violations for when keys are missing (which I'm guessing is what 📌 [PP-1] Configuration schema & required keys Needs review wants to do?), and it's at the point when we do that that I think we should use setRequired().

Also, to understand #39 more, I want to see what tests fail if we take out the MR's changes to SchemaCheckTrait, so here's a patch to probe that.

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. I'd expect there to be fewer cases of sites setting values of config keys explicitly to NULL than there are of sites missing config keys entirely.

Ah, so this turns out to only be true for simple config objects. But for config entities, the config_export key in the @ConfigEntityType annotation fills in NULL values for keys that are missing, so there's no distinction. Given that, the use of setRequired() in this MR makes sense.

However, in that case it seems like the scope of this issue isn't captured in the issue title. Seems like the issue title should be more like, "Make all config keys required by default unless they're explicitly allowed to be nullable"?

#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 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 ✨ Add optional validation constraint support to ConfigFormBase Fixed , 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 📌 [PP-1] Configuration schema & required keys Needs review 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 think all the answers in #42-#43-#44 answer the questions by @effulgentsia; moving this back to rtbc.

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!

I don't understand how this hasn't been committed yet, to be honest. The changes make sense, and so does Wim's explanation in #46. This is a major blocker that holds up virtually all other config validation work at this point, which has the knock-on effect of delaying work on recipe support.

I, to, feel like @effulgentsia's questions have been addressed. For whatever it's worth, +1 RTBC here.

Making this opt-in using the mechanism proposed at

• #3395099-11: Allow config types to opt in to config validation, use as signal to opt in to "keys/values are required by default" →
• #3395099-19: Allow config types to opt in to config validation, use 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: Allow config types to opt in to config validation, use 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 📌 [PP-1] Configuration schema & required keys Needs review to use the same mechanism.

A follow-up is needed to refine 🐛 Follow-up for #3361534: config validation errors can still occur for contrib modules, disrupting contrib Active , 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.

I have a few points of non-blocking feedback, but they're non-blocking. :)

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:

• 🐛 DefaultSingleLazyPluginCollection::setConfiguration() accepts NULL but ConfigurableInterface::setConfiguration() does not Active
• 🐛 PluginExistsConstraintValidator should return early if given NULL Needs review
• 🐛 When setting `NotNull` constraint on type: required_label, two validation errors appear Needs review

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 →

Hey, look - 🐛 When setting `NotNull` constraint on type: required_label, two validation errors appear Needs review landed too!

The final blocker is RTBC, so...

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

The current MR looks really good to me and addresses all my prior concerns.

+    // All values are optional by default (meaning they can be NULL), except for
+    // mappings and sequences. A sequence can only be NULL when `nullable: true`
+    // is set on the config schema type definition. This is unintuitive and
+    // contradicts Drupal core's documentation.
+    // @see https://www.drupal.org/node/2264179
+    // @see https://www.drupal.org/node/1978714
+    // To gradually evolve configuration schemas in the Drupal ecosystem to be
+    // validatable, this needs to be clarified in a non-disruptive way. Any
+    // config schema type definition — that is, a top-level entry in a
+    // *.schema.yml file — can opt into stricter behavior, whereby a property
+    // cannot be NULL unless it specifies `nullable: true`, by adding
+    // `FullyValidatable` as a top-level validation constraint.

I'd like to take an attempt at wordsmithing this a bit more, but not right now. I'll either do that before committing this myself, or if someone else ends up committing it, I'm fine with opening a follow-up for improving code comments.

This issue is tagged "Needs subsystem maintainer review", so would be great to get @alexpott's review if possible. However, if he doesn't have bandwidth for it, I won't block commit of this on that.

       $root_type_has_opted_in = FALSE;
+      foreach ($parent->getRoot()->getConstraints() as $constraint) {
+        if ($constraint instanceof FullyValidatableConstraint) {
+          $root_type_has_opted_in = TRUE;

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.

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

• Pushed 1 forgotten revert (yay, smaller MR!) after the rebase on top of 🐛 When setting `NotNull` constraint on type: required_label, two validation errors appear Needs review 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 🐛 When setting `NotNull` constraint on type: required_label, two validation errors appear Needs review from #59.
• Change record: https://www.drupal.org/node/3404425 →
• #49 tagged the need for a follow-up. Created: 📌 [PP-1] Run config validation for config schema types opted in to be fully validatable Postponed , 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 📌 [PP-2] Fully validatable config schema types: support dynamic types Postponed .

🐛 DefaultSingleLazyPluginCollection::setConfiguration() accepts NULL but ConfigurableInterface::setConfiguration() does not Active is in, so this is unblocked pending subsystem maintainer review.

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

I've reviewed this a bunch of times and although it's complicated code in very deep and dark places, I think I understand it and it's as clear as it can be.

I think this is ready to go; @alexpott is the subsystem maintainer, so he can check that box if he reviews it for commit. We've already put this on his "desk".

I've added a comment to the MR that needs thinking about.

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.

Committed 5aa9704 and pushed to 11.x. Thanks!

🚀

Rebased/updated:

• #3379091: #3379091-30: Make NodeType config entities fully validatable → is on the verge of RTBC now
• #3364109: 📌 Configuration schema & required values: add test coverage for `nullable: true` validation support Fixed

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

Is this 🐛 Config inspector stopped working after Drupal 10.2 update Active related?

Has this been cherry picked for 10.2?

I can't see it https://github.com/drupal/drupal/blob/11.x/core/core.services.yml#L847

ok patch/diff array_null_config_fatal_errort_fix_10_2.diff fixes the problem in 10.2.

No longer getting fatal error at /admin/reports/config-inspector/search_api_solr.solr_cache.cache_document_default_7_0_0/list

@alexpott can you cherry pick this (or even better change the default target brach to the current release for bug fixes)?

Thank you.

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 👍