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.

#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:

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.

@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 #3361534: KernelTestBase::$strictConfigSchema = TRUE and BrowserTestBase::$strictConfigSchema = TRUE do not actually strictly validate.

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

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!

• I should've removed Needs tests 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 a fresh composer install on PHP 8.1. And neither can testbot on PHP 8.2. Any chance this was PEBKAC? 😅
• Did some clean-up.

+++ b/core/modules/update/update.install
@@ -76,6 +77,13 @@ function update_requirements($phase) {
+  // Populate the fetch URL (if not set).
+  $update_settings = \Drupal::configFactory()->getEditable('update.settings');
+  if (!$update_settings->get('fetch.url')) {
+    $update_settings->set('fetch.url', UpdateFetcher::UPDATE_DEFAULT_URL);
+  }
+  $update_settings->save(TRUE);

FYI this is identical to the pattern in system_install().

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!

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 #2408549: Display status message on configuration forms when there are overridden values because it's trying to implement different approach for mapping form elements to config.

This would indeed enormously simplify that.

Hah! #3361534: KernelTestBase::$strictConfigSchema = TRUE and BrowserTestBase::$strictConfigSchema = TRUE do not actually strictly validate 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.

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

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!

   Before

   

   Before — with Inline Form Errors installed

   

   After

   

   After — with Inline Form Errors installed

   

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!

See the attached interdiff :)

Done.

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 #3361534: KernelTestBase::$strictConfigSchema = TRUE and BrowserTestBase::$strictConfigSchema = TRUE do not actually strictly validate and comes with an update path 😇

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 😅

#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! 👍

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!

@lauriii in #69:

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

See #2408549-178: Display status message on configuration forms when 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:

3. 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 #2408549: Display status message on configuration forms when there are overridden values.

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

Fixed #72.

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

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 😊

#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 🙏

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: #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.

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 #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 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).

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: #3384123: Detect invalid `copyFormValuesToConfig()` implementations and provide helpful error messages.

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 🤷‍♂️

And finally, the fix for #89.1 👍

@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!

It is indeed slightly more verbose. But … not by much? And with a trade-off that clearly has some benefits too.

P.S.: Note that this affects only this form, which previously had this logic:

      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)]));
      }

P.P.S.: Most config forms either have no validation or barely any. This hence is very unlikely to result in a usability regression. I just checked and I found 8 overrides of public function validateForm in the 28 non-test ConfigFormBase subclasses 🫣— slightly more than 25%. If anything … it's going to make it more likely that config forms will have validation at all, so that config forms are not "saving successfully" but then mysteriously the config seems to have not any effect: a far bigger boost in usability than the slightly more verbose messages would decrease usability. (I've lost count of how many times I've had to debug modules to figure out why mysteriously 🧙‍♀️ the config wasn't working after using config forms, only due to missing validation …)

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 Usability 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 😊🤝

Yay!

Done. IS + CR updated.

Comment 💯 — a sign?🤞

1. 👍 It was necessary due to it living in a trait, but now it was overlooked! Matched the pattern used at #3372097: Consider replacing hook_field_type_category_info with YML based plugin discovery 👍 Created follow-up to update the 16 ConfigFormBase::__construct() overrides: #3384782: [PP-1] Follow-up for #3364506: add deprecation once all simple config forms in core implement.
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 #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 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 #3382581: Add new `EntityBundleExists` constraint 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: #3384790: Update all remaining ConfigFormBase subclasses in Drupal core to use #config_target.

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 #3384782: [PP-1] Follow-up for #3364506: add deprecation once all simple config forms in core implement.)

+++ 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 #3384782: [PP-1] Follow-up for #3364506: add deprecation once all simple config forms in core implement.

Interdiff:

1. moves the deprecation addition that asks maintainers to adopt config validation to #3384782-2: [PP-1] 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

Missed 3 cases. Fixed.

🥳🥳🥳

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

As promised:

1. #3382510-10: 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
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: Display status message on configuration forms when there are overridden values 🤓