Contrib.social

PrimitiveTypeConstraintValidator vs ConfigSchemaChecker: test coverage to establish the status quo

Open on Drupal.org →
Created on 16 February 2024, 4 months ago
Updated 21 March 2024, 3 months ago

Problem/Motivation

While working on 📌 Expose API to sort arbitrary config arrays Needs work (whose purpose is to be able to get a canonical representation of any config according to the config schema), two fundamental things were discovered:

  1. the values that are set are ARE the values that are saved ✅
  2. the values that are validated ARE NOT the values that are saved ❌
  3. PrimitiveTypeConstraintValidator considers some values invalid prior to the magical casting that happens in \Drupal\Core\Config\StorableConfigBase::castValue(), and only thanks to this casting does the config not trigger errors in ConfigSchemaChecker ❌

Extra complication: the "trusted data" concept introduced by #2432791: Skip Config::save schema validation of config data for trusted data → bypasses \Drupal\Core\Config\StorableConfigBase::castValue(). Which makes it the only way to trigger ConfigSchemaChecker errors for "primitives" (config schema types whose class implements PrimitiveInterface).

Note: the "trusted data" concept causes problems and 📌 Deprecate the trusted data concept in configuration Active aims to deprecate it.

This is the situation in Drupal core HEAD today:

For more detail, see the detailed background at the bottom of this issue summary.

Steps to reproduce

See tests added here.

Proposed resolution

