KernelTestBase::$strictConfigSchema = TRUE and BrowserTestBase::$strictConfigSchema = TRUE do not actually strictly validate

Here's a trivial patch that modifies media.settings.yml to have

oembed_providers_url: "Ceci n'est pas une URL."

I bet that this will still pass the core test suite.

(Except for maybe some functional test that depends on it? 🤔)

P.S.: yes, to be Belgian is to be surreal.

Ceci c'est une patch.

… all it takes is a dash of additional test logic! 😊

Now we should see failures like:

1) Drupal\Tests\media\Kernel\OEmbedIframeControllerTest::testBadHashParameter with data set "no hash" ('')
Exception: Exception when installing config for module media, message was: Schema errors for media.settings with the following errors: 0 [iframe_domain] This value should be of the correct primitive type., 1 [oembed_providers_url] This value should be of the correct primitive type.

… and ah, the iframe_domain key-value pair is also invalid in the default media.settings.yml! 😅

Makes sense, because iframe_domain: '' is not a valid URI, whereas the config schema dictates it should be:

    iframe_domain:
      type: uri
      label: 'Domain from which to serve oEmbed content in an iframe'

/var/www/html/core/modules/media/config/install/media.settings.yml:3:24 - Unknown word (Ceci)

CSpell: failed

🫠 Thanks for killing my joke, DrupalCI! Redid them.

Fix typo in IS — thanks @Gábor Hojtsy!

The failures in #5 are hard failures because the errors occur so early:

00:44:55.360 [Tests/Install Profile Test] Test Suite
00:44:55.360 ──────────────────────────────────────────────────────────────────────────────
00:44:55.360 - Connecting to chromedriver-jenkins-drupal-patches-183208 on port 9515...
00:44:55.360 
00:44:55.635   Using: chrome (106.0.5249.103) on LINUX.
00:44:55.635 
00:44:55.635 ℹ Connected to chromedriver-jenkins-drupal-patches-183208 on port 9515 (275ms).
00:44:58.375 
00:44:58.375 In ConfigSchemaChecker.php line 94:
00:44:58.375                                                                                
00:44:58.375   Schema errors for media.settings with the following errors: 0 [iframe_domai  
00:44:58.375   n] This value should be of the correct primitive type., 1 [oembed_providers  
00:44:58.375   _url] This value should be of the correct primitive type.                    
00:44:58.375                                                                                
00:44:58.375 
00:44:58.375 install [--setup-file [SETUP-FILE]] [--db-url [DB-URL]] [--base-url [BASE-URL]] [--install-profile [INSTALL-PROFILE]] [--langcode [LANGCODE]] [--json]
00:44:58.375 
00:44:58.445   ✖ NightwatchAssertError
00:44:58.445    Command failed: php ./scripts/test-site.php install --setup-file "core/tests/Drupal/TestSite/TestSiteInstallTestScript.php" --install-profile "demo_umami"  --base-url http://php-apache-jenkins-drupal-patches-183208/subdirectory --db-url mysql://drupaltestbot:drupaltestbotpw@172.18.0.4/jenkins_drupal_patches_183208 --json
00:44:58.445 
00:44:58.445 In ConfigSchemaChecker.php line 94:
00:44:58.445                                                                                
00:44:58.445   Schema errors for media.settings with the following errors: 0 [iframe_domai  
00:44:58.445   n] This value should be of the correct primitive type., 1 [oembed_providers  
00:44:58.445   _url] This value should be of the correct primitive type.       

This is hence causing widespread failures:

00:37:09.840 Drupal\Tests\media_library\FunctionalJavascript\ViewsUiInteg   0 passes             1 exceptions             
00:37:10.046 FATAL Drupal\Tests\media_library\FunctionalJavascript\ViewsUiIntegrationTest: test runner returned a non-zero error code (2).
00:37:10.141 Drupal\Tests\media_library\FunctionalJavascript\ViewsUiInteg   0 passes   1 fails                            
00:37:10.141 Drupal\Tests\media_library\FunctionalJavascript\EmbeddedForm   0 passes             1 exceptions             
00:37:10.927 FATAL Drupal\Tests\media_library\FunctionalJavascript\EmbeddedFormWidgetTest: test runner returned a non-zero error code (2).
00:37:10.944 Drupal\Tests\media_library\FunctionalJavascript\EmbeddedForm   0 passes   1 fails          

