Problem/Motivation
We've added the ability to link a config object to a form element using #config_target in #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. We still require the config to be in \Drupal\Core\Form\ConfigFormBaseTrait::getEditableConfigNames() but this is no longer necessary. The reason this existed was to ensure the $this->config() returns non-overridden config to ensure that overrides in settings.php to not get saved to config. We can get the non-overriden config in \Drupal\Core\Form\ConfigFormBase::loadDefaultValuesFromConfig().
The advantages of this is that:
- You don't have to repeat yourself in - of course #config_target should get non-overridden config and \Drupal\Core\Form\ConfigFormBase::copyFormValuesToConfig() should use editable config
- Form alters like dblog_form_system_logging_settings_alter() will not need an extra submit
Proposed resolution
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
Issue fork drupal-3398891
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:
Comments
Comment #3
alexpottComment #4
alexpottSo we need to decide if we should implement getEditableConfigNames ins ConfigFormBase...
I think we could and then somewhere we could check if there's no map and getEditableConfigNames() returns an empty array we could throw an exception.
Comment #5
wim leersWOW! 🤩 That looks splendid!
+1
And that also opens the door to eventually deprecating and removign
getEditableConfigNames()I think? 🤔Comment #6
alexpottre #5 - Yep eventually could do that. There will be some fun because \Drupal\Core\Form\ConfigFormBaseTrait() is used elsewhere but maybe that can continue and we'll just remove it from ConfigFormBase.
Comment #7
smustgrave commentedChange makes sense to me, with the empty array return at least.
Tests are all green so nothing noticeably broke.
Comment #8
alexpottOpened #3400033: Deprecate \Drupal\Core\Form\ConfigFormBase::getEditableConfigNames() - use #config_target instead to work out how to deprecate getConfigNames() in the ConfigFormBase context.
I thought about implementing ConfigFormBase::getEditableNames() and returning an empty array here and then adding another after build method to check whether there is a map and if there is no map and the return value of ConfigFormBase::getEditableNames() is empty trigger a warning or something. The problem with this is that if you do that you will no longer be told to implement
::getEditableNames()- so I think we should only do this when we're ready to do #3400033: Deprecate \Drupal\Core\Form\ConfigFormBase::getEditableConfigNames() - use #config_target instead and completely deprecate it.Comment #9
alexpottI realised that the exception being thrown by ::copyFormValuesToConfig() is pointless now that we're calling the method based on what is in the map. So we can remove some dead code.
Comment #10
borisson_More deleted code is more better, I'm super interested to see how we pick up #3400033: Deprecate \Drupal\Core\Form\ConfigFormBase::getEditableConfigNames() - use #config_target instead in the future, but this is already a big win for altering exisiting config forms.
Comment #11
wim leersVERY nice! 🤩
This makes
ConfigFormBasemore capable, with a better DX, with less code! 👏Comment #14
catchThis is very nice and I never thought we'd be able to remove those custom submits in form alters of config forms or might even make them harder.
Suggested a trait for the empty array implementation of getEditableConfigNames() not because there's any code re-use, but because it'll save copying the same comment around dozens of times and be easier to remove when the time comes.
Committed/pushed to 11.x and cherry-picked to 10.2.x, thanks!
Comment #15
alexpottUpdated the CR https://www.drupal.org/node/3373502 with this.
Comment #16
wim leersAlright, we're almost done here!
For the final piece to make
#config_targeta delight to use and usable in also the more refinedConfigFormBaseforms … see #3398982: ConfigFormBase + validation constraints: support non-1:1 form element-to-config property mapping again 😁That is ready for final review too now! 🥳