Problem/Motivation

We need to have all configuration that a user can define, which has translatable information on it, to be translatable in the Configuration Translation module's user interface.

There are several pieces of information from the Entity/Field system, which may include translatable strings, but which are not translatable using the Config Translation UI:

a) Field settings. For instance, on a Number field, there are translatable settings for the Prefix and Suffix in the settings config. But if you go to the Translate page for the field, all you can translate is the Label and Help for the field, not the prefix/suffix.

===> This is because the schema is wrong. See #2546356: Numeric field prefix/suffix settings should be translatable. Fields are translatable along with their settings, supposedly anyway.

b) Entity view displays - field formatter settings. For instance, on #2449597: Number formatters: Make it possible to configure format_plural on the formatter level we are adding the ability to numbers to have formatPlural() type formatting, but there is no way to translate the "1 item/@count items" strings. These are stored on the entity view display config items.

c) Entity form displays - widget settings. For instance, many text fields have widget options to enter Placeholder text, and obviously this would need to be translated. These are stored on the entity form display config items.

d) Base field overrides, which seem to include labels.
==> will do this on a separate issue

The technical reason is, at least for the entity view/form display formatter/widget settings, that there is not an edit form for these things -- the config schema indicates they should be translatable, but there's no way to edit them so config translation module doesn't put up a UI for translating them either. They are settings that appear as part of the Manage Form or Manage Display pages in Field UI, but the individual fields do not have their own settings forms.

Comment #5 also looked at other config items to see if they were translatable and #8 discussed them; more issues will be open.

Proposed resolution

Make sure that these items have a translation UI.

Remaining tasks

Figure out how to make them translatable. Maybe add tests too?

User interface changes

You'll be able to translate your site. Can't now.

API changes

Probably not, but we'll have to see what the patch entails.

Data model changes

I wouldn't think so.

Comments

jhodgdon created an issue. See original summary.

Gábor Hojtsy’s picture

Issue tags: +sprint, +language-config
mpdonadio’s picture

Issue tags: +blocker
tstoeckler’s picture

a) Field settings can be translated already, that is not correct. The fact that you cannot translate the prefix and suffix is the fault of the config schema. It might make sense to update that and make it translatable but there is nothing Config Translation can do about it.

b)/c) So these are the entity form|view displays that you want to translate. The form|view modes are just very thin objects with an id and a label (e.g. 'Teaser') and those you can translate (i.e. the 'Teaser' part), but that's not very interesting. Where the actual widget|formatter settings get stored is the entity form|view displays.

Because these displays do not have an 'edit-form' link (because their edit forms are spread around over various entity types) they do not benefit from the generic config entity integration that Config Translation does. Thus, we need to provide explicit integration just like we do for field settings already (because they have the same problem).

The challenge here is not so much in writing the code (we "just" need another config mapper class) but in the Information Architecture. In fact, having thought about this, I remember discussing this problem with @webflo and @yched (and @Bojhan?) all the way back in Prague. It was just never resolved and then forgotten.

The problem is the following: Because the configuration translations are not only accessible through the nice list at /admin/config/regional/config-translation but they are integrated with each business object's actual edit UI, i.e. as a tab next to the edit form.

Entity view|form displays are already *secondary* tabs under the first level of Manage fields | Manage display | Manage form display level. So adding a Translation tab to each display would require a *third* level of tabs, which currently are simply not possible with our information architecture (nor would they probably present good usability, if implemented). So in essence the question is: Where should the "Translate" links go for entity view|form displays? (And the Config Translation overview is not the correct answer ;-))

That's not to say we shouldn't do this, just to say it's not that trivial.

jhodgdon’s picture

Issue summary: View changes

I wondered if this problem was more widespread, so I went to
a) admin/config/development/configuration/single/export (single config export)
vs.
b) admin/config/regional/config-translation (config translation)
after enabling all the core modules, to see what config types are available for export but not on the translation page.