Now let's revert the silly changes to media.settings for the oembed_providers_url key and instead fix the config schema for iframe_domain key.

The test failure that isn't

That actually passed tests. The only failure is Drupal\FunctionalTests\Installer\InstallerExistingConfigSyncDirectoryMultilingualTest::testConfigSync, which is a random (but frequent) failure in core since it updated to Symfony 6.3 — see #3361121: [random test failure] InstallerExistingConfig[SyncDirectory]MultilingualTest::testConfigSync.

Next

+++ b/core/lib/Drupal/Core/Config/Schema/SchemaCheckTrait.php
@@ -57,6 +58,17 @@ public function checkConfigSchema(TypedConfigManagerInterface $typed_config, $co
+    // @todo Remove the condition; run it for all config.
+    if ($config_name === 'media.settings') {

Let's do this now… and find out what other default config in core does not match the config schema.

Wrong patch. Here's the right one.

Just like #8 before, now that we're properly validating all config instead of just media.settings, we've discovered new problems:

00:29:29.324 In ConfigSchemaChecker.php line 94:
00:29:29.324                                                                                
00:29:29.324   Schema errors for system.theme.global with the following errors: 0 [logo.ur  
00:29:29.324   l] This value should be of the correct primitive type.        

Let's fix that too, just like we fixed media.settings. 👍

Many more tests are passing, but I only updated the default config in system module. I also needed to update the profile-level overrides in minimal and demo_umami.

Lots of failures (269 failures ⚠️) throughout now:

1. 1) Drupal\Tests\block\Kernel\NewDefaultThemeBlocksTest::testNewDefaultThemeBlocks
   Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for block.block.stark_rsdw8gga with the following errors: 0 [dependencies.theme.0] Theme 'stark' is not installed., 1 [plugin] The 'user_login_block' plugin does not exist.
   

   (thanks to #3324150: Add validation constraints to config_entity.dependencies)

2. 5) Drupal\Tests\ckeditor5\Kernel\CKEditor5PluginManagerTest::testProvidedElements with data set "heading text container combo" (array('ckeditor5_plugin_elements_tes...gCombo', 'ckeditor5_paragraph'), array(array()), array(array(true), array(true, true)), '<p data-everytextcontainer> <...ainer>')
   There should be no errors in configuration 'editor.editor.dummy'. Errors:
   Schema key 0 failed with: [settings.plugins.ckeditor5_heading] Configuration for the enabled plugin "<em class="placeholder">Headings</em>" (<em class="placeholder">ckeditor5_heading</em>) is missing.
   

   (thanks to CKEditor 5 having detailed config validation already)

3. 1) Drupal\Tests\big_pipe\Kernel\BigPipeInterfacePreviewThemeSuggestionsTest::testBigPipeThemeHookSuggestions
   Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for block.block.test_block1 with the following errors: 0 [dependencies.theme.0] Theme 'stark' is not installed., 1 [plugin] The 'test_html' plugin does not exist.
   

   (thanks to #3324150: Add validation constraints to config_entity.dependencies)
   and after fixing that

   1) Drupal\Tests\big_pipe\Kernel\BigPipeInterfacePreviewThemeSuggestionsTest::testBigPipeThemeHookSuggestions
   Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for block.block.test_block1 with the following errors: 0 [plugin] The 'test_html' plugin does not exist.
   

   (thanks to #2920682: Add config validation for plugin IDs)

4. et cetera

It's gonna be interesting to fix these … 😅 BUT!

This means that all the config validation work is starting to pay off! If we get this to green, then we'll be gradually making our tests more reliable and make our config more accurate!

Alphabetically solving these:

1. 1) Drupal\Tests\system\Kernel\Action\ActionTest::testDependencies
   Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for system.action.user_add_role_action.anonymous with the following errors: 0 [dependencies.config.0] The 'user.role.anonymous' config does not exist.
   

   → missing config

2. 1) Drupal\KernelTests\Core\Asset\ResolvedLibraryDefinitionsFilesMatchTest::testCoreLibraryCompleteness
   Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for update.settings with the following errors: 0 [fetch.url] This value should be of the correct primitive type.
   

   → wrong config schema for update.settings:fetch.url, same problem and solution as media.settings:iframe_domain and system.theme.global:logo.url

