While working on #3230826: Expose API to sort arbitrary config arrays: TypedConfigManager::getCanonicalRepresentation() should override its parent (whose purpose is to be able to get a canonical representation of any config according to the config schema), two fundamental things were discovered:
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 #3347842: Deprecate the trusted data concept in configuration aims to deprecate it.
This is the situation in Drupal core HEAD today:
| Information flow: | 1️⃣ | 2️⃣ | 3️⃣ | assessment | |
|---|---|---|---|---|---|
type: |
value | PrimitiveTypeConstraintValidator |
StorableConfigBase::castValueresult |
ConfigSchemaChecker |
|
boolean |
true |
✅ | true |
✅ | 🤩 perfectly compliant data |
boolean |
'1' |
✅ | true |
✅ | 🤔 reasonable magical casting! 👍 |
boolean |
'test' |
❌ | true |
✅ | 🙈 this makes no sense at all! |
| 👆 validation of config barely happens in Drupal core today, but that's changing: see #2869792: [meta] Add constraints to all config entity types and #2952037: [meta] Add constraints to all simple configuration | |||||
For more detail, see the detailed background at the bottom of this issue summary.
See tests added here.
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 #3347842: Deprecate the trusted data concept in configuration simpler rather than harder: when that eventually is removed, it should be possible to trivially remove the test coverage specific to that.
Review.
None.
None.
None.
N/A
\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:
input[type=checkbox] (Sep 2012, #1696640: Implement API to unify entity properties and fields)Nothing major has happened in the past 6 years on the "make config (schema) consistent" front, with the exception of #2852557: Config export key order is not predictable, use config schema to order keys for maps.
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 🤘!
| Comment | File | Size | Author |
|---|---|---|---|
| #14 | 3421993-nr-bot.txt | 90 bytes | needs-review-queue-bot |
Show commandsStart 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 leersComment #4
wim leersComment #5
smustgrave commentedDid not test but seems there is a failing kernel test.
Comment #7
smustgrave commentedCrediting phenaproxima for the reviews on the MR.
Comment #8
wim leersComment #9
smustgrave commentedSo the feedback appears addressed but will leave for @phenaproxima to take another look.
Comment #10
phenaproximaLooks pretty good but leaving at NR because I'm still not sure some of the commentary is as clear as it could/should be.
Comment #11
borisson_I think the comments are pretty good; in my opinion this is good to go.
Comment #12
alexpottIt'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.
Comment #13
wim leers@alexpott did you see:
?
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 #3230826: Expose API to sort arbitrary config arrays: TypedConfigManager::getCanonicalRepresentation() should override its parent 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 #3230826: Expose API to sort arbitrary config arrays: TypedConfigManager::getCanonicalRepresentation() should override its parent 😇
Comment #14
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 #15
luke.stewart commentedJust noting that #2142995 might have some overlap here?
Comment #17
bbralaDid a rebase.
Comment #18
bbralaWell, all is green again after a rebase. Fun times.
Comment #19
quietone commentedI read the comments in the MR and I do think a change is needed. So setting this to needs work. There is also an existing unresolved comment.
I admit I haven't read all the comments but the title doesn't help to understand what is being done here. Maybe something like 'Add robust tests for PrimitiveTypeConstraintValidator and ConfigSchemaChecker'? Just an idea, so I am not tagging for a title update.
Comment #20
bbralaResolved all the comments and made some small changes (all naming), hopefully it will be green. Skipping back to rtbc since changes where nothing special.
TEst failure is unrealted, restarted tests, but will be bold and set back to rtbc.
Comment #21
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 #23
dcam commentedRebased. Restoring status.
Comment #24
longwaveKinda agree with @alexpott here, it's odd to be triggering any of these as deprecations? Given #3230826: Expose API to sort arbitrary config arrays: TypedConfigManager::getCanonicalRepresentation() should override its parent has been open for four years already I'm wary of adding something "temporary" to the deprecation skips which will likely not turn out to be not that temporary.
This is apparently not triggered, so should this be a failure?
For the other cases, shouldn't we assert that we know the cases are bad, then when we fix the cases in followups these tests will start failing, and we will be forced to update them? As we are skipping deprecations this test will get out of date if we are simply ignoring the things it reports instead of making explicit assertions - there is no guarantee we will remember to come back and swap TRUE for FALSE here in those cases as far as I can see.
Comment #25
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 #26
bbralaLet me paraphrase @longwave. Instead of triggering the deprecation, just make sure the test expects the casting in those cases right?
Comment #27
longwaveYes, to put it in simple terms: turn the deprecations into assertions.
Comment #29
andypostwhat is a state of MR for 12.x as all assertions will need update
Comment #30
bbralaLocally the tests are green, tried changing some 'expected' values and did get failures, so I think it should be ok like this. Enable/disable the config checker was a little annoying, but think this is a fine way to do this. Think this is ready to nr.
Comment #31
borisson_I have a few documentation questions, but I think this is looking really solid.
Comment #32
bbralaAll threads should be resolved. Added links, and some comments.
Comment #33
bbralaFailing pipeline because of gitlab instability
Comment #34
borisson_Can we rebase this now that the trusted data has been removed and remove all things here that were testing it's functionality?
Comment #35
bbralaCleaned up and rebased.
Comment #36
bbralaEmpty commit because test runner node was cleaned while testing.
Comment #37
borisson_I think this is looking good now, but I'd love to find signoff about this from someone who knows more about the history of this.
Comment #38
bbralaThat would pretty much be longwave (or alex), which it will reach eventually when RTBC?
Comment #39
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. 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 #40
bbralaRebased, all validation that was said to be broken is not broken. Ah well. Back to NR.
Edit:
Failure is: 75.773s Drupal\Tests\ckeditor5\FunctionalJavascript\CKEditor5MarkupTest 3 passed, 1 failed, exit code 1
Which is quite unrelated.