The list of things on (a) not on (b):
Base field override -- this is part of the Field UI problem on this issue, I think. Adding to summary.
Content Language Settings [OK, not translatable I think]
Entity form display -- already part of this issue
Entity view display -- already part of this issue
Field storage [OK, not translatable I think]
Migration [I don't have any defined, so that may be why it isn't listed on Config UI]
RDF Mapping [may not be translatable?]
Text editor [probably not translatable?]
Tour [There is no UI for tours to create them in the UI either, so they all come from config/install or config/optional so ... maybe OK?]

jhodgdon’s picture

Regarding the "prefix" and "suffix" fields on the integer fields, in core/config/schema/core.data_types.schema.yml I find

field.field_settings.integer:
  type: mapping
  label: 'Integer'
  mapping:
...
    prefix:
      type: string
      label: 'Prefix'
    suffix:
      type: string
      label: 'Suffix'

So it looks like those are marked as type "string" which I am pretty sure means translatable, right?

jhodgdon’s picture

Issue summary: View changes

Editing issue summary to clarify form mode vs. form display, whoops.

tstoeckler’s picture

Re #5:
The general rule of thumb is that translation UIs are bound to editing UIs. So if there's no way to edit something, we also provide no way to translate it.

Let me go through the list in detail:

Base field overrides
There is not a generic UI for these, so it's OK that there is no generic integration. However, when editing node types base field overrides get created and updated, so the node type translation form should add the respective config names so that they are added. In effect this means that the custom "Title label" that you can set per-node-type is not translatable at the moment. This is a great catch, wow, awesome work! This should be fixed in a separate issue, however, will open one.
Content language settings
These do not have a dedicated UI (instead all content language settings are managed on a single page, namely /admin/config/regional/content-language) so we would need custom integration for these as well. There is no translatable data contained in these, so this is not necessary with just core. However, they are extensible via third party settings, so it's absolutely possible that a contrib module extends these with translatable settings. So we should open a separate issue where we can discuss whether we want to support that. (I would say yes, but I suppose it's arguable.) Again, awesome find. No one would have ever thought about that!
Entity form/view display
See #4, will be fixed here.
Field storage
Can in fact contain translatable values but was already made translatable recently in #2409701: Field storage configuration is not exposed to config translation UI. The translatable fields of field storages get munged together with that of the field itself on the field translation form, so there is no dedicated UI for this. (But again, this already works.) You can try this out by adding a List field and entering some allowed values with labels. You will then be able to translate those labels as part of the field translation.
Migration
Does not have a UI in core, so it would be up to a contrib Migration UI module to provide integration for this (given it would not be automatically given by Config Translation's generic integration).
RDF Mapping
Same argument: No edit UI -> no translation UI.
Text editor
These get edited on the same page as text formats, so editor module should add the respective config item(s) to the text format translation page. It currently does not do that, however, which should be fixed in a separate issue. I'm not aware of any translatable editor settings in core, but this is not at all far-fetched to imagine in contrib. (Think of allowing to configure the 'title' attribute of editor buttons, for example, you would totally want to translate that.) Again, very impressive find. This is something that I presume would have been discovered ages after release (if at all), otherwise.
Tour
Again, no edit UI -> no translation UI. I will check but I would assume that Tour UI in contrib benefits from the automatic integration, however.

So in short:
A) I will open issues for

  1. Node type base field overrides
  2. Content language settings
  3. Text editors

B)
You are awesome! Really, really awesome!

Re #6:
No string is in fact not translatable, because it could be used for all kinds of things (i.e. an HTML class name, a URI, ...) and thus we cannot assume it to be translatable. If they should be in fact translatable (they should!) they should declare type: label. Yes, that's horribly unintuitive because these are not labels, but that's what it is.

We should open another issue for that one.

Berdir’s picture

No string is in fact not translatable, because it could be used for all kinds of things (i.e. an HTML class name, a URI, ...) and thus we cannot assume it to be translatable. If they should be in fact translatable (they should!) they should declare type: label. Yes, that's horribly unintuitive because these are not labels, but that's what it is.

AFAIK, label is just a string with the translatable flag set to true by default. I'd assume you can also simply keep type string and set translatable to true yourself? label should maybe have been named translatable or something like that, but too late for that ;)