Config schema fix rather than test fix ⇒ reuploading patch, since it is likely to fix many failures.

Alphabetically continuing:

1. 1) Drupal\Tests\block\Functional\BlockTest::testBlockUserRoleDelete
   Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for block.block.lOmY5EJr with the following errors: 0 [dependencies.theme.0] This value should not be blank., 1 [dependencies.theme.0] Theme '' is not installed.
   

   → incomplete block config

2. 2) Drupal\Tests\block\Functional\BlockTest::testBlockTitle
   Undefined array key 0
   

   → obscured by a functional test hitting a 500 error now ⇒ dump of the page shows:

   The website encountered an unexpected error. Please try again later.<br><br><em class="placeholder">Drupal\Core\Config\Schema\SchemaIncompleteException</em>: Schema errors for block.block.test with the following errors: 0 [plugin] The &amp;#039;foo&amp;#039; plugin does not exist. in <em class="placeholder">Drupal\Core\Config\Development\ConfigSchemaChecker-&gt;onConfigSave()</em> (line <em class="placeholder">94</em> of <em class="placeholder">core/lib/Drupal/Core/Config/Development/ConfigSchemaChecker.php</em>). <pre class="backtrace">call_user_func(Array, Object, &#039;config.save&#039;, Object) 
   

   → use a block plugin that actually exists, such as system_powered_by_block instead of foo

3. 1) Drupal\Tests\block_content\Functional\BlockContentCreationTest::testBlockContentCreation
   Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for block.block.foobargorilla with the following errors: 0 [plugin] The 'block_content:fb5e8434-3617-4a1d-a252-8273e95ec30e' plugin does not exist.
   

   → core/modules/block_content/tests/modules/block_content_test/config/install/block.block.foobargorilla.yml is referencing

   plugin: 'block_content:fb5e8434-3617-4a1d-a252-8273e95ec30e'
   

   → but that block_content entity does not exist: we need to ensure it already exists. But … really there should have been a content dependency, which is possible since #2282519: Add content dependency information to configuration entities. Newer config exports like core/profiles/demo_umami/config/optional/block.block.umami_banner_home.yml do do this correctly. But … that has a whole lot of custom infrastructure (see \Drupal\demo_umami_content\InstallHelper::importContentFromFile()). Precisely because this is an unsolved problem, #3324150: Add validation constraints to config_entity.dependencies did not add validation constraints to content dependencies.

   → Fixed by adding block_content_test_module_preinstall() modeled after Umami's example.

Test module fix rather than test fix ⇒ reuploading patch, since it's likely to fix multiple failures.

⚠️ But it's become pretty apparent that continuing this will result in one massive patch. So rather than continuing to fix all problems in all tests, let's specifically detect certain validation errors and ignore them, with each of them having a corresponding follow-up issue. That way, we can at least start stabilizing/improving things, beginning with complying with the primitive types :)

Correct patch + interdiff. I was missing 2 hunks.

#16 did get it down from 269 to 223 failures! 🥳

#19: Definitely not 250 issues 🤪🤣. One issue per validation constraint IMHO. Because for each of them, the same handful of patterns of fixes should apply.

This refines the validation to exclude the failures for non-primitive validation constraints, and defers to follow-ups:

1. #3362453: Fix all ConfigExists constraint violations in tests
2. #3362456: Fix all ExtensionExistsConstraint constraint violations in tests
3. #3362457: Fix all PluginExistsConstraint constraint violations in tests

Let's see which others I need to exclude and create follow-up issues for… 🤞

1. Reverting all the fixes that were made so far in #16 + #17 + #18: interdiff-revert.txt
2. Fixing the root cause of many (most?) of the failures: core/modules/config/tests/config_test/config/install/config_test.validation.yml is missing a langcode key-value pair: interdiff-fix_config_test.txt.

(Combined: interdiff.txt.)

