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! 😊

/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.

It's great catch! The added @todo needs link to issue where we can get rid of it

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 last submitted patch, 8: 3361534-8.patch, failed testing. View results →

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

+++ 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 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 ✨ Add validation constraints to config_entity.dependencies Fixed )

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 ✨ Add validation constraints to config_entity.dependencies Fixed )
   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 ✨ Add config validation for plugin IDs Fixed )

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, ✨ Add validation constraints to config_entity.dependencies Fixed 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.

⚠️ 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 :)

That will probably lead to a huge list of new issues, but hopefully they should all be super small and easy to land, is there a way we can group them by type, so that we don't have 250 new issues? I think it would be super fun to see [PP-250] in this issue but that shouldn't be a reason to do that.
Is postponing this issue on the new issues better/easier/faster to land than writing specific code here to only detect certain errors?

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

#19: Definitely not. 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. 📌 Fix all ConfigExists constraint violations in tests Fixed
2. 📌 Fix all ExtensionExistsConstraint constraint violations in tests Fixed
3. 📌 Fix all PluginExistsConstraint constraint violations in tests Fixed

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.)

• #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!

1. +++ b/core/lib/Drupal/Core/Config/Schema/SchemaCheckTrait.php
   @@ -57,6 +58,31 @@ public function checkConfigSchema(TypedConfigManagerInterface $typed_config, $co
   +    $filtered_violations = array_filter(
   +      iterator_to_array($violations),
   +      fn (ConstraintViolation $v) => preg_match(sprintf("/^(%s)$/", implode('|', $ignored_validation_constraint_messages)), (string) $v->getMessage()) !== 1
   +    );
   +    $validation_errors = array_map(
   +      fn (ConstraintViolation $v) => sprintf("[%s] %s", $v->getPropertyPath(), (string) $v->getMessage()),
   +      $filtered_violations
   +    );
   

   😍

2. +++ b/core/modules/config/tests/config_test/config/install/config_test.validation.yml
   @@ -6,3 +6,6 @@ giraffe:
   +# Required because inherited from `type: config_object`.
   +# @see core/config/schema/core.data_types.schema.yml
   +langcode: en
   

   I don't think this is needed to keep in the comment, it is useful for this issue but not for the endresult imho

3. +++ b/core/tests/Drupal/FunctionalTests/Update/UpdatePathTestBase.php
   @@ -51,6 +51,19 @@ abstract class UpdatePathTestBase extends BrowserTestBase {
   +    // @todo Remove this in Drupal 11.
   

   Should we open an issue for this todo as well? So that we know that this should be removed, because just the todo will not be enough I think.

• #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 → ).

The last submitted patch, 26: 3361534-25.patch, failed testing. View results →

#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.

The last submitted patch, 28: 3361534-27.patch, failed testing. View results →

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

The last submitted patch, 29: 3361534-29.patch, failed testing. View results →

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!

The last submitted patch, 32: 3361534-32.patch, failed testing. View results →

@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: 📌 [11.x] Remove UpdatePathTestBase::$configSchemaCheckerExclusions Closed: outdated ✅

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

I'm a bit unsure about the removal of \Drupal\KernelTests\Core\Config\DefaultConfigTest, but #34 explains why it was removed fairly well and when reading this it totally seems like a logical thing to do.
All the other changes are very simple and just get the configuration correct, so those are all very logical to me.

As mentioned in #19 I agree with there being 3 followups, luckily those are a lot fewer then I expected there to be because we can group the issues together.

I've read trough the patch a couple of times and there is nothing in there that I think needs to be changed, this is basically a test-only change that increases our strictness and validation coverage, so rtbc imho

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 📌 Fix batch process race conditions by making ‘bid (batch id)’ auto-increment Fixed and 🐛 TypedData instances created by TypedConfigManager::createFromNameAndData() are incomplete Fixed .

Adding 🌱 [meta] Make config schema checking something that can be ignored when testing contrib modules Active as a related issue.

@lauriii suggested making this change early in 10.2.x which seems good, however I also think it would be good to try to get the above issue sorted, so that modules can enable/disable strict schema checking via DrupalCI's deprecation failure config rather than actual code changes to the tests.

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: 🐛 Stop using `first-uuid` and `second-uuid` in tests: violates config schema Fixed .

🐛 Stop using `first-uuid` and `second-uuid` in tests: violates config schema Fixed 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: 📌 Tighten CKEditor 5 kernel tests Fixed .

NW to remove the @todos added in 📌 Tighten CKEditor 5 kernel tests Fixed . Also the last CI run failed hard, not sure what happened there.

📌 Tighten CKEditor 5 kernel tests Fixed 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 😊

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 📌 Fix all ConfigExists constraint violations in tests Fixed , 📌 Fix all ExtensionExistsConstraint constraint violations in tests Fixed and 📌 Fix all PluginExistsConstraint constraint violations in tests Fixed . 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 📌 Add config validation for the allowed characters of machine names Fixed because they A) barely have test coverage, B) have no UI?