tstoeckler’s picture

I'd assume you can also simply keep type string and set translatable to true yourself?

In terms of the pure config schema, yes, that works fine. Config Translation (i.e. the automatic form generation) will break in that case, however. I realized that recently because I was planning to update type: label to type: string with translatable: true just like #9 says all over core, because label is just very confusing (and incorrect). So as of now, we're stuck with type: label. We should open a bug report against Config Translation, however, that it doesn't support setting type: translatable manually.

jhodgdon’s picture

Title: Entity view/form mode field settings, and base field settings, have no translation UI » Entity view/form mode formatter/widget settings have no translation UI
Issue summary: View changes

OK then we need another issue for the prefix settings not being marked as translatable. Here:
#2546356: Numeric field prefix/suffix settings should be translatable
Updating summary.

jhodgdon’s picture

Regarding how to actually solve *this* issue, which is the lack of translatability for the widget and formatter settings (and is it possible that you'd want to also change which fields are displayed vs. hidden by language? Possibly...)... Basically we need a place to translate the Form and View displays.

So one thought is that currently if you are looking at a content type, you have tabs for:
Edit | Manage fields | Manage form display | Manage display | Translate

It seems like it might make sense for the Translate tab to have the ability to translate *all* of the other tabs? Right now if you click Translate, you only get to translate the content type (same as what you get on Edit). But maybe that page could give you a dashboard, showing all the translatable stuff related to this content type:
- Fields - both the storage settings and the instance settings [currently you can translate field instance settings from the Manage Fields tab, but not storage settings, as noted above]
- Form displays with their widget settings
- View displays with their formatter settings
- The content type name/description/etc. itself [which is all you currently get from Translate]

I mean, if I were presented with these tabs, and I knew I was trying to translate something, I would go to Translate first to try to find it, wouldn't you?

Also it seems like if you are translating a form/view display, it would probably be good to translate it all on one page. Kind of like the translate interface for a View -- it has a hierarchy of stuff that you can translate, starting with the various displays, then within that the fields, sorts, etc.

Gábor Hojtsy’s picture

@jhodgdon: I think when you go to the config translation overview, you at admin/config/regional/config-translation you also expect field settings translations to show under field settings (will not necessarily look at content type translation). Same when you go edit a field setting and see the translate tab there. The thing is config translation was architected to work with the units of config where they are edited. So where the field settings edit form is, you'll find the field setting translation tab. This aligns well with content translation. We made some small adjustments where the UI would have been horrific (eg. the field storage config and field settings both translated on the same screen). That config are differently structured is indeed a backend design artifact, views are all in one file while view modes and field settings are distinct. But then the same field/widget is used for input even if there are 5 different view modes, so making it look like you can translate them 5 ways would be misleading, no?

jhodgdon’s picture

OK...

Well right now we have a problem because there are all these things that don't have a translate tab at all:
- Field storage settings - the Edit for these is attached to the Operations list for a field on Manage Fields. But there is only one "Translate" there and it is for the field instance settings.
- Entity view displays with formatters - the Edit for these is in a tab group with the Edit for the content type settings, and again there is only one "Translate" there and it is for the content type settings.
- Entity form displays with widgets - ditto

So we need to make the additional "Translate" links/tabs/whatever available *somewhere*. Maybe the best thing to do would be to add additional Translate to the actions list or tabs list in these cases?

So for instance on the Field operations list, it would be
Edit, Storage settings, Translate, Translate storage settings, Delete

And for the form/view displays, the Content Type tabs would be
Edit, Manage fields, Manage form display, Manage display, Translate content type, Translate form display, Translate display

???

Gábor Hojtsy’s picture

Field storage settings was supposed to be resolved in #2409701: Field storage configuration is not exposed to config translation UI, that should already be there. Sounds like then Entity view displays with formatters and Entity form displays with widgets can be folded into the content type translate screen as a first attempt? There can be quite a few of them :/ That is my best idea so far. Let's try that IMHO and see.

Gábor Hojtsy’s picture

Ok, I wanted to look into this. How do we tell the location of the UI for the parent entity of an entity display?

jhodgdon’s picture

Field UI module has a way to figure out where to put its Manage Display etc. settings, so we should be able to piggy-back on that?

tstoeckler’s picture

Re #16:
So that's a pretty good question: So Node declares a field_ui_base_route which points to entity.node_type.edit_form. So from there we can get to the NodeType entity type and find it's edit form. I think this proves, however, that this is architecturally not a very sound approach. Because the field_ui_base_route can in theory point anywhere. In core it's mostly used for the edit form of bundle entity types, but I'm not sure we should be baking that knowledge into the system. Since the displays are provided by field_ui itself, I think the translation page should be as well. Maybe we can get away with a separate Translate displays tab (instead of splitting Translate displays & Translate form displays) ??

Gábor Hojtsy’s picture

@tstoeckler: but we already have a translate tab and so many other tabs :/

tstoeckler’s picture

Re #19: That's a valid point. I'll have a go at your suggestion.

BTW: I re-checked that the list in #5 / #8 is in fact exhaustive. And I opened issues for the missing things, see the Related issues section here.

tstoeckler’s picture

Status: Active » Postponed

Found #2571407: Config translation forms cannot be altered sanely, postponing for now. We can still discuss the actual implementation, though now.

Trying this out, I found that this will actually not work for user displays, as those do not have the equivalent of the "Translate content type". So not really sure, how to proceed here.

Also we will almost certainly be needing #2565031: Expose $entity in ConfigEntityMapper here.

Gábor Hojtsy’s picture

Status: Postponed » Active

@tstoeckler why would we need to alter the forms? We can add the additional config keys to the mappers with:

/**
 * Alter existing translation tabs for translation of configuration.
 *
 * This hook is useful to extend existing configuration mappers with new
 * configuration names, for example when altering existing forms with new
 * settings stored elsewhere. This allows the translation experience to also
 * reflect the compound form element in one screen.
 *
 * @param array $info
 *   An associative array of discovered configuration mappers. Use an entity
 *   name for the key (for entity mapping) or a unique string for configuration
 *   name list mapping. The values of the associative array are arrays
 *   themselves in the same structure as the *.config_translation.yml files.
 *
 * @see hook_translation_info()
 * @see \Drupal\config_translation\ConfigMapperManagerInterface
 */
function hook_config_translation_info_alter(&$info) {
  // Add additional site settings to the site information screen, so it shows
  // up on the translation screen. (Form alter in the elements whose values are
  // stored in this config file using regular form altering on the original
  // configuration form.)
  $info['system.site_information_settings']['names'][] = 'example.site.setting';
}
tstoeckler’s picture

Assigned: Unassigned » tstoeckler

Working on this with @jan.stoeckler and @mmaihoff at Barcelona now...

tstoeckler’s picture

Oh, and thanks @Gábor Hojtsy for the hint, totally was unaware. I still think it's weird that we alter the "dumb" arrays instead of our neat mapper objects themselves, but that's out of scope here...

Gábor Hojtsy’s picture

Yeah the idea is you usually alter dumb arrays in a form alter as well, so a "similar" alter for the config form sounds like a match. There are some core things, where we are not "altering" forms per say, eg. #2571337: Node type title label cannot be translated in the UI is baked into the content type form, not altered in, but its still from a different config file.

tstoeckler’s picture

Status: Active » Postponed

@Gábor Hojtsy: We're now blocked on #2571655: ConfigNamesMapper::hasTranslatable has flawed logic now, because we need to add config names which - with just core alone - do not have any translatable data, but might get translatable data with contrib modules installed (such as FieldGroup). We could work around it maybe, but I don't think that makes a lot of sense, that's something that needs fixing if we want to provide a flexible and usable system for contrib IMO.

We hit another pretty large roadblock, though: Taking the example of node types, we only want to attach displays of the "Article" node type to the "Article" translation page. Which node type we are translating, however, gets determined dynamically in populateFromRequest() and setEntity(). That whole logic cannot be reached from other modules, however, so we need to add another mechanism there.

Not really sure what to do.

Gábor Hojtsy’s picture

That sounds like is where #2565031: Expose $entity in ConfigEntityMapper would in fact come in handy! I praise your visionary mind :)

tstoeckler’s picture

Title: Entity view/form mode formatter/widget settings have no translation UI » [PP-2] Entity view/form mode formatter/widget settings have no translation UI

Re #27 that's correct. However, that still doesn't solve the issue. We still need some sort of mechanism to dynamically add config names.

#2571655: ConfigNamesMapper::hasTranslatable has flawed logic is postponed on #2295255: Convert ConfigMapperInterface::populateFromRequest() to use RouteMatch so updating title

tstoeckler’s picture

tstoeckler’s picture

tstoeckler’s picture

Title: [PP-2] Entity view/form mode formatter/widget settings have no translation UI » [PP-1] Entity view/form mode formatter/widget settings have no translation UI

Don't really see what the second issue is supposed to be. Once we have #2577761: We need a way to dynamically alter the list of config names for config mappers, this is easy-peasy ;-P

tstoeckler’s picture

Issue summary: View changes
Status: Postponed » Needs review
Issue tags: +Needs tests
FileSize
4.9 KB
40.43 KB

Since we now have the [PP-*] title system, I can actually upload a patch for this and only mildly confuse people, so here goes...

I rolled a patch on top of #2577761: We need a way to dynamically alter the list of config names for config mappers. Because that is not yet super finalized I did not include that here in order not to distract people, but this does actually work, as the screenshot shows. The screenshot also proves that we actually have something in core that is currently not translatable: form placeholders. Those are widget settings, which end up in the form display configuration. So that underlines the major-ness of this issue (at least!). The screenshot also shows that we have some usability problems currently, I did not yet investigate how we can improve that.

A screenshot showing a node type translation form with a 'Field widgets' details element below the regular form elements which in turn contains a 'Field widget' details element which in turn contains a 'Einstellungen' (German for 'Settings') details element which then contains a 'Placeholder' textfield element.

In any case, this could now use some code (and usability) reviews. If you apply this on top of (any of the) latest patch(es) in #2577761: We need a way to dynamically alter the list of config names for config mappers you should get the same UI, that I did.

Edit: underlines, not undermines!

jhodgdon’s picture

The placeholders in widgets are mentioned in the issue summary. ;) And hopefully you meant "underlines the major-ness" not "undermines the major-ness" of the issue. !! :)

