Make getEditableConfigNames() public and use it in config_translation

Updated the issue summary with specific steps to do.

Here's the patch that we (smithworx, thomas4019) made for this.

Two things I noticed.

1. You forgot to include ConfigFormInterface in the patch :-)
2. Configurable plugins. Let me explain:
* I checked \Drupal\aggregator\Form\SettingsForm whether it in fact only references the config file given in the patch.
* I noticed then that it uses \Drupal\Core\Plugin\PluginFormInterface to include configuration of plugins in its forms.
* These plugin forms can (should) also store their settings in configuration.
* So we need to find the configuration names of the plugin forms that are included.
* So we should add a getConfigNames() method to \Drupal\Core\Plugin\PluginFormInterface and have \Drupal\aggregator\Form\SettingsForm call that in its own getConfigNames(). Note: I don't think it makes sense to add a separate PluginConfigFormInterface because the methods on PluginFormInterface are already called buildConfigurationForm(), etc.

I've added ConfigFormInterface which was missing for the first patch. I added getConfigNames() to PluginFormInterface. I also added stub implementations to the three base classes, BlockBase, AggregatorPluginSettingsBase, ConfigurableActionBase. Now what needs to be done is that the subclasses of these base classes need to provide real implementations where possible.

Updated issue summary with more specific instructions

updated remaining tasks

Let's see if this passes.

I've corrected some typo's. Cleaned up the code, renamed a method and added some comments to make the code better readable.

Have not been able to figure out how to find the "config names" used by AggregatorPluginSettingsBase().

Here's a preliminary review. Still only aggregator module :-)