Let's find out!

The last submitted patch, 54: 3361534-54.patch, failed testing. View results →

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.

Re #56 see 📌 randomMachineName() should conform to processMachineName() pattern Fixed

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: 📌 Fix all type: machine_name constraint violations in tests Fixed .

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 📌 Add config validation for the allowed characters of machine names Fixed 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 😊

As mentioned in #44, we should make this early in the 10.2 release cycle, we still have the time to do that. Moving this to rtbc. I agree with keeping this smaller so that the followups can fix the remaining errors.

This was already rtbc in #41 and in the meantime @Wim Leers made everything green but has removed most of those fixes again to make this patch smaller.

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.)

I talked to @longwave about this issue earlier today at drupal dev days, we figured that 📌 randomMachineName() should conform to processMachineName() pattern Fixed (now back to rtbc), was a blocker for this, but because of #62 (where we are ignoring the machine name errors in the tests, but making the validation already happen), this is actually not a blocker.

I think this can be committed as-is and in a follow-up we can remove the ignoring of the machine name errors and make sure they use the new Random machine name generator and that they are all correct in the tests.

The last submitted patch, 62: 3361534-62.patch, failed testing. View results →

✨ Add a `langcode` data type to config schema Fixed 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…

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 📌 randomMachineName() should conform to processMachineName() pattern Fixed landed.

Let's find out.

#69 surfaces an oversight in 📌 Add config validation for the allowed characters of machine names Fixed : 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: 📌 [meta] Tasks to deprecate Actions UI module Active .

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

The last submitted patch, 68: 3361534-68.patch, failed testing. View results →

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

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

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

Back to rtbc. This issue has surfaced a couple of issues with our configuration schema's and I think this would make the other issues dealing with configuration schema's even more correct on their first attempts.