So, one of the things we were stumbling on for this issue was where to put the link for translating. A screen shot of what you did for that would be helpful!

tstoeckler’s picture

1. Totally missed the placeholders part up to now, sorry about that!

2. Yes, under*lines*!!! (Sorry)

3. I went with the suggestion above to add this to the node type translation Form, I.e. you reach this from the "Translate" contextual link on the content type overview page or from the "Translate content type" local task when you're editing the content type or on Manage fields, etc.

(Sorry, on phone...)

jhodgdon’s picture

I'm tested this on simplytest.me, with patches from #33 here, on top of the latest patch from #2577761: We need a way to dynamically alter the list of config names for config mappers, as well as the latest patch on #2545730: Misuse of formatPlural() in Numeric field prefix/suffix (which adds translatable settings to the Formatter as well)... will post screen shots shortly.

jhodgdon’s picture

Hm. I couldn't install on simplytest.me. It got hung up somewhere around installing the Contextual Links or Configuration Manager module (I tried twice and the batch message was one of those each time)... When I stopped it it redirected to a white screen with this message on the error page:
Fatal error: Class 'Drupal\config_translation\Event\ConfigTranslationEvents' not found in /home/dfxvv/www/core/modules/field_ui/src/ConfigTranslation/EntityDisplaySubscriber.php on line 88

