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

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:

Comments

alexpott created an issue. See original summary.

alexpott’s picture

Status: Active » Needs review
alexpott’s picture

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

wim leers’s picture

WOW! 🤩 That looks splendid!

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.

+1

And that also opens the door to eventually deprecating and removign getEditableConfigNames() I think? 🤔

alexpott’s picture

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

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Change makes sense to me, with the empty array return at least.

Tests are all green so nothing noticeably broke.

alexpott’s picture

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

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

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

borisson_’s picture

Status: Needs review » Reviewed & tested by the community

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.

wim leers’s picture

VERY nice! 🤩

This makes ConfigFormBase more capable, with a better DX, with less code! 👏

  • catch committed 14bf457f on 10.2.x
    Issue #3398891 by alexpott, Wim Leers: Do not require the config in #...

  • catch committed c1a42fda on 11.x
    Issue #3398891 by alexpott, Wim Leers: Do not require the config in #...
catch’s picture

Version: 11.x-dev » 10.2.x-dev
Status: Reviewed & tested by the community » Fixed

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

alexpott’s picture

Updated the CR https://www.drupal.org/node/3373502 with this.

wim leers’s picture

Alright, we're almost done here!

For the final piece to make #config_target a delight to use and usable in also the more refined ConfigFormBase forms … see #3398982: ConfigFormBase + validation constraints: support non-1:1 form element-to-config property mapping again 😁

That is ready for final review too now! 🥳

Status: Fixed » Closed (fixed)

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