ConfigFormBase + validation constraints: support composite form elements

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 🐛 Follow-up for #3382510: FormStateInterface::setErrorByName() needs not #name but a variation Closed: duplicate as well and you'll see:

I think we definitely need some tests here.

Did not test, moving to NW for tests requested in #4

This now contains 🐛 Follow-up for #3382510: FormStateInterface::setErrorByName() needs not #name but a variation Closed: duplicate and more test coverage of \Drupal\Core\Form\ConfigFormBase::validateForm() handling violations.

👍 I'm hoping @phenaproxima can finish this — I'm working on 📌 Configuration schema & required keys Fixed .

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

It's proven to work in tandem with a contrib module: #3394172-13: [PP-1] Adopt Drupal core 10.2 config validation infrastructure → .

In a nutshell, this restores the ability that 📌 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 Fixed 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().

Concrete example

The CDN module's CdnSettingsForm in #3394172-13: [PP-1] Adopt Drupal core 10.2 config validation infrastructure → uses:

1. #config_target 6 times, for all the 1:1 mappings 👍
2. For the non-1:1 mapping:
   

     /**
      * {@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' : '';
     }
   

I like Wim's idea in #9.

Restoring some of the removed infrastructure gives modules like CDN an "out" for the more complex cases. #config_target really is only meant for the simpler cases. I spent quite a bit of time on Friday trying to figure out how to make #config_target smarter, to handle these complex cases, but all I really ended up doing was tying my brain into knots. That leads me to think that we shouldn't try to make #config_target "smarter", which will probably introduce more problems than it would solve, but instead make sure developers have the tools to do things the right way. Therefore, Wim's idea makes sense to me.

Indeed, while working on 📌 [PP-1] Update all remaining ConfigFormBase subclasses in Drupal core to use config validation Active , 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!

Re #11: I've updated the issue summary with a few core forms that have complicated mapping logic.

🥳 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 📌 [PP-1] Update all remaining ConfigFormBase subclasses in Drupal core to use config validation Active .

Re #18: Sure; done!

Took a shot at a change record: https://www.drupal.org/node/3399660 →

Found one minor nitpick, otherwise this looks good to go imho.

#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 🐛 Display status message on configuration forms when there are overridden values Fixed will be more effective. 👍

Applied to the CDN contrib module: see interdiff-13-16.txt in #3394172-17: [PP-1] Adopt Drupal core 10.2 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

1. 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!)
2. LocaleSettingsForm maps 1 form element to N "nodes" in the config tree

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

Needed to introduce ConfigMultiTarget to support the LocaleSettingsForm use case.

Looking forward to feedback :)

📌 Allow all callables in ConfigTarget Fixed landed, so I've merged those changes into the MR.

Before I hit the hay for the night, here's where I'm at: I'm trying to remove ConfigMultiTarget and make ConfigTarget handle multiple property paths. It's a bit tricky to do that while aiming to keep the DX as sensible as possible. Some tests are failing but they're very clearly related to flawed logic I haven't fully finished implementing yet.

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

Long story short -- if a ConfigTarget instance is handling only one property path, then it should behave the way it does in HEAD. If it's handling multiple property paths, then the toConfig callback should return an associative array of values, keyed by property path.

To keep this logic relatively cleanly encapsulated, I'm thinking we might want to put this directly in ConfigTarget.

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 9 to 8 test failures.

Just pushed 67628221b0 which will make the multi-target case actually work again, which should get us down to 2 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 tests will also start passing again 👍 (With the exception of the unit test, which due to @phenaproxima's significant changes will need matching significant changes. So down to 1 failure.)

(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.

📌 Do not require the config in #config_target to be listed in getEditableConfigNames() Active 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: [PP-1] Adopt Drupal core 10.2 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: [PP-1] Adopt Drupal core 10.2 config validation infrastructure → .

Just updating where I'm leaving this tonight: I added a failing test case to prove a point to Wim. Other than that, as far as I know, all feedback is addressed and all other tests are passing.

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.

I'm satisfied here, although I've worked on this too. Hopefully someone who hasn't worked on it will come by and +1 RTBC...

This is lovely, I agree with the rtbc in #40. The changes here make it so much more capable.

YAY! 😊

Added some comments to the MR.

All of @alexpott's feedback has been addressed 👍

This is great: https://git.drupalcode.org/project/drupal/-/merge_requests/5240/diffs?co...
Good error messages are so helpful.

elseif ($extraneous_keys = array_diff(array_keys($value), $this->propertyPaths)) {
throw new \LogicException(sprintf('The toConfig callable returned an array that has key-value pairs that are extraneous because they do not match targeted property paths: %s.', implode(', ', $extraneous_keys)));

I though we retired the extraneous keys naming in the other issue, should we use something else here as well?

Good call! Fixed.

All remarks by @alexpott and me have been fixed. This now looks like it has very good test coverage as well, back to rtbc.

Added some comments to the MR. They need to be addressed.

Nice work on having an MR. I think the MR should be focussed on how module developers can use this for complex config forms rather than the internals of ConfigTarget.

Addressed all feedback.

That included opening 📌 Follow-up for #3382510: Throw \LogicException when >1 #config_target in the same form targets the same property path RTBC — 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: [PP-1] Adopt Drupal core 10.2 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.

If we still feel icky about communicating by exception (a feeling I generally share), another option here is to leverage an enum.

enum ConfigTargetValueEnum {

  case NoMapping;

}

Callbacks could return ConfigTargetValueEnum::NoMapping as a way of explicitly signaling they don't want the value to be mapped. Since it's a non-backed enum, there's no ambiguity.

The downside here is that it calls for a single-case enum, which is kind of lame.

RE #51, #52 - I think though that toConfig: FALSE would work for #3394172-19: [PP-1] Adopt Drupal core 10.2 config validation infrastructure → . We need to change where the extensions are being set... to something like what is below.

      '#config_target' => new ConfigTarget('cdn.settings', 'mapping.conditions',
         fromConfig: fn (array $mapping_conditions): string => match(TRUE) {
           $mapping_conditions === static::CONDITIONS_PRESET_NOCSSJS => 'nocssjs',
           !array_key_exists('extensions', $mapping_conditions) || empty($mapping_conditions['extensions']) => 'all',
           default => 'limited',
         },
         toConfig: function (string $extensions_condition_toggle, FormStateInterface $form_state): array => match ($extensions_condition_toggle) {
           // Conditions: none.
           'all' => [],
           // Conditions: no CSS/JS extensions (i.e. all files except *.css/*.js).
           'nocssjs' => static::CONDITIONS_PRESET_NOCSSJS,
           // Conditions: user-configured extensions.
           // @see ::setExtensionsConditionIfDropdownIsOnlyFiles()
           'limited' => ['extensions' => explode(' ', trim($$form_state->getValue(['mapping', 'simple', 'extensions_condition_extensions'])))],
         },
       ),

      '#config_target' => new ConfigTarget('cdn.settings', 'mapping.conditions.extensions',
         fromConfig: fn (?array $extensions): string => implode(' ', $extensions ?: []),
         // @see something about the toConfig above.
         toConfig: FALSE,
       ),

(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 (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 (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 📌 Configuration schema & required keys Fixed , 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.

If there's not yet a need to explicitly unset keys...but there will be (you seem pretty certain about that, Wim, and I believe you), then yes, the enum seems like the best solution to me as well.

I think Wim's points make sense, and I don't really have anything to complain about at the code level.

@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 → .

• Created issue per @alexpott's request, for something out of scope, but that was made more apparent here: 📌 Convert 3 LOCALE_TRANSLATION_OVERWRITE_* constants used in the UI to a backed enum Needs work .
• Also per @alexpott: #3205480-9: Drop PECL YAML library support in favor of only Symfony's 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 → 🤞

Committed and pushed 421942795e4 to 11.x and 728f592f538 to 10.2.x. Thanks!

I backported this to 10.2.x because #config_target is new and this completes the functionality so that it can be used for more complex UIs like the cdn module.

🥳

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

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

This unblocked 📌 [PP-1] Update all remaining ConfigFormBase subclasses in Drupal core to use config validation Active 👍

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 → .

Automatically closed - issue fixed for 2 weeks with no activity.

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

Another interesting follow-up where a config value has to be mapping to multiple element properties: 🐛 Two #config_targets error when used on a text_format form element Active