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

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 Ye Olden Way, 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 older-style 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. Introducing something unfamiliar will be counterproductive.

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.

Updated the issue summary to explain the rationale for removing copyFormValuesToConfig() from the API.

ideally we'd be able to adopt the same pattern for config entity forms

Entity forms already kinda do this mapping in a half-assed way; see \Drupal\Core\Entity\EntityForm::copyFormValuesToEntity().

So we could probably just rely on that method (even though it is rife with problems) and, in validateForm(), validate the config entity as typed data, mapping any violations back to the elements which correspond to the invalid property paths.

I updated the change record to explain how to use this (as a new revision of the existing CR): https://www.drupal.org/node/3373502/revisions/13264455/view

🏓

I think … that perhaps this is fine. It helps keeps things simple for the 99% case, and for the remaining 1%, you'd just have to implement copyFormValuesToConfig() instead of using #config_target. WDYT about that?

That'd be good to incorporate in the change record.

Makes sense to me. Tagging this issue for post-commit change record updates (since there are already change record updates needed, and I can't go edit the revision that has the required changes).

Committers, please note that this should be set to "needs work" post-commit, so that I can make the change record explain when one should override copyFormValuesToConfig().

I am not convinced that the transformer function should be bidirectional; per single responsibility principle, a function should generally only have one purpose - here the transformer has two depending on the type of the argument. Because form API is not currently strictly typed (numeric inputs arrive as strings, for example) then I think it might end up being tricky detecting the direction of transformation - especially if the config value is also a string. If we don't want to declare two functions should each transformer be a class that implements an interface with two methods? Or for one off cases, could we allow two closures to be declared in the FAPI array?

What if we added a new param to the transform function -- bool $is_saving? That way the function would have more to go on than just "how does the argument look".

I agree that it sort of flies in the face of the single responsibility principle, but IMHO that's a sacrifice worth making in the name of keeping the DX as simple as possible.

We can't use closures, as far as I know, because form arrays are serialized and cached, and you can't serialize a closure (PHP will fatal).

Maybe we can replace that array/string with a very light class:

class ConfigTarget {
  public function __construct(
    public readonly string $configTarget,
    public readonly array|string $transformOnLoadOrSave,
    public readonly array|string|null $transformOnSave = NULL,
  ) {
    // @todo:: prepare callbacks to do a proper check.
    \assert(is_callable($transformOnLoadOrSave));
    \assert(transformOnSave === NULL || is_callable($transformOnLoadOrSave));
  }
}

If $transformOnSave is missing, $transformOnLoadOrSave is bidirectional.

Then we have:

$form['foo'] = [
  ...
  '#config_target' => new ConfigTarget(
    'media.settings:iframe_domain',
    '::nullIfEmptyString',
    [SomeOtherClass::class, 'transformOnSave'],
  ),
  ...
];

Now that I made the separation between save and load callbacks explicit, this needs review again.

I agree that there are usecases where the saving and loading are different, and it is a pattern that we also use in different places.
The argument that this is now a single responsibility instead of doing both ways is a good argument as well.

I looked at the changes made in the last commit and they make sense as well, discussed this with @lauriii at drupalcon.

Does this still need subsystem maintainer review?

Added some comments.

Discussed with @Wim Leers, @borisson_ and @longwave. The callbacks - save_callback and load_callback are tricky to name. They are not loading or saving anything. As the new document says they are for transforming the value between the config and the form system.

We discussed several ideas...

1. Add a new ConfigTransformer class & interface that has toForm() and toConfig() as static methods.

So the code to use this becomes

      '#config_target' => [
        'target' => 'book.settings:allowed_types',
        'transformer' => '\Drupal\Namespace\ConfigTransformer',
      ],

The advantage of this is that the transformers are reusable and well documented. The disadvantage of this is that the transforms are moving away from the config form class and we'll have more and more classes.

2. Add a new ConfigTarget value object

The code to use this becomes

      '#config_target' => new ConfigTarget(
        'update.settings:notification.emails',
        '::loadEmailsFromConfig',
        '::storeEmailsInConfig',
      ),

The advantage of this is that we're not building on the massive ArrayPI of forms so it is easy to discovery what is possible and documenting what the callbacks do is much simpler. Ie. the class would have a method like ConfigTarget::getFormToConfigTransform() which in the above instance would return '::storeEmailsInConfig'

3. Should these transforms happen using the Form API?

The form API already has #value_callback that transforms input to an expected format. That's what we're doing here. However there's no API for manipulating #default_value so we'd need to add that. Also using #value_callback would mean that if we use a Checkboxes element we'd become responsible for calling \Drupal\Core\Render\Element\Checkboxes::valueCallback() which does not feel right. So #value_callback is happening at the incorrect level.

4. Come up with better names for save_callback and load_callback

Maybe something like config_to_form_transform() and form_to_config_transform()

I think I prefer option 1 or 2 but could live with 4 is other agree.

I'm for 2., which is more or less what I've proposed in #36, because:

• It's not a blind array that we should know how the build the sequence
• It's typed
• It's documented
• Just a tiny class
• Easier in IDE

Here's another idea: what if we introduce two new protected functions to ConfigFormBase (I'm not married to these specific names):

protected function configValueToFormValue(string $config_name, string $property_path, mixed $value): mixed {
  // Default implementation -- no transformation.
  return $value;
}

protected function formValueToConfigValue(string $config_name, string $property_path, mixed $value): mixed {
  // Default implementation -- no transformation.
  return $value;
}

