We need a way to dynamically alter the list of config names for config mappers

Blocker then

Blocker then

Any suggested options? :)

Just looking at the config schema of core.entity_view_display.*.*.* (in core.entity.schema.yml) and I don't see any translatable elements there. Here is few more points to prove above statement.

1. 'label' under core.entity_view_display.*.*.*.content.[field_name] is a placement (hidden/above/inline) *string* not translatable *label*
2. field.formatter.settings.[%parent.type] is a dynamic definition under this schema and as far as core implementation, none of them have translatable element(s).
3. field.formatter.settings.[%parent.type] is another dynamic item and core has no implementation/example for this.

Standard profile provides list of entity_view_displays (below) out of box, but there isn't any translatable elements.

core.entity_view_display.block_content.basic.default
core.entity_view_display.comment.comment.default
core.entity_view_display.node.article.default
core.entity_view_display.node.article.rss
core.entity_view_display.node.article.teaser
core.entity_view_display.node.page.default
core.entity_view_display.node.page.teaser
core.entity_view_display.user.user.compact
core.entity_view_display.user.user.default

now, question #1. Am I missing a big picture here? #2. Are we worried about the 3rd party settings? #3. Do we have any working example with the issue to start with?

That looks fine with me. Modules can subscribe to that event and then examine the mapper class and contents for extending config names. Indeed needs tests.

Here's a new patch trying to make phpunit tests pass.
Now I'll work on new test coverage.

Thanks @tstoeckler, I didn't know about KernelTestBase, and I though it was weird to put this test with the UI tests, but I didn't know where else to put it. I'll make the changes.

The other tests are failing because of denied access to the translation pages, caused by config_translation_test adding a config name to the mapper. I don't know why, maybe you do?

@tstoekler:
1) I changed the config name that I add to 'config_translation_test.content' and it made most of the tests pass. There is still some that fail with a PHP error, something about only one language allowed by config mapper (?), I don't remember exactly, I'll verify when I'm back home.

2) I have troubles moving my test and use KernelTestBase. As you said, '\Drupal::service()' doesn't work, and '$this->container->get()' does, but it doesn't seem to return a ConfigMapperManager object (when i call getMappers() on it the test fails). And it's hard to debug because when I try to print something to the console, nothing get printed, so I can't have any clue about what is returned. Any inputs?

Sure, I'll do that tonight.

Here's what I have.
It fails when I add th line '$mappers = $manager->getMappers();'

Update: \Drupal::service('plugin.manager.config_translation.mapper') does work in a KernelTestBase test, and it returns a ConfigMapperManager as expected. But still, when I call getMappers() on it, the test fails.

Here's a patch with the new test changed for a KernelTestBase one.
Thanks @tstoeckler for your help debugging this.

About the failing UI test, this is the fatal error I get when I run it on my computer:

Uncaught PHP Exception RuntimeException: "A config mapper can only contain configuration for a single language." at /home/maxocub/code/drupal/core/modules/config_translation/src/ConfigNamesMapper.php line 427

This is what I used to subscribe to the event and add a config name to a mapper:

class ConfigTranslationTestSubscriber implements EventSubscriberInterface {
  static function getSubscribedEvents() {
    $events[ConfigTranslationEvents::POPULATE_MAPPER][] = ['addConfigNames'];
    return $events;
  }

  public function addConfigNames(ConfigMapperEvent $event) {
    $mapper = $event->getMapper();
    $mapper->addConfigName('config_translation_test.content');
  }

}

This adds the config name to all mappers, and I think thats why the test fails.

Here's a patch where I add the config name only to a specific mapper, like this:

  public function addConfigNames(ConfigMapperEvent $event) {
    $mapper = $event->getMapper();
    if ($mapper->getBaseRouteName() == 'system.site_information_settings') {
      $mapper->addConfigName('config_translation_test.content');
    }
  }

Is that how a module could add a config name to one mapper?

I think the error mentioned in #28 is caused by one of the test from ConfigTranslationUiTest (line 541) who changes the langcode of the system.site config to 'not-specified', while the config name I add in config_translation_test is in English.

I'll try to investigate more this weekend, but do you have any idea how to solve that @tstoeckler?

@tstoeckler: Good idea, thanks! Let's see how it goes...

Thanks for the test coverage!