So, not sure how to test.

tstoeckler’s picture

Hmm.. sorry about that, will roll a combined patch then

tstoeckler’s picture

FileSize
23.52 KB

Not sure about the missing class, but I did find a problem. In an earlier local version of EntityDisplaySubscriber I needed the entity manager, but then realized I didn't anymore before rolling the patch, so I ripped it out, but left it in the service declaration, leading to a fatal upon service construction. This fixes that.

This also includes #2577761-29: We need a way to dynamically alter the list of config names for config mappers, so the screenshot in #32 should be reproducible without any further modifications.

Status: Needs review » Needs work

The last submitted patch, 38: 2546212-38.patch, failed testing.

maxocub’s picture

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

I updated your patch with the latest one from #2577761: We need a way to dynamically alter the list of config names for config mappers, just to have less failling tests.

Status: Needs review » Needs work

The last submitted patch, 40: 2546212-40.patch, failed testing.

maxocub’s picture

I think the missing class mentionned in #36 is because config_translation module is not enabled while field_ui is referencing a config_translation event.

If config_translation is activated beforehand, the error goes away.

maxocub’s picture

I found that when you go to the translation form of a field, you get the configuration of all the fields from all the content types, like here in the translation form of the 'Body' field of the 'Article' content type:

 add_french_translation_for_body.png