Change NOTHING (it'd be too risky and complex to do in a single commit), but only add test coverage, to allow other issues to reconcile the inconsistencies.

Take special care to make 📌 Deprecate the trusted data concept in configuration Active simpler rather than harder: when that eventually is removed, it should be possible to trivially remove the test coverage specific to that.

Remaining tasks

Review.

User interface changes

None.

API changes

None.

Data model changes

None.

Release notes snippet

N/A

Detailed background: what is going on and how did we get here? 🕵️‍♀️

\Drupal\Core\Config\Development\ConfigSchemaChecker (which is used by kernel & functional tests unless the default of protected $strictConfigSchema = TRUE; is overridden to FALSE) performs a different kind of validation and different set of casting rules.

Turns out that \Drupal\Core\Config\Schema\SchemaCheckTrait::checkValue() (used by ConfigSchemaChecker) actually has the strictest validation:

    if ($element && is_scalar($value) || $value === NULL) {
      $success = FALSE;
      $type = gettype($value);
      if ($element instanceof PrimitiveInterface) {
        $success =
          ($type == 'integer' && $element instanceof IntegerInterface) ||
          // Allow integer values in a float field.
          (($type == 'double' || $type == 'integer') && $element instanceof FloatInterface) ||
          ($type == 'boolean' && $element instanceof BooleanInterface) ||
          ($type == 'string' && $element instanceof StringInterface) ||
          // Null values are allowed for all primitive types.
          ($value === NULL);
      }
      …
    } elseif (…) {
      …
    }
    else {
      $errors = [];
      if (!$element instanceof TraversableTypedDataInterface) {
        $errors[$error_key] = 'non-scalar value but not defined as an array (such as mapping or sequence)';
      }

      // Go on processing so we can get errors on all levels. Any non-scalar
      // value must be an array so cast to an array.
      if (!is_array($value)) {
        …

👆 the top if accepts only scalar (int/float/string/bool) and NULL, the else accepts only arrays. On top of that, integer/float/bool/string must be exactly that (look at the gettype()), they must NOT be strings.

This is in stark contrast with \Drupal\Core\Validation\Plugin\Validation\Constraint\PrimitiveTypeConstraintValidator which considers for example '1' (string!) and 0 (integer!) as valid booleans! Why doesn't this cause problems? Because \Drupal\Core\TypedData\Plugin\DataType\BooleanData::getCastedValue() does this:

  public function getCastedValue() {
    return (bool) $this->value;
  }

(which is called by StorableConfigBase::castValue()).

In other words: contradictions — what the actual validator considers valid is considered invalid by the primitive type checker!

Now, it actually makes perfect sense for this all to work this way:

  1. the config system has grown organically and to minimize disruption it accepted boolean-like values from input[type=checkbox] (Sep 2012, #1696640: Implement API to unify entity properties and fields → )
  2. the config schema system was added later (Jan 2013, #1866610: Introduce Kwalify-inspired schema format for configuration → )
  3. validation was added later (Feb 2013, #1845546: Implement validation for the TypedData API → ) … but never actually used except it a few tests
  4. when we noticed that we'd never get all config + config schema complete, valid and in sync before 8.0.0, we added the config schema checker to help us get to that point (Nov 2014, #2371843: Add event listener to check schema on config save → )
  5. … and then when we wanted to automatically enable the config schema checker to all development sites after 8.0.0 shipped (Nov 2016, #2625212: Add ConfigSchemaChecker to development.services.yml → ), we noticed it broke the entire ecosystem: #2625212-65: Add ConfigSchemaChecker to development.services.yml → … so we kind of gave up 😅

Nothing major has happened in the past 6 years on the "make config (schema) consistent" front, with the exception of 📌 Config export key order is not predictable, use config schema to order keys for maps Fixed .

This issue will make it possible to evolve the codebase towards a single set of validation rules and a single set of casting rules and will hence help the Drupal module developer experience 🚀 Let's evolve our codebase towards consistency without breaking backwards compatibility 🤘!

📌 Task
Status

Needs work

Version

11.0 🔥

Component
Configuration  →

Last updated about 8 hours ago

Created by

🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺

Live updates comments and jobs are added and updated live.
  • DrupalWTF

    Worse Than Failure. Approximates the unpleasant remark made by Drupal developers when they first encounter a particular (mis)feature.

Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @Wim Leers
  • Merge request !6650Resolve #3421993 "Primitivetypeconstraintvalidator vs configschemachecker" → (Open) created by Wim Leers
  • Comment 4 months ago →
    🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
  • Issue was unassigned.
  • Status changed to Needs review 4 months ago
  • Comment 4 months ago →
    🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
  • Pipeline finished with Failed
    4 months ago
    Total: 519s
    #96911
  • Status changed to Needs work 4 months ago
  • Comment 4 months ago →
    🇺🇸United States smustgrave

    Did not test but seems there is a failing kernel test.

  • Comment 4 months ago →
    🇺🇸United States phenaproxima Massachusetts
  • Comment 4 months ago →
    🇺🇸United States smustgrave

    Crediting phenaproxima for the reviews on the MR.

  • Pipeline finished with Failed
    4 months ago
    Total: 573s
    #98760
  • Status changed to Needs review 4 months ago
  • Comment 4 months ago →
    🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
  • Pipeline finished with Success
    4 months ago
    Total: 470s
    #98780
  • Comment 4 months ago →
    🇺🇸United States smustgrave

    So the feedback appears addressed but will leave for @phenaproxima to take another look.

  • Comment 4 months ago →
    🇺🇸United States phenaproxima Massachusetts

    Looks pretty good but leaving at NR because I'm still not sure some of the commentary is as clear as it could/should be.

  • Status changed to RTBC 3 months ago
  • Comment 3 months ago →
    🇧🇪Belgium borisson_ Mechelen, 🇧🇪

    I think the comments are pretty good; in my opinion this is good to go.

  • Status changed to Needs review 3 months ago
  • Comment 3 months ago →
    🇬🇧United Kingdom alexpott 🇪🇺🌍

    It's really different to be triggering deprecations in a test and then adding generic ignores. This feels wrong and very odd. I'm not really sure what we're communicating here.

  • Status changed to RTBC 3 months ago
  • Comment 3 months ago →
    🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺

    @alexpott did you see:

    # Temporarily while updating PrimitiveTypeConstraintValidator and ConfigSchemaChecker to agree with each other.
# @todo Remove in https://www.drupal.org/project/drupal/issues/3230826.
%The `type: (string|boolean|float|int)` config schema type uses magical casting when saving.%
%⚠️ The `type: boolean` config schema type uses magical casting for the string value `(no|test|false|)` that is inconsistent with the validation logic in Drupal\\Core\\Validation\\Plugin\\Validation\\Constraint\\PrimitiveTypeConstraintValidator.%
%⚠️ The `type: (int|float)` config schema type uses magical casting for the string value `(55\%|)` that is inconsistent with the validation logic in Drupal\\Core\\Validation\\Plugin\\Validation\\Constraint\\PrimitiveTypeConstraintValidator.%
%⚠️ The `type: (int|float)` config schema type uses magical casting for the boolean value `` that is inconsistent with the validation logic in Drupal\\Core\\Validation\\Plugin\\Validation\\Constraint\\PrimitiveTypeConstraintValidator.%

    ?

    The reason this MR is adding generic ignores is to allow this issue to be tightly scoped to ONLY documenting the current behavior in explicit tests. That then enables 📌 Expose API to sort arbitrary config arrays Needs work to remove the magic (i.e. not just documenting current behavior, but making it consistent), modify the expectations of the tests added here, and remove the generic ignores.

    The generic ignores do not negatively impact anyone? But they do allow this pure-test-coverage issue to help unblock the much more complex 📌 Expose API to sort arbitrary config arrays Needs work 😇

  • Status changed to Needs work 3 months ago
  • Comment 3 months ago →
    needs-review-queue-bot

    The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

    This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

    Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

contrib.social Blog FAQ Discussions
Production build 0.69.0 2024