Problem/Motivation

This came up in #2546212: [PP-1] Entity view/form mode formatter/widget settings have no translation UI. We want to translate node displays on the node type translation form, but we cannot just attach *all* node displays to the mapper alltogether, but we need to dynamically attach those displays that belong to the node type that is currently being translated. And we cannot do this in a dedicated mapper, because we need to do this generically from Field UI module for all entity types.

I.e. when ConfigEntityMapper::setEntity() is called for the node type mapper, it ends up setting the node.type.article config name, for example. We need to somehow hook into that from Field UI and also attach core.entity_view_display.node.article.teaser for example.

This is blocking a major bug, so is tagged as major as well.

Proposed resolution

Have config_translation module dispatch an event when populateFromRouteMatch() is called so that other modules can subscribe to the event and add config names to the config mapper.

Remaining tasks

Review.
Evaluate if we need the route match in the event. See this comment for a reason why we should.

User interface changes

None.

API changes

Additions:

  • New event (ConfigMapperPopulateEvent)
  • New argument (optional for BC) for ConfigNamesMapper & ConfigEntityMapper ($event_dispatcher)

Data model changes

None.

Comments

tstoeckler created an issue. See original summary.

penyaskito’s picture

Blocker then

penyaskito’s picture

Issue tags: +blocker

Blocker then

Gábor Hojtsy’s picture

Any suggested options? :)

vijaycs85’s picture

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?


update: found admin/structure/display-modes/view/manage/node.full/translate/ta/add has just name as translatable.
tstoeckler’s picture

Sorry for not explaining the use-case earlier.

Are we worried about the 3rd party settings?

Yes, we are. :-) Fieldgroup module will want to add it's information there including the label of the fieldgroup, I presume. It would be very unfortunate to not be able to translate that. Also it's conceivable that widgets or formatters have translatable settings in contrib even if they don't in core.

tstoeckler’s picture

Some ideas:

The easy way out here would be to just throw an event either in ConfigNamesMapper::populateFromRouteMatch() or in ConfigEntityMapper::setEntity(). Then Field UI module could react to that event and add its names to the mapper. The former would have the upside of not being tied to ConfigEntityMapper, i.e. if there were another dynamic use-case of the mappers then they could re-use that event. The latter would have the advantage that the implementor in Field UI module could work with the entity obejct directly instead of having to retrieve it from the route match, thus duplicating code from ConfigEntityMapper::populateFromRouteMatch().

A cooler way to solve this would be to add config names with placeholders to the mappers and then have ConfigNamesMapper::populateFromRouteMatch() do the replacing automatically. I.e. add node.type.{node_type} to the node type mapper and then Field UI module could add core.entity_view_display.node.{node_type}.teaser (among others). I think that would be a much nicer implementation. Right now I think it's very weird that e.g. the node type mapper has no config names in it until populateFromRouteMatch() is called, so in that way it's not distinguishable from any other config entity mapper. But I think that would be a much larger change and include API changes (whereas the above would be strictly API additions) so I think that's off the table at this point.

Thoughts?

tstoeckler’s picture

Actually on throwing an event in ConfigNamesMapper::populateFromRouteMatch() or in ConfigEntityMapper::setEntity():

With #2565031: Expose $entity in ConfigEntityMapper in we now have a semantic way to fetch the entity from the mapper without duplicated logic. So I don't see any arguments against ConfigNamesMapper::populateFromRouteMatch(). Will try that out if there aren't any other ideas.

tstoeckler’s picture

Also on the "cooler" solution with placeholders in config names: The example above is slightly lying because we can't just add core.entity_view_display.node.{node_type}.teaser because we don't know if the teaser display exists explicitly for each node type. So what we would in fact is something like core.entity_view_display.node.{node_type}.* where the * would signify that we would have to call ConfigFactoryInterface::listAll() in populateFromRequest() or something like that. In any case, I don't think that's something we can get away with just now, maybe in 8.1 if we manage to avoid BC-breaks.

tstoeckler’s picture

Status: Active » Needs review
Issue tags: +Needs tests
FileSize
10.78 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,592 pass(es), 3 fail(s), and 0 exception(s). View

Something like this?

Status: Needs review » Needs work

The last submitted patch, 10: 2577761-proposal.patch, failed testing.

The last submitted patch, 10: 2577761-proposal.patch, failed testing.

Gábor Hojtsy’s picture

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.

maxocub’s picture

Status: Needs work » Needs review
FileSize
14.79 KB
4 KB

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

maxocub’s picture

Status: Needs review » Needs work
maxocub’s picture

Status: Needs work » Needs review
FileSize
18.52 KB
3.37 KB

Status: Needs review » Needs work

The last submitted patch, 16: adding_config_names-2577761-16.patch, failed testing.

tstoeckler’s picture

Issue tags: -Needs tests

Awesome work @maxocub, that is a great test. Not really sure why it is failing the way it is, the test looks pretty harmless to me...

In any case, while the test itself is great, I think we can improve one thing about it:

The test doesn't need any UI functionality in order to work - the only interaction with the system is fetching a service - so we should really be making it a KernelTestBase, preferably a \Drupal\KernelTests\KernelTestBase (yes, there are two KernelTestBase classes in core...).

In that case it should go into a tests/src/Kernel directory in the config_translation module directory. There is already a Unit directory there with some tests in it which you can use to see how the namespace has to be (that is somewhat unintuitive).

I'm not sure how we should name the test (maybe something generic like ConfigMapperTest?), but if you extend KernelTestBase and put a $modules = ['config_translation', 'config_translation_test'] at the top you should be able to copy your test method 1-to-1. The only thing I'm not sure about: I don't know if \Drupal::service() works in a KernelTestBase, if it doesn't you can use $this->container->get(), that should definitely work.

