Add optional validation constraint support to ConfigFormBase

Now, a demo of how to actually use it. And to show that gradual adoption is totally possible.

user.settings:anonymous is missing validation constraints:

$ drush config:inspect --filter-keys=user.settings --detail --list-constraints
 Legend for Data: 
  ✅❓  → Correct primitive type, detailed validation impossible.
  ✅✅  → Correct primitive type, passed all validation constraints.
 ----------------------------------------------------- --------- ------------- ------ --------------------------------- 
  Key                                                   Status    Validatable   Data   Validation constraints           
 ----------------------------------------------------- --------- ------------- ------ --------------------------------- 
  user.settings                                         Correct   74%           ✅❓                                    
   user.settings:                                       Correct   NOT           ✅❓                                    
   user.settings:_core                                  Correct   NOT           ✅❓                                    
   user.settings:_core.default_config_hash              Correct   NOT           ✅❓                                    
   user.settings:anonymous                              Correct   NOT           ✅❓            
…

So let's define it:

  user.settings:
    type: config_object
    label: 'User settings'
    mapping:
      anonymous:
        type: label
        label: 'Name'
+       constraints:
+         NotBlank: []
+         Regex:
+           pattern: '/[[:alnum:][:space:]]+/'
+           message: 'Using only emojis is not allowed because it may not be supported in all contexts.'

Then simply changing