It's already clear we're going to need more follow-up issues though. (#20 will finish running in ~30 mins.)

• #20 brought it from 220 to 152.
• #21 brought it from 152 to 66.

🥳

This should fix pretty much all failing update path tests, bringing it down to 51 45 if I'm lucky!

• #22 brought it down to … 45! 😁🚀

This issue surfaced one genuine bug in the CKEditor 5 test coverage! 👍

Beyond that bugfix, we need to make some tweaks in the CKEditor 5's test coverage because ::assertConfigSchema() now does more.

This will hopefully bring it down to ~20.

Will continue tomorrow.

phpcs violation fixed. Plus one obvious bug in a test that causes a validation error now: 0 ain't a UUID!

• #23/#24 brought it down to 21!

The default config for Umami Demo had CKEditor 5 validation constraint violations (which this issue's added infrastructure will forever prevent — we have special additional test coverage in Standard for exactly this, introduced in #3271097: Replace CKEditor 4 with CKEditor 5 in the Standard profile and StandardTest).

This should fix 2 failures and allow the Nightwatch test for Umami Demo to stop crashing, which is why d.o is not accurately reporting failures 🤪

#26 brought it down from 21 to 17, and now the results actually show up! Which means Nightwatch tests are now indeed passing again. Will stop these recaps going forward 👍

That concludes all fixes for Block. Just leaves one for Views. Turns out that's just forgotten to inject the validation constraint manager into TypedConfigManager — easy! 👍

This should bring it down to 13 failures.

100% of the Layout Builder test failures are because the strings first-uuid and second-uuid are used as if they are UUIDs. That's no longer accepted.

Simple solution for that too: replace first-uuid with 10000000-0000-0000-0000-000000000000, similar for the second one. But … turns out that UUID validation is strict by default, which imposes additional requirements … so had to go with 10000000-0000-1000-a000-000000000000 and 20000000-0000-1000-a000-00000000000 — which I think is equally simple. (And actually makes the tests clearer.)

That should bring us down to <10 failures.

CKEditor5PluginManagerTest was interesting:

1. The saving of config in ::setUp() caused the CKEditor 5 plugin manager that's available via a convenience property on the test class to become primed with information of the modules installed at the time that ::setUp() runs. But many test methods on this test class enable additional modules to test additional edge cases. This was easily remedied, but was mystifying at first. Keeping references to services is always dangerous 🙈
2. ::testProvidedElements() now triggers additional validation (previously, only the config schema was asserted), and in doing so it was unveiled that the heading text container combo test case was not entirely accurate: even if testing the correctness of the text editor config entity was not the purpose of this test, that is now enforced even for this test. Which is why the default toolbar items (defined by \Drupal\ckeditor5\Plugin\Editor\CKEditor5::getDefaultSettings()) are inherited for all these tests. Which is why every test case was required to contain configuration for the ckeditor5_heading plugin! Again, a simple tweak fixes it.

PHPCS

1. TypedConfigTest was trivial (It's intentionally making mistakes to test config schema validation)
2. SchemaCheckTraitTest was also trivial: one addition because now validation constraints are being checked.
3. But \Drupal\KernelTests\Core\Config\DefaultConfigTest is almost impossible to fix while keeping its current simplicity.

   The reason: its too simple:

     /**
      * Tests default configuration data type.
      */
     public function testDefaultConfig() {
       $typed_config = \Drupal::service('config.typed');
       // Create a configuration storage with access to default configuration in
       // every module, profile and theme.
       $default_config_storage = new TestInstallStorage();
       foreach ($default_config_storage->listAll() as $config_name) {
         if (in_array($config_name, $this->toSkip)) {
           continue;
         }
   
         $data = $default_config_storage->read($config_name);
         $this->assertConfigSchema($typed_config, $config_name, $data);
       }
     }
   

   This reads every piece of configuration in isolation, and asserts it conforms to the config schema. But … it does not import dependencies. It does not even install these modules.

   IOW: it only checks conformity to the storage types. Which is exactly what this issue aims to change. And in doing so, we already found many bugs in config and tests in this issue.

   DefaultConfigTest was introduced in January 2014. 3 months later, \Drupal\Tests\config\Functional\ConfigImportAllTest was added. Another 3 months later, use SchemaCheckTestTrait; was added to ConfigImportAllTest. Since that moment, DefaultConfigTest has basically been obsolete. It served its purpose!

   So I think the solution is clear: delete it. Otherwise we need to modify it to do almost the same as ConfigImportAllTest!

Patch should be green now!

#25 @borisson_ thanks for the review! 😊

1. FYI it's a pattern used in many other places in core already. Grep for $v->getMessage() 😊
2. Fair! Done ✅
3. Done: #3362709: [11.x] Remove UpdatePathTestBase::$configSchemaCheckerExclusions ✅

While addressing #25.3, I realized:

1. that we're still missing update hooks for updating media.settings, update.settings and system.theme.global in case they are still at their default values
2. which means there's a better solution than adding UpdatePathTestBase::$configSchemaCheckerExclusions(): with those update hooks in place, update tests won't fail anyway!

Oops 😅

Updated issue summary. This is now ready for final review.

PHPCS

1. +++ b/core/modules/media/tests/src/Functional/Update/MediaSettingsDefaultIframeDomainUpdateTest.php
   @@ -0,0 +1,59 @@
   + * @group system
   

   @group media

2. +++ b/core/modules/update/tests/src/Functional/Update/UpdateSettingsDefaultFetchUrlUpdateTest.php
   @@ -0,0 +1,109 @@
   + * @group system
   

   @group update

this is basically a test-only change that increases our strictness and validation coverage

There is an ecosystem impact though!

Tests in contributed modules will start failing if they are using invalid configuration. Just like happened for Drupal core in this issue. But clearly, the number of config schema violations is very low. Still, that is some level of disruption.

So, updated issue summary and created a change record: https://www.drupal.org/node/3362879. I tried to make it as clear and helpfully concrete a change record as possible, but feedback welcome of course! 😊

There will be more failures once we start:

1. Removing entries from $ignored_validation_constraint_messages in \Drupal\Core\Config\Schema\SchemaCheckTrait::checkConfigSchema()
2. Adding more validation constraints to Drupal core's config schema

But all of those will be incremental improvements and will help increase the reliability of the entire Drupal ecosystem. In fact, it will even make update paths and migrations more reliable, because configuration integrity problems will be much more clear.

Rerolled after conflicts with #3337513: Fix batch process race conditions by making ‘bid (batch id)’ auto-increment and #3360991: TypedData instances created by TypedConfigManager::createFromNameAndData() are incomplete.

I worked on ensuring that issue is sorted! 🤓😄

~1/3rd of this patch is for updating Layout Builder's tests. Splitting that off to a child issue: #3363873: Stop using `first-uuid` and `second-uuid` in tests: violates config schema.

#3363873: Stop using `first-uuid` and `second-uuid` in tests: violates config schema landed 🥳

From 48 KB in #43 to 34 KB now 👍

The CKEditor 5 kernel test changes here are the most distracting/out-of-scope looking changes here. I think I can extract the trickiest pieces in a separate issue. Did that: #3364378: Tighten CKEditor 5 kernel tests.

#3364378: Tighten CKEditor 5 kernel tests landed, rerolling #48.

I believe that the remaining patch now is much clearer in scope, should make it much more feasible for a core committer to feel confident when committing this 😊

P.S.: Regarding the last CI run failing hard: I bet it's because #2920678: Add config validation for the allowed characters of machine names landed.

Yep, my P.S. in #52 was correct — e.g.

1) Drupal\KernelTests\Core\Entity\EntityDeprecationTest::testCreateUserDeprecation
Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for system.action.user_add_role_action.dfjdgg2p with the following errors: 0 [id] This value is not valid.

So this falls in the same category as #3362453: Fix all ConfigExists constraint violations in tests, #3362456: Fix all ExtensionExistsConstraint constraint violations in tests and #3362457: Fix all PluginExistsConstraint constraint violations in tests. Creating a follow-up for this one too.

But first, making that validation message much more helpful. Let's override \Drupal\Core\Validation\Plugin\Validation\Constraint\RegexConstraint::$message's super unhelpful 'This value is not valid.' message to something A) more specific, B) something that actually shows the value.