One way to avoid that, I think, would be to check if the base route name is 'entity.node_type.edit_form'

Gábor Hojtsy’s picture

@maxocub: that looks unintended / unfortunate indeed.

Gábor Hojtsy’s picture

I think most of the patch is the same as #2577761: We need a way to dynamically alter the list of config names for config mappers the unique part is only at the bottom right? Reviewing that:

  1. +++ b/core/modules/field_ui/src/ConfigTranslation/EntityDisplaySubscriber.php
    @@ -0,0 +1,91 @@
    +    if ($entity_type_id === $entity->getEntityType()->getBundleOf()) {
    +      $bundle = $entity->id();
    +    }
    

    Should the bundle be the entity id? That sounds confusing...

  2. +++ b/core/modules/field_ui/src/Routing/RouteSubscriber.php
    @@ -45,6 +45,12 @@ protected function alterRoutes(RouteCollection $collection) {
    +        // Set a route option to easily identify all Field UI base routes and to
    +        // determine the entity type the route is for.
    +        $entity_route->setOption('_field_ui_base_route', $entity_type_id);
    

    Should it be _field_ui_base_route_for or something like that? The value is not really a route.

  3. +++ b/core/modules/field_ui/src/Routing/RouteSubscriber.php
    @@ -53,7 +59,11 @@ protected function alterRoutes(RouteCollection $collection) {
    -        // Special parameter used to easily recognize all Field UI routes.
    +        // Set a route option to easily identify all Field UI base routes and to
    +        // determine the entity type the routes are for.
    +        $options['_field_ui_route'] = $entity_type_id;
    

    Not the same name as above. Also not a route name(?)

maxocub’s picture

About the missing class mentionned in #36:

Fatal error: Class 'Drupal\config_translation\Event\ConfigTranslationEvents' not found in /home/dfxvv/www/core/modules/field_ui/src/ConfigTranslation/EntityDisplaySubscriber.php on line 88

It's caused by the fact that config_translation is not enabled while field_ui tries to subscribe to a config_transaltion event. I tried to find a way to subscribe to the event only if config_translation is enabled but I haven't found a way yet.
In the static method getSubsctibedEvents(), neither \Drupal::moduleHandler() or $this->container->get('module_handler') are available. Anyone have an idea how to avoid the missing class?

tstoeckler’s picture

So this is a bit tricky...

So instead of the entry in field_ui.services.yml which the latest patch adds we need to implement a service provider in field_ui module which dynamically registers the event subscriber if the config_translation module is available. See http://cgit.drupalcode.org/file_entity/tree/src/FileEntityServiceProvide... for an example in contrib that does something similar. But instead of what is being done there we probably want to use $container->register() to reflect what is currently being done in the YAML file.

maxocub’s picture

Status: Needs work » Needs review

I replace the service definition in field_ui.services.yml by a ServiceProvider to subscribe to config_translation's event only if it's enable.
The missing class should be gone now.
Thanks for your help @tstoeckler!

maxocub’s picture

Forgot the files...

maxocub’s picture

Issue tags: +SprintWeekend2016
tstoeckler’s picture

I know me saying this has gotten old at this point, but you are quite awesome @maxocub, thanks a lot!

One minor quibble:

+      $service_definition = new Definition('\Drupal\field_ui\ConfigTranslation\EntityDisplaySubscriber', array(
+        new Reference('config.factory'),
+      ));
+      $service_definition->addTag('event_subscriber');
+      $container->setDefinition('field_ui.config_translation_subscriber', $service_definition);

I think it is a little more intuitive to use $container->register() instead of $container->setDefinition(). setDefinition() is useful when needing to override an existing definition (i.e.

$container->getDefinition(); // Do something. $container->setDefinition();) but here we're providing a new service, so I would suggest something along the lines of this (untested!) example code:
<code>
$container->register('field_ui.config_translation_subscriber', '\Drupal\field_ui\ConfigTranslation\EntityDisplaySubscriber')
  ->addArgument(new Reference('config.factory'))
  ->addTag('event_subscriber');

But this is only a minor point. Let's see if the patch is green - if so, we are really close here, I think.

maxocub’s picture

I see your point, it's more intuitive this way, let's do that.

maxocub’s picture

Status: Needs review » Needs work
maxocub’s picture

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

@Gábor Hojtsy, re #45: Yes, the changes in this issue are only in the field_ui files, at the bottom of the patch.

1)

$bundle = $entity->id();
Should the bundle be the entity id? That sounds confusing...

When I print the $entity->id() of a node type here, I get 'page' or 'article', so I guess it's OK to call the variable $bundle?

2)

$entity_route->setOption('_field_ui_base_route', $entity_type_id);
Should it be _field_ui_base_route_for or something like that? The value is not really a route.

What about $entity_route->setOption('_entity_type_id', $entity_type_id); since we are assigning it the entity type id?

3)

