Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.Problem/Motivation
#3324150: Add validation constraints to config_entity.dependencies added ConfigEntityValidationTestBase in December 2022. We've been expanding it ever since.
While working on #3425349: [PP-1] Deprecate \Drupal\ckeditor5\Plugin\Editor\CKEditor5::validatePair() and `type: ckeditor5_valid_pair__format_and_editor`, 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
- ✅ Test coverage: https://git.drupalcode.org/project/drupal/-/merge_requests/6997/diffs?co...
- Fix. Either:
- Ensure validation using either approach yields the same results
- Not viable, see #7:
Only allow one of the two; in that case theConfigEntityAdapterone is probably preferred.
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.
| Comment | File | Size | Author |
|---|---|---|---|
| #20 | 3427106-nr-bot.txt | 90 bytes | needs-review-queue-bot |
Issue fork drupal-3427106
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #3
wim leersTests pushed.
This blocks #3425349: [PP-1] Deprecate \Drupal\ckeditor5\Plugin\Editor\CKEditor5::validatePair() and `type: ckeditor5_valid_pair__format_and_editor` as well as many other issues, such as #3423459: [PP-4] JSON:API DELETE support for config entities. And #3401925: [PP-2] After a recipe is successfully applied, validate all of the config that was imported or modified by config actions in the Recipes initiative.
Comment #4
wim leersComment #5
wim leersComment #6
wim leersAFAICT
is actually impossible, because
ImmutablePropertiesConstraintValidatorrequires 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: 😞
Comment #7
wim leersActually, approach #2 is not a viable approach, because it prevents the validation that is executed by
ConfigSchemaCheckerduringConfigInstaller. So we MUST have both. Fortunately, I found a way forward (ironically, thanks to reading the code inConfigInstaller😄🥳👍).There's two things to still iron out:
@todoContentTranslationSynchronizedFieldsconstraint is being added to config entity types 🤯Comment #8
wim leers99% if not 100% of the failing kernel tests will be fixed by the todo other than the one I just added.
Comment #9
wim leersOne of the two @todos is solved. The MR is now 99.9% green.
Comment #10
phenaproximaSome 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.
Comment #11
wim leersInteresting take!
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 :)
Comment #12
phenaproximaNope, we're in full agreement. You know me...I like pragmatism.
Comment #13
borisson_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.
Comment #14
wim leersI 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 🙏
Comment #15
phenaproximaComment #16
phenaproximaWrote a change record. It's a bit punchier than my usual style, but hopefully it's clear.
Comment #17
phenaproximaComment #18
wim leers🤣
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 :)
Comment #19
borisson_Looks great to me.
Comment #20
needs-review-queue-bot commentedThe 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.
Comment #21
wim leersMerged in upstream changes.
This conflicted with #2575105: Use cache collector for state (for updating expectations in
StandardPerformanceTest) and with #1170362: Install profile is disabled for lots of different reasons and core doesn't allow for that (for updatingModuleInstaller).This seemed like a trivial conflict resolution, but let's await test results first.
Comment #22
wim leersA single kernel test and a single functional test failed — looks like I resolved the conflict with #1170362: Install profile is disabled for lots of different reasons and core doesn't allow for that incorrectly after all.
Comment #23
phenaproximaComment #24
wim leershttps://www.drupal.org/project/experience_builder is why I haven't had time for this 😬
Comment #26
joachim commentedThe IS should explain the consequences of this bug more clearly. For instance, top-level constraints are not validated with this method:
Also, the CR needs some editing for style. It should explain what is new, and how it has changed. The chatty style isn't appropriate.
Comment #27
borisson_updated the CR to be a bit easier.