Now we should see

1) Drupal\KernelTests\Core\Entity\EntityDeprecationTest::testCreateUserDeprecation
Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for system.action.user_add_role_action.porqtizm with the following errors: 0 [id] The <em class="placeholder">"user_add_role_action.porqtizm"</em> machine name is not valid.

On second thought, I wonder if we don't need a follow-up but just need to tweak the regex for a few config entity types' machine names? Cases that were missed in #2920678: Add config validation for the allowed characters of machine names because they A) barely have test coverage, B) have no UI?

Let's find out!

AFAICT the #1 cause for failures are random machine names such as rJuF3jOd that are generated by \Drupal\TestTools\Random::machineName(), even though machine names are supposed to be all lowercase.

So … just strtolower() in that method should fix most failures?

Great, that strtolower() fixed 64% of all failures. 👍

Many (most?) of the remaining failures are just related to the fact that the existing config validation test coverage expects Regex: "/some regex/", but in #53 I had to specify a custom validation constraint message to make the validation errors actually useful. We can choose to revert #53, or we can choose to refine \Drupal\KernelTests\Core\Config\ConfigEntityValidationTestBase::testInvalidMachineNameCharacters(), to expect the more helpful validation errors. I chose the latter.

That's largely green! 🤩

Failures:

1. BlockSystemBrandingTest + BlockTest + LanguageUILanguageNegotiationTest + LocaleConfigTranslationImportTest + ArgumentDefaultTest create test blocks with dashes in the ID instead of underscores. Easy fix!
2. BlockContentWizardTest even created a test block type with spaces in the ID 😅 Easy fix!
3. The REST+JSON:API modules has a whole range of tests for various things with problems:
   1. \Drupal\Tests\jsonapi\Functional\ShortcutSetTest + ShortcutSetJsonAnonTest + ShortcutSetJsonBasicAuthTest + ShortcutSetJsonCookieTest + ShortcutSetXmlAnonTest + ShortcutSetXmlBasicAuthTest + ShortcutSetXmlCookieTest create a shortcut set with underscores instead of dashes
   2. TemporaryJsonapiFileFieldUploaderTest creates a test role with spaces in the ID! 😳 Easy fix!
