Updated: Comment #36

Problem/Motivation

There is no way to associate config names with forms, and keep the information with the form.

This is needed for when config_translation is added into core.
Without this config_translation has its own yml it discovers it from.

For example, now, http://drupalcode.org/project/config_translation.git/blob/HEAD:/config_t...

site_information:
type: names
base_path: 'admin/config/system/site-information'
names:
 - 'system.site'
title: 'System information'
add_edit_tab: '1'

looks up the path and the "names" (cmi names).

Other use cases?

Proposed resolution

  1. Add a ConfigFormInterface that extends FormInterface with a getConfigNames() method that returns an array of config names.
  2. Make ConfigFormBase implement that interface
  3. Update all sub-classes of ConfigFormBase to provide that method.
  4. Add a PluginConfigFormInterface that extends PluginFormInterface with the same method for plugins that save into separate configuration files.
  5. Update appropriate plugins to extend that interface and provide this method. This is aggregator and search plugins currently.

For now that method will not be called anywhere in core, until config_translation is in.

Remaining tasks

  • Review patch
  • Write tests
Contributor tasks needed
Task Novice task? Contributor instructions Complete?
Create a patch Instructions
Update the issue summary Instructions
Update the issue summary noting if allowed during the beta Instructions

User interface changes

API changes

No. Adding a new method, not changing APIs.

Blocks

Related

Proposed commit message (will need updating)

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

Files: 
CommentFileSizeAuthor
#76 2095289-76-config-translation-auto.patch44.25 KBtstoeckler
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,993 pass(es), 60 fail(s), and 0 exception(s). View

Comments

YesCT’s picture

Issue summary: View changes

add the actual config trans issue

tstoeckler’s picture

Updated the issue summary with specific steps to do.

thomas4019’s picture

Status: Active » Needs review
FileSize
16.46 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

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

tstoeckler’s picture

Status: Needs review » Needs work

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.

thomas4019’s picture

FileSize
7.05 KB
22.95 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

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.

thomas4019’s picture

Issue summary: View changes

Updated issue summary with more specific instructions

thomas4019’s picture

Issue summary: View changes

updated remaining tasks

tstoeckler’s picture

Status: Needs work » Needs review

Let's see if this passes.

Status: Needs review » Needs work

The last submitted patch, drupal-config-2095289-4.patch, failed testing.

Sutharsan’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests
FileSize
3.39 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch interdiff-2095289-4-7.patch. Unable to apply patch. See the log in the details link for more information. View
23.11 KB
PASSED: [[SimpleTest]]: [MySQL] 58,847 pass(es). View

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

Status: Needs review » Needs work

The last submitted patch, interdiff-2095289-4-7.patch, failed testing.

tstoeckler’s picture

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.

tstoeckler’s picture

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');

tstoeckler’s picture

  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.

tstoeckler’s picture

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!

tstoeckler’s picture

Assigned: Unassigned » tstoeckler

Actually, I'll fix this now.

tstoeckler’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
7.88 KB
27.63 KB
FAILED: [[SimpleTest]]: [MySQL] 58,551 pass(es), 17 fail(s), and 0 exception(s). View

Here we go.

Also, this doesn't need tests.

Status: Needs review » Needs work

The last submitted patch, 2095289-config-form-get-names-14.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
591 bytes
27.88 KB
PASSED: [[SimpleTest]]: [MySQL] 58,551 pass(es). View

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.

vijaycs85’s picture

Issue tags: +Needs manual testing

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

tstoeckler’s picture

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.

tstoeckler’s picture

Issue tags: +Needs manual testing
FileSize
679 bytes
95.82 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2095289-config-form-get-names-19.patch. Unable to apply patch. See the log in the details link for more information. View

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

Will now do the manual testing.

Here's an updated patch for #17.

Status: Needs review » Needs work

The last submitted patch, 2095289-config-form-get-names-19.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
5.05 KB
32.09 KB
FAILED: [[SimpleTest]]: [MySQL] 58,543 pass(es), 1 fail(s), and 0 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, 2095289-config-form-get-names-20.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
5.41 KB
30.37 KB
FAILED: [[SimpleTest]]: [MySQL] 58,560 pass(es), 3 fail(s), and 0 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, 2095289-config-form-get-names-23.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review

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

tstoeckler’s picture

Status: Needs review » Needs work

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

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
789 bytes
32.66 KB
PASSED: [[SimpleTest]]: [MySQL] 59,001 pass(es). View

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.

tstoeckler’s picture

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.

tim.plunkett’s picture

Status: Needs review » Needs work

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

tstoeckler’s picture

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

tstoeckler’s picture

