Introduce a new #config_target Form API property to make it super simple to use validation constraints on simple config forms, and adopt it in several core config forms

Wim Leers → credited borisson_ → .

Wim Leers → credited effulgentsia → .

Wim Leers → credited lauriii → .

Added a working PoC implementation to the IS 😊

See #3364506-103: Add optional validation constraint support to ConfigFormBase → .

Blocks 📌 [PP-1] Update all remaining ConfigFormBase subclasses in Drupal core to use config validation Active .

✨ Add optional validation constraint support to ConfigFormBase Fixed is in!

Did what I said in #6.

Observations:

1. we do need transformation methods like @effulgentsia originally proposed, because for example:
   1. in MediaSettingsForm, iframe_domain must either be a valid URI or null, NOT the empty string
   2. in BookSettingsForm, #type => checkboxes is used, and that results in '0' in form values for the unchecked checkboxes … so it MUST be filtered away
2. In BookSettingsForm::submitForm() there was some weird logic from almost exactly a decade ago ( #1933548: Book allowed_types settings repetitive and in under certain conditions can change unexpectedly → ) … that I was able to remove by specifying orderby: key instead (see #2361539: Config export key order is not predictable for sequences, add orderby property to config schema → ).
3. BookSettingsForm::validateForm() cannot be deleted here yet because it needs that validation logic to be converted to a validation constraint. That'd be too much out of scope here. So I'd say we should either revert BookSettingsForm changes or create a follow-up and point to that.

Curious what y'all think.

I like the concept here, but I have some questions about the implementation...

I spent most of the day thinking about this issue and, long story short, I think we should change the approach here and go for syntax like this (proposed in another issue, but I'm not sure which):

$form['security']['iframe_domain'] = [
      '#type' => 'url',
      '#title' => $this->t('iFrame domain'),
      '#size' => 40,
      '#maxlength' => 255,
      '#config_target' => 'media.settings:iframe_domain',
      '#description' => $this->t('Enter a different domain from which to serve oEmbed content, including the <em>http://</em> or <em>https://</em> prefix. This domain needs to point back to this site, or existing oEmbed content may not display correctly, or at all.'),
    ];

(This example comes from MediaSettingsForm.) Let me explain more about how this could work:

When loading the default value from config: ConfigFormBase::buildForm() could recursively loop through all the form's elements, and load the config value for any element which has a #config_target key, but not a #default_value key. Most (?) config forms call parent::buildForm(), so they would be automatically opted into this behavior.

When saving the value to config: We could loop through the entire form recursively (starting from $form_state->getCompleteForm()), and for any element which defines a #config_target key, we could use the element's #parents property to get the submitted value from $form_state->getValue(). Then we could set that value in the specified config object and property path.

But what if we need to transform the value before saving? Well, #config_target would make an allowance for that. It could either be a string, if there is no transformation to be done:

'#config_target' => 'media.settings:iframe_domain'

Or, if there is a transformation needed, it could be an array whose second element is the name of a string callable which will transform it:

'#config_target' => ['media.settings:iframe_domain', '::nonEmptyStringOrNull']

It could be a fully-qualified static callable as a string:

'#config_target' => ['media.settings:iframe_domain', HelperClass::class . '::transformationMethod']

If it was not fully qualified, as in the first example, we would expect it to be the name of a trusted callable. That would be enforced by DoTrustedCallbackTrait.

I think that this approach, although old-school, will have these benefits:

• The goal of this issue is to make it super-duper drop-dead easy to adopt config validation. To that end, I think we want to minimize the effort it takes contrib to adopt it. Core will adopt what it needs to, but contrib is the challenge here. Adding a new FAPI property, even though it's kind of old-school, is a very familiar pattern for contrib and custom module/theme developers. The value of the property is relatively simple (at least if there's no transformation needed). There's no need for an arcane syntax in a static array. So, to me, this approach greases the slide to contrib adoption, and that's the main reason to do it.
• But if you're not convinced, there's another reason: this means we can easily support hook_form_alter (and all of its variations). Yes, some modules want to alter other modules' config forms to add additional properties -- Automatic Updates is an example of this. It's a common pattern. Using a static array of the form class is un-alterable, and would leave all modules which need to alter another module's config forms out in the cold.
• Since the priority is to get people to use config validation, I think that adopting the more old-school FAPI property with relative "ease of understanding" and broad compatibility is an appropriate trade-off. When everybody's using config validation for their config forms, we can talk about deprecating the new FAPI property and moving to something more modern.

The value of the property is relatively simple (at least if there's no transformation needed).

This is a fairly big assumption. Transformations will be necessary in many cases. So it will be more complex than presented here in many cases.

It's a common pattern.

It's not common for simple config forms though. Even more so because the only reasons for contrib modules to alter a simple config form are to:

1. inject a form to add third party settings to the edited simple config
2. inject a form whose values are saved elsewhere (in which case it’s completely irrelevant anyway)
3. do cosmetic changes

But you're right it MUST be supported! 👍

Automatic Updates is an example of this.

Except that it's not a good example 😅, because it applies the second pattern I mentioned above: it alters the form provided by UpdateSettingsForm for update.settings, but stores its values in automatic_updates.settings! So … config validation is irrelevant in that case anyway, since \Drupal\update\UpdateSettingsForm::getEditableConfigNames() says:

  protected function getEditableConfigNames() {
    return ['update.settings'];
  }

(The only way it could add config validation is to subclass UpdateSettingsForm, add automatic_updates.settings to that and override the update.settings route definition. Which is fine … except that it then prevents other modules from doing the same! Horizontal extensibility bites once again!)

Based on a private Slack between you, @effulgentsia and I, AFAICT @effulgentsia is on board with this change in direction. The primary reason being: if it's a static array, contrib form alters become impossible to support.

Tagging for that.

Should we make this a non-blocker and do 📌 [PP-1] Update all remaining ConfigFormBase subclasses in Drupal core to use config validation Active first?

Looking at the implementations in the last few commits, this seems like such a simple DX, works very well.
I had one remark on the pull request, but it might not be something we often do.

I don't see the link to the comment, so https://git.drupalcode.org/issue/drupal-3382510/-/commit/8e4abd89e594877...

Other than that small remark, I think this is good to go, waiting for rtbc on an answer on that comment. I'm happy if we feel like that goes too far.

I have a bit more than one remark 😅

But: excellent work here, @phenaproxima! 👏

1. The issue summary does not say that this is removing mapConfigKeyToFormElementName(), which ✨ Add optional validation constraint support to ConfigFormBase Fixed added during the 10.2.x cycle (and which we're still in). So it's fine to remove, but we need to articulate why. We arrived at that solution after long and careful debate.
2. The changes in core/modules/update/src/UpdateSettingsForm.php make this really convincing. FAR less code! And yay for the test coverage that was added in the previous issue, that means I am immediately confident this works correctly for sequences too 🥳
3. I really like how this gives you one more reason to update your config forms, even if you do not yet have validation constraints: your MR shows that we'll be able to remove most ::submitForm() overrides! 🤩
4. But … other concerns I raised at #2408549-178: There is no indication on configuration forms if there are overridden values → still stand:
   • 
     → we could ensure this by validating that each #config_target value points to a config name that actually exists and is one of the getEditableConfigNames() and its property path actually exists in the schema
   • Related: it now is suspicious when a ::submitForm() override exists. Most of the time you should not need that. As is the presence of #default_value.
   • 
     → this is now no longer true. But ideally we'd be able to adopt the same pattern for config entity forms, although then it wouldn't be something like '#config_target' => 'node.settings:key', but something like '#config_target' => 'node.type.*:key'. Could we do a brief PoC to verify that this would be possible? 🙏