$options['_field_ui_route'] = $entity_type_id;
Not the same name as above. Also not a route name(?)

Same comment as in 2)

jhodgdon’s picture

RE #55 item 1 -- The confusion is because in this case, $entity is an entity bundle config entity (NodeType, for example), not the content entity for the entity item itself (Node, for example).

jhodgdon’s picture

In other words, an in-code // comment stating this might clear up some confusion. ;)

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.

maxocub’s picture

Status: Needs work » Needs review
FileSize
24.41 KB
3.78 KB
5.6 KB

Here some new files!

1) A combined patch with the most recent one from #2577761: We need a way to dynamically alter the list of config names for config mappers
2) An interdiff showing changes addressing #45 and correcting the problem mentioned in #43
3) A patch with only the changes for this issue, for review

tstoeckler’s picture

The interdiff looks good to me, thanks!

Regarding the route options, I think the "_for" makes it a bit more clear, so ++.

Coming back to this now I have a different suggestion. We already have _field_ui but we now need to distinguish between the routes added by Field UI itself and the route that Field UI attaches itself to. In addition we need to access the entity type ID of the entity type. So maybe we could do:

$route->setOption('_field_ui_base_route', TRUE);
$route->setOption('_field_ui_entity_type_id', $entity_type_id);

for the base routes and

$route->setOption('_field_ui_route', TRUE);
$route->setOption('_field_ui_entity_type_id', $entity_type_id);
// Only for BC
$route->setOption('_field_ui', TRUE);

for the other routes.

I think that's a little bit more self-documenting that the current "_for" suffix and I also think we should go with "_field_ui_entity_type_id" instead of just "_entity_type_id" to keep the route options properly namespaced.

Thoughts?

maxocub’s picture

I think your suggestion is great, it would be more clear to separate the information in two separate options.

One question though, AFAIK, the options we set on other routes (not the base ones) are not used right now, are we setting them just in case we need them one day?

tstoeckler’s picture

Yeah, I think that was the idea. Or for contrib to use, so that it does not have to perform the same checks.

maxocub’s picture

FileSize
24.57 KB
5.76 KB
2.46 KB

Here are the changes suggested in #60.

