Issue #3402168 by Wim Leers, alexpott, Berdir, bircher, borisson_, effulgentsia: Follow-up for #3361534: Config validation errors can still occur for contrib modules, disrupting contrib

(cherry picked from commit c8b39f0117)

core/lib/Drupal/Core/Config/Development/ConfigSchemaChecker.php

@@ -53,8 +53,12 @@ class ConfigSchemaChecker implements EventSubscriberInterface {
    *   The typed config manager.
    * @param string[] $exclude
    *   An array of config object names that are excluded from schema checking.
+   * @param bool $validateConstraints
+   *   Determines if constraints will be validated. If TRUE, constraint
+   *   validation errors will be added to the errors found by
+   *   SchemaCheckTrait::checkConfigSchema().
    */
-  public function __construct(TypedConfigManagerInterface $typed_manager, array $exclude = []) {
+  public function __construct(TypedConfigManagerInterface $typed_manager, array $exclude = [], private readonly bool $validateConstraints = FALSE) {
     $this->typedManager = $typed_manager;
     $this->exclude = $exclude;
   }
@@ -82,7 +86,7 @@ class ConfigSchemaChecker implements EventSubscriberInterface {
     $checksum = Crypt::hashBase64(serialize($data));
     if (!in_array($name, $this->exclude) && !isset($this->checked[$name . ':' . $checksum])) {
       $this->checked[$name . ':' . $checksum] = TRUE;
-      $errors = $this->checkConfigSchema($this->typedManager, $name, $data);
+      $errors = $this->checkConfigSchema($this->typedManager, $name, $data, $this->validateConstraints);
       if ($errors === FALSE) {
         throw new SchemaIncompleteException("No schema for $name");
       }

core/lib/Drupal/Core/Config/Schema/SchemaCheckTrait.php

@@ -63,12 +63,15 @@ trait SchemaCheckTrait {
    *   The configuration name.
    * @param array $config_data
    *   The configuration data, assumed to be data for a top-level config object.
+   * @param bool $validate_constraints
+   *   Determines if constraints will be validated. If TRUE, constraint
+   *   validation errors will be added to the errors found.
    *
    * @return array|bool
    *   FALSE if no schema found. List of errors if any found. TRUE if fully
    *   valid.
    */
-  public function checkConfigSchema(TypedConfigManagerInterface $typed_config, $config_name, $config_data) {
+  public function checkConfigSchema(TypedConfigManagerInterface $typed_config, $config_name, $config_data, bool $validate_constraints = FALSE) {
     // We'd like to verify that the top-level type is either config_base,
     // config_entity, or a derivative. The only thing we can really test though
     // is that the schema supports having langcode in it. So add 'langcode' to
@@ -86,29 +89,22 @@ trait SchemaCheckTrait {
       $errors[] = $this->checkValue($key, $value);
     }
     $errors = array_merge(...$errors);
-    // Also perform explicit validation. Note this does NOT require every node
-    // in the config schema tree to have validation constraints defined.
-    $violations = $this->schema->validate();
-    $filtered_violations = array_filter(
-      iterator_to_array($violations),
-      fn (ConstraintViolation $v) => !static::isViolationForIgnoredPropertyPath($v),
-    );
-    $validation_errors = array_map(
-      fn (ConstraintViolation $v) => sprintf("[%s] %s", $v->getPropertyPath(), (string) $v->getMessage()),
-      $filtered_violations
-    );
-    // If config validation errors are encountered for a contrib module, avoid
-    // failing the test (which would be too disruptive for the ecosystem), but
-    // trigger a deprecation notice instead.
-    if (!empty($validation_errors) && $this->isContribViolation()) {
-      @trigger_error(sprintf("The '%s' configuration contains validation errors. Invalid config is deprecated in drupal:10.2.0 and will be required to be valid in drupal:11.0.0. The following validation errors were found:\n\t\t- %s\nSee https://www.drupal.org/node/3362879",
-        $config_name,
-        implode("\n\t\t- ", $validation_errors)
-      ), E_USER_DEPRECATED);
-    }
-    else {
+    if ($validate_constraints) {
+      // Also perform explicit validation. Note this does NOT require every node
+      // in the config schema tree to have validation constraints defined.
+      $violations = $this->schema->validate();
+      $filtered_violations = array_filter(
+        iterator_to_array($violations),
+        fn(ConstraintViolation $v) => !static::isViolationForIgnoredPropertyPath($v),
+      );
+      $validation_errors = array_map(
+        fn(ConstraintViolation $v) => sprintf("[%s] %s", $v->getPropertyPath(), (string) $v->getMessage()),
+        $filtered_violations
+      );
+      // @todo Decide in https://www.drupal.org/project/drupal/issues/3395099 when/how to trigger deprecation errors or even failures for contrib modules.
       $errors = array_merge($errors, $validation_errors);
     }
+
     if (empty($errors)) {
       return TRUE;
     }
@@ -177,17 +173,6 @@ trait SchemaCheckTrait {
     return FALSE;
   }
 
-  /**
-   * Whether the current test is for a contrib module.
-   *
-   * @return bool
-   */
-  private function isContribViolation(): bool {
-    $test_file_name = (new \ReflectionClass($this))->getFileName();
-    $root = dirname(__DIR__, 6);
-    return !str_starts_with($test_file_name, $root . DIRECTORY_SEPARATOR . 'core');
-  }
-
   /**
    * Helper method to check data type.
    *

core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php

@@ -135,9 +135,12 @@ trait FunctionalTestSetupTrait {
       $yaml = new SymfonyYaml();
       $content = file_get_contents($directory . '/services.yml');
       $services = $yaml->parse($content);
+      $test_file_name = (new \ReflectionClass($this))->getFileName();
+      // @todo Decide in https://www.drupal.org/project/drupal/issues/3395099 when/how to trigger deprecation errors or even failures for contrib modules.
+      $is_core_test = str_starts_with($test_file_name, DRUPAL_ROOT . DIRECTORY_SEPARATOR . 'core');
       $services['services']['testing.config_schema_checker'] = [
         'class' => ConfigSchemaChecker::class,
-        'arguments' => ['@config.typed', $this->getConfigSchemaExclusions()],
+        'arguments' => ['@config.typed', $this->getConfigSchemaExclusions(), $is_core_test],
         'tags' => [['name' => 'event_subscriber']],
       ];
       file_put_contents($directory . '/services.yml', $yaml->dump($services));

core/tests/Drupal/KernelTests/Core/Config/SchemaCheckTraitTest.php

@@ -39,8 +39,10 @@ class SchemaCheckTraitTest extends KernelTestBase {
 
   /**
    * Tests \Drupal\Core\Config\Schema\SchemaCheckTrait.
+   *
+   * @dataProvider providerCheckConfigSchema
    */
-  public function testTrait() {
+  public function testCheckConfigSchema(bool $validate_constraints, array $expectations) {
     // Test a non existing schema.
     $ret = $this->checkConfigSchema($this->typedConfig, 'config_schema_test.no_schema', $this->config('config_schema_test.no_schema')->get());
     $this->assertFalse($ret);
@@ -54,20 +56,36 @@ class SchemaCheckTraitTest extends KernelTestBase {
     // error messages.
     $config_data = ['new_key' => 'new_value', 'new_array' => []] + $config_data;
     $config_data['boolean'] = [];
-    $ret = $this->checkConfigSchema($this->typedConfig, 'config_test.types', $config_data);
-    $expected = [
-      // Storage type check errors.
-      // @see \Drupal\Core\Config\Schema\SchemaCheckTrait::checkValue()
+
+    $ret = $this->checkConfigSchema($this->typedConfig, 'config_test.types', $config_data, $validate_constraints);
+    $this->assertEquals($expectations, $ret);
+  }
+
+  public function providerCheckConfigSchema(): array {
+    // Storage type check errors.
+    // @see \Drupal\Core\Config\Schema\SchemaCheckTrait::checkValue()
+    $expected_storage_type_check_errors = [
       'config_test.types:new_key' => 'missing schema',
       'config_test.types:new_array' => 'missing schema',
       'config_test.types:boolean' => 'non-scalar value but not defined as an array (such as mapping or sequence)',
-      // Validation constraints violations.
-      // @see \Drupal\Core\TypedData\TypedDataInterface::validate()
+    ];
+    // Validation constraints violations.
+    // @see \Drupal\Core\TypedData\TypedDataInterface::validate()
+    $expected_validation_errors = [
       '0' => "[new_key] 'new_key' is not a supported key.",
       '1' => "[new_array] 'new_array' is not a supported key.",
       '2' => '[boolean] This value should be of the correct primitive type.',
     ];
-    $this->assertEquals($expected, $ret);
+    return [
+      'without validation' => [
+        FALSE,
+        $expected_storage_type_check_errors,
+      ],
+      'with validation' => [
+        TRUE,
+        $expected_storage_type_check_errors + $expected_validation_errors,
+      ],
+    ];
   }
 
 }

core/tests/Drupal/KernelTests/KernelTestBase.php

@@ -577,10 +577,14 @@ abstract class KernelTestBase extends TestCase implements ServiceProviderInterfa
     $container->setParameter('language.default_values', Language::$defaultValues);
 
     if ($this->strictConfigSchema) {
+      $test_file_name = (new \ReflectionClass($this))->getFileName();
+      // @todo Decide in https://www.drupal.org/project/drupal/issues/3395099 when/how to trigger deprecation errors or even failures for contrib modules.
+      $is_core_test = str_starts_with($test_file_name, $this->root . DIRECTORY_SEPARATOR . 'core');
       $container
         ->register('testing.config_schema_checker', ConfigSchemaChecker::class)
         ->addArgument(new Reference('config.typed'))
         ->addArgument($this->getConfigSchemaExclusions())
+        ->addArgument($is_core_test)
         ->addTag('event_subscriber');
     }