Issue tags: -Needs manual testing

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

alexpott’s picture

Status: Needs work » Needs review

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.

tstoeckler’s picture

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

tim.plunkett’s picture

Status: Needs review » Needs work
--- 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?

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
7.73 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2095289-config-form-get-names-interdiff-27-35.patch. Unable to apply patch. See the log in the details link for more information. View
35.39 KB
FAILED: [[SimpleTest]]: [MySQL] 58,414 pass(es), 1 fail(s), and 0 exception(s). View

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

tstoeckler’s picture

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

Status: Needs review » Needs work

The last submitted patch, 2095289-config-form-get-names-interdiff-27-35.patch, failed testing.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.

tstoeckler’s picture

Issue summary: View changes

Updated issue summary.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
7.1 KB
38.5 KB
PASSED: [[SimpleTest]]: [MySQL] 58,387 pass(es). View

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.

tstoeckler’s picture

Issue summary: View changes

Updated issue summary.

tim.plunkett’s picture

Status: Needs review » Needs work

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

tstoeckler’s picture

FileSize
1.2 KB
38.82 KB
PASSED: [[SimpleTest]]: [MySQL] 58,383 pass(es). View

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

tstoeckler’s picture

Status: Needs work » Needs review
tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Looks good, it should pass.
Thanks @tstoeckler!

Status: Reviewed & tested by the community » Needs work
Issue tags: -API change, -D8MI, -language-config, -blocker, -Approved API change

The last submitted patch, 2095289-config-form-get-names-40.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: +API change, +D8MI, +language-config, +blocker, +Approved API change
tim.plunkett’s picture

Issue summary: View changes

fix markup

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

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.

tstoeckler’s picture

Issue summary: View changes

Added proposed commit message

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Postponed

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

tstoeckler’s picture

Status: Postponed » Reviewed & tested by the community

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.

Gábor Hojtsy’s picture

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.

tstoeckler’s picture

Status: Reviewed & tested by the community » Postponed

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.

tstoeckler’s picture

Title: Add a getConfigNames method to ConfigFormBase to return the cmi names for a form » Add a getConfigNames method on a newly introduced ConfigFormInterface

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.

Gábor Hojtsy’s picture

Status: Postponed » Reviewed & tested by the community

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

tstoeckler’s picture

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.

effulgentsia’s picture

Status: Reviewed & tested by the community » Postponed

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.

tim.plunkett’s picture

Also, how will hook_form_alter work here?

Gábor Hojtsy’s picture

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

tstoeckler’s picture

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.

Gábor Hojtsy’s picture

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.

effulgentsia’s picture

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?

tstoeckler’s picture

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.

tstoeckler’s picture

Issue summary: View changes

Added tim.plunkett to the proposed commit message.

tstoeckler’s picture

Assigned: tstoeckler » Unassigned
Status: Postponed » Needs work
Gábor Hojtsy’s picture

Status: Needs work » Closed (duplicate)

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

Gábor Hojtsy’s picture

Status: Closed (duplicate) » Postponed

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.

tstoeckler’s picture

Status: Postponed » Needs work

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!

YesCT’s picture

before investing a bunch of time in this, let's do a beta evaluation. adding task instructions to the summary.

Also, I think this could use a general issue summary update.

YesCT’s picture

Issue summary: View changes
tstoeckler’s picture

Assigned: Unassigned » tstoeckler
Gábor Hojtsy’s picture

Issue tags: +SprintWeekend2015
tstoeckler’s picture

Title: Add a getConfigNames method on a newly introduced ConfigFormInterface » Make getEditableConfigNames() public and use it config_translation
Status: Needs work » Needs review
Issue tags: +Needs tests
FileSize
40.26 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View

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.

Status: Needs review » Needs work

The last submitted patch, 68: 2095289-68-config-translation-auto.patch, failed testing.

Gábor Hojtsy’s picture

Title: Make getEditableConfigNames() public and use it config_translation » Make getEditableConfigNames() public and use it in config_translation

Fix title.

Berdir’s picture

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

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
4.84 KB
45.1 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View

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?

Status: Needs review » Needs work

The last submitted patch, 72: 2095289-72-config-translation-auto.patch, failed testing.

Berdir’s picture

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.

tstoeckler’s picture

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.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
7.32 KB
44.25 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,993 pass(es), 60 fail(s), and 0 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, 76: 2095289-76-config-translation-auto.patch, failed testing.

tim.plunkett’s picture

  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.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

tstoeckler’s picture

Version: 8.3.x-dev » 9.x-dev
Assigned: tstoeckler » Unassigned

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

alexpott’s picture

Issue tags: -Approved API change

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.