ConfigFormBase + validation constraints: support non-1:1 form element-to-config property mapping again

As of https://git.drupalcode.org/project/drupal/-/merge_requests/5240/diffs?co..., this is what you'll see instead of an error:

… but this is obviously still not associated with the right form element.

Apply #3398974: Follow-up for #3382510: FormStateInterface::setErrorByName() needs not #name but a variation as well and you'll see:

👍 I'm hoping @phenaproxima can finish this — I'm working on #3364108: Configuration schema & required keys.

I propose an alternative solution, which applies cleanly to Drupal core at commit 13883cc3808f9442268c904f4d96379372ecf0bf. This works for everything, not just composite form elements.

In a nutshell, this restores the ability that #3382510: 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 removed: the ability to implement non-1:1 mappings by allowing modules to override ::copyFormValuesToConfig() (but still call the parent method to do as much as possible using #config_target) and optionally implement mapConfigKeyToFormElementName().

This patch allows modules with human-optimized UIs to use config validation again in their config forms, by:

1. making ::copyFormValuesToConfig protected instead of private
2. allowing ::mapConfigKeyToFormElementName() to be implemented again, and automatically calling it for any property path not found using #config_target

… which is what #3364506: Add optional validation constraint support to ConfigFormBase originally introduced, until #3382510 removed it.

Concrete example

The CDN module's CdnSettingsForm in #3394172-13: Adopt Drupal core 10.3 config validation infrastructure uses:

1. #config_target 6 times, for all the 1:1 mappings 👍
2. For the non-1:1 mapping of a few form elements in mapping][simple onto cdn.settings:mapping.conditions in config:
   

     /**
      * {@inheritdoc}
      */
     protected static function copyFormValuesToConfig(Config $config, FormStateInterface $form_state): void {
       // The 1:1 things can be handled by the base class.
       parent::copyFormValuesToConfig($config, $form_state);
   
       // The custom UI for configuring `mapping.conditions`, optimized for the
       // user's mental model, that does not map 1:1 to the underlying config.
       if ($form_state->getValue(['mapping', 'type']) === 'simple') {
         $toggle = $form_state->getValue(['mapping', 'simple', 'extensions_condition_toggle']);
         // Only the 'extensions' condition is supported in this UI, to KISS.
         if ($toggle === 'limited') {
           // Set the 'extensions' condition unconditionally.
           $config->set('mapping.conditions', [
             'extensions' => explode(' ', trim($form_state->getValue(['mapping', 'simple', 'extensions_condition_value']))),
           ]);
         }
         // Plus one particular common preset: 'nocssjs', which means all files
         // except CSS and JS.
         elseif ($toggle === 'nocssjs') {
           $config->set('mapping.conditions', static::CONDITIONS_PRESET_NOCSSJS);
         }
         else {
           $config->set('mapping.conditions', []);
         }
       }
     }
   
     /**
      * {@inheritdoc}
      */
     protected static function mapConfigKeyToFormElementName(string $config_name, string $key) : string {
       // TRICKY: cannot use #config_target because it is conditionally present.
       if ($key === 'mapping.conditions.extensions') {
         return 'mapping][simple][extensions_condition_value';
       }
   
       return str_starts_with($key, 'mapping.conditions') ? 'mapping][simple' : '';
     }
   

Indeed, while working on #3384790: Update all remaining ConfigFormBase subclasses in Drupal core to use #config_target, I already found a few spots in core where we're going to need the ability to do complex logic that is out of #config_target's reach.

Where? Examples in core would be most valuable!

Per #10

The sole Nightwatch test failure for fe9b0f11 is unrelated. All other tests passed 👍

Just pushed a commit that fixes the test expectations, which will cause it to fail: the solution so far is known to be incomplete/inadequate!

🥳 That passed tests, with the exception of AjaxTest::testAjaxFocus(), which is unrelated. Next: prove that #config_target and a copyFormValuesToConfig() override can co-exist.

Then I'm done here 👍

@phenaproxima Can you please implement 1 of the 3 examples in this issue? Because per #10, this is actually a hard blocker for #3384790: Update all remaining ConfigFormBase subclasses in Drupal core to use #config_target.

#20: I think it'd be clearer if we updated https://www.drupal.org/node/3373502 once more? 😅

Working on trying to implement @alexpott's feedback.

Addressed @alexpott's feedback!

I think do like this better.

Regardless of the DX, it has one major ecosystem/UX benefit: #config_target is present even for conditionally config property paths, which means #2408549: Display status message on configuration forms when there are overridden values will be more effective. 👍

Applied to the CDN contrib module: see interdiff-13-16.txt in #3394172-17: Adopt Drupal core 10.3 config validation infrastructure.

Next up: apply this to LocaleSettingsForm in core.

Turns out that LocaleSettingsForm is quite different from CdnSettingsForm (see previous comment):

• CdnSettingsForm has conditionality and maps 1 form element (a dropdown) to 1 subtree, unless that dropdown has a particular value, then a second form element corresponds to a value inside that same subtree
• LocaleSettingsForm has no conditionality but maps 1 form element to 2 config property paths (to be more abstract: 2 independent subtrees), and vice versa

If you zoom out/abstract this, the above means that

N form elements → 1 config target

CdnSettingsForm maps N form elements to 1 "node" in the config tree (which would never make sense of course, unless there's conditionality, which is the case here!)

• N form elements: $form['mapping']['simple']['extensions_condition_toggle'] = … and $form['mapping']['simple']['extensions_condition_value'] = …
• 1 config tree node: cdn.settings:mapping.conditions.extensions (the first form element targets cdn.settings:mapping.conditions, the second targets cdn.settings:mapping.conditions.extensions, which means the second targets a subset of the former)

1 form element → N config targets

LocaleSettingsForm maps 1 form element to N "nodes" in the config tree

• 1 form element: $form['overwrite'] = …
• N config tree nodes: locale.settings:translation.overwrite_customized + locale.settings:translation.overwrite_not_customized

👍 So AFAICT we got 2 perfectly complementary examples here! 😄

Needed to introduce ConfigMultiTarget to support the LocaleSettingsForm use case.

Looking forward to feedback :)

Started doing the remaining things that @phenaproxima did not get to.

$values = array_map($this->fromConfig, $values); does not work correctly for the multiple values case, because it calls fromConfig for each value separately … which means that it no longer receives all the values at once. IOW: in an attempt to simplify the logic, the very purpose (being able to target multiple target property paths in config at once) was made impossible 😅

Fixing that got us from 11 to 10 test failures.

Just pushed 67628221b0 which will make the multi-target case actually work again, which should get us down to 4 failures.

I also think that we should always pass $form_state as the first argument to the toConfig callback, just for consistency's sake.

I discussed this with @alexpott in Drupal Slack.

He and I agree that this was a wrong direction. It keeps Drupal core simple but deteriorates the DX for actual users of this API. That's the wrong trade-off, it must be the other way around.

By reverting that change, the remaining failing functional tests will also start passing again 👍

(Most of @phenaproxima's changes are still in the MR though: removal of ConfigMultiTarget, stricter validation, logic moved out of ConfigFormBase and into ConfigTarget.)

Issue summary updated.

#3398891: Do not require the config in #config_target to be listed in getEditableConfigNames() just landed. Rebasing. But first getting this to green 🤓

Green! Time to clean up and then rebase. 👍

The rebased MR is still green 👍

Note that #3394172-17: Adopt Drupal core 10.3 config validation infrastructure still works fine, but I've also updated that patch to leverage the use of closures & arrow functions, and that really shows just how much cleaner/simpler all of this together makes config forms: #3394172-19: Adopt Drupal core 10.3 config validation infrastructure.

The failing test case was incorrect AFAICT. See https://git.drupalcode.org/project/drupal/-/merge_requests/5240?commit_i... for the detailed explanation.

This is now green again 👍

This is ready IMO.

YAY! 😊

All of @alexpott's feedback has been addressed 👍

Good call! Fixed.

Addressed all feedback.

That included opening #3401186: Follow-up for #3382510: Throw \LogicException when >1 #config_target in the same form targets the same property path — and having a test + fix ready there too.

The change record is wildly outdated. IMHO we should delete it and update https://www.drupal.org/node/3373502 instead, like we've done for all the other #config_target issues. Since it's all new, a single comprehensive change record is much more usable.

😬 Just as I was about to update the issue summary for #50 after having implemented @alexpott's proposal, I realized that this actually does not work 😇

By allowing toConfig: FALSE, you remove the ability to conditionally throw new \OutOfBoundsException(). See #3394172-19: Adopt Drupal core 10.3 config validation infrastructure, which uses:

+      '#config_target' => new ConfigTarget('cdn.settings', 'mapping.conditions.extensions',
+        fromConfig: fn (?array $extensions): string => implode(' ', $extensions ?: []),
+        toConfig: function (string $extensions_condition_value, FormStateInterface $form_state): array {
+          if ($form_state->getValue(['mapping', 'simple', 'extensions_condition_toggle']) !== 'limited') {
+            throw new \OutOfBoundsException();
+          }
+          return explode(' ', trim($extensions_condition_value));
+        },
+      ),

I'll work on adding explicit functional test coverage for this use case.

(I just walked @phenaproxima through #53 and explained why I agreed, but while implementing @alexpott's proposal, I realized that there's still a problem. But, I do think this line of questioning has gotten us to a point where we've actually gotten to a comprehensive solution 😊)

#53: I agree that pattern works for #3394172. But it doesn't work in all cases where there's conditional logic to determine which key-value pairs should be set in config.

That proposal works because one config target targets some config subtree (mapping.conditions), and another config target conditionally targets a node in that subtree (mapping.conditions.extensions).

I tried to find a more general case for the test coverage I aimed to add here (and apparently succeeded): one config targets one config subtree (favorite_fruits), and another config target conditionally targets a different subtree (could_not_live_without).

Possible cases

I think there is a need for multiple different cases:

1. toConfig can return a value that must be set at the property path for this ConfigTarget ⇒ return that value ← this already works in HEAD ✅
2. toConfig can SOMEHOW indicate that it's "no-op", because it knows some other ConfigTarget's toConfig callable will set this property path ← this is the case in #53/CdnSettingsForm, and in that particular example an unconditional "no-op" is possible, but that's not true in all cases
3. toConfig can SOMEHOW indicate that the property path this ConfigTarget targets must be REMOVED ← this will become a hard requirement starting in #3364108: Configuration schema & required keys, and there may already be cases where this would be necessary (even if it there isn't explicit validation logic for it yet)

Possible approaches for case #2 (a form value NOT mapped to a value in config aka "no-op")

We've only been talking about point 2 here. The 3 choices so far:

1. Callable can throw \OutOfBoundsException (@Wim Leers)
2. "FALSE instead of callable" (@alexpott)
3. Callable can return ConfigTargetValueEnum::NoMapping (@phenaproxima)

The limitation of choice 2.B here is that it only supports the unconditional scenario.

Not yet considered so far: case #3 (delete a key)

In principle, we don't need to support case 3 just yet, because few modules are likely to be explicitly relying on this. But AFAICT it is going to happen (we've got +1s from @bircher and @alexpott), plus it's possible some modules already are doing something like this, so we should take that into account here already. Hence added explicit test coverage for case 3.We could choose two different exception types, but both @alexpott and @phenaproxima raised concerns about communicating using exceptions.

Conclusion

So then … @phenaproxima's proposal actually seems the most appealing, because then there would be at least two cases:

enum ConfigTargetValueEnum {
  case NoMapping;
  case DeleteKey;
}

Combined with match(), this can then be implemented very elegantly:

  match ($value) {
      // No-op.
      ConfigTargetValue::NoMapping => function (): void {},
      // Delete.
      ConfigTargetValue::DeleteKey => $config->clear($property),
      // Set.
      default => $config->set($property, $value),
    }

Just pushed explicit test coverage, which IMO makes it obvious that this is not yet supported and should be.

Issue summary updated for #54 and its conclusion + implementation.

@alexpott in Drupal Slack:

Lets call ConfigTargetValue::NoMapping something else. Maybe ConfigTargetValue::NoOp or ConfigTargetValue::NoOperation

Also ConfigTargetValue as a little awkward

it is not the value of a config target

ConfigTargetToConfigReturnValue is what it is. But that is awkward. To say the least.

• +1 for NoOp.
• What about ToConfig::NoOp and ToConfig::DeleteKey?

Implemented those renames: see commits I just pushed. Since it's "just" naming, keeping at RTBC.

Added docs to make the connection between \Drupal\Core\Form\ConfigTarget and \Drupal\Core\Form\ToConfig explicit.

Update IS for #58.

Created draft update to the change record — preview it here.

Also updated contrib module patch at #3394172-22: Adopt Drupal core 10.3 config validation infrastructure.

• Created issue per @alexpott's request, for something out of scope, but that was made more apparent here: #3401493: Convert 3 LOCALE_TRANSLATION_OVERWRITE_* constants used in the UI to a backed enum.
• Also per @alexpott: #3205480-9: Drop PECL YAML library support in favor of only Symfony YAML — again out of scope, and related to #3401493

Really hoping this will land prior to 10.2.0-beta1, because this is the very last piece to make #config_target/validatable simple config forms be usable everywhere and have a nicer DX 🤞

🥳

Deleted the obsolete draft CR at https://www.drupal.org/node/3399660.

Very glad (and relieved) to have this in 10.2!

This unblocked #3384790: Update all remaining ConfigFormBase subclasses in Drupal core to use #config_target 👍

Now we can also start thinking about how to deprecate the old infrastructure: see #3384782-7: [PP-1] Follow-up for #3364506: add deprecation once all simple config forms in core implement + #3400033-7: Deprecate \Drupal\Core\Form\ConfigFormBase::getEditableConfigNames() - use #config_target instead.

New follow-up: #3420770: Expand what #config_target can do: automatically set #attributes[pattern] for HTML5 client-side validation when RegEx constraint is present.