-class AccountSettingsForm extends ConfigFormBase {
+class AccountSettingsForm extends ValidatableConfigFormBase {

and moving some logic out of ::submitForm() (which we'll eventually refactor away, but AccountSettingsForm edits THREE different Simple Configs!):

   public function submitForm(array &$form, FormStateInterface $form_state) {
     parent::submitForm($form, $form_state);
 
-    $this->config('user.settings')
-      ->set('anonymous', $form_state->getValue('anonymous'))
-      ->set('register', $form_state->getValue('user_register'))
-      ->set('password_strength', $form_state->getValue('user_password_strength'))
-      ->set('verify_mail', $form_state->getValue('user_email_verification'))
-      ->set('cancel_method', $form_state->getValue('user_cancel_method'))
-      ->set('notify.status_activated', $form_state->getValue('user_mail_status_activated_notify'))
-      ->set('notify.status_blocked', $form_state->getValue('user_mail_status_blocked_notify'))
-      ->set('notify.status_canceled', $form_state->getValue('user_mail_status_canceled_notify'))
-      ->save();

and into the new mapFormValuesToConfig():

+  /**
+   * {@inheritdoc}
+   */
+  protected static function mapFormValuesToConfig(FormStateInterface $form_state, Config $config): Config {
+    switch ($config->getName()) {
+      case 'user.settings':
+        $config->set('anonymous', $form_state->getValue('anonymous'))
+          ->set('register', $form_state->getValue('user_register'))
+          ->set('password_strength', $form_state->getValue('user_password_strength'))
+          ->set('verify_mail', $form_state->getValue('user_email_verification'))
+          ->set('cancel_method', $form_state->getValue('user_cancel_method'))
+          ->set('notify.status_activated', $form_state->getValue('user_mail_status_activated_notify'))
+          ->set('notify.status_blocked', $form_state->getValue('user_mail_status_blocked_notify'))
+          ->set('notify.status_canceled', $form_state->getValue('user_mail_status_canceled_notify'));
+    }
+    return $config;
+  }

is all we need to get validation constraints working:

cspell.

+++ b/core/lib/Drupal/Core/Form/ValidatableConfigFormBase.php
@@ -0,0 +1,100 @@
+  public function submitForm(array &$form, FormStateInterface $form_state) {
+    foreach ($this->getEditableConfigNames() as $config_name) {
+      $config = static::mapFormValuesToConfig($form_state, $this->config($config_name));
+      $config->save();
+    }
+    parent::submitForm($form, $form_state);

This doesn't seem like it is specific to the validate version of this?

Tried testing this out by adding an emoji to the anonymous user field and it saves vs throwing an error.

#5: Can you elaborate? I'm not sure what the point is that you're making 😅

out by adding an emoji

The regex is not forbidding emojis completely, it's only forbidding a value that contains onlyemoji 😊

#5:

@Wim Leers, I wasn't sure about if we should include that code in this class or if it should be in the base class instead since it seems to be something that shouldn't be specific to this being a validatable config form.

Fixing issue tag 😅

This is the base class, isn't it? It can't live in the parent class (ConfigFormBase) because the mapFormValuesToConfig() method it calls lives in ValidatableConfigFormBase, not ConfigFormBase.

Maybe you're suggesting that mapFormValuesToConfig() should be added to ConfigFormBase instead? We can't do that, unfortunately, because it'd be a BC break.

@Wim Leers that's what I mean. I added just an emoji to the field and it still saved.

Retested and the error is correctly shown. So there is proof this approach is working

Talking with @Wim Leers at DrupalCon talked about adding 1 converted form as an example and landed on the Media Settings Form.

Maybe you're suggesting that mapFormValuesToConfig() should be added to ConfigFormBase instead? We can't do that, unfortunately, because it'd be a BC break.

I watched the talk you did at drupalcon, I now understand more the usecase for this form, and I agree with @smustgrave that this should be implemented in at least one form to land.

Not a *hard* contrib blocker, but there are now contrib modules (Spambot for example) that would like to be using this instead of implementing it locally/copying.

For anyone in contrib that wants to use this base form, I added it to the Helper module in ✨ Add ValidatableConfigFormBase from upcoming core issue Fixed .

I do wish we didn't necessarily need a new form base and could opt-in with the existing one though.

@Dave Reid: oh, interesting idea! No strong feelings. This felt logical/less complex, but perhaps what you propose would be easier for the ecosystem to adopt?

Addressing the feedback from @borisson_ and @smustgrave…

In order to use the media settings form for this, I'd need to either modify the media.schema.yml file (i.e. its config schema) or the default iframe_domain: '' value, because the default value (the empty string) is a violation of the config schema type 😅 That would be out of scope here. It's in scope of 📌 KernelTestBase::$strictConfigSchema = TRUE and BrowserTestBase::$strictConfigSchema = TRUE do not actually strictly validate Fixed .

So I'm going for UpdateSettingsForm instead!

First, let's add a test for that form 😅

#17 should pass tests — assuming it does, that establishes the baseline. We need to match the same UX while removing the validation logic at UpdateSettingsForm::validateForm().

The interdiff:

1. adds ValidatableConfigFormBase
2. adds the word to the cspell dictionary
3. updates UpdateSettingsForm to use ValidatableConfigFormBase → results in 1 file changed, 19 insertions(+), 68 deletions(-) 🤩
4. adds the Email constraint to the type: email config schema type

The 4th point means a system-wide change, but nothing is executing validation yet, so it should be fine. Also … type: email containing valid e-mails doesn't seem like a controversial change? 😅 But, still, if preferred, I could remove it from there and instead add this constraint to update.schema.yml's schema for update.settings.

Applied the patch on a Drupal 11.x with a standard install.
I can't save the form with a valid email "smustgrave@gmail.com" my d.o. one I get
This value should be of the correct primitive type.

Adding "http://example.com" I get

This value should be of the correct primitive type.
"smustgrave@gmail.com " is not a valid email address.
"http://example.com" is not a valid email address.

Can't seem to save the form.

This value should be of the correct primitive type.

is due to

…
fetch:
  url: ''
…

in the default config at core/modules/update/config/install/update.settings.yml which has type: uri in the config schema, which the above patch has solved by setting:

+++ b/core/modules/update/tests/src/Functional/UpdateSettingsFormTest.php
@@ -0,0 +1,87 @@
+    // Match what a full Drupal installation would do.
+    $this->config('update.settings')
+      ->set('fetch.url', UpdateFetcher::UPDATE_DEFAULT_URL)
+      ->save();

😬

You're right though that this won't be acceptable for Drupal core. My bad!

So I have two choices:

1. fix the default config (and provide an update path)
2. add infrastructure to ValidatableConfigFormBase to allow certain property paths to be exempted, under the guise of "well, these property paths aren't editable anyway via this form!". The problem with that is that it's totally possible that there's validation logic that covers a combination of property paths. So config validation really should always be applied on the entire config. Otherwise we open up a wholly new can of worms…

So, going with choice 1.

This should reproduce what @smustgrave reported.

Update path test: expected to add one more failure to the sole failure I'm expecting for #21.

Finally: the update path!

• Did some clean-up.
• I should've removed in #17, now removed.
• Change record created: https://www.drupal.org/node/3373502 →

• #19 "smustgrave@gmail.com " is not a valid email address. 👈 I cannot reproduce this on latest 11.x with
• Did some clean-up.

The last submitted patch, 21: 3364506-21.patch, failed testing. View results →

The last submitted patch, 22: 3364506-22.patch, failed testing. View results →

The last submitted patch, 23: 3364506-23.patch, failed testing. View results →

The last submitted patch, 25: 3364506-25.patch, failed testing. View results →

As of #23, the 2 previously failing tests are passing, but now there's a sole new failure:

There was 1 error:

1) Drupal\KernelTests\Config\DefaultConfigTest::testModuleConfig with data set "update" ('update')
Exception: update.settings: \Drupal\Component\Diff\Engine\DiffOpChange::__set_state(array(
   'type' => 'change',
   'orig' =>
  array (
    0 => '  url: \'\'',
  ),
   'closing' =>
  array (
    0 => '  url: \'https://updates.drupal.org/release-history\'',
  ),
))

Easy fix!

Issue previously reported seems resolved!

Looks like custom commands failed

It looks like this could benefit 🐛 Display status message on configuration forms when there are overridden values Fixed because it's trying to implement different approach for mapping form elements to config.

Custom commands failed on a new policy introduced in the last few days:

Running spellcheck on *all* files.
/var/www/html/core/modules/update/tests/src/Functional/UpdateSettingsFormTest.php:49:44 - Forbidden word (e-mail)
/var/www/html/core/modules/update/tests/src/Functional/UpdateSettingsFormTest.php:50:64 - Forbidden word (e-mail)
/var/www/html/core/modules/update/tests/src/Functional/UpdateSettingsFormTest.php:58:28 - Forbidden word (e-mail)

CSpell: failed

🙃

Fixed by doing s/e-mail/email/

It looks like this could benefit 🐛 Display status message on configuration forms when there are overridden values Fixed because it's trying to implement different approach for mapping form elements to config.

This would indeed enormously simplify that.

The last submitted patch, 34: 3364506-34.patch, failed testing. View results →

Hah! 📌 KernelTestBase::$strictConfigSchema = TRUE and BrowserTestBase::$strictConfigSchema = TRUE do not actually strictly validate Fixed landed and already did the hard work us of providing the upgrade path for the invalid default update.settings config! 🥳

Rerolled, this is now even simpler, and especially less distracting! 😊

Self re-RTBC'ing because I literally only deleted code 😁

Rerolled, the patch stopped applying today. No conflicts.

@lauriii has indicated in Slack that this is missing and hard-blocking commit. We need a review from either @tim.plunkett or @effulgentsia, because both of them are maintainers of the Form API and Plugin subsystems per MAINTAINERS.txt, which makes them ideally suited.

This looks really good. I'm happy to sign off on this as subsystem maintainer. Things I noticed while reviewing:

-      elseif (count($invalid) == 1) {
-        $form_state->setErrorByName('update_notify_emails', $this->t('%email is not a valid email address.', ['%email' => reset($invalid)]));
-      }
-      else {
-        $form_state->setErrorByName('update_notify_emails', $this->t('%emails are not valid email addresses.', ['%emails' => implode(', ', $invalid)]));
-      }

With the patch, I think we're losing info if multiple email addresses are invalid, because while I think the replacement logic in ValidatableConfigFormBase would call $form_state->setErrorByName() for each email in the sequence, the implementation of setErrorByName() can only set a single error per form element name. Or, does our constraint validator handle merging violations of multiple items in a sequence into a single violation that contains info about all of them?

+  protected static function mapConfigPropertyPathToFormElementName(string $config_name, string $property_path) : string {
+    if ($property_path === 'notification.emails') {

Related to above, interesting that the config property path is notification.emails, not notification.emails.#. Perhaps the validator is merging violations of multiple items of a sequence into a single one?

+  /**
+   * Maps the form values from form state onto the given editable Config.
+   *
+   * @param \Drupal\Core\Form\FormStateInterface $form_state
+   *   The current state of the form.
+   * @param \Drupal\Core\Config\Config $config
+   *   The configuration being edited.
+   *
+   * @return \Drupal\Core\Config\Config
+   *   The updated configuration object.
+   */
+  abstract protected static function mapFormValuesToConfig(FormStateInterface $form_state, Config $config): Config;

Is there a use-case for ever returning a different $config object than the one that was passed in? Or is the pattern to always mutate the $config that was passed in? If the latter, should we change the return type to void to help clarify that?

+  /**
+   * Maps the given violation constraint property path to a form name.
...
+   * @return string
+   *   The corresponding form name.
+   */
+  protected static function mapConfigPropertyPathToFormElementName(string $config_name, string $property_path) : string {

In the docs, s/form name/form element name/. Also I think the @return doc should expand on that to say that in this context the form element name should be the full #parents path of the form element as documented in FormStateInterface::setErrorByName().

1. +++ b/core/lib/Drupal/Core/Form/ValidatableConfigFormBase.php
   @@ -0,0 +1,98 @@
   +    protected TypedConfigManagerInterface $typedConfigManager
   ...
   +      $container->get('config.typed')
   

   Nit: trailing comma, please

2. +++ b/core/lib/Drupal/Core/Form/ValidatableConfigFormBase.php
   @@ -0,0 +1,98 @@
   +    foreach ($this->getEditableConfigNames() as $config_name) {
   +      $config = static::mapFormValuesToConfig($form_state, $this->config($config_name));
   

   Consider adding some assertion here to ensure the config hasn't already been saved (it shouldn't be!)

3. +++ b/core/lib/Drupal/Core/Form/ValidatableConfigFormBase.php
   @@ -0,0 +1,98 @@
   +    foreach ($this->getEditableConfigNames() as $config_name) {
   +      $config = static::mapFormValuesToConfig($form_state, $this->config($config_name));
   
   +++ b/core/modules/update/src/UpdateSettingsForm.php
   @@ -116,34 +84,24 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +  protected static function mapFormValuesToConfig(FormStateInterface $form_state, Config $config): Config {
   +    $config
   +      ->set('check.disabled_extensions', $form_state->getValue('update_check_disabled'))
   

   This is called once per editable config, and while this implementation only has 1 available, I think as the first and only example in core this should have a switch/match/if on $config->getName() to demonstrate how that would need to be handled

4. +++ b/core/modules/update/src/UpdateSettingsForm.php
   @@ -2,50 +2,18 @@
   +class UpdateSettingsForm extends ValidatableConfigFormBase implements ContainerInjectionInterface {
   

   The implements ContainerInjectionInterface was never needed, but now it's confusing without the create() override

5. +++ b/core/modules/update/src/UpdateSettingsForm.php
   @@ -116,34 +84,24 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +    if ($property_path === 'notification.emails') {
   +      return 'update_notify_emails';
   

   I'm guessing it's because it's the only one we know has any config validation, but why is this the only one mapped? The other 3 values have mismatches too. I think as the example form being converted, this should establish a pattern for doing this for all 4 properties (whether it's switch/match/if)

I was about to write a comment about the mutable vs returned Config but I see @effulgentsia brought it up just now.

#40

• RE: multiple vs single invalid: I'll need to expand test coverage for that — great catch! In general, validators do not automatically handle that as nicely as you describe.
• RE: sequence property path: another excellent point — that will be addressed together with the above! 👍
• RE: different config object: no, it'd be the same. It just felt natural to me that it'd be returned, but you're right that in the case of PHP, objects are always mutable by reference, so … switched to void ✅
• RE: s/form name/form element name/: done ✅

#41.2: Now that I tried to implement that, I'm not actually sure we can do this? The only way could theoretically work is:

      if ($this->config($config_name->getRawData() == $config->getRawData()) {
        throw new \LogicException('::mapFormValuesToConfig() implementation already saved the config but should not!');
      }

… but that would not work in case nothing is being modified 😅

#41: 1 + 3 + 4 + 5: done ✅

Will address @effulgentsia's remaining feedback tomorrow, curious about thoughts regarding #41.2 🤓

To address #40's remarks wrt validation, let's resurface the test-only patch from #17. (Since then, a few simplifications landed in that test coverage in the patches above, but only in the setUp() part, I won't show those in the interdiff, only the changes relevant to #40.)

In testing this, I found another edge case bug: \r was not being trimmed from individual lines! 😅 Added test coverage for that too.

Alright, #43 proves that those things are nicely handled in HEAD's UpdateSettingsForm 👍

Now it's up to me to match that UX in this refactored implementation 🤓

Let's first apply the updated test coverage to the main patch: now its test coverage should fail, which would confirm the bugs that @effulgentsia discovered.

The last submitted patch, 44: 3364506-44.patch, failed testing. View results →

Great, #44 failed as expected 👍 Specifically, it failed on the test coverage that verifies that the error message is associated with a specific form element (so basically a consequence of the first two points of @effulgentsia's review at #40).

Now, let's fix this entirely.

There are essentially two tricky things at play here:

1. mapConfigPropertyPathToFormElementName() should specifically be aware of sequences, and it wasn't until now. 🙈 I really tried to use PHP 8's match (…) {…} to our benefit here, but unfortunately it's either a match on the parameter you pass into match (…) (which prevents partial string matches like we'd need here) OR you hardcode match (TRUE) and then specify a lot of expressions, which reduces readability a lot. So I went with the best balance I could find — and I tried many possible approaches 😅 Really curious what y'all think!
2. Made \Drupal\Core\Form\ValidatableConfigFormBase::validateForm() a LOT smarter when it comes to dealing with the relatively common "type: sequence is mapped to a single form element" case that we run into here, where we also run into the (reasonable!) limitation that FormState::setErrorByName() has: only a single validation error message can be associated with a particular form element.

   Basically: detect when >1 validation constraint violation targets the same form element, and whenever that happens, use the index in the sequence to actually provide better (more precise) validation error messages! 🤓🤩 Even more curious what y'all think about that one!

And of course, #46 will fail because

+++ b/core/modules/update/tests/src/Functional/UpdateSettingsFormTest.php
@@ -46,22 +46,43 @@ public function testUpdateSettingsForm() {
+    $this->assertSession()->statusMessageContains('http://example.com, http://example.com/also-not-an-email-address are not valid email addresses.', MessengerInterface::TYPE_ERROR);

does not match what you see in #46. But it changes for the better: it becomes more precise!

The last submitted patch, 46: 3364506-46.patch, failed testing. View results →

We need to remove the change to the drupalci.yml again, to see what happens with all the tests.

Done.

This already had a subsystem maintainer review, and the change here doesn't break anything until we use that base class for more things. Implementing this for more forms would be great, but should be done in followups. Back to rtbc.

Thanks @Wim Leers for addressing my (and @effulgentsia's) feedback! +1 to RTBC

Sorry I'm coming in late with a question...

There's discussion above about whether this could an opt-in on the base class, that was also my first thought when I saw the issue title.

First of all I thought about the base class implementing mapConfigPropertyPathToFormElementName(), and using an interface to determine whether mapFormValuesToConfig was implemented. The problem with that is mapFormValuesToConfig() is protected and it would have to be public, so not great.

But... traits can have abstract methods, so would that work?

It would mean something like:

subclasses use the trait.

ConfigFormBase checks method_exists(static::mapFormValuesToConfig()) and if it exists, then we know the trait is being used and can do the validation steps. Or it could check an empty interface that the trait provides methods for.

My other question is 'This value should be of the correct primitive type' when does this show up?

Interesting. Nobody surfaced the trait route. It’s at least theoretically possible:https://3v4l.org/7fIUk 👍

@Dave Reid had asked for opt-in in #14. Tim didn’t mention it in #41, but he did consider that too, he thought this was simpler.

I’ve just checked it, and even abstract methods on traits work just fine: https://3v4l.org/ASMl2. So … yes, possible! 😄

👨‍💻 So I went ahead and implemented it …👨‍💻 Conclusions:

• It's possible
• But it has a very poor DX, and would be very brittle: the mapConfigPropertyPathToFormElementName() default implementation is just that, a default, a fallback. Pretty much every form would have to override this. Why? Because if they don't, that means they'd be literally exposing the (internal) config data structure to the (external) user interface! 😱🙈

  Unfortunately, if you want to call a trait method, you need to explicitly alias it:

    use ValidatableConfigFormTrait {
      mapConfigPropertyPathToFormElementName as defaultMapConfigPropertyPathToFormElementName;
    }
  

  and then call it using the alias. Brittleness #1 This would apply to 99% of uses of this trait.

• The second is even worse: if you need to extra things upon submit (which the sample form happens to do), you need to be REALLY careful to remember that parent::something() calls the base class's method, not the trait's! Which in this case means that you are calling ConfigFormBase::submitForm() and not ValidatableConfigFormTrait::submitForm(), which would result in the form not actually saving anything (I know this because the test failed and it took me 15 mins to figure this out 😬🤷‍♂️).

  So here, you'd need to do:

    use ValidatableConfigFormTrait {
      mapConfigPropertyPathToFormElementName as defaultMapConfigPropertyPathToFormElementName;
      // TRICKY: parent::submitForm() refers to ConfigFormBase's, not the trait's!
      submitForm as defaultSubmitForm;
    }
  …
    public function submitForm(array &$form, FormStateInterface $form_state) {
      // do extra stuff
  
      $this->defaultSubmitForm($form, $form_state);
    }
  

  Brittleness #2.

• Based on the above, I think the current RTBC patch is still the best possible trade-off? 😅

RE: other question: that shows up if you haven't applied DB updates. The update.settings default config was fixed in 📌 KernelTestBase::$strictConfigSchema = TRUE and BrowserTestBase::$strictConfigSchema = TRUE do not actually strictly validate Fixed and comes with an update path 😇

But it has a very poor DX, and would be very brittle: the mapConfigPropertyPathToFormElementName() default implementation is just that, a default, a fallback. Pretty much every form would have to override this. Why? Because if they don't, that means they'd be literally exposing the (internal) config data structure to the (external) user interface! 😱🙈
Unfortunately, if you want to call a trait method, you need to explicitly alias it:

I don't think this is right. A lot of classes would need to override mapConfigPropertyPathToFormElement() name, but since it's just a one-liner, I don't think they'd ever have to call the raw trait method from their override or from anywhere else?

If you define a method from a trait in your class, then it overrides, no aliases required.

The second is even worse: if you need to extra things upon submit (which the sample form happens to do), you need to be REALLY careful to remember that parent::something() calls the base class's method, not the trait's!

I'm not suggesting overriding any of ConfigFormBase's existing methods in the trait.

What I was suggesting was providing a trait with only the two new methods (the abstract and non-abstract one). In ConfigFormBase it would then have to use an interface or method_exists() to determine whether the form is validatable or not - directly in its own methods that it already has. So if you call parent:: it'll just be ConfigFormBase's stuff.

+++ b/core/modules/update/src/UpdateSettingsForm.php
@@ -12,7 +13,13 @@
  */
-class UpdateSettingsForm extends ValidatableConfigFormBase {
+class UpdateSettingsForm extends ConfigFormBase {
+
+  use ValidatableConfigFormTrait {
+    mapConfigPropertyPathToFormElementName as defaultMapConfigPropertyPathToFormElementName;
+    // TRICKY: parent::submitForm() refers to ConfigFormBase's, not the trait's!
+    submitForm as defaultSubmitForm;
+  }
 
   /**
    * {@inheritdoc}
@@ -110,7 +117,7 @@ protected static function mapConfigPropertyPathToFormElementName(string $config_

@@ -110,7 +117,7 @@ protected static function mapConfigPropertyPathToFormElementName(string $config_
       'check.interval_days' => 'update_check_frequency',
       'notification.emails' => 'update_notify_emails',
       'notification.threshold' => 'update_notification_threshold',
-      default => parent::mapConfigPropertyPathToFormElementName($config_name, $property_path),
+      default => self::defaultMapConfigPropertyPathToFormElementName($config_name, $property_path),
     };

Ahh OK, so UpdateSettingsForm is doing that.

However that seems solvable with two methods - one that does the logic, and one that calls it but can be overridden.

I like it! 😄

Done! I think this is what you had in mind? 😊 That indeed completely removes "brittleness #1" I was worried about in #54! 👍😄 Wish I had thought of this.

(Had to remove the readonly, that triggered that PHPStan error.)

What I was suggesting was providing a trait with only the two new methods (the abstract and non-abstract one). In ConfigFormBase it would then have to use an (empty) interface or method_exists() to determine whether the form is validatable or not - directly in its own methods that it already has. So if you call parent:: it'll just be ConfigFormBase's stuff.

Now implemented that too, which indeed completely removes "brittleness #2". It's sort of a hybrid between what @Dave Reid suggested balanced against the need to have an abstract method to ensure a good DX.

This means the code changes to adopt this become even simpler! 🤩

Updated issue summary and change record.

Oops 😅

Thanks! This is much more what I hand in mind, and seems like it worked out OK.

Two minor nits/questions:

1. +++ b/core/lib/Drupal/Core/Form/ConfigFormBase.php
   @@ -47,10 +64,90 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +      $config = $this->config($config_name);
   +      // @phpstan-ignore-next-line
   +      static::mapFormValuesToConfig($form_state, $config);
   +      $typed_config = $this->typedConfigManager->createFromNameAndData($config_name, $config->getRawData());
   +
   

   Why is it OK to call ::mapFormValuesToConfig() here without checking method_exists()? edit: crosspost, fixed above.

2. +++ b/core/lib/Drupal/Core/Form/ConfigFormBase.php
   @@ -47,10 +64,90 @@ public function buildForm(array $form, FormStateInterface $form_state) {
       */
      public function submitForm(array &$form, FormStateInterface $form_state) {
   +    if (in_array(ValidatableConfigFormTrait::class, class_uses($this))) {
   +      foreach ($this->getEditableConfigNames() as $config_name) {
   

   Here though I see it's using class_uses() before calling the method, but also I'm not sure we can use class_uses().

   https://www.php.net/manual/en/function.class-uses.php - this only checks the actual current class.

   So if you subclass a config form that uses the trait, and you don't explicitly add the trait yourself, this will return FALSE. Can we just use method_exists() though?

#62:

1. ✅ Indeed already addressed by #61 👍

2. So if you subclass a config form that uses the trait, and you don't explicitly add the trait yourself, this will return FALSE. Can we just use method_exists() though?

   Damn, you're right: https://3v4l.org/odajL 😱

   Oh PHP 😅

   We could do https://www.php.net/manual/en/function.class-uses.php#110752 or https://www.php.net/manual/en/function.class-uses.php#125274, but that's far more complicated than a simple method_exists().

   Done! 👍

The changes requested in #62 have both been fixed, this new version is now much more in line with how I figured it was going to work, introducing this in contrib or a new form in core is now much easier as well. This is great. It's now as easy as using the trait and implementing mapFormValuesToConfig instead of having to use the new base class.

The last submitted patch, 63: 3364506-63.patch, failed testing. View results →

Looks like a known random fail

Unassigning @catch because I know he's on vacation 🏝️😊

P.S.: as #3341682-68: New config schema data type: `required_label` → shows, there's often a mismatch between which form elements are required/optional vs which config is required/optional. The introduction of mapConfigPropertyPathToFormElementName() in this issue would in theory enable us to e.g. show warnings to developers when system.logging:error_level is set to verbose or when assertions are enabled — that'd help more developers keep their forms in sync with their config, which would result in Drupal being perceived as more reliable! 😊 Just one of many things this issue helps enable!

The current solution with trait looks pretty nice! I'm still wondering how does the current solution integrate with 🐛 Display status message on configuration forms when there are overridden values Fixed which needs to map config with the form as well? I don't think it should have to implement its own trait, but using `ValidatableConfigFormTrait` for mapping config to form seems a bit off too. Since the trait doesn't do the actual validation, and mapping config to the form can enable several different functionalities, maybe we need to rename the trait into something more generic?

Needs review for #69 however there's another problem too. ConfigFormBase now relies on the existence of the methods to determine whether to validate or not, if we use it for other things, then we can't use the existence of those methods as an explicit opt-in to being validatable.

One option would be to make the trait naming more generic, but additionally add an empty interface so that forms can indicate they're validatable that way - they'd still need to use the trait too though.

@lauriii in #69:

I'm still wondering how does the current solution integrate with 🐛 Display status message on configuration forms when there are overridden values Fixed

See #2408549-178: There is no indication on configuration forms if there are overridden values → . Adding override indicators would be as simple as (pseudocode):

foreach ($overridden_property_paths as $property_path) {
  $form_element_name = static::mapConfigPropertyPathToFormElementName($property_path);
  // add override indicator to $form_element_name
}

I don't see the problem in the name ValidatableConfigFormTrait: it is using far richer validation than >90% of config forms — because >90% of config forms have either zero validation, and AFAICT none have full validation.

I agree it'd be slightly awkward to have to use ValidatableConfigFormTrait to get override indicators. But is that really such a big deal? Won't we eventually roll ValidatableConfigFormTrait into ConfigFormBase? Isn't it just a way to allow config forms to gradually start opting in?

IMHO we can put the "override indicator" logic in ConfigFormBase itself, just like we're doing here for 90% of the logic: the trait is at this point just a way to opt in and ensure the necessary logic is written, by having that abstract method. We can expand the documentation addition we have on ConfigFormBase:

+ * Subclasses of this form can choose to use config validation instead of
+ * form-specific validation logic. To do that, add:
+ * @code
+ *   use ValidatableConfigFormTrait;
+ * @endcode

We could add To provide override indicators on the form, ValidatableConfigFormTrait must be used as well. — wouldn't that be sufficient? 🤔

@catch in #70: I think an even more generic trait name would be more confusing. Adding an interface to the mix (so: trait + interface) would make it more confusing to adopt IMHO.

Adjusted proposal 🤓

Would it be simpler to remove the trait altogether, and instead:

1. move final protected static function defaultMapConfigPropertyPathToFormElementName into ConfigFormBase as-is
2. move protected static function mapConfigPropertyPathToFormElementName(string $config_name, string $property_path) : string into ConfigFormBase as-is
3. move abstract protected static function mapFormValuesToConfig(FormStateInterface $form_state, Config $config): void; to a new ValidatableConfigFormInterface and make it non-abstract and public ⚠️ (interfaces cannot have private/protected methods!)

That'd mean every form that wants to adopt this would have to:

1. extend the interface
2. implement the method dictated by the interface

… which is not a whole lot different to #63's 1) use the trait, 2) implement the abstract method

That would satisfy @Dave Reid's concern 😅 My only concern is that this feels like a rather silly interface, for one tiny concern, so it doesn't feel cohesive. And we'd still want to do the override indicator stuff automatically, to ensure as many right forms as possible would benefit, right?

Adjusted adjusted proposal 😅

What if we omit the interface altogether?!

What if we did this as step 3 instead:

1. move abstract protected static function mapFormValuesToConfig(FormStateInterface $form_state, Config $config): void; to ConfigFormBase and make it non-abstract, keep it protected, but make its default implementation throw new SomeException, and use that as a way to detect if the current config form is using validation constraints or not!

The implication of putting this into ConfigFormBase itself is that we want eventually all config forms to adopt this. Which … I think we do anyway?! Who doesn't want override indicators? And to get to the full potential of a more reliable Drupal, all config should be validatable using validation constraints, eventually. So we could … trigger a deprecation notice, to encourage people to adopt this 😊 That'd actually be a major improvement over #63 — at least for me, when looking at this from config schema validation angles 🤓

Curious what y'all think!

Despite how I probably appear, I don't feel very strongly about the exact implementation for ConfigFormBase and friends. I care especially about the end result: precise validation, consistently in the UI and through the API, resulting in improved reliability of Drupal, and indirectly enabling additional UX improvements, such as the override indicators of 🐛 Display status message on configuration forms when there are overridden values Fixed .

Fixed #71. (PHPStan often refuses to run locally…)

Fixed #72.

Fixed #72.

If @catch and @lauriii like either of the new approaches better, the IS + CR need to be updated.

The last submitted patch, 75: 3364506-72-fixed.patch, failed testing. View results →

YAYYYY that worked!

 Remaining self deprecation notices (2)

  2x: Implementing form-coupled validation logic is deprecated in drupal:10.2.0 and and will trigger a PHP error from drupal:11.0.0. Implement ::mapFormValuesToConfig() instead. See https://www.drupal.org/node/3373502 

👍

Will add that to the ignored deprecation notices tomorrow, ready for review 😊

I like the idea of moving the concept and implementation of validating the config that's edited by a form into ConfigFormBase itself. Seems like the question then is how do forms gradually opt into that. #75's approach is for the form to implement mapFormValuesToConfig().

I'd like to propose that instead of that, perhaps it would be better DX for the form to implement just a static property? Like so:

protected const CONFIG_NAME = 'update.settings';

protected static $formElementNamesToConfigKeys = [
  'update_check_disabled' => [self::CONFIG_NAME, 'check.disabled_extensions'],
  'update_check_frequency' => [self::CONFIG_NAME, 'check.interval_days'],
  'update_notify_emails' => [self::CONFIG_NAME, 'notification.emails', ['transform' => MultilineToSequence::class]],
  'update_notification_threshold' => [self::CONFIG_NAME, 'notification.threshold'],
];

Then individual forms wouldn't need to implement mapFormValuesToConfig() or mapConfigPropertyPathToFormElementName() unless they're doing something exotic that can't be expressed declaratively in $formElementNamesToConfigKeys, because ConfigFormBase's implementations could implement what is declared in $formElementNamesToConfigKeys.

In the above example, MultilineToSequence might be a class that's in \Drupal\Core\Form\Confg\ and that implements something like \Drupal\Core\Form\Confg\FormValueToConfigValueInterface containing 3 methods:

  function getConfigValueFromFormValue($form_value)
  function getFormValueFromConfigValue($config_value)
  function getSingleViolationMessage($violation_messages)

As we convert other forms, we'll probably discover additional form value <=> config value transformations that we'll need to create the corresponding classes for that implement this interface.

As a bonus, in the future (not necessarily in this issue), a declarative $formElementNamesToConfigKeys property as opposed to an imperative mapFormValuesToConfig() method would allow us to change:

$form['update_check_frequency'] = [
  ...
  '#default_value' => $config->get('check.interval_days'),

to:

$form['update_check_frequency'] = [
  ...
  '#default_value' => $this->getConfigValueFor('update_check_frequency'),

or even remove that line entirely and have ConfigFormBase automatically populate it :)

Reviews #69. Works fine!

#79: What you describe/show there (a static property on the form class), has always been my ideal DX scenario! 😄🤝

I just never saw a way we could implement it that was too complicated DX-wise. 😞

I'm still not entirely convinced that your proposal would result in a good DX 😅 But I'd love to give it a try.

More worryingly, I don't think it can work for everything. What if the form UI is entirely human concepts and nothing in the UI maps 1:1 to key-value pairs in config? What if you have to combine multiple form elements' values to a single value for a key in config?

I'd like to hear from a few more people what they think about @effulgentsia's very interesting proposal before I start writing code 🙏

I can see how #79 could simplify the mappings, especially for simple use cases. The downside of this is that we are introducing a new array structure that needs to be learned – you would have to read the docs to understand how it works. With mapFormValuesToConfig() and mapConfigPropertyPathToFormElementName(), you are essentially just writing code, meaning that your IDE can support you.

For this reason, I personally might still prefer the approach of defining mapFormValuesToConfig() and mapConfigPropertyPathToFormElementName() instead of defining them in the static property. I'm not against adding the static property but if there's a risk of derailing this issue, maybe it's something we could provide in a follow-up issue? It seems that it would be a pure DX addition on top of #75.

I agree with #82, learning another new magic array structure doesn't sound like fun to me. This could be added a followup later and I don't think the DX win is very big compared to what we already have in this issue today.

Good points! I hadn't realized that we could indeed implement it as a "nice syntactic sugar-esque" DX enhancement on top of mapFormValuesToConfig().
I think a OneToOneConfigFormTrait would be a great name for this. That would be a GREAT way to start creating a config form ("to get up and running"), and simultaneously a superb indicator of a likely not-so-great admin UI! 😄
If @effulgentsia is on board for that, I'll create a follow-up for this. Tagging to ensure we don't forget.

Updated #72, which failed tests only because lots of the new deprecation notice. Updated core/.deprecation-ignore.txt (and fixed a typo: and and → and).

IS + CR updated for the current patch.

Follow-up for #79 created: 📌 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 .

+++ b/core/lib/Drupal/Core/Form/ConfigFormBase.php
@@ -47,11 +60,150 @@ public function buildForm(array $form, FormStateInterface $form_state) {
+   */
+  protected static function mapFormValuesToConfig(FormStateInterface $form_state, Config $config): void {
+    @trigger_error('Implementing form-coupled validation logic is deprecated in drupal:10.2.0 and will trigger a PHP error from drupal:11.0.0. Implement ::mapFormValuesToConfig() instead. See https://www.drupal.org/node/3373502', E_USER_DEPRECATED);
+    // This allows ::submitForm() and ::validateForm() to know that this config
+    // form is not yet using constraint-based validation.

So in 11.x would we make this method abstract? If so we could probably explicitly say that in the deprecation error.

What happens if you implement the method but don't map all your config (I know the answer is in the patch somewhere...). If the answer is that it doesn't really do anything, I'm wondering if we might want to make the new method abstract in 12.x, but then in 12.x also trigger a deprecation if the mapping is incomplete (if we can?). That might reduce temptations to do a 'fake' implementation.

So in 11.x would we make this method abstract? If so we could probably explicitly say that in the deprecation error.

Yes. Done 👍

What happens if you implement the method but don't map all your config (I know the answer is in the patch somewhere...).

There's two directions, so a two-part answer:

1. form value → config: no automatic detection, depends on the mapFormValuesToConfig() implementation. I don't see how we can automatically detect this: a form may only allow editing some subset of the config, a form may be altered, there could be complex transformation logic from one form element to many config property paths, et cetera.
2. config property paths → form element names: as long as the mapConfigPropertyPathToFormElementName() override calls defaultMapConfigPropertyPathToFormElementName() per the recommendations, then its fallback logic will run:
   

       return str_replace('.', '][', $property_path);
   

   The return value of that is passed to FormState::setErrorByName(). An non-existent form element name will just result in the error being shown at the form level, without being associated with a particular form element.

   This could theoretically be updated to scan the form to actually find the form element. But that'd frankly be something Form API should do. In fact, that'd still be a net DX improvement for Form API too!

Conclusion: only when there's less dynamism/less logic involved, can we detect these problems. And that's what 📌 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 will provide.

That might reduce temptations to do a 'fake' implementation.

I very much agree that'd be wonderful to do! But:

1. in the form values → config direction, I don't see how we could ever verify completeness given that we cannot know which config properties are actually editable in the form
2. in the other direction, the only thing we could do, is complain when the computed form element name does not exist, and this is A) no worse than HEAD, B) an improvement that would benefit every form! 🤓

@catch indicated in Drupal Slack that he's worried about contrib modules adopting this and providing bogus implementations.

For mapConfigPropertyPathToFormElementName() that's not a major concern because:

1. the string return type forces something sensible to be returned
2. worst case the empty string would just mean the validation error would be associated with the entire form rather than a specific form element. That's the same problem as in HEAD.

For mapFormValuesToConfig(), it's a bigger concern because the given Config object must be updated. Fortunately, we can quite easily detect the case that it's a bogus implementation: if none of the edited config objects are in fact being updated!

So, added detection logic for that, which will now throw a \LogicException to nudge developers in the right direction. For forms editing a single config it'll generate an exception like

LogicException: Drupal\update\UpdateSettingsForm::mapFormValuesToConfig() is invalid because it did not update the edited Config object 'update.settings'. in Drupal\Core\Form\ConfigFormBase->validateForm() (line 146 of core/lib/Drupal/Core/Form/ConfigFormBase.php).

and for multiple:

LogicException: Drupal\mymodule/MySettingsForm::mapFormValuesToConfig() is invalid because it did not update any of the edited Config objects: 'mymodule.settings', 'mymodule.moresettings'. in Drupal\Core\Form\ConfigFormBase->validateForm() (line 149 of core/lib/Drupal/Core/Form/ConfigFormBase.php).

1. +        // When only a single message exists, just set it.
   +        if (count($violation_messages) === 1) {
   +          $form_state->setErrorByName($form_element_name, $violation_messages[0]);
   +          continue;
   +        }
   

   What if only 1 email address is invalid, but it's not the 1st (0th) one that's in the textarea?

2. +    return match ($property_path) {
   +      'check.disabled_extensions' => 'update_check_disabled',
   +      'check.interval_days' => 'update_check_frequency',
   +      'notification.emails' => 'update_notify_emails',
   +      'notification.threshold' => 'update_notification_threshold',
   +      default => self::defaultMapConfigPropertyPathToFormElementName($config_name, $property_path),
   +    };
   

   I think we should put an if ($config_name === 'update.settings') check around this. Technically not needed since that's the only config that's in getEditableConfigNames(), but I think it would both be more robust and be more clear to have another explicit check. Or would you like to argue against that?

3. +  public static function mapFormValuesToConfig
   

   Do we want this to be public? Or is that just a leftover from prior iterations where this was on a trait/interface?

4. +  public static function mapFormValuesToConfig(FormStateInterface $form_state, Config $config): void {
   +  protected static function mapConfigPropertyPathToFormElementName(string $config_name, string $property_path) : string {
   

   This might be bikeshedding, so I won't hold up RTBC/commit on it, but FWIW, there's something about both of these method names starting with map, but one of them returning its result and the other one mutating an argument, that bothers me. Also, while the PropertyPath terminology comes from Symfony's ConstraintViolationInterface, I don't think we need to expose simple config forms to that internal terminology. Within $config->get(), we simply call that a key. Therefore, perhaps alternate method names could be:

   • Option 1: setConfigFromFormValues() and getFormElementNameFromConfigKey()?
   • Option 2: copyFormValuesToConfig() and mapConfigKeyToFormElementName()?

   Note that EntityForm has a copyFormValuesToEntity() method, which might make option 2 a little nicer, though in that case it might also be good to match the parameter order of that method (i.e., $config, $form, $form_state).

Discussed #88 with @catch. He actually did not intend for that to happen in this issue. Doing that kind of validation is great for DX, but is also kind of new ground. We could do it using assert() but that'd reach fewer developers than the exception I added. So both @catch and I see quite a lot of bikeshed potential that we both agree should not stand in the way of this issue landing. So, deferred #88 to a follow-up: 📌 Detect invalid `copyFormValuesToConfig()` implementations and provide helpful error messages Closed: outdated .

So, to address #89, I'm posting a patch with an interdiff relative to #87.

#89:

1. 👍 Great catch! Added explicit test coverage.
2. 🤔 I specifically did not do that, precisely to keep this first example as simple as possible. 99% of forms edit only a single config, so why make it more complex then? Then again, in mapFormValuesToConfig(), the code is providing that generic use case 🤔 So … let's match that same pattern? 😊
3. 👍 Another good catch — although fortunately the base class did get it right — fixed.
4. 🤩 I like everything in this proposal! Went with option 2 for the reasons you stated, but didn't include $form since it's not needed (and risks introducing side effects — which is less likely in the case of content entity forms since only a few dozen people in the world ever write field widgets). Added @sees in both directions, to help ease the learning curve, and show that that there's a pattern 😊

Addressed everything @effulgentsia raised in #89, but didn't fix the error yet, to prove the added test coverage works.

P.S.: caveat for point 2: looks like phpcs does not correctly handle match inside a switch yet, so I was forced by PHPCS to format it in a really weird way. This is happening despite the latest PHPCS 3.7.2 being installed 🤷‍♂️

+1 to the new naming.

And finally, the fix for #89.1 👍

+++ b/core/lib/Drupal/Core/Form/ConfigFormBase.php
@@ -47,11 +60,157 @@ public function buildForm(array $form, FormStateInterface $form_state) {
+          $transformed_message_parts[] = $this->t('Entry @human-index: @validation-error-message', [

This feels a bit verbose compared to the current message. For example, currently if I had two invalid errors, these would be rendered as "invalid, another invalid are not valid email addresses.", but with this it would be "Entry 2: "invalid" is not a valid email address. Entry 3: "another invalid" is not a valid email address.".

Tagging with usability since there are at least some usability implications from this.

@lauriii That was requested by core committer @effulgentsia in #40 to improve usability 🫣

See the before vs after screenshots in #46 and the issue summary. How can this not be seen as an improvement? Before you'd have to go find the entries by hand, now it says clearly which ones!

The last submitted patch, 91: 3364506-91.patch, failed testing. View results →

That was requested by core committer @effulgentsia in #40 to improve usability

Actually, I didn't intend in #40 to request that we change HEAD's multi-invalid-email message, only that the patch in this issue not limit itself to only including the 1st invalid email address in its error message. Personally, I don't have a strong opinion on how we display multiple invalid email addresses, only that all of the invalid ones get listed.

But, if UX review is needed to change the message string from HEAD's comma separated list to #93's more verbose concatenation of sentences, then in order to unblock this issue, what do you think of something along the lines of this interdiff proposal as a way to retain HEAD's current message format for this form, and opening a follow-up issue to discuss the merits of changing the message?

That works for me!

That means the only known case in HEAD remains completely unchanged, and even allows specific forms to opt in to make messages for sequences more human-friendly! 👏 😊

AFAICT, then there is zero change in terms of UX and we'd be able to remove the tag?

P.S.: it's still amazing to me how you can jump into an issue and articulate a compromise others didn't see that allows us all to keep moving forward together 😊🤝

This looks great 👍 Removing the usability tag since this is no longer introducing end-user facing changes after we implement #97.

Yay!

Done. IS + CR updated.

Comment 💯 — a sign?🤞

Reviewed the interdiff and that's a really good, transparent approach. Lots of revisions here but we ended up with a really nice API in the end, nice when it ends up like that and not back where you started in comment #5 or something.

1. +++ b/core/lib/Drupal/Core/Entity/EntityForm.php
   @@ -323,6 +323,8 @@ public function buildEntity(array $form, FormStateInterface $form_state) {
   +   * @see \Drupal\Core\Form\ConfigFormBase::copyFormValuesToConfig()
   
   +++ b/core/lib/Drupal/Core/Form/ConfigFormBase.php
   @@ -47,11 +60,177 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +  public function validateForm(array &$form, FormStateInterface $form_state) {
   +    if (!isset($this->typedConfigManager)) {
   +      $this->typedConfigManager = \Drupal::service('config.typed');
   +    }
   

   Why is this not added to __construct()?

2. +++ b/core/lib/Drupal/Core/Form/ConfigFormBase.php
   @@ -2,15 +2,28 @@
   + * form-specific validation logic. To do that, override mapFormValuesToConfig().
   

   Nit: out of date reference.

3. +++ b/core/lib/Drupal/Core/Form/ConfigFormBase.php
   @@ -47,11 +60,177 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +      $transformed_message_parts[] = $this->t('Entry @human-index: @validation-error-message', [
   

   Nit: I was surprised to see dashes in the $this->t() placeholder, but looks like we have some pre-existing instances of that. However, it looks like underscores are definitely more commonly used than dashes.

4. +++ b/core/lib/Drupal/Core/Form/ConfigFormBase.php
   @@ -47,11 +60,177 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +    return new TranslatableMarkup(implode("\n", $transformed_message_parts));
   

   Nit: can we use $this->t() like we do few lines earlier?

5. +++ b/core/lib/Drupal/Core/Form/ConfigFormBase.php
   @@ -47,11 +60,177 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +    @trigger_error('Implementing form-coupled validation logic is deprecated in drupal:10.2.0 and will trigger a PHP error from drupal:11.0.0. Implement ::copyFormValuesToConfig() instead, which will become an abstract method on ConfigFormBase. See https://www.drupal.org/node/3373502', E_USER_DEPRECATED);
   

   The CR, the issue and code comments talk about optional config validation. But is it really optional if we are triggering a deprecation warning if someone doesn't implement it? 🤔

Sorry realising this late.

The MR is ignoring the deprecation with a follow-up to convert core then un-ignore it. Generally we've avoided doing that and would do it in reverse - convert core then add the deprecation. So could we do it like that? Would mean a pp-2 follow-up after the conversions adding it back in.

1. 👍 It was necessary due to it living in a trait, but now it was overlooked! Matched the pattern used at 📌 Consider replacing hook_field_type_category_info with YML based plugin discovery Fixed 👍 Created follow-up to update the 16 ConfigFormBase::__construct() overrides: 📌 [PP-2] Follow-up for #3364506: add deprecation once all simple config forms in core implement Postponed .
2. 👍 Fixed.
3. 👍 Changed.
4. 👍 Changed.
5. 👍 Fair point. The nudging/deprecation was added because rather than an opt-in optional trait, around @catch in #69 we switched to doing it entirely in ConfigFormBase. Which has some implications — quoting myself from #72:
   

   The implication of putting this into ConfigFormBase itself is that we want eventually all config forms to adopt this. Which … I think we do anyway?! Who doesn't want override indicators? And to get to the full potential of a more reliable Drupal, all config should be validatable using validation constraints, eventually. So we could … trigger a deprecation notice, to encourage people to adopt this 😊 That'd actually be a major improvement over #63 — at least for me, when looking at this from config schema validation angles 🤓

   Curious what y'all think!

   It seems that most of y'all are on board with that. @catch asked in #86 whether we should set the target not to 11, but 12 — to which I wrote in Slack: "let's do 11 now, we can still choose to bump it? For example, #3295421: Update TranslationWrapper deprecation to removal in 11.0.0 → bumped something else from 10 to 11 as the target.

   I'd be fine with setting it to 12 immediately if that is a big concern. But I personally think it's very feasible to get a solid set of examples in place:

   1. As can be seen in #3324984-33: Create test that reports % of config entity types (and config schema types) that is validatable → , a solid number of config types is validatable now.
   2. … and also visible in the test there: 3 simple configs in core are 100% validatable already: media_library.settings (UI, but a single boolean), menu_ui.settings (no UI) and node.settings (no UI).
   3. As soon as this lands, I'll push 📌 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 forward to help make it simpler for the simplest forms to use this. Media(Library)SettingsForm are the perfect match for this: 1 (resp. 4) values, but no form validation. BookSettingsForm is also a perfect match: 2 values that are edited through the form, one of which has custom validation … which soon we'll be able to replace by a constraint, once 📌 Add new `EntityBundleExists` constraint Fixed lands.
6. 👆 At that point we'd already have 4 of the 29 non-test ConfigFormBase subclasses converted. It'll be easy to do all the rest. That'd provide a body of examples to follow! Issue created: 📌 [PP-1] Update all remaining ConfigFormBase subclasses in Drupal core to use config validation Active .

CR updated 👍

The MR is ignoring the deprecation with a follow-up to convert core then un-ignore it. Generally we've avoided doing that and would do it in reverse - convert core then add the deprecation. So could we do it like that? Would mean a pp-2 follow-up after the conversions adding it back in.

That's not possible. This issue must land first, because otherwise we cannot update any of the existing forms to actually use the new infrastructure 😅

The only alternative is to not start triggering deprecations just yet. But that'd just be splitting things up into extra work for no reason — literally the only downside here is a single line being added to core/.deprecation-ignore.txt 😅

(Although in this very patch iteration I'm introducing a new deprecation per @lauriii's request, and for that one, we could definitely do it here. That'd mean I'd close 📌 [PP-2] Follow-up for #3364506: add deprecation once all simple config forms in core implement Postponed .)

+++ b/core/.deprecation-ignore.txt
@@ -18,7 +18,8 @@
-%Implementing form-coupled validation logic is deprecated.*copyFormValuesToConfig\(\) instead,%
+%Drupal\\Core\\Form\\FormBuilder::submitForm\(\).* will require a new "mixed \.\.\. \$args" argument in the next major version of its interface%
+%Calling ConfigFormBase::__construct\(\) without the \$typedConfigManager argument is deprecated in drupal:10.2.0 and will be required in drupal:11.0.0%

Oops!

Talked to @catch meanwhile:

catch: I am out walking the dog but I would say it's not for no reason because if we don't convert all of core we can't remove in 11.x - it stops a REQUEST_TIME situation.
catch: Ideally we'd never add anything in there except for non-Drupal code.
wimleers: But how can we update all existing config forms if the infra does not exist yet? :sweat_smile:
catch: I mea just not firing the deprecation
wimleers: How about:
wimleers: not triggering deprecation → remove that
wimleers: add it later, when core is done
wimleers: ah! hah!
wimleers: +1?
catch: Yes that.

Doing that 👍

Did #105. Rescoped/repurposed 📌 [PP-2] Follow-up for #3364506: add deprecation once all simple config forms in core implement Postponed .

Interdiff:

1. moves the deprecation addition that asks maintainers to adopt config validation to #3384782-2: [PP-3] Follow-up for #3364506: add deprecation once all simple config forms in core implement →
2. updates all ConfigFormBase subclasses in core that override __construct() to pass in the new parameter
3. and thanks to those two, removes all additions to core/.deprecation-ignore.txt

The last submitted patch, 106: 3364506-106.patch, failed testing. View results →

Missed 3 cases. Fixed.

Looks good! Thanks for addressing the feedback @Wim Leers!

Ticking credit checkboxes for reviewers.

Pushed to 11.x and published the CR!

🥳🥳🥳

Can’t wait to continue the follow-ups on Monday, happy weekend y’all!

As promised:

1. #3382510-10: OneToOneConfigFormTrait: make it super simple to use validation constraints on simple config forms, adopt in Media(Library)SettingsForm + BookSettingsForm →
2. #3384123-4: Detect invalid `copyFormValuesToConfig()` implementations and provide helpful error messages →

… and updated the issues blocked on those to indicate they're less blocked now.

Also rebased #2408549 on top of this, see #2408549-179: There is no indication on configuration forms if there are overridden values → 🤓

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