4. NodeRevisionWizardTest: node types with dashes in the ID instead of underscores
5. ArgumentDefaultTest
6. ExcludedModulesEventSubscriberTest + MenuLinksTest: create menus with underscores, not hyphens 🤷‍♀️ Easy fix!
7. MigrateShortcutSetTest + MigrateShortcutSetUsersTest + MigrateShortcutTest +
8. Since #54, action and tour have updated machine name regexes, so their tests are now of course failing. Easy fix!

… which should make this green! 🤞

I see I uploaded the wrong interdiff for #59 🙈

All interdiffs starting at #53 and ending at #59 were about not creating a follow-up like the 3 we already have for widely violated validation constraints. While not complex, it does require pervasive changes: the patch grew from 32.12 KiB to 61.44 KiB. Almost double!

Proposal for keeping this issue actionable

I propose to not do what I was hoping for in #54: to reduce the scope of this issue and just ignore validation violations for machine names for now. That does require keeping some of the changes between #53 and #59, to allow us to actually detect specifically type: machine_name violations instead of generic RegEx constraint violations.

Follow-up: #3372972: Fix all type: machine_name constraint violations in tests.

Working to move most of #53 through #59 out of this patch and into the follow-up…

This goes back to #52, but adds #53 + #57. That means we're not changing anything WRT machine name validations here except for the message, so we can explicitly exempt it for now.

That is not a BC break, because #2920678: Add config validation for the allowed characters of machine names landed in 10.2.x, not 10.1.x.

Now actually add

      // @see "machine_name" in core.data_types.schema.yml
      // @todo Remove this in https://www.drupal.org/project/drupal/issues/3372972
      "The <em class=\"placeholder\">.*<\/em> machine name is not valid.",

to $ignored_validation_constraint_messages in SchemaCheckTrait, so that this passes tests once again 😊

Thanks for the swift review, @borisson_!