1. +++ b/core/modules/aggregator/lib/Drupal/aggregator/Form/SettingsForm.php
   @@ -100,6 +100,41 @@ public function getFormID() {
      /**
   +   * Collect configurable instances of this form's plugins.
   +   */
   +  protected function collectConfigurableInstances() {
   ...
   +  }
   

   I think the code flow would be more natural if the function was called getConfigurableInstances() and it would return the instances directly. I could utilize $this->configurableInstances internally as a cache, but that would be contained to this function.

2. +++ b/core/modules/aggregator/lib/Drupal/aggregator/Form/SettingsForm.php
   @@ -154,35 +189,11 @@ public function buildForm(array $form, array &$form_state) {
   -    // Implementing processor plugins will expect an array at $form['processors'].
   -    $form['processors'] = array();
   ...
   +      // @todo This may fail if $form['processors'] is missing for DefaultProcessor::buildConfigurationForm().
   

   Can you explain why we don't initialize $form['processors'] like before?

3. +++ b/core/modules/aggregator/lib/Drupal/aggregator/Form/SettingsForm.php
   @@ -223,4 +234,19 @@ public function submitForm(array &$form, array &$form_state) {
   +    foreach ($this->configurableInstances as $instance) {
   +      $configNames[] = $instance->getConfigNames();
   +    }
   ...
   +    return $configNames;
   

   We should do an array_unique() on this.

I will now go through the actual aggregator plugins.

The following config names are used by the aggregator plugins. I did not yet re-roll the patch, as I'm not sure who's still working on this.

The parsers and fetchers in core don't actually reference any configuration.

Processors:
These have a getConfiguration() from ConfigurablePluginInterface where the used config is referenced.
- DefaultProcessor: array('aggregator.settings');
- TestProcessor: array('aggregator_test.settings');

1. +++ b/core/modules/language/lib/Drupal/language/Form/NegotiationSelectedForm.php
   @@ -48,4 +48,11 @@ public function submitForm(array &$form, array &$form_state) {
   +    return array('language.negotiation');
   

   This is correct. While reviewing I found #2096699: Missing config schema for language.negotiation.selected_langcode, however.

2. +++ b/core/modules/language/lib/Drupal/language/Form/NegotiationSessionForm.php
   @@ -49,4 +49,11 @@ public function submitForm(array &$form, array &$form_state) {
   +    return array('language.settings');
   

   This should be array('language.negotiation').

   Also see #2096697: NegotiationSessionForm saves to wrong config object

3. +++ b/core/modules/search/lib/Drupal/search/Form/SearchSettingsForm.php
   @@ -265,4 +265,11 @@ public function searchAdminReindexSubmit(array $form, array &$form_state) {
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getConfigNames() {
   +    return array('search.settings');
   +  }
   

   Search settings currently include configuration forms as well, but they don't use PluginFormInterface. Opened #2096701: Search plugins should use PluginFormInterface

4. +++ b/core/modules/system/lib/Drupal/system/Form/ImageToolkitForm.php
   @@ -109,4 +109,11 @@ public function submitForm(array &$form, array &$form_state) {
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getConfigNames() {
   +    return array('system.image');
   +  }
   

   Image toolkits also embed forms of their plugins. I opened #2096703: Image toolkits should use PluginFormInterface and ContainerFactoryPluginInterface

5. +++ b/core/modules/system/lib/Drupal/system/Form/ThemeSettingsForm.php
   @@ -454,4 +454,18 @@ protected function validatePath($path) {
   +  public function getConfigNames() {
   +    $configNames = array('system.theme.global');
   +
   +    $themes = list_themes();
   +    foreach ($themes as $theme_name => $theme) {
   +      $configNames[] = $theme_name . '.settings';
   +    }
   +
   +    return $configNames;
   +  }
   

   Nice!

6. +++ b/core/modules/user/lib/Drupal/user/AccountSettingsForm.php
   @@ -445,4 +445,11 @@ public function submitForm(array &$form, array &$form_state) {
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getConfigNames() {
   +    return array('user.settings','user.mail','system.site');
   +  }
   

   Nice catch on the system.site! We are currently missing that in config_translation. The names should be separated by spaces though.

So needs work for:
- 10.1 and 10.2 in case you guys agree
- 10.3 definitely
- 11
- 12.2 and 12.6
If noone beats me to this I will re-roll this later today, but feel free!

Actually, I'll fix this now.

Here we go.

Also, this doesn't need tests.

The !isset() will never return TRUE if $this->configurableInstances defaults to an empty array. At least the unit test (++ BTW) passes locally, I hope the rest tags along.

Thanks for your work on this @tstoeckler. Here is a minor review comment. We need to test this manually to make sure we covered all.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Form/SettingsForm.php
@@ -100,6 +100,45 @@ public function getFormID() {
+      $config = $this->configFactory->get('aggregator.settings');

Can we initiate the configurableInstances here, so that a) we don't call when no instances b) no warning on foreach

I just realized that we didn't tag this with "API change". This was part of the "hard conversion" in Prague on config_translation, where @effulgentsia, @GáborHojtsy, @alexpott, @YesCT, @xjm, @webflo, @vijaycs85 and I decided on this direction. As far as I'm aware one comitter to agree on an issue is enough, so I'm tagging this as "Approved API change".

To be clear: This change is not backwards-compatible. It will break every module that provides a config form. I still think it should go in as is. However, we *could* make this change backwards-compatible by adding

public function getConfigNames() {
  return array();
}

to ConfigFormBase. That would, however, lead to lots of contrib not implementing this method, IMO, because the form would still work without it.

Re-adding tag, that got lost in the cross-post.

Will now do the manual testing.

Here's an updated patch for #17.

Here we go.

I noticed that despite me having reviewed it several times aggregator's SettingsForm was building the array in getConfigNames() incorrectly. It was correct in the original patch, but was broken in the meantime. I also added tests for that.

Also: The patch in #19 is completely borked because my D8 was too old. Sorry for that. I've attached an interdiff from #16 instead of from #19.

That was actually a random fail.

But I found out that array_merge() does in fact not work in the way I expected. I added additional tests that would make #21 fail. Kudos to @thomas4019 @smithworx for getting it right, right from the start!

I think this should really be ready to go.

Just marking this "Needs review" again, because it *really* needs reviews :-)

Oops, sorry wrong issue. This shouldn't fail :-/

So the PhpUnit test was easy to test. The Blocks one looked legit as well, but I'm pretty sure that it's unrelated / random now. Let's see.

Great, anyone care for an RTBC? I think I've contributed enough to not be able to RTBC myself, but I can vouch that all the actual implementations are correct.

Discussed more in IRC, hopefully we can remove unnecessary propagation of this interface.

So to paraphrase the discussion in IRC (please do correct me Tim!):

Tim suggested that the getConfigNames() method should be on its own interface or in other words ConfigFormInterface should not extend FormInterface (and potentially be renamed as part of that). The fact one extends the other is unnecessary tight coupling for no benefit, from his point of view.

In my opinion the getConfigNames() is inherently bound to the concept of (config) forms so to have an interface with just that method on it, feels unnatural to me.

It would be great if we could have some more opinions on this. I will roll a patch either way, though, I don't care *that* much :-)

Oh, also I tested this manually. I added fake translatable schemas for aggregator settings and an aggregator plugin and they both showed up.

I can't I'm not sure I care *that* much either... but we don't have a use-case yet for GetConfigNameInterface at the moment and if we do we can just refactor, no?

Personally I can't see how this is going to be used outside the current use-case.

So someone care to RTBC this? It's one of the main blocker for config_translation at the moment.

--- a/core/lib/Drupal/Core/Action/ConfigurableActionBase.php
+++ b/core/lib/Drupal/Core/Action/ConfigurableActionBase.php

+++ b/core/lib/Drupal/Core/Action/ConfigurableActionBase.php
@@ -52,4 +52,10 @@ public function setConfiguration(array $configuration) {

@@ -52,4 +52,10 @@ public function setConfiguration(array $configuration) {
   public function validateConfigurationForm(array &$form, array &$form_state) {
   }
 
+  /**
+   * {@inheritdoc}
+   */
+  public function getConfigNames() {
+    return array();
+  }
 }

This is still completely wrong. No action ever writes to CMI. I'm not even scrolling past here at this point

We talked about this in Prague, there was to be a new patch I thought?

OK, here we go. There were two issues that we conflated in our discussion. At least they were conflated in my head, which is why I incorrectly thought the previous patch was good and agreed upon.
1. Whether ConfigFormInterface should or should not extend FormInterface. I still feel rather strongly that it should. @alexpott seemed to agree, and said that if we ever run into a use-case of getConfigNames() being useful on its own, we can refactor then. @tim.plunkett disagreed above.
2. Whether getConfigNames() should be on PluginFormInterface, i.e. whether all plugin configuration forms should be tied to separate configuration objects per the interface. This is what the previous patches did, but that is conceptually very wrong. Thanks @tim.plunkett for persisting on that point. Only when I read #34 after coming back to this now, I get your point now. There are many *plugin* configuration forms that do not save to their own configuration files as the settings are saved as part of a configuration entity; for example blocks or actions. This is fixed in this patch by providing a separate PluginConfigFormInterface (which in turn *extends* PluginFormInterface per 1.) which plugins that *do* save to a separate settings form can implement. Examples of the latter are aggregator and search plugins, currently. This patch therefore removes the pointless getConfigNames() from ConfigurableActionBase and BlockBase.

Speaking of search plugins, this needed some updates for #2086201: Use PluginFormInterface instead of one-off form methods for search plugins

+++ b/core/modules/search/lib/Drupal/search/Form/SearchSettingsForm.php
@@ -280,7 +281,16 @@ public function searchAdminReindexSubmit(array $form, array &$form_state) {
+    // Handle per-plugin submission logic.

Oh damn, wrong copy-paste.

Updated issue summary.

This fixes the tests, and the comment in #36 and adds tests for the search settings form logic that was added in the previous patch. I think there's not much left to do.

Updated issue summary.

35.2 is spot on, I'm glad we're all on the same page.

I never actually disagreed about 35.1, I was just trying to convince you of 35.2 :) And with the PluginConfigFormInterface, I think we're in good shape.

There was only one oddity in the patch.

AggregatorPluginSettingsBase needs to implement PluginConfigFormInterface, and I would prefer if it doesn't provide an empty implementation of getConfigNames().

That totally makes sense, nice suggestion! Here you go.

Looks good, it should pass.
Thanks @tstoeckler!

#40: 2095289-config-form-get-names-40.patch queued for re-testing.

fix markup

Back to RTBC per #42. Also added a proposed commit message to the issue summary, as @thomas4019 and @smithworx made the original patch by pair programming in Prague. The proposed commit message is:

Issue #2095289 by thomas4019, smithworx, tstoeckler, Sutharsan | YesCT, tim.plunkett: Add a getConfigNames method to ConfigFormBase to return the cmi names for a form.

Added proposed commit message

Invoking all these may not be feasible as per #2098197: Add getAllRoutes() method to RouteProvider , so we may need to postpone this until that one is figured out :/

No, we need this in any case. We might find a way to not invoke them all at once, but we still need the information which configuration forms relate to which config names. There's no way around that.

My point of view is if we end up never invoking these methods because we figure out it is way overkill, then implementing this for config forms would only be pain for no gain, so not sure this would be valuable to push without agreement that invoking them is feasible.

OK, let's keep it in postponed for now, until we have a solution in #2095291: Remove *.config_translation.yml for config forms, use form getConfigNames() that perhaps doesn't require #2098197: Add getAllRoutes() method to RouteProvider . I still think there's no way around this, though. As I've just re-iterated over there, we would *like* to do a static method or annotation instead of an instance method, but as can be seen in the patch, that's just not possible. And getting this in would make testing of #2095291: Remove *.config_translation.yml for config forms, use form getConfigNames() easier - although it's certainly possible now as well.

Re-titling and bumping.
I'm pretty far with the new approach, of which I gave a quick rundown in #2095291: Remove *.config_translation.yml for config forms, use form getConfigNames() , and there's no way we're getting around getConfigNames(). So I think the earlier we get this in, the better. I'm not going to mark it RTBC again myself, but I really think that's what the status should be.

Ok, better leave this for core committers to decide then :)

Just to note: This issue and in particular #2095291: Remove *.config_translation.yml for config forms, use form getConfigNames() was discussed via e-mail recently where @alexpott suggested committing config_translation.module without #2095291 and then committing this issue together with #2095291 afterwards so that we have a use-case in core directly. I would be totally fine with that, of course.

I agree with postponing this until we can do it with a use case, per #52. Here's some examples why:

1. +++ b/core/modules/system/lib/Drupal/system/Form/ThemeSettingsForm.php
   @@ -454,4 +454,18 @@ protected function validatePath($path) {
   +  public function getConfigNames() {
   +    $configNames = array('system.theme.global');
   +
   +    $themes = list_themes();
   +    foreach ($themes as $theme_name => $theme) {
   +      $configNames[] = $theme_name . '.settings';
   +    }
   +
   +    return $configNames;
   +  }
   

   This is a case where the same form class is used on different routes, and which config file is used depends on the route. Returning all the possible config files isn't necessarily what makes the most sense here. I'm wondering if we need to support this pattern of same form class reused on different routes with different config files, or if that's what config entities are for, and if we should therefore convert theme settings to that.

2. +++ b/core/modules/user/lib/Drupal/user/AccountSettingsForm.php
   @@ -445,4 +445,11 @@ public function submitForm(array &$form, array &$form_state) {
   +  public function getConfigNames() {
   +    return array('user.settings', 'user.mail', 'system.site');
   +  }
   

   This is a case where getConfigNames() might be insufficiently granular for solving the config translation use case. There's already a different form that deals with (most of) system.site.yml. The AccountSettingsForm only manages one key within system.site.yml. We probably don't want the translation of AccountSettingsForm to let you translate all of system.site.yml, but the API of getConfigNames() doesn't allow for expressing that information.

Also, how will hook_form_alter work here?

@tim.plunkett: in config_translation's current model, we have a config_translation_info() hook that augments the .yml based discovery and then a config_translation_info_alter() which alters the information gathered with .yml discovery, the info hook (used in our case to add dynamic elements, more precisely all config entities). That info alter is targeted to allow for form alter changes to remove / add config keys as needed. As discussed in Prague, there is no good way at all to selectively remove parts of the config translation form outside of actually altering the config translation form (which we did structure in an alter friendly way by the way :)

Not sure at all how the getConfigNames() system would do this, but it would end up somehow with config_translation and that can still add up its own layer of altering possibilities. Sounds to me like that would keep the definition and altering pretty far apart, but it may work just as well.

I've kept that discussion out of this issue so far, to avoid scope-creep, but I think we should add hook_config_names_alter() or whatever to complement hook_form_alter(). Ideally, once would have the form object at hand when altering the form, so we could just add a addConfigName() function, but that's not the case. So to cleanly support contrib altering forms and adding config elements, we need to allow the list of config names to be altered.

This is a case where getConfigNames() might be insufficiently granular for solving the config translation use case. There's already a different form that deals with (most of) system.site.yml. The AccountSettingsForm only manages one key within system.site.yml. We probably don't want the translation of AccountSettingsForm to let you translate all of system.site.yml, but the API of getConfigNames() doesn't allow for expressing that information.

You're totally correct that this is not ideal, but I don't see any to solve that. I've thought about this quite a bit, and I think the only way to support this would be to autogenerate config forms based on config schema. We could then replicate that building for the translation form.

Well, question is how often will random config pages include pieces from other random config files. If we can kill these cases one by one by storing those settings granularly or at the "right place" from the UI POV that could solve this. Of course, tying how the data is stored to how we generate forms out of them is a bit too much coupling, but that is the same we do for content entities vs. fields (especially with per-entity fields now instead of sharing fields across entities). So doing similar separation in config land does not sound unnatural.

I've kept that discussion out of this issue so far, to avoid scope-creep, but I think we should add hook_config_names_alter() or whatever

Since the alter hook would be invoked from the outside (not from within the getConfigNames() implementations), I agree with keeping it out of this patch until the combining from #52 happens. As I mentioned in Prague, I think eventually the config names should become an annotation on the form, in which case the alter would be done via our normal discovery patterns, but that obviously can't happen until if/when we decide to add annotations to forms at all, which is definitely out of this issue's scope.

Well, question is how often will random config pages include pieces from other random config files. If we can kill these cases one by one by storing those settings granularly or at the "right place" from the UI POV that could solve this.

Seems like the only core case is AccountSettingsForm storing a key in system.site.yml. Seems reasonable to me to move that key to user.settings.yml or user.mail.yml. Is there any reason not to?

So doing similar separation in config land does not sound unnatural.

Seems like we can at least do so in core. If contrib wants to go crazy and mix and match, then I suppose it could be that module's responsibility to also implement hook_config_translation_info_alter() to then tweak what gets included on the translate form accordingly.

This is fixed in this patch by providing a separate PluginConfigFormInterface (which in turn *extends* PluginFormInterface per 1.) which plugins that *do* save to a separate settings form can implement. Examples of the latter are aggregator and search plugins, currently.

Is there any good reason for aggregator and search plugins to save their configurations that way, vs. using PluginBag or their own config entities? Do we need new issues to "fix" those plugins?

Since the alter hook would be invoked from the outside

Well it could be invoked by e.g. ConfigFormBase::getConfigNames(). But I still think it should be left out for now.

Is there any good reason for aggregator and search plugins to save their configurations that way, vs. using PluginBag or their own config entities? Do we need new issues to "fix" those plugins?

+1000. Yes, we should totally do that.

I agree that separating/moving the config files is a good idea.

Added tim.plunkett to the proposed commit message.

#2392319: Config objects (but not config entities) should by default be immutable is actually doing this under the name getEditableConfigNames().

As per @tstoeckler on #2392319: Config objects (but not config entities) should by default be immutable, there are still some things that would need to be done here, so moving to postponed on that issue instead.

Awesome that that got in. That already got us 70-80% of the way here. So this - unexpectedly - seems in reach now. Will take a stab at this on the weekend. Will have to look into the plugin bag stuff discussed above as that was not covered by #2392319: Config objects (but not config entities) should by default be immutable as far as I know. But the rest of the patch will be mostly

- protected function getEditableConfigNames()
- public function getEditableConfigNames()

Yay!

Here we go. Translation of site information and account settings still worked locally (even though the patch removes the corresponding mapper files). Let's see if anything fails.

This will need dedicated tests, though.

Fix title.

Just want to point out that this is going to be another annoying API change (fatal error on module install) for every module that has settings forms, the Approved API change tag was added in september 2013, not sure if that still applies ;)

This one should be better.

Re #71: It is in only an API change for forms that do not extend ConfigFormBase. I don't think that is very disuptive. Am I missing something?

getEditableConfigNames() is currently protected. Making it public will throw a fatal error on any form that is currently implementing it as a protected method, saying that you can not make a method protected when it is public in the interface.

Ahh yes for modules that have adapted since #2392319: Config objects (but not config entities) should by default be immutable it will break. Otherwise it won't.

Let's see if this is green. I still think this is worthwhile doing. For BC we could also add a new method, which just calls getEditableConfigNames() internally, but I'm not too fond of that...

1. +++ b/core/lib/Drupal/Core/Form/ConfigFormBase.php
   @@ -8,13 +8,14 @@
   +abstract class ConfigFormBase extends FormBase implements FormInterface, ConfigFormInterface {
   

   implements FormInterface is redundant

2. +++ b/core/modules/aggregator/tests/src/Unit/Form/SettingsFormTest.php
   @@ -0,0 +1,128 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public static function getInfo() {
   +    return array(
   +      'name' => 'Aggregator settings form tests',
   +      'description' => 'Unit tests for the aggregator settings form',
   +      'group' => 'Aggregator',
   +    );
   +  }
   

   obsolete

3. +++ b/core/modules/search/src/SearchPageListBuilder.php
   @@ -22,7 +23,7 @@
   +class SearchPageListBuilder extends DraggableListBuilder implements FormInterface, ConfigFormInterface {
   

   I think either ConfigFormInterface should extend FormInterface, or it should be renamed.

the Approved API change tag was added in september 2013, not sure if that still applies ;)

- @Berdir, January 2015

;-)

Just found this issue. Would be nice, but there is no way this is going into 8.* ...

Yeah this can't go in as is. But We could add a public getter to ConfigFormBase that calls the protected method. Or there might be other alternatives.

This would be a minor-only change. Since 8.9.x and 9.0.x are now in beta, I'm moving this to 9.1.x. Thanks!

Hmm, ok - I'm still using a reflection class hack to get around this limitation.