Problem/Motivation
Parallel issue to #1696648: [META] Untie content entity validation from form validation, but for config. Placing this in the "configuration entity system" component for now, though possibly we want to cover single config as well?
Currently, core does not implement web services (i.e., a REST plugin) for config. However, contrib may wish to. Contrib will have a difficult time doing that if there's no way to validate config independently of a form submission.
Steps to reproduce
N/A
Proposed resolution
To achieve this, use Drupal's existing config schema infrastructure (see the documentation), which has supported validation constraints
- 🟢
For every config entity type, add a kernel test that invokes validation on it, to ensure that at all times, 100% of config entity types in Drupal core can pass validation — note that this may also simply mean there aren't any validation constraints. This is important to ensure Drupal core is at minimum at all times free-of-incorrect-validation-errors.→ #3324150: Add validation constraints to config_entity.dependencies - 🟢
Add test that allows us to track which config schema subtrees are 100% validatable. This would enable us to A) track progress, B) avoid regressions.→ #3324984: Create test that reports % of config entity types (and config schema types) that is validatable - 🤞 Ideally, get that test committed to Drupal core.
- 🟢
Provide a concrete example that shows how the output of that test can be of assistance in working to add validation constraints to more config types and config entity types.→ #3324140-8: Convert field_storage_config and field_config's form validation logic to validation constraints - Provide a concrete example of removing validation logic for a config entity form and relying on the validation constraints in the config schema instead.
Want to add validation for one more config type and/or config entity type?
- Run the test made available at #3324984: Create test that reports % of config entity types (and config schema types) that is validatable on the current version of Drupal core to identify which config entity types (which correspond to one particular config type, but are "decorated" in an entity) or which config types are not yet validatable. On Jan 1, 2023, 29 of 628 config types were validatable, 0 of 30 config entity types and 1369 of 3732 property paths.
- Analyze the
::preSave()
,::postSave()
,::save()
,::validateForm()
,::submitForm()
logic, as well as the default config to determine what validation constraints would be appropriate. What is the shape of the data? - Try to reuse existing validation constraints. You can find them by searching Drupal core for
@Constraint
. - Be very aware that one typically needs more validation logic than is contained in the pre-existing forms, because forms only allow strings to be submitted, and are constrained by the shape of the form. Assume entities are being created through REST/JSON:API/PHP code, which means arbitrary data can be received!
- Expand the pre-existing config-entity-type-specific test, to ensure that the expected validation errors are thrown.
- Create a follow-up issue that aims to remove the validation logic from the associated form(s) and rewire it to rely on the validation errors. ⚠️ Examples to follow.
User interface changes
None.
API changes
TBD
Data model changes
TBD
Release notes snippet
TBD
Comment | File | Size | Author |
---|---|---|---|
#48 | validatability.png | 366.57 KB | Wim Leers |
#37 | diff.txt | 10.86 KB | Wim Leers |
#37 | 2023-01-11.txt | 309.22 KB | Wim Leers |
#37 | 2023-01-01.txt | 309.32 KB | Wim Leers |
#16 | 2164373-16.patch | 3.2 KB | Wim Leers |
Comments
Comment #1
jhedstromComment #2
alexpottThis can't be done during 8.0.0 so postponing to 8.1.x
Comment #3
alexpottComment #4
xjm@alexpott, @cilefen, and I discussed these issues today during a CMI meeting.
The title of this issue is a little misleading as "config validation" is a different thing from the kind of validation for data done on form submission. I was going to retitle it to "use API validation for configuration entity data" or something like that. Seems like implementation-wise there is overlap with #1928868: Typed config incorrectly implements Typed Data interfaces and the potential blocker #1818574: Support config entities in typed data EntityAdapter?
Comment #5
xjmComment #7
Wim LeersDoes this just mean
?Comment #8
Wim LeersAnd I don't think this needs to be postponed anymore? This can be worked on now.
Comment #9
dawehnerWell, conceptually #1928868: Typed config incorrectly implements Typed Data interfaces would solve the underlying API capabilities. This issue is then more about organizing an effort to add all the various constraints .
Comment #16
Wim LeersI reported over at #2952037-10: [meta] Add constraints to all simple configuration that as of #2969065: Use typed config validation constraints for validation of cdn.settings simple config (which is using #1928868: Typed config incorrectly implements Typed Data interfaces which @dawehner pointed to in #9), the https://www.drupal.org/project/cdn module that I maintain uses config validation. As part of that, it is now relying completely on config validation to provide form validation too: https://git.drupalcode.org/project/cdn/blob/8.x-3.x/cdn_ui/src/Form/Vali... — this is working great 🥳.
I think that in Drupal 9 we could deprecate
ConfigFormBase
and require everything to move toValidatableConfigFormBase
by Drupal 10?Initial patch attached.
Comment #17
Wim LeersComment #18
alexpottI think I'd prefer a flag on the ConfigFormBase that defaults to the current behaviour but you can opt in to the new behaviour. And then trigger a deprecation error if it is not set to the new behaviour.
Comment #19
Wim LeersWorks for me :)
Comment #20
alexpottForgot to say - really lovely to see some progress on this @Wim Leers++
Comment #23
thursday_bw CreditAttribution: thursday_bw as a volunteer commentedAfter a nudge from larowlan in #australia-nz slack - suggest we remove the meta from this ticket.
Many of the blocking issues mentioned are resolved now, it's ready for implementation, there's even a patch.
Comment #28
Wim LeersFor the 34 current followers: good news, we have significant progress on this front despite the silence in this issue! 🤓🥳
Simple config
#2952037: [meta] Add constraints to all simple configuration would be nice but it would only solve validation for simple config. The CDN contrib module has been using that for >3 years (see #2969065: Use typed config validation constraints for validation of cdn.settings simple config).
Config entities
The real challenge is config entities. Well … the CKEditor 5 module (experimental module in Drupal core since 9.3) is about to become stable and has no validation logic in its
::validateConfigurationForm()
implementation — it's all in validation constraints which are applied using theconstraints
key in the config schema! 🥳Applying it to all config entities in Drupal core
So … the next step here is to make it easy to repeat this pattern, and to mass-convert existing form validation logic to validation constraints. #3231342: [PP-2] Introduce ConfigEntityForm to standardize use of validation constraints for config entities will introduce the infrastructure to make that easy.
It'll actually be relatively simple (mostly)
The CKEditor 5 validation case is more complex for three reasons
editor
config entities, which are tightly coupled and must be kept in sync withfilter_format
config entities — #3231354: [PP-1] [META] Discuss: merge the Editor config entity into the FilterFormat config entity would remove that complexityWhat's next? What does this bring us?
The above is why I'm optimistic about expanding this to other config entities. Let's make a big push forward in Drupal 10.1? 🤓
This is what made the decoupled Admin UI initiative nearly impossible. Today, we have the proven blueprints for making #2300677: JSON:API POST/PATCH support for fully validatable config entities possible!
But that's not the only advantage! Examples:
Comment #29
Wim LeersMaybe we can start with
Vocabulary
config entities, like @fago proposed ~8.5 years ago in #2002174: Allow vocabularies to be validated via the API, not just during form submissions? 🤓Comment #30
Wim LeersBig news!
Huge tangible progress in #3324150: Add validation constraints to config_entity.dependencies by @phenaproxima (which brings some validation to ALL config entities! 🤩), and I also got started on #3324140: Convert field_storage_config and field_config's form validation logic to validation constraints, both will be used by https://www.drupal.org/project/field_ui_modern. 🤓
Also working to expose the schema information through JSON:API Schema, and working on bugfixes there: #3324769: Schema incorrect for config entity types with multi-part IDs (field_storage_config, field_config, entity_view_mode, entity_form_mode, entity_view_display and entity_form_display) + #3324824: Schema incorrect for config entity "fields" that are Maps and Sequences.
Comment #31
Wim LeersUpdate: created #3324982: [Proposal/Exploration] Expose generating JSON Schema that matches the config schema for decoupled admin UIs based on trying to expose the pre-existing config schema for
field_storage_config
via JSON:API Schema. In there, I proposed an exploration direction to find out how we can build on top of the existing JSON Schema ecosystem and the JSON Schema Validation RFC.Key things we should evaluate (and which this meta issue is tangentially related to):
Comment #32
Wim LeersNow that #3324150: Add validation constraints to config_entity.dependencies is RTBC and #3324140: Convert field_storage_config and field_config's form validation logic to validation constraints is moving forward, I created #3324984: Create test that reports % of config entity types (and config schema types) that is validatable to help us track overall progress! 😊
Comment #33
Wim LeersThe very first piece of system-wide config entity validation landed! 🤩🚀 See #3324150: Add validation constraints to config_entity.dependencies. Next up: #3324140: Convert field_storage_config and field_config's form validation logic to validation constraints and #3324984: Create test that reports % of config entity types (and config schema types) that is validatable.
Comment #34
Wim LeersHuge progress!
ValidKeys: ['<infer>']
is now being applied to alltype: mapping
config, and hence helps ensure a huge part of configuration is already partially validatable! (#3324984: Create test that reports % of config entity types (and config schema types) that is validatable is applying that generically.)So … now that we've arrived at this point … it's finally time to articulate a plan to crowdsource updating all of Drupal core. Added that to the issue summary! 🚀
Comment #35
Wim LeersHTML fix.
Comment #36
Wim LeersTypo fix.
Comment #37
Wim LeersYay, the plan from #34 is starting to come to fruition: #2920682: Add config validation for plugin IDs landed with a new validation constraint and used it in two places (somewhere in Block config entity's schema and somewhere in Text Editor config entity's).
Results of the output from #3324984: Create test that reports % of config entity types (and config schema types) that is validatable's test coverage for
drupal/core
at the time of #34 and today are attached and diffed.As expected:
and
And so it begins … 🤓🤞
Comment #38
Wim LeersFYI: The next step forward is happening at #2920678-113: Add config validation for the allowed characters of machine names! 🥳
Comment #39
Wim LeersFor the ~50 interested people: @phenaproxima proposed to work on #3341682: New config schema data type: `required_label` next. I'm working on #3324140: Convert field_storage_config and field_config's form validation logic to validation constraints 👍
Comment #41
Wim Leers#2920678: Add config validation for the allowed characters of machine names landed!
#3373570: #3360991 changed TypedConfigManager::createFromNameAndData() in 10.1.x added 10.1 support to the Config Inspector module.
Focus now shifts to:
ValidatableConfigFormBase
: #3364506: Add optional validation constraint support to ConfigFormBasetype: label
validation: #3341682: New config schema data type: `required_label`IMHO the two next most important problems to tackle (which hopefully @phenaproxima will 🤞), with the most benefit for the ecosystem, would be to:
type: langcode
, which would benefit bothtype: config_object
andtype: config_entity
, so would impact 100% of config.type: _core_config_info
, which again would benefit bothtype: config_object
andtype: config_entity
, so would impact 100% of config.Comment #42
phenaproximaOpened #3373653: Add a `langcode` data type to config schema to tackle:
Comment #43
phenaproximaOpened #3374394: Add validation constraints to _core_config_info for:
Comment #44
Wim Leers#3374394: Add validation constraints to _core_config_info and #3373653: Add a `langcode` data type to config schema have landed, and together they significantly improve on the status quo. Compared to #37:
Next up: continue the plan from #41 (#41.1 (working to get back to passing tests + RTBC after #3373653: Add a `langcode` data type to config schema landed!). In addition to expanding validatability the way I described in #41.3, @phenaproxima has been working on that same problem space (#42 + #43), and I just opened #3376794: `type: mapping` should use `ValidKeys: <infer>` constraint by default 🆕, which will bring a big boost.
P.S.: #41.2 has been RTBC for 2 weeks now! 👍
Comment #45
Wim LeersUpdate for the plan in #41 from ~1 month ago:
and the two suggested next steps have also been completed:
Lots of other Configuration schema validation issues have landed besides those. For example: #3377055: Validate config schema compliance of Demo Umami and Minimal profiles, just like we do for Standard
A fun observation for #3361534: @jibran, maintainer of https://www.drupal.org/project/dynamic_entity_reference, reported over at #3361534-89: KernelTestBase::$strictConfigSchema = TRUE and BrowserTestBase::$strictConfigSchema = TRUE do not actually strictly validate that his module started failing tests against
11.x
. That means he's probably the first (and likely the only! 😄😮) module maintainer who's running testa againstDrupal 11
. So, prioritized #3379899: Follow-up for #3361534: config validation errors in contrib modules should cause deprecation notices, not test failures, which now also is reviewable. Added to the numbered list above 👍Many thanks to everyone involved:
(I hope I didn't forget anyone! 🙈)
Comment #46
borisson_Issues created during drupalcon:
#3364108 is still rtbc and #3364109 needs a few more test fixes.
Comment #47
marvil07 CreditAttribution: marvil07 at Adapt commentedJust in case this is useful, there are three child issues marked as RTBC, that are waiting core maintainer action 😅.
Namely #3382581: Add new `EntityBundleExists` constraint at 2023-10-10, #3395616: Add validation constraints to image.settings at 2023-11-08, and #3395631: Add validation constraints to dblog.settings at 2023-10-30.
Comment #48
Wim LeersI kinda lost track of this meta 😅 But we've made a TON of progress. Most of the issues listed in #46 have landed. And thanks to #3422641: Add a "Validatable config" tests job to GitLab CI to help core evolve towards 100% validatability having landed yesterday, we won't be making backwards steps anymore, only forwards 🥳
As of today, https://project.pages.drupalcode.org/config_inspector/ looks like this:
Also, the original purpose of this very issue has been unblocked: see the green MR over at #2300677: JSON:API POST/PATCH support for fully validatable config entities! We absolutely can and should proceed there, even before every config entity type has been made validatable. We now have the necessary infrastructure! 🤘
Yesterday, I've created a new meta issue with a clearer, more holistic view of things: #3427641: [meta] Config validation for a more reliable Drupal + reliable Recipes from the start. It splits things 2-ways at a high-level: patterns to apply all across core (with meta/plan issues for those, since those things have dozens of child issues) vs missing infrastructure.
Let's continue there 🙏 This issue has served us well over the past decade, time for a fresh start for the new decade! 😄
Comment #49
Wim LeersImproving discoverability.