Updated the issue summary for core committers to help them assess the ecosystem impact. (If preferred, we could change it so that only core tests are affected, and for contrib we'd make config schema checks that fail only due to validation errors trigger deprecation errors instead.)

#3373653: Add a `langcode` data type to config schema landed and caused many new test failures! Let's find out if we can fix them here easily or whether it warrants a new follow-up issue…

Turns out that #3373653: Add a `langcode` data type to config schema was awesome but missed one edge case 😅 Not its fault though: the problem is that test coverage is all implicit, and this issue is the one that makes it somewhat more explicit!

Aight, #68 seems to be passing all of the kernel tests that failed in #62 ever since type: langcode landed. 👍

If #65 is true, we should be able to remove

+++ b/core/lib/Drupal/Core/Config/Schema/SchemaCheckTrait.php
@@ -57,6 +58,34 @@ public function checkConfigSchema(TypedConfigManagerInterface $typed_config, $co
+      // @see "machine_name" in core.data_types.schema.yml
+      // @todo Remove this in https://www.drupal.org/project/drupal/issues/3372972
+      "The <em class=\"placeholder\">.*<\/em> machine name is not valid.",

because #2972573: randomMachineName() should conform to processMachineName() pattern landed.

Let's find out.

#69 surfaces an oversight in #2920678: Add config validation for the allowed characters of machine names: the IDs of action config entities can contain periods — see user_user_role_insert() for at 2 examples:

  $add_id = 'user_add_role_action.' . $role->id();
  if (!Action::load($add_id)) {
    $action = Action::create([
      'id' => $add_id,
…
      'plugin' => 'user_add_role_action',
    ]);
    $action->trustData()->save();
  }
  $remove_id = 'user_remove_role_action.' . $role->id();
  if (!Action::load($remove_id)) {
    $action = Action::create([
      'id' => $remove_id,
…
      'plugin' => 'user_remove_role_action',
    ]);
    $action->trustData()->save();
  }

There is barely any test coverage for the Action functionality, which is how this slipped through the cracks. Which coincidentally is one of the reasons that Drupal core is working to remove the Action module: #3343369: [meta] Tasks to deprecate Actions UI module .

Anyway, it's easy enough to fix: just tweak the regex!

What I wrote in #70 about action config entities is also true for tour config entities…

I'd swear I've made exactly these tweaks before in another issue! 😳 Did I dream it? 🙈

Nope, I didn't dream it — that's what I said in #60 😅 That's what #3372972: Fix all type: machine_name constraint violations in tests is for, and we still need it — because there's much more to fix than only #2972573: randomMachineName() should conform to processMachineName() pattern!

Continuing #68, abandoning #69 + #70 — those belong in that follow-up.

#74:

1. See #23. Indeed, ::checkConfigSchema() does more now: it also applies validation. And in this particular case, we were asserting that the config schema storage types complied, even if it was invalid. Note this was an assert() call, so it already was entirely optional, and only intended to simplify things for developers. Now that CKEditor 5 is fully mature, there's no need for this anymore anyway.
2. Oh, yes, that's absolutely possible! I just followed the example from the past. But I see that olivero_post_update_add_olivero_primary_color() and system_post_update_remove_asset_entries() are setting a new example, so … following that instead! (In hindsight: makes sense: we're not repairing a broken Drupal, we're using the existing APIs, so we must use the post-update system 👍)

Self-re-RTBC'ing because this is literally just renaming+moving functions, there's no logic changes at all. Even the update path tests don't need to change!

FYI:

+++ b/core/profiles/demo_umami/config/install/editor.editor.basic_html.yml
@@ -33,6 +33,9 @@ settings:
+    ckeditor5_list:
+      reversed: false
+      startIndex: false

+++ b/core/profiles/demo_umami/config/install/editor.editor.full_html.yml
@@ -38,8 +38,13 @@ settings:
+    ckeditor5_list:
+      reversed: false
+      startIndex: false

These and other demo_umami changes here can be removed from this issue if #3377055: Validate config schema compliance of Demo Umami and Minimal profiles, just like we do for Standard lands first.

#75 failed tests. Turns out @covers syntax is tricky.

I missed this because PHPStan often refuses to work locally with certain contrib modules installed…

#77:

1. ✅ Cross-posted fix in #78 😊
2. ✅ Oops! Definitely unintentional. Fixed.
3. Correct — that's because we're not yet running config validation in these forms. #3364506: Add optional validation constraint support to ConfigFormBase is introducing the necessary infrastructure. I see two possible paths forward:
   1. Don't wait for #3364506: Add optional validation constraint support to ConfigFormBase and just always convert '' to NULL, to ensure config remains valid.
   2. Wait for #3364506: Add optional validation constraint support to ConfigFormBase to land, then adopt ValidatableConfigFormBase here, and then the empty string would trigger a validation error. Silly me — that won't work! 😅 It won't work because it still needs to be possible to not enter an iframe domain.

   So went with option 1. Hope you like it. 🤓

   update.settings:fetch.url is not affected because UpdateSettingsForm does not modify it. Similarly, system.theme.global:logo.url is not affected because ThemeSettingsForm only supports modifying system.theme.global:logo.path — in fact, it turns out that logo.url is computed at runtime only, by theme_get_setting() 😳 Looks like this is such an obscure part of the theme system that there is not even a single issue about it 😳 Either way: that's unaffected, and any improvements in the theme system are out-of-scope here (although I'm happy to file an issue against the theme system if you like.)

   Also: GREAT catch! If there were a functional test of the media settings UI, the very test coverage introduced here would've detected it… 😬 So now I'm wondering if you want me to write test coverage for that? 🤔 It seems not worth it on the one hand, but on the other hand it may be valuable as a stepping stone in bringing config validation to core. Thoughts?

Issue summary updates for #74–#79.

@longwave in Slack:

i was kinda on the fence but think we should set a good precedent here of adding the test coverage too, as hopefully it won't take very long

On it 👍

Explicit test coverage for `MediaSettingsForm` setting `media.settings:iframe_domain` to `null`.

🥳🚀

Now we're really off to the races!

Every next addition of validation constraints will instantly get a solid overview of invalid config in tests! Starting with #3376794: `type: mapping` should use `ValidKeys: <infer>` constraint by default and #3341682: New config schema data type: `required_label`.

Plus, this unblocks 4 follow-ups:

1. #3362453: Fix all ConfigExists constraint violations in tests
2. #3362456: Fix all ExtensionExistsConstraint constraint violations in tests
3. #3362457: Fix all PluginExistsConstraint constraint violations in tests
4. #3372972: Fix all type: machine_name constraint violations in tests

All 4 already have MRs, and 3 of the 4 are mostly done! Currently working on #3372972: Fix all type: machine_name constraint violations in tests 🤓

All 4 follow-ups have landed already, the last one about a week ago! 🚀

Next up: #3364109: Configuration schema & required values: add test coverage for `nullable: true` validation support, which will trigger an even bigger set of follow-up issues.

@jibran: Wow you're already testing against 11.x?! I think you're probably literally the only contrib module who does that; you'd have to patch your *.info.yml file for that to be possible too 😅

An actual answer:

1. Reproduced locally! 👍 $constraint_manager = $this->getTypedDataManager()->getValidationConstraintManager(); results in constraint_manager in your test. But that line is not new nor changed here, it's existed since 2015 😬

   Turns out that it is a bug in your test, which you copied straight from core's buggy test at \Drupal\Tests\views\Kernel\TestViewsTest::testDefaultConfig(), which this issue indeed fixed.

   The change you need:

   diff --git a/tests/src/Kernel/DynamicEntityReferenceTestViewsTest.php b/tests/src/Kernel/DynamicEntityReferenceTestViewsTest.php
   index 74390b1..d3a8125 100644
   --- a/tests/src/Kernel/DynamicEntityReferenceTestViewsTest.php
   +++ b/tests/src/Kernel/DynamicEntityReferenceTestViewsTest.php
   @@ -30,6 +30,7 @@ class DynamicEntityReferenceTestViewsTest extends KernelTestBase {
          \Drupal::service('module_handler'),
          \Drupal::service('class_resolver')
        );
   +    $typed_config->setValidationConstraintManager(\Drupal::service('validation.constraint'));
    
        // Create a configuration storage with access to default configuration in
        // every module, profile and theme.
   
   

2. After you've done the above, you'll get actual config schema violations. Expected test failure:
   

   1) Drupal\Tests\dynamic_entity_reference\Kernel\DynamicEntityReferenceTestViewsTest::testDefaultConfig
   There should be no errors in configuration 'views.view.test_dynamic_entity_reference_entity_test_mul_view'. Errors:
   Schema key 0 failed with: [dependencies.module.0] Module 'entity_test' is not installed.
   
   Failed asserting that Array &0 (
       0 => '[dependencies.module.0] Module 'entity_test' is not installed.'
   ) is true.
   

   Because you're WAY ahead of the pack, I've gone ahead and prioritized fixing that: created #3379899: Follow-up for #3361534: config validation errors in contrib modules should cause deprecation notices, not test failures 👍

   Thank you! 😊🙏

Awesome! 😊

I checked your commit history and I bet that this commit on July 5 further increases your likelihood of winning the "first contrib module to test against Drupal 11"-award 🥇😄

@effulgentsia: see point 2 in the proposed resolution.