The issue definitely needs an issue summary update to outline the solution. For me its not clear for example which use cases would benefit from the route match passed around.

Also as an API change/addition, this would need to go to 8.1. I think it should also aim to be backwards compatible. Now it swaps the argument order around for example and does not have a fallback way to work when the new arguments are not passed to the mappers. Marking needs work for that.

Really not sure what I'm doing here, I just put the arguments order as it was before, but I don't know yet what are the implications. Just want to see how the tests go.

Thanks @tstoeckler!
Here's a new patch, let's see if it pass.
Still need to update issue summary, will do soon.

@tstoeckler: I will add the missing docs.

   public function populateFromRouteMatch(RouteMatchInterface $route_match) {
-    parent::populateFromRouteMatch($route_match);
+    $this->setLangcode($route_match->getParameter('langcode'));
+
     $entity = $route_match->getParameter($this->entityType);
     $this->setEntity($entity);
+
+    $this->dispatchMapperEvent(ConfigTranslationEvents::POPULATE_MAPPER);
   }

I was wondering if we really needed this change? parent::populateFromRouteMatch() already call setLangcode() and dispatchMapperEvent(). Is it because we need to call setEntity() before dispatchMapperEvent()?

Here's the new patch with the missing docs.

public function populateFromRouteMatch(RouteMatchInterface $route_match) {
  $this->setLangcode($route_match->getParameter('langcode'));
  $this->dispatchMapperEvent(ConfigTranslationEvents::POPULATE_MAPPER);
}

protected function dispatchMapperEvent($event_name) {
  $event = new ConfigMapperPopulateEvent($this);
  $this->eventDispatcher->dispatch($event_name, $event);
}

@tstoeckler: I'm trying your suggestion about the route match to see what it brings, I'm asking myself:
Do we need the dispatchMapperEvent() function or can we move its code to populateFromRouteMatch?
Because we would need the $route_match to construct the new ConfigMapperPopulateEvent, which right now we don't in dispatchMapperEvent().

Or else, what would be the best way to pass the $route_match to dispatchMapperEvent()? Add it as an argument?

Updated the issue summary.
Uploaded a new patch with the route match in the (renamed) event.

Thanks! Some very minor nitpicks:

1. +++ b/core/modules/config_translation/src/ConfigEntityMapper.php
   @@ -78,9 +79,11 @@ class ConfigEntityMapper extends ConfigNamesMapper {
   +   * @param \Symfony\Component\EventDispatcher\EventDispatcherInterface $event_dispatcher
   

   Wrap param to fit in 80 chars? Not sure that is best practice.

2. +++ b/core/modules/config_translation/src/ConfigNamesMapper.php
   @@ -119,12 +129,14 @@ class ConfigNamesMapper extends PluginBase implements ConfigMapperInterface, Con
   +   * @param \Symfony\Component\EventDispatcher\EventDispatcherInterface $event_dispatcher
   

   Same as above.

3. +++ b/core/modules/config_translation/src/ConfigNamesMapper.php
   @@ -136,6 +148,8 @@ public function __construct($plugin_id, $plugin_definition, ConfigFactoryInterfa
   +
   

   Remove empty space?

4. +++ b/core/modules/config_translation/src/ConfigNamesMapper.php
   @@ -371,7 +386,11 @@ public function getWeight() {
   +    // Dispatches the ConfigTranslationEvents::POPULATE_MAPPER event
   

   Period at end of sentence.

5. +++ b/core/modules/config_translation/tests/src/Kernel/ConfigMapperTest.php
   @@ -0,0 +1,52 @@
   +    // Test that it now contains the new config name from config_translation_test.
   

   More than 80 chars.

I updated the patch based on feedback in #49. Note:

• 1 and 2 were ignored because there is not a good way to limit to 80 characters.
• For 3, the variables were reordered to match the ordering in the function parameters.

I'm interested in testing this so I did a simple reroll

Looks great. Mostly nits :)

1. +++ b/core/modules/config_translation/src/ConfigEntityMapper.php
   @@ -105,9 +109,9 @@ public static function create(ContainerInterface $container, array $configuratio
      public function populateFromRouteMatch(RouteMatchInterface $route_match) {
   -    parent::populateFromRouteMatch($route_match);
        $entity = $route_match->getParameter($this->entityType);
        $this->setEntity($entity);
   +    parent::populateFromRouteMatch($route_match);
      }
   

   Why change call order here?

   EDIT: because \Drupal\config_translation\ConfigNamesMapper::populateFromRouteMatch() is now dispatching an event, and at that point we want all metadata to be set. Therefore we must first set metadata ($this->setEntity(…)) and then call the parent method.

   This is technically a BC break. At minimum this needs a CR to indicate that any subclasses of \Drupal\config_translation\ConfigNamesMapper must update their populateFromRouteMatch() method to call the parent method last.

2. +++ b/core/modules/config_translation/src/ConfigMapperInterface.php
   @@ -282,12 +282,12 @@ public function hasTranslatable();
   -   * @todo Replace $request with RouteMatch https://www.drupal.org/node/2295255.
   

   This TODO was not actually addressed AFAICT.

   EDIT: lol, no, it just was an todo that's no longer relevant :)

3. +++ b/core/modules/config_translation/src/ConfigNamesMapper.php
   @@ -115,23 +125,24 @@ class ConfigNamesMapper extends PluginBase implements ConfigMapperInterface, Con
   +  public function __construct($plugin_id, $plugin_definition, ConfigFactoryInterface $config_factory, TypedConfigManagerInterface $typed_config, LocaleConfigManager $locale_config_manager, ConfigMapperManagerInterface $config_mapper_manager, RouteProviderInterface $route_provider, TranslationInterface $string_translation, LanguageManagerInterface $language_manager, EventDispatcherInterface $event_dispatcher = NULL) {
   ...
   +    $this->eventDispatcher = $event_dispatcher ?: \Drupal::service('event_dispatcher');
   

   We do this to maintain BC, because this is a base class?

   If so, let's add a @todo to make this parameter required in 9.0.0.

4. +++ b/core/modules/config_translation/src/Event/ConfigMapperPopulateEvent.php
   @@ -0,0 +1,66 @@
   +/**
   + * @file
   + * Contains \Drupal\config_translation\Event\ConfigMapperPopulateEvent.
   + */
   
   +++ b/core/modules/config_translation/src/Event/ConfigTranslationEvents.php
   @@ -0,0 +1,23 @@
   +/**
   + * @file
   + * Contains \Drupal\config_translation\Event\ConfigTranslationEvents.
   + */
   
   +++ b/core/modules/config_translation/tests/modules/config_translation_test/src/EventSubscriber/ConfigTranslationTestSubscriber.php
   @@ -0,0 +1,40 @@
   +/**
   + * @file
   + * Contains \Drupal\config_translation_test\EventSubscriber\ConfigTranslationTestSubscriber.
   + */
   
   +++ b/core/modules/config_translation/tests/src/Kernel/ConfigMapperTest.php
   @@ -0,0 +1,52 @@
   +/**
   + * @file
   + * Contains \Drupal\Tests\config_translation\Kernel\ConfigMapperTest.
   + */
   

   Nit: we don't do @file docblocks anymore :)

5. +++ b/core/modules/config_translation/src/Event/ConfigMapperPopulateEvent.php
   @@ -0,0 +1,66 @@
   +    $this->route_match = $route_match;
   

   Nit: $this->routeMatch.

6. +++ b/core/modules/config_translation/tests/modules/config_translation_test/src/EventSubscriber/ConfigTranslationTestSubscriber.php
   @@ -0,0 +1,40 @@
   +   * Reacts to the population of a configuration mapper.
   

   Nit: s/population/populating/

7. +++ b/core/modules/config_translation/tests/modules/config_translation_test/src/EventSubscriber/ConfigTranslationTestSubscriber.php
   @@ -0,0 +1,40 @@
   +   *   The configuration mapper event.
   

   s/mapper event/mapper populate event/

8. +++ b/core/modules/config_translation/tests/src/Kernel/ConfigMapperTest.php
   @@ -0,0 +1,52 @@
   +    // Call populateFromRouteMatch() to dispatch an event.
   

   s/an event/the "config mapper populate" event/

This looks almost ready.

+/**
+ * @file
+ * Contains \Drupal\config_translation_test\EventSubscriber\ConfigTranslationTestSubscriber.
+ */

We need to remove this.

-    $this->langcode = $route_match->getParameter('langcode');
+    $this->setLangcode($route_match->getParameter('langcode'));

Is there a reason for this change? I don't see one (setLangcode() still only assigns the value to $this->langcode)

+  /**
+   * The name of the event dispatched when a configuration mapper is populated.
+   *
+   * @see \Drupal\config_translation\ConfigMapperInterface::populateFromRouteMatch()
+   */
+  const POPULATE_MAPPER = 'config_translation.populate_mapper';

This might be nitpickish, but I'd love to see a sentence explaining a use case for this.
Something like "Allows modules to add related config for translation on a specific translation form."

Addressed #60 review

Thank you! This now looks ready.

Small nitpick that can be ignored, or addressed at commit:

+    // Dispatches the ConfigTranslationEvents::POPULATE_MAPPER event.
+    $event = new ConfigMapperPopulateEvent($this, $route_match);
+    $this->eventDispatcher->dispatch(ConfigTranslationEvents::POPULATE_MAPPER, $event);

I generally dislike comments that retell what trivial code is doing. We already know what's happening from seeing the dispatch call and the event constant. So I'd remove the comment.

Only some minor nits from me, in addition to @bojanz's above.

In line with https://twitter.com/xjmdrupal/status/922090356077932544 - probably should be fixed beforehand

1. +++ b/core/modules/config_translation/tests/modules/config_translation_test/src/EventSubscriber/ConfigTranslationTestSubscriber.php
   @@ -0,0 +1,35 @@
   +    $events[ConfigTranslationEvents::POPULATE_MAPPER][] = ['addConfigNames'];
   +    return $events;
   

   nit:do we need the intermediate variable here?

2. +++ b/core/modules/config_translation/tests/src/Kernel/ConfigMapperTest.php
   @@ -0,0 +1,48 @@
   +    $this->assertFalse(in_array('config_translation_test.content', $config_names));
   ...
   +    $this->assertTrue(in_array('config_translation_test.content', $config_names));
   

   assertContains/assertNotContains is your friend :)

For #62 and #63.

Fixed #62 & #63

+++ b/core/modules/config_translation/tests/modules/config_translation_test/src/EventSubscriber/ConfigTranslationTestSubscriber.php
@@ -0,0 +1,35 @@
+  static function getSubscribedEvents() {

added visibility

The interdiff in #65 correctly solves the #62 and #63.

Unrelated failure (#2919773: Fail to download drupal/coder because github.com/klausi/coder has gone missing).

This is looking very good

1. +++ b/core/modules/config_translation/src/ConfigNamesMapper.php
   @@ -115,23 +125,26 @@ class ConfigNamesMapper extends PluginBase implements ConfigMapperInterface, Con
   +   * @todo Make $event_dispatcher required in Drupal 9.
   

   We need a follow up issue for this - which should be referenced in the code. I.e the URL to the issue for the follow-up.

2. +++ b/core/modules/config_translation/src/ConfigNamesMapper.php
   @@ -115,23 +125,26 @@ class ConfigNamesMapper extends PluginBase implements ConfigMapperInterface, Con
   -    $this->routeProvider = $route_provider;
   -
   ...
   -
   +    $this->routeProvider = $route_provider;
   

   this change looks out of scope

thanks for working on this

Adding credit for reviewers that helped shape the patch.

I reviewed the patch in #73 for style and consistency and didn't notice anything obvious. The interdiff confirms that point 2 of #69 was covered and, regarding point 1 in #69, the note in #73 explains why the todo was removed.

It looks like everyone's feedback has been incorporated. I would RTBC but I provided a patch way back when so probably shouldn't?

We're definitely ready again.

Unrelated failure (DST).

Unrelated failure.

Back to RTBC. This poor issue has been RTBC since february :)

HEAD problem, looks like bd67565 missed a spot.

Fixed on commit

diff --git a/core/modules/config_translation/src/Event/ConfigTranslationEvents.php b/core/modules/config_translation/src/Event/ConfigTranslationEvents.php
index da392fa5c7..6a95f69ba2 100644
--- a/core/modules/config_translation/src/Event/ConfigTranslationEvents.php
+++ b/core/modules/config_translation/src/Event/ConfigTranslationEvents.php
@@ -18,4 +18,3 @@
   const POPULATE_MAPPER = 'config_translation.populate_mapper';

 }
-

Committed ffc8ecd and pushed to 8.7.x

C/p as b4a02f8 and pushed to 8.6.x.

Published change record