Problem/Motivation

See plan in #3364506-103: Add optional validation constraint support to ConfigFormBase.

Blocked on #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. This is unblocked thanks to #config_target having landed and been matured, see the change record.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3384790

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

Wim Leers created an issue. See original summary.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: [PP-2] Update all remaining ConfigFormBase subclasses in Drupal core to use config validation » [PP-1] Update all remaining ConfigFormBase subclasses in Drupal core to use config validation

#3364506: Add optional validation constraint support to ConfigFormBase landed, so less blocked 👍

This is not hard-blocked on #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, but the infrastructure being proposed there would make this issue simpler.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: Unassigned » phenaproxima
Status: Postponed » Active
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: +blocker
Related issues: +#3384782: [PP-1] Follow-up for #3364506: add deprecation once all simple config forms in core implement

phenaproxima made their first commit to this issue’s fork.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Active » Postponed
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Postponed » Needs work

#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 was committed to 11.x, so we can proceed here.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Title: [PP-1] Update all remaining ConfigFormBase subclasses in Drupal core to use config validation » Update all remaining ConfigFormBase subclasses in Drupal core to use config validation
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented

This issue has a pretty clear goal: every form in core that extends ConfigFormBase should either have its submitForm() method removed, or minimized.

phenaproxima opened merge request !5223

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: Update all remaining ConfigFormBase subclasses in Drupal core to use config validation » [PP-1] Update all remaining ConfigFormBase subclasses in Drupal core to use config validation

Apparently this is hard-blocked on #3398982 per @phenaproxima in #3398982-10: ConfigFormBase + validation constraints: support non-1:1 form element-to-config property mapping again.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Postponed
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: [PP-1] Update all remaining ConfigFormBase subclasses in Drupal core to use config validation » Update all remaining ConfigFormBase subclasses in Drupal core to use config validation
Assigned: phenaproxima » Unassigned
Status: Postponed » Needs work

#3398982: ConfigFormBase + validation constraints: support non-1:1 form element-to-config property mapping again is in — this can now continue! 😊

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: Update all remaining ConfigFormBase subclasses in Drupal core to use config validation » Update all remaining ConfigFormBase subclasses in Drupal core to use #config_target
Issue summary: View changes

This is now more ready than ever before to be pushed forward 🤓

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Priority: Normal » Major

There's one last property path left in \Drupal\system\Form\SiteInformationForm::submitForm() 🤓

Also still left:

  • \Drupal\language\Form\NegotiationBrowserForm::submitForm()
  • \Drupal\language\Form\NegotiationConfigureForm::submitForm()
  • \Drupal\language\Form\NegotiationUrlForm::submitForm()
  • \Drupal\locale\Form\LocaleSettingsForm::submitForm()
  • …
  • \Drupal\system\Form\ThemeSettingsForm::submitForm()
  • \Drupal\update\UpdateSettingsForm::submitForm()
  • \Drupal\views_ui\Form\AdvancedSettingsForm::submitForm()
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented

Re #15: most of those forms have very complicated validation and submit logic, and I'm not sure how extensively they're tested, or even if they're tested. I'm therefore very hesitant to even attempt all of those complex conversions in this one issue, lest I break stuff.

\Drupal\language\Form\NegotiationBrowserForm::submitForm()
\Drupal\language\Form\NegotiationConfigureForm::submitForm()

Both of these are complex, and should have their own issues.

\Drupal\language\Form\NegotiationUrlForm::submitForm()
The submitForm() looks simple, but it's actually saving a tree of elements. There's also a lot going on in validateForm(). This deserves its own issue.

\Drupal\views_ui\Form\AdvancedSettingsForm::submitForm()
We can do this one now. It's not too hard.

\Drupal\locale\Form\LocaleSettingsForm::submitForm()
\Drupal\update\UpdateSettingsForm::submitForm()

These need to remain as they are. Neither is doing any config mappings, but they are doing legitimate cache clears as needed. That's a proper use of a submitForm() override.

\Drupal\system\Form\ThemeSettingsForm::submitForm()
This should be done in its own issue because it's very complicated due to the dynamism of theme settings.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented

Does #16 mean we make this a plan and create new child issues for the more complicated ones?

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented 
1) Drupal\Tests\system\Functional\Form\SiteInformationFormTest::testValidation
Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for system.mail with the following errors: system.mail:mailer_dsn missing schema, 0 [mailer_dsn] 'mailer_dsn' is not a supported key.

That key does exist. I wonder what's going on here. 🤔

#17: There's a lot of work/progress in this issue already. But I think you're right: it'd be clearer to turn this into a meta/plan issue and then create a child issue per … module? For the system module we'd be able to do everything except ThemeSettingsForm, because of #3416178: Add validation constraints to `type: theme_settings` — there's a whole dragon nest there 🐉😅

What do you think, @phenaproxima?

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented

Scoping is always difficult, but I'd suggest we create one issue for the easier ones (the issue we currently have basically), and create followups per form?

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

That works for me too. We'll let @phenaproxima choose which of those directions he prefers 🤓

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented

@phenaproxima, what do you think is the best option here? Do we want to turn this in a meta or do we want to create followups?

samit.310@gmail.com made their first commit to this issue’s fork.

samitk’s picture

samitk commented

I have rebased it with latest 11.x and resolved conflicts from following files.

  • core/modules/statistics/src/StatisticsSettingsForm.php
  • core/modules/system/src/Form/SiteInformationForm.php

Also reverted the code of core/modules/language/src/Form/NegotiationSessionForm.php file back to 11.x, because getting following error due to failing test cases.

1)
Drupal\Tests\language\Functional\LanguageUILanguageNegotiationTest::testUILanguageNegotiation
Behat\Mink\Exception\ResponseTextException: The text "In zh-hans In zh-hans
In zh-hans" was not found anywhere in the text of the current page.
/builds/issue/drupal-3384790/vendor/behat/mink/src/WebAssert.php:907
/builds/issue/drupal-3384790/vendor/behat/mink/src/WebAssert.php:293
/builds/issue/drupal-3384790/core/tests/Drupal/Tests/WebAssert.php:979
/builds/issue/drupal-3384790/core/modules/language/tests/src/Functional/LanguageUILanguageNegotiationTest.php:433
/builds/issue/drupal-3384790/core/modules/language/tests/src/Functional/LanguageUILanguageNegotiationTest.php:413
FAILURES!
Tests: 5, Assertions: 115, Failures: 1.

Here is the reference link https://git.drupalcode.org/issue/drupal-3384790/-/jobs/2532792

Thanks
Samit K.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.