maxocub’s picture

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?

tstoeckler’s picture

+++ b/core/modules/config_translation/tests/modules/config_translation_test/src/EventSubscriber/ConfigTranslationTestSubscriber.php
@@ -0,0 +1,21 @@
+    $mapper->addConfigName('config_translation_test.content.test.label');

Ahh good catch. That's probably because no config exists with that name. It has to be config_translation_test.content because that's the actual config object.

Presumably, the failures will go away, anyway, though, when we move this to a separate test.

maxocub’s picture

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

tstoeckler’s picture

@maxocub, it's hard to understand what is happening without seeing the code. Can you post what you currently have? It doesn't matter if it's not finished yet.

maxocub’s picture

Sure, I'll do that tonight.

maxocub’s picture

FileSize
3.37 KB

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

maxocub’s picture

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.

maxocub’s picture

Status: Needs work » Needs review
FileSize
18.56 KB
4.22 KB

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

Status: Needs review » Needs work

The last submitted patch, 26: adding_config_names-2577761-26.patch, failed testing.

maxocub’s picture

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

maxocub’s picture

Status: Needs work » Needs review
FileSize
18.64 KB
944 bytes

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?

Status: Needs review » Needs work

The last submitted patch, 29: adding_config_names-2577761-29.patch, failed testing.

maxocub’s picture

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’s picture

@maxocub: Ahh, nice find! In that case, let's just add a $mapper->getLangcode() === 'en' to the condition in ConfigTranslationTestSubscriber, that seems to be the easiest.

maxocub’s picture

Status: Needs work » Needs review
FileSize
18.68 KB
989 bytes

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

tstoeckler’s picture

Yay, this is now ready for some serious reviews. One thing that I thought about when working on #2546212: [PP-1] Entity view/form mode formatter/widget settings have no translation UI was whether we should also be providing the route match in the event. In the end did not require it in that issue, but it seems like it might be useful for generally. I am slightly leaning towards "Yes", but not sure.

If we do, we should rename the event. It's now generically named "ConfigMapperEvent" because all it knows about is a config mapper, but if it also knows about a route match we should name it after its specific purpose: "ConfigMapperPopulateEvent".

Gábor Hojtsy’s picture

Status: Needs review » Needs work
Issue tags: +Needs issue summary update

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.

Gábor Hojtsy’s picture

Version: 8.0.x-dev » 8.1.x-dev
maxocub’s picture

Status: Needs work » Needs review
Issue tags: +SprintWeekend2016
FileSize
18.04 KB
4.05 KB

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.

maxocub’s picture

Status: Needs review » Needs work

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

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now 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.

tstoeckler’s picture

In terms of the argument order or added arguments:
If that's considered a BC-break we could do the following:

Instead of:

public function __construct( ... EventDispatcherInterface $event_dispatcher) {
  ...
  $this->eventDispatcher = $event_dispatcher;
  ...
}

we could do:

public function __construct( ... EventDispatcherInterface $event_dispatcher = NULL) {
  ...
  $this->eventDispatcher = $event_dispatcher ?: \Drupal::service('event_dispatcher');
  ...
}

As far as I'm aware that's 100% backwards-compatible, and that's also what we've done elsewhere before.

So marking needs work for that. That way this should be uncontroversial for 8.1.

maxocub’s picture

Status: Needs work » Needs review
FileSize
18.33 KB
4.18 KB

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

tstoeckler’s picture

Yay, @maxocub++!!!

This looks pretty much ready to go to me.

One minor thing:
ConfigTranslationTestSubscriber could use some docs, i.e. @file at the doc and a docblock for the class and inheritdoc / proper docblock for the methods.

maxocub’s picture

@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()?

maxocub’s picture

FileSize
18.75 KB
1.67 KB

Here's the new patch with the missing docs.

tstoeckler’s picture

Docs look good.

@maxocub re #43: Yes, exactly. The current code is a bit unfortunate but I couldn't figure out how do to it any better. We need to avoid dispatching the event twice and we also need to call setEntity() (and setLangcode()) before dispatching it so that event subscribers have all the info they need. Looking at it again now, I think we should be able to do

$this->setEntity(...);
parent::populateFromRouteMatch(...);

right? Not sure...

Also, coming back to this now after quite a while, I do think we should pass the route match to the event. It's called POPULATE_MAPPER after all, and that "populate" comes from the method name populateFromRouteMatch(), so not passing it along is not nice IMO. We should then rename the event class to ConfigMapperPopulateEvent because it would be confusing for a generic ConfigMapperEvent (the current class name) to have a getRouteMatch() method. I still do not feel strongly, though, so if others disagree we can also leave it as is. Would just be unfortunate if we then realize that some contrib module, does actually want access to the full route match.

maxocub’s picture

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?

tstoeckler’s picture

No, we don't really need it. Feel free to remove it and/or reorganize the code in any way you think it should be.

maxocub’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update
FileSize
18.81 KB
7.52 KB

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

Kristen Pol’s picture

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.

Kristen Pol’s picture

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.
tstoeckler’s picture

Status: Needs review » Needs work

Code looks really nice now, thanks @maxocub and @Kristen Pol !!!

I think it's good that we make the event dispatcher optional in ConfigNamesMapper, but I think we should do the same in ConfigEntityMapper as well. The = NULL is missing there.

tstoeckler’s picture

Status: Needs work » Needs review

Oops, sorry, it's there indeed. Don't know what I was hallucinating...

Looks perfect, then. Unfortunately, I can't RTBC.

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

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now 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.

rodrigoaguilera’s picture

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

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

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now 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.

Wim Leers’s picture

Status: Needs review » Needs work
Issue tags: +Needs change record

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/

Wim Leers’s picture

Issue tags: +DevDaysSeville
tstoeckler’s picture