If you need to do transformations, you can override one or both of these methods. This way, all the complexity of #config_target disappears: it's no longer an ArrayPI; you don't need to write a bidirectional transformation function; and it's very clear how to do a transformation in either direction.

Since transformations are a relatively "advanced" use case, I think we can rely on developers to grok this.

Transformer functions are hopefully the exception rather than the rule and for good DX given the rest of form API lives in a single class we want to try and keep the transformers in the same class as well. Can we use (or perhaps abuse) PHP attributes so transformer methods can live in the same class?

public function buildForm(array $form, FormStateInterface $form_state) {
  $form['update_notify_emails'] = [
    '#type' => 'textarea',
    '#title' => $this->t('Email addresses to notify when updates are available'),
    '#rows' => 4,
    '#config_target' => 'update.settings:notification.emails',
  ];
}

#[FromConfig('update.settings:notification.emails')]
public static function fromNotificationEmails(array $value): string {
  return implode("\n", $value);
}

#[ToConfig('update.settings:notification.emails')]
public static function toNotificationEmails(string $value): array {
  return array_map('trim', explode("\n", trim($value)));
}

Multiple config keys can share the same transformer functions if they need to. If there is no method tagged with the attribute no transformation is done.

Not sure how to scale this when we want to share transformers between forms though.

...we want to try and keep the transformers in the same class as well

I don't think so. For example strtolower() is also a transformer. Why creating a method just to do that? Then [\Drupal\Component\Utility\Bytes::class, 'toNumber'] is also a transformer. Could we use it directly but still allow ::transform() (which is in the class)?

I do think we could add logic to check that if a "from" exists, the "to" must also exist, and vice versa?

👎 I don't recommend this. Of the cases where transformations have to be done in this MR, only one of them needs both to- and from- transformations. The rest of them only need to be changed when the value is being saved.

What if we allowed the attributes to reference other callables (like strtolower()), if they needed to?

So for example, if you do this, you are saying that stringToArray() is the transformation callback for update.settings:notification.emails:

#[ToConfig('update.settings', 'notification.emails')]
public function stringToArray($value) {...}

But you could also do this, on buildForm():

#[ToConfig('system.site', 'name', callback: 'strtolower')]
public function buildForm($form, $form_state) {...}

In this case, you're saying "I want to use strtolower() as the transformer for the system.site:name config property." This would be a very advanced use case but it would address what @claudiu.cristea pointed out in #45 and make reuse possible.

But also...this is all quite a bit more complex than what I proposed in #42. What was wrong with that idea?

I'm not that keen on #46 and the attribute stuff because I think that have attributes + #config_target in form array feels odd and hard to get used to.

I think #42 might be okay especially if we encourage the use of match. But I think I've come up with a reason why attributes and #42 won't work in the long run. Form alters. How is dblog_form_system_logging_settings_alter() or something similar able to use these. Therefore I think we need this information in the form array.

@longwave and I discussed whether or not we could use schema for this. This is kind of how config translation works - see config_translation_config_schema_info_alter(). This method adds a new form_element_class to config schema. The problem putting the transformer information into schema is that this will tie schema and fomr stuff together. So I don't think this is correct.

I think of all the options is something like #36 (sorry @claudiu.cristea for not crediting you with the idea in #40 is the best. It can be used by form alters, it can use both functions on the form and functions anywhere else and the very light class can clearly document with the transformers do.

Okay, I took a shot at implementing this with a ConfigTarget value object.

To keep things frictionless for simpler cases, I made it so that you can still have #config_target be a string, and it will automatically be converted to a ConfigTarget object for you. These objects can be serialized, so they can be stored directly in the config-key-to-form-element map without trouble.

For advanced cases (that is, if you need a transformation in either or both directions), I think we can expect people to call ConfigTarget::create().

Everything in ConfigTarget is immutable (except for a couple of explicitly internal properties), but if a form alter needs to change it, they can just replace it with a completely new ConfigTarget instance.

Back to you for final review!

Change record updated!

Crediting myself for writing the current approach, Wim for reviewing it, and @claudiu.cristea for suggesting the approach we ultimately used.

Oh, and @longwave for review.

re my comment on the MR: simpler is good so that was not meant to hold up anything.

I much prefer this over the unshipped API so +1 for RTBC.
The issue summary update looks good too now.

Creditting @bircher as they were involved in the conversations at Drupalcon that led to the current API.

Spent lots of time discussing this at Drupalcon. I think the new API is the easiest way forward for now. I do wonder if something else will come up as we go forward and I have ideas for two follow-ups but I think doing them will only change internal implementation details so I'm not worried about that.

The two ideas are:

1. Do not require the config in #config_target to be listed in getEditableConfigNames() - this would make doing form alters like dblog_form_system_logging_settings_alter() really easy as you would not need dblog_logging_settings_submit()
2. Allow all callables in ConfigTarget - instead of saving ConfigTarget objects to the map we could save the location in the form array instead so we can get the config target from the form array

Committed e1c95b2 and pushed to 11.x. Thanks!

I'm going to ask the RMs if we can backport this to 10.2

Discussed with @catch and we agreed to backport this.

I think this should be part of a config validation related release highlight segment :) Feel free to identify more bits to include in that.

I've created #3398891: Do not require the config in #config_target to be listed in getEditableConfigNames() - I disagree with

extra guardrails around for the 99% use case

- I'm not sure why they need to exist and what they are for. Let's debate on the issue.

Opened #3399295: Allow all callables in ConfigTarget to address #62.2

Is there any documentation about how `#config_target` should/can be used?