Config validation: config entities should get the same validation errors when validated as plain config vs ConfigEntityAdapter

Created on 11 March 2024, about 1 month ago
Updated 4 April 2024, 9 days ago

Problem/Motivation

✨ Add validation constraints to config_entity.dependencies Fixed added ConfigEntityValidationTestBase in December 2022. We've been expanding it ever since.

While working on πŸ“Œ Deprecate \Drupal\ckeditor5\Plugin\Editor\CKEditor5::validatePair() and `type: ckeditor5_valid_pair__format_and_editor` Active , it became apparent that:

$config_entity = Editor::load('basic_html');
$violations = ConfigEntityAdapter::createFromEntity($config_entity)->validate();

and

$config_entity = Editor::load('basic_html');
$violations = \Drupal::service('config.typed')->createFromNameAndData(
  $config_entity->getConfigDependencyName(),
  $config_entity->toArray()
)->validate();

leads to different results 😱

The first has two constraints at the root level (ImmutableProperties + RequiredConfigDependencies, defined in the entity type definition), the second has two different constraints at the root level (CKEditor5FundamentalCompatibility + CKEditor5MediaAndFilterSettingsInSync, defined in the config schema definition).

This has security implications: the same config can in principle be considered valid or invalid depending on how it is validated. Not good πŸ™ˆ

Steps to reproduce

Validating using ConfigEntityAdapter vs using the "plain config object" by using ::createFromNameAndData() .

Proposed resolution