1. +++ b/core/modules/ckeditor5/src/Plugin/Editor/CKEditor5.php
   @@ -729,8 +726,6 @@ public function validateConfigurationForm(array &$form, FormStateInterface $form
   -    assert(TRUE === $this->checkConfigSchema(\Drupal::getContainer()->get('config.typed'), 'editor.editor.id_does_not_matter', $submitted_editor->toArray()), 'Schema errors: ' . print_r($this->checkConfigSchema(\Drupal::getContainer()->get('config.typed'), 'editor.editor.id_does_not_matter', $submitted_editor->toArray()), TRUE));
   

   Are we removing this because \Drupal\Core\Config\Schema\SchemaCheckTrait::checkConfigSchema does more now, and would therefore find violations?

2. +++ b/core/modules/media/media.install
   @@ -180,3 +180,15 @@ function media_requirements($phase) {
   +function media_update_10200() {
   
   +++ b/core/modules/system/system.install
   @@ -1843,5 +1843,16 @@ function system_update_10101(&$sandbox = NULL) {
   +function system_update_10200() {
   
   +++ b/core/modules/update/update.install
   @@ -175,3 +175,15 @@ function _update_requirement_check($project, $type) {
   +function update_update_10200() {
   

   Does these need to be update hooks or could these be implemented as post update?

#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 📌 Validate config schema compliance of Demo Umami and Minimal profiles, just like we do for Standard Fixed lands first.

1. PHPStan is complaining about @covers.

2. +++ b/core/modules/system/system.install
   @@ -1843,5 +1843,4 @@ function system_update_10101(&$sandbox = NULL) {
   -
   

   Whitespace change to an otherwise unchanged file is out of scope.

3. +++ b/core/modules/media/config/install/media.settings.yml
   @@ -1,4 +1,4 @@
   -iframe_domain: ''
   +iframe_domain: ~
   

   This is changed here and in an update hook, but the UI doesn't handle nullable values:

   1. Go to /admin/config/media/media-settings and set an iframe domain
   2. Save changes
   3. Go to /admin/config/media/media-settings and set the iframe domain back to an empty string
   4. Save changes
   5. If I inspect the config at /admin/config/development/configuration/single/export then iframe_domain is '' instead of ~ or null.

   I suspect it might be the same for the other nullable URIs config values?

#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. ✨ Add optional validation constraint support to ConfigFormBase Fixed is introducing the necessary infrastructure. I see two possible paths forward:
   1. Don't wait for ✨ Add optional validation constraint support to ConfigFormBase Fixed and just always convert '' to NULL, to ensure config remains valid.
   2. 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?

I think test coverage will be nice here and hopefully not take long. It proves that we need functional test coverage to catch some of these cases - we're likely to have to introduce more of it as we go along, so let's set a good precedent by starting here.

Agree that the other two aren't affected, and yes let's open a followup issue to remove system.theme.global:logo.url from config given it shouldn't be stored anyway.

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`.

This was quick, Test coverage now proves that this should always be `null` instead of an empty string, that is how we configured the schema as well so that looks great.

+++ b/core/modules/update/config/schema/update.schema.yml
--- /dev/null
+++ b/core/modules/update/tests/src/Functional/Update/UpdateSettingsDefaultFetchUrlUpdateTest.php

+++ b/core/modules/update/tests/src/Functional/Update/UpdateSettingsDefaultFetchUrlUpdateTest.php
@@ -0,0 +1,110 @@
+      DRUPAL_ROOT . '/core/modules/system/tests/fixtures/update/drupal-9.4.0.bare.standard.php.gz',
...
+    $extensions['module']['update'] = 0;

We could probably simplify this by using filled database but don't think this needs to block this.

Going ahead and committing this to enable all of the config validation efforts to move forward. I discussed this with @catch earlier and he was onboard with this in principle.

Committed f13ad69 and pushed to 11.x. Thanks!

🥳🚀

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 📌 `type: mapping` should use `ValidKeys: ` constraint by default Fixed and 📌 New config schema data type: `required_label` Fixed .

Plus, this unblocks 4 follow-ups:

1. 📌 Fix all ConfigExists constraint violations in tests Fixed
2. 📌 Fix all ExtensionExistsConstraint constraint violations in tests Fixed
3. 📌 Fix all PluginExistsConstraint constraint violations in tests Fixed
4. 📌 Fix all type: machine_name constraint violations in tests Fixed

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

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

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

Dynamic Entity Reference views test started failing https://app.circleci.com/pipelines/github/jibran/dynamic_entity_referenc... since this commit.

Drupal\Tests\dynamic_entity_reference\Kernel\DynamicEntityReferenceTestViewsTest::testDefaultConfig
Error: Call to a member function create() on null

/data/app/core/lib/Drupal/Core/TypedData/TypedData.php:123
/data/app/core/lib/Drupal/Core/TypedData/Validation/TypedDataMetadata.php:45
/data/app/core/lib/Drupal/Core/TypedData/Validation/TypedDataMetadata.php:38
/data/app/core/lib/Drupal/Core/TypedData/Validation/RecursiveContextualValidator.php:152
/data/app/core/lib/Drupal/Core/TypedData/Validation/RecursiveContextualValidator.php:106
/data/app/core/lib/Drupal/Core/TypedData/Validation/RecursiveValidator.php:93
/data/app/core/lib/Drupal/Core/TypedData/TypedData.php:132
/data/app/core/lib/Drupal/Core/Config/Schema/SchemaCheckTrait.php:63
/data/app/core/tests/Drupal/Tests/SchemaCheckTestTrait.php:26
/data/app/modules/dynamic_entity_reference/tests/src/Kernel/DynamicEntityReferenceTestViewsTest.php:40
/data/app/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

Can I have a quick pointer how to fix the test? I had a look at the change record but it was not helpful.

@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 📌 Follow-up for #3361534: config validation errors in contrib modules should cause deprecation notices, not test failures Fixed 👍

   Thank you! 😊🙏

@Wim Leers thank you for the kind words. I run test against HEAD every morning on CircleCI. The tests starts failing since the commit was made and I was just lazy in fixing it. :D

Thank you for jumping on to it and helping me with the fix. I have pushed the fix to both DER 3.x and 4.x.

Committed and push 39cc0a2 and it is all green
https://app.circleci.com/pipelines/github/jibran/dynamic_entity_referenc....

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 🥇😄

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

Hi. I'm coming here while reviewing 📌 Configuration schema & required values: add test coverage for `nullable: true` validation support Fixed as part of background research for it. I see that this issue added:

           type: uri
+          nullable: true

for some? all? URI types.

However, SchemaCheckTrait already allows all primitive types to be null. So why was it necessary to add the additional nullable: true for URIs?

@effulgentsia: see point 2 in the proposed resolution.