Berdir’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/field_ui/src/ConfigTranslation/EntityDisplaySubscriber.php
    @@ -0,0 +1,95 @@
    +    if (
    +      !($mapper instanceof ConfigEntityMapper) ||
    +      !($base_route->hasOption('_field_ui_base_route'))
    +    ) {
    +      return;
    +    }
    

    As pointed out above, this currently doesn't work yet for user or any other entity type that doesn't have bundles.

    Unless I'm missing something, I think that would actually be pretty easy to solve. We already have field_ui_base_route and can use that in config_translation_config_translation_info() (or field_ui, I don't care :)) by exposing a mapper for every entity type without bundles but with field_ui?

  2. +++ b/core/modules/field_ui/src/ConfigTranslation/EntityDisplaySubscriber.php
    @@ -0,0 +1,95 @@
    +    // Determine the bundle (if any) of the route.
    +    $bundle = NULL;
    +    if ($entity_type_id === $entity->getEntityType()->getBundleOf()) {
    +      // Here, $entity is an extension of
    +      // \Drupal\Core\Config\Entity\ConfigEntityBundleBase,
    +      // so the entity's ID is a bundle name.
    +      $bundle = $entity->id();
    +    }
    +    elseif ($base_route->getDefault('bundle')) {
    +      $bundle = $base_route->getDefault('bundle');
    +    }
    +
    +    // Add all form and view displays of the respective entity type and bundle
    +    // (if any) to the mapper.
    +    foreach (['entity_form_display', 'entity_view_display'] as $display_type) {
    +      $prefix = "core.$display_type.$entity_type_id.";
    +      if ($bundle) {
    +        $prefix .= "$bundle.";
    +      }
    

    The bundle logic here is wrong.

    a) There is always a bundle, if there's no bundle, then the bundle is the entity type ID.

    b) You're coming from a config entity mapper. I honestly don't see how, in that scenario, you could *not* have a bundle or a bundle entity type. Because you are exactly that? Having a check makes sense if someone does something weird, but I'd just return if the first if doesn't match.

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.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
24.57 KB
2.07 KB
126.87 KB

How about this? Just checked few sample pages and seem to be OK.

Status: Needs review » Needs work

The last submitted patch, 66: 2546212-63.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
24.39 KB
2.07 KB

Wrong patch...

Status: Needs review » Needs work

The last submitted patch, 68: 2546212-68.patch, failed testing.

vijaycs85’s picture

exception: [Notice] Line 208 of core/modules/config_translation/src/Form/ConfigTranslationFormBase.php:
Undefined index: core.entity_view_display.node.znsgv4n5pfxzmhzg.default

exception: [Notice] Line 208 of core/modules/config_translation/src/Form/ConfigTranslationFormBase.php:
Undefined index: core.entity_view_display.node.znsgv4n5pfxzmhzg.teaser

exception: [Notice] Line 208 of core/modules/config_translation/src/Form/ConfigTranslationFormBase.php:
Undefined index: core.entity_view_display.node.fhhv55g7gahyqmj9.default

exception: [Notice] Line 208 of core/modules/config_translation/src/Form/ConfigTranslationFormBase.php:
Undefined index: core.entity_view_display.node.fhhv55g7gahyqmj9.teaser

all notices are related to view_display which doesn't have any field to translate. For now I leave it for review and if this approach is Ok to proceed.

ckaotik’s picture

I've tried the patch in #68 and noticed some problems, which have already been mentioned before. My test case was a plain node type (ootb page) for which I've added a formatted text field, included in both form mode and the default view mode.

1. Drupal\config_translation\Form\ConfigTranslationFormBase::submitForm() causes Notice: Undefined index: core.entity_view_display.node.page.default notices for both "default" and "teaser" view modes (but not the form mode) for which no configuration is available. The translation data for the form mode is still saved and displayed in the correct places.
Guess that's what ConfigNamesMapper::addConfigName() refers to:

Note that it is the responsibility of the calling code to ensure that the configuration exists.

2. The input fields within the translation UI are not labeled, in my test case I had 4 identical (empty) placeholder input fields (Field widgets > Field widget > Settings > Placeholder), with no idea whatsoever to which field they belong.
Is there a way to add the field names and/or avoid the deep nesting?

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.