This is an oversight in ConfigEntityAdapter::createFromEntity() (which was introduced in #1818574: Support config entities in typed data EntityAdapter β†’ but then subsequently not really used; that's only started to happen recently!).

Remaining tasks

  1. βœ… Test coverage: https://git.drupalcode.org/project/drupal/-/merge_requests/6997/diffs?co...
  2. Fix. Either:
    1. Ensure validation using either approach yields the same results
    2. Not viable, see #7:

User interface changes

None.

API changes

API addition:

  • ConfigEntityAdapter::getConfigTypedData() (was a private API before, now a public one)

Data model changes

None.

Release notes snippet

None.

πŸ› Bug report
Status

Needs work

Version

11.0 πŸ”₯

Component
ConfigurationΒ  β†’

Last updated less than a minute ago

Created by

πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Live updates comments and jobs are added and updated live.
  • Security improvements

    It makes Drupal less vulnerable to abuse or misuse. Note, this is the preferred tag, though the Security tag has a large body of issues tagged to it. Do NOT publicly disclose security vulnerabilities; contact the security team instead. Anyone (whether security team or not) can apply this tag to security improvements that do not directly present a vulnerability e.g. hardening an API to add filtering to reduce a common mistake in contributed modules.

  • Contributed project blocker

    It denotes an issue that prevents porting of a contributed project to the stable version of Drupal due to missing APIs, regressions, and so on.

Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @Wim Leers
  • Status changed to Needs work about 1 month ago
  • Pipeline finished with Failed
    about 1 month ago
    Total: 524s
    #116556
  • πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
  • πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
  • Pipeline finished with Failed
    about 1 month ago
    Total: 629s
    #116580
  • πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

    AFAICT

    1. Ensure validation using either approach yields the same results

    is actually impossible, because ImmutablePropertiesConstraintValidator requires a config object rather than an array to work: it is otherwise impossible to know the original ID (to verify no disallowed property mutation is occurring). And config entities cannot be loaded by UUID; otherwise that'd be a viable alternative.

    So … switching to approach #2: 😞

  • Pipeline finished with Failed
    about 1 month ago
    Total: 585s
    #116612
  • Pipeline finished with Failed
    about 1 month ago
    Total: 476s
    #116712
  • Pipeline finished with Failed
    about 1 month ago
    #116717
  • Issue was unassigned.
  • Status changed to Needs review about 1 month ago
  • πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

    Actually, approach #2 is not a viable approach, because it prevents the validation that is executed by ConfigSchemaChecker during ConfigInstaller. So we MUST have both. Fortunately, I found a way forward (ironically, thanks to reading the code in ConfigInstaller πŸ˜„πŸ₯³πŸ‘).

    There's two things to still iron out:

    1. The one @todo
    2. Figure out how/why the ContentTranslationSynchronizedFields constraint is being added to config entity types 🀯
  • Pipeline finished with Failed
    about 1 month ago
    Total: 574s
    #116734
  • πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

    99% if not 100% of the failing kernel tests will be fixed by the todo other than the one I just added.

  • Pipeline finished with Failed
    about 1 month ago
    Total: 549s
    #117189
  • Pipeline finished with Failed
    about 1 month ago
    Total: 519s
    #117220
  • Pipeline finished with Failed
    about 1 month ago
    Total: 633s
    #117233
  • Pipeline finished with Failed
    about 1 month ago
    Total: 512s
    #117273
  • Pipeline finished with Failed
    about 1 month ago
    Total: 523s
    #117296
  • Pipeline finished with Failed
    about 1 month ago
    Total: 540s
    #117316
  • πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

    One of the two @todos is solved. The MR is now 99.9% green.

  • Pipeline finished with Failed
    about 1 month ago
    Total: 485s
    #117338
  • Pipeline finished with Success
    about 1 month ago
    Total: 573s
    #117455
  • Status changed to Needs work about 1 month ago
  • πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

    Some comments, but I think I see the nature of the problem and why this fix is so important.

    What gives me pause is the fact that these two "sides" of validation -- config schema, and ConfigEntityAdapter -- have to stay actively in sync with each other -- means we haven't necessarily fixed this problem, just mitigated it, or moved it to where it's less likely to cause trouble (not to mention added test coverage). That's probably good enough to get past the critical problem, but to me it still smacks of the "more than one way of doing things, even important things" problem.

    Not sure if I have any ideas on how to resolve that once and for all...but that's not necessarily the goal of this issue anyway. Just thought I'd call it out and be transparent about my thinking.

  • πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

    just mitigated it

    Interesting take!

    to me it still smacks of the "more than one way of doing things, even important things" problem.

    This is definitely true.

    Overall, I think @phenaproxima is saying that he is not fully satisfied with the solution in that The Proper Solution would be to not have two distinct mechanisms. I echo that desire but … the fact/reality is that all this wasn't fully thought through when config schema was added to Drupal core. The only way to eventually get to such a point where there's only one way, is to first make it all validatable, and then introduce a deprecation that disallows one or the other.

    Meanwhile, before we get to such a HUGE refactoring, let's get things that we have today working :)

  • πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

    Nope, we're in full agreement. You know me...I like pragmatism.

  • πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

    I agree with this being a good stop gap solution, we can improve the situation further down the line but this is already a good improvement and we have the test coverage to prove that this is an improvement.
    I found one extra nitpick.

  • πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

    I processed the feedback at a high level, but I won't have time to push this forward. Would be great if one of you could push it forward πŸ™

  • Pipeline finished with Failed
    30 days ago
    Total: 171s
    #118970
  • Pipeline finished with Canceled
    30 days ago
    Total: 391s
    #119281
  • Pipeline finished with Failed
    30 days ago
    Total: 185s
    #119289
  • Pipeline finished with Failed
    30 days ago
    Total: 494s
    #119351
  • Pipeline finished with Failed
    30 days ago
    Total: 585s
    #119372
  • Pipeline finished with Failed
    29 days ago
    Total: 482s
    #119380
  • Pipeline finished with Failed
    29 days ago
    Total: 697s
    #119428
  • Pipeline finished with Failed
    29 days ago
    Total: 592s
    #119784
  • Pipeline finished with Success
    29 days ago
    Total: 533s
    #120091
  • Status changed to Needs review 29 days ago
  • πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts
  • πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

    Wrote a change record. It's a bit punchier than my usual style, but hopefully it's clear.

  • πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts
  • πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

    It's a bit punchier than my usual style

    🀣

    Clarified the CR.

    Reviewed and removed one obsolete @todo. I +1'd your request for a clarifying comment in one place (could you add it? πŸ™) and added remarks on the trickier bits of the MR.

    I cannot RTBC this since I wrote ~99% of this, but I think that once you agree this makes sense, that you can still RTBC it, @phenaproxima: you've just implemented your own review remarks, but I still wrote ~99%, so should be fine :)

  • Pipeline finished with Success
    26 days ago
    Total: 498s
    #122135
  • Status changed to RTBC 26 days ago
  • πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

    Looks great to me.

  • Status changed to Needs work 22 days ago
  • 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.

  • Status changed to Needs review 10 days ago
  • πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

    Merged in upstream changes.

    This conflicted with πŸ“Œ Use cache collector for state Needs review (for updating expectations in StandardPerformanceTest) and with πŸ› Install profile is disabled for lots of different reasons and core doesn't allow for that RTBC (for updating ModuleInstaller).

    This seemed like a trivial conflict resolution, but let's await test results first.

  • Pipeline finished with Failed
    10 days ago
    Total: 634s
    #136114
  • Assigned to Wim Leers
  • πŸ‡§πŸ‡ͺBelgium Wim Leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

    A single kernel test and a single functional test failed β€” looks like I resolved the conflict with πŸ› Install profile is disabled for lots of different reasons and core doesn't allow for that RTBC incorrectly after all.

  • Status changed to Needs work 9 days ago
  • πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts
Production build https://api.contrib.social 0.62.1