Problem/Motivation

Add missing configuration translation interface.

There are several pieces of information from the Entity/Field system, which may include translatable strings, but which prior to this patch 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 #2545730: Misuse of formatPlural() in Numeric field prefix/suffix 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.

How to test

  1. Use core version applicable to the patch and apply the patch
  2. Configure Article node type with a boolean field, displayed in Default view mode using custom texts
  3. Enable Configuration Translation module (/admin/modules)
  4. Add at least one language (/admin/config/regional/language)
  5. Go to configuration translation page (/admin/config/regional/config-translation)
  6. Click on List button for Content view display
  7. Click on Translate button for Default view mode for Articlecontent type
  8. See what is on the configuration translation form for the ArticleDefaultview mode
  9. @todo - Add more steps for translating specific configuration

Proposed resolution

  1. Create a ConfigTranslation entity list builder extending ConfigTranslationFieldListBuilder to manage the translation of entity view and form modes and override the necessary methods to load, filter, and display entity view/form modes for translation.
  2. Extend the Config Entity Mapper in the Field UI module to handle translation mappings and titles for entity view and form modes and implement methods to set route parameters, generate overview route names, and generate titles specific to entity view/form modes.
  3. Alter translation forms. Iterate through the relevant form elements and associate translation-related details, such as titles and descriptions, for formatter and widget settings.

Remaining tasks

Review patch
Determine any follow ups
Commit

User interface changes

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

API changes

No

Data model changes

No

CommentFileSizeAuthor
#269 2546212-269.patch11.89 KBmanojbisht_drupal
#265 2546212-265.patch11.78 KBmanojbisht_drupal
#264 2546212-264.patch196.04 KBJonasanne
#254 layout_builder_before.png80.23 KBomkar.podey
#254 layout_builder_after.png80.85 KBomkar.podey
#251 with_2.png132.95 KBomkar.podey
#251 with_1.png222.3 KBomkar.podey
#251 without_2.png158.82 KBomkar.podey
#251 without_1.png162.19 KBomkar.podey
#244 drupal-n2546212-242-95x.patch24.28 KBDamienMcKenna
#243 drupal-n2546212-224-95x.patch22.44 KBDamienMcKenna
#243 drupal-n2546212-224-95x.patch22.44 KBDamienMcKenna
#229 2546212-nr-bot.txt90 bytesneeds-review-queue-bot
#224 interdiff-219-224.txt12.3 KBilya.no
#224 2546212-224.patch25.74 KBilya.no
#221 order.jpg104.02 KBrkoller
#219 interdiff-218-219.txt2.42 KBJose Reyero
#219 2546212-219.patch20.83 KBJose Reyero
#218 interdiff_215-218.txt8.07 KBAkram Khan
#218 2546212-218.patch20.73 KBAkram Khan
#215 patch-2546212-215-translate-form-display.png57.46 KBJose Reyero
#215 patch-2546212-215-translate-display-view.png62.54 KBJose Reyero
#215 interdiff-207-215.txt11.01 KBJose Reyero
#215 2546212-215.patch20.39 KBJose Reyero
#212 2546212-nr-bot.txt3.16 KBneeds-review-queue-bot
#207 2546212-207.patch14.1 KBsmustgrave
#207 interdiff-203-207.txt645 bytessmustgrave
#203 2546212-203.patch14.09 KBsmustgrave
#203 2546212-203-tests-only.patch3.07 KBsmustgrave
#203 interdiff-202-203.txt2.29 KBsmustgrave
#202 2546212-202.patch13.23 KBsmustgrave
#202 interdiff-168-202.txt3.59 KBsmustgrave
#196 translation.png114.14 KBvacho
#196 view-displays.png104.29 KBvacho
#193 2546212-193.patch11.92 KBAnchal_gupta
#179 Manage-form-display-Drupal-9.png182.24 KBvacho
#179 Add-Spanish-translation-for-Article-Default-form-display-Drupal-9.png94.94 KBvacho
#174 2021-04-19_18-20.png84.15 KBparijke
#168 2546212-168.patch11.91 KBckaotik
#168 2546212-168-actions.patch3.16 KBckaotik
#165 interdiff_163_165.txt671 bytesanmolgoyal74
#165 2546212_165.patch15.08 KBanmolgoyal74
#163 2546212_163.patch14.78 KBvsujeetkumar
#160 drupal-entity_display_mode_translation-2546212-160.patch14.75 KBckaotik
#160 interdiff-158-160.txt5.56 KBckaotik
#160 drupal-2546212-160.PNG9.37 KBckaotik
#158 2546212-158.patch13.3 KBDom.
#158 interdiff.txt3.01 KBDom.
#155 interdiff-2546212-119_to_155.txt774 bytesjoseph.olstad
#155 2546212-155.patch13.02 KBjoseph.olstad
#155 2546212-155-TEST-ONLY.patch1.56 KBjoseph.olstad
#153 2546212-153-TEST-ONLY.patch1.56 KBjoseph.olstad
#153 interdiff-2546212-119_to_153.txt772 bytesjoseph.olstad
#153 2546212-153.patch13.01 KBjoseph.olstad
#137 Screen Shot 2020-05-10 at 11.41.03 AM.png157.58 KBKristen Pol
#135 Screenshot 2020-05-07 at 09.25.02.png37.62 KBMartijn de Wit
#135 Screenshot 2020-05-07 at 09.23.15.png29.17 KBMartijn de Wit
#128 Screenshot_2020-03-17 Manage display.png10.32 KBsorlov
#126 Screenshot_2020-03-16 Manage display Example.png6.88 KBsorlov
#120 2546212-119-109-interdiff.txt2.51 KBmbovan
#119 2546212-119.patch13 KBmbovan
#119 2546212-119-TEST-ONLY.patch12.96 KBmbovan
#109 2546212-109.patch11.42 KBpenyaskito
#108 interdiff-2546212-103-108.patch2.56 KBmartin107
#108 2546212-108.patch11.4 KBmartin107
#103 interdiff-2546212-101-103.patch4.64 KBmartin107
#103 2546212-103.patch11.37 KBmartin107
#101 2546212-101.patch11.31 KBckaotik
#99 2546212-99.patch11.27 KBckaotik
#99 2546212-translate_display_config_list.png17.19 KBckaotik
#99 2546212-translate_display_local_task.PNG14.48 KBckaotik
#93 shot-2018-09-27_12-11-37.png191.44 KBTwoD
#89 2546212-89.patch6.16 KBandypost
#89 2546212-interdiff-88.txt1.84 KBandypost
#88 2546212-88.patch6.19 KBandypost
#85 Selection_319.png55.05 KBBerdir
#84 interdiff-2546212-82-84.txt6.62 KBjohnchque
#84 2546212-84.patch24.43 KBjohnchque
#84 2546212-84-no-test.patch8.27 KBjohnchque
#82 interdiff-2546212-74-82.txt2.51 KBjohnchque
#82 2546212-82.patch24.33 KBjohnchque
#74 2546212-74.patch24.54 KBpk188
#68 2546212-diff-63-68.txt2.07 KBvijaycs85
#68 2546212-68.patch24.39 KBvijaycs85
#66 2546212-66-1.png126.87 KBvijaycs85
#66 2546212-diff-63-66.txt2.07 KBvijaycs85
#66 2546212-63.patch24.57 KBvijaycs85
#63 interdiff-59-63.txt2.46 KBmaxocub
#63 2546212-63-do-not-test.patch5.76 KBmaxocub
#63 2546212-63-combined.patch24.57 KBmaxocub
#59 2546212-59-do-not-test.patch5.6 KBmaxocub
#59 interdiff-52-59.txt3.78 KBmaxocub
#59 2546212-59-combined.patch24.41 KBmaxocub
#52 interdiff.txt1.24 KBmaxocub
#52 2546212-52.patch24.13 KBmaxocub
#49 2546212-49.patch24.27 KBmaxocub
#49 interdiff.txt1.84 KBmaxocub
#19 Manage fields | drupal 8.0.x 2015-09-19 15-11-00.png185.33 KBGábor Hojtsy
#40 2546212-40.patch23.56 KBmaxocub
#32 2546212-32--do-not-test.patch4.9 KBtstoeckler
#40 interdiff.txt989 bytesmaxocub
#32 config-trans-entity-display.png40.43 KBtstoeckler
#38 2546212-38.patch23.52 KBtstoeckler
#43 add_french_translation_for_body.png89.93 KBmaxocub

Issue fork drupal-2546212

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Support from Acquia helps fund testing for Drupal Acquia logo

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? Afterthought: It would help A LOT if we could use placeholders such as [%key] within the label of a schema definition, instead of only for type keys.

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.

idebr’s picture

Issue tags: +Needs reroll
pk188’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
24.54 KB

Re rolled.

tacituseu’s picture

Could use a reroll with latest one from #2577761: We need a way to dynamically alter the list of config names for config mappers. When a patch depends on some other, it is good to post both '-combined.patch' and '-do-not-test.patch' versions (like #63).

Re #71.2: There seems to be some provision for this in core/modules/config_translation/src/FormElement/ListElement.php::getGroupTitle().
You can also add it in core/modules/config_translation/src/Form/ConfigTranslationFormBase.php::createFormElement().

tacituseu’s picture

There is an issue already for #71.2, unfortunately not much in it.

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

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dawehner’s picture

  1. +++ 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.
    + */
    

    🙃 Let's get rid of that

  2. +++ b/core/modules/field_ui/src/FieldUiServiceProvider.php
    @@ -0,0 +1,33 @@
    +/**
    + * @file
    + * Contains \Drupal\field_ui\FieldUiServiceProvider.
    + */
    

    🙃 Let's get rid of that

  3. +++ b/core/modules/field_ui/src/FieldUiServiceProvider.php
    @@ -0,0 +1,33 @@
    +      $container->register('field_ui.config_translation_subscriber', '\Drupal\field_ui\ConfigTranslation\EntityDisplaySubscriber')
    

    🙃 Could you use EntityDisplaySubscriber::class instead?

  4. +++ b/core/modules/field_ui/src/Routing/RouteSubscriber.php
    @@ -43,12 +43,25 @@ protected function alterRoutes(RouteCollection $collection) {
    +        // Set route options 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', TRUE);
    +        $entity_route->setOption('_field_ui_entity_type_id', $entity_type_id);
    

    I like the namespacing here

  5. +++ b/core/modules/field_ui/src/Routing/RouteSubscriber.php
    @@ -43,12 +43,25 @@ protected function alterRoutes(RouteCollection $collection) {
    +        // RouteCollection::add() overwrites exiting routes with the same name.
    +        $collection->add($route_name, $entity_route);
    

    Given that $entity_route is an object already, can't we skip the overriding completely? It will just change?

tstoeckler’s picture

Assigned: tstoeckler » Unassigned

Closed #2925584: Custom boolean field is not translatable as duplicate.

Also noticed this is still assigned to me, changing that now.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Anybody’s picture

#2980929: Link text is not translatable is also an example where a translation is needed.

johnchque’s picture

Addressing suggestions from #78.

martin107’s picture

In #82 .. looking at the patch

There are more @file tags, that need to be removed.

johnchque’s picture

Rebasing with the latest patch on #2577761: We need a way to dynamically alter the list of config names for config mappers and removing these @file tags. :) My bad, didn't check them before.

Berdir’s picture

FileSize
55.05 KB

Confirmed that this is working now.

The labels are indeed very generic, but I think that's hard to improve as it is done based on the fairly generic config schema, we don't support dynamic labels there. If I understand correctly then the event doesn't allow to alter the schema itself, just where it can be edited:

So each field is just "Field Widget" and "Field Formatter" and each form/view display is just the plural of that, so finding the right field in the right view display is a lot of fun.

I initially thought that third party settings of widgets/the whole display aren't working. But turns out that both examples that I was testing with didn't have their schema defined properly (looking at you field_group and maxlength! ;) If you switch string to label there it works just fine :)

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

tacituseu’s picture

Title: [PP-1] Entity view/form mode formatter/widget settings have no translation UI » Entity view/form mode formatter/widget settings have no translation UI
Issue tags: +Needs reroll
andypost’s picture

andypost’s picture

FileSize
1.84 KB
6.16 KB

A bit of clean-up

Wim Leers’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/field_ui/src/ConfigTranslation/EntityDisplaySubscriber.php
    @@ -0,0 +1,87 @@
    +    if (!($base_route->hasOption('_field_ui_base_route'))) {
    
    +++ b/core/modules/field_ui/src/Routing/RouteSubscriber.php
    @@ -43,12 +43,25 @@ protected function alterRoutes(RouteCollection $collection) {
    +        $entity_route->setOption('_field_ui_base_route', TRUE);
    

    This new route option is necessary 👍

  2. +++ b/core/modules/field_ui/src/ConfigTranslation/EntityDisplaySubscriber.php
    @@ -0,0 +1,87 @@
    +    $entity_type_id = $base_route->getOption('_field_ui_entity_type_id');
    
    +++ b/core/modules/field_ui/src/Routing/RouteSubscriber.php
    @@ -43,12 +43,25 @@ protected function alterRoutes(RouteCollection $collection) {
    +        $entity_route->setOption('_field_ui_entity_type_id', $entity_type_id);
    ...
    +        $options['_field_ui_entity_type_id'] = $entity_type_id;
    

    So is this one 👍

  3. +++ b/core/modules/field_ui/src/FieldUiServiceProvider.php
    @@ -0,0 +1,28 @@
    +    if (isset($modules['config_translation'])) {
    +      // Add only when config_translation module enabled.
    

    Pointless comment?

  4. +++ b/core/modules/field_ui/src/FieldUiServiceProvider.php
    @@ -0,0 +1,28 @@
    +      $container->register('field_ui.config_translation_subscriber', EntityDisplaySubscriber::class)
    

    Actually, why only register this if that module is installed? Why not do it always; the cost of having a event subscriber service that is never invoked is tiny?

  5. +++ b/core/modules/field_ui/src/Routing/RouteSubscriber.php
    @@ -43,12 +43,25 @@ protected function alterRoutes(RouteCollection $collection) {
    +        // This is retained for backwards-compatibility.
    +        // @todo Remove in Drupal 9
             $options['_field_ui'] = TRUE;
    

    Let's create an issue for this and link to it.

tstoeckler’s picture

Thanks for the review!

Re 4.: Because the event name is provided by Config Translation, so not registering the service conditionally yields a fatal in getSubscribedEvents().

Wim Leers’s picture

👌 Of course!

P.S.: #2571371: Editor settings cannot be translated is a sister issue of this, and is now also unblocked!

TwoD’s picture

FileSize
191.44 KB

I may have done something wrong here when trying to test this (applied #2577761: We need a way to dynamically alter the list of config names for config mappers and #89 to an 8.5.6 site) but the result looks a bit odd and confusing to me. Are the actual field widget/formatter labels supposed to be shown here?
Content Translation UI with fields

tstoeckler’s picture

Re #93: Actually you did everything correctly. We are know hitting (exceeding?) the limits of the Config Translation form building. At the moment, that's just how it works, and I'm not aware of any low-hanging quick fixes to improve it. We already have some more larger scoped issues to improve it, but they are - at least codewise - independent of this issue. I think the original plan was for this to go in as is, and then consider some improvements to the form in follow-ups, but (unless I'm misremembering) the last time this was discussed explicitly with committers and the UX team was in DrupalCon Prague (!), so I guess things may have changed in that regard... ;-)

Gábor Hojtsy’s picture

Hm, why exactly is that not possible? First this needs an issue summary update so we are clear on scope and what things are deferred.

tacituseu’s picture

Re #93: Looked into it some time ago (see #75), ended up adding:

diff --git a/core/modules/config_translation/src/Form/ConfigTranslationFormBase.php b/core/modules/config_translation/src/Form/ConfigTranslationFormBase.php
index 9f95a76..e7d10d7 100644
--- a/core/modules/config_translation/src/Form/ConfigTranslationFormBase.php
+++ b/core/modules/config_translation/src/Form/ConfigTranslationFormBase.php
@@ -241,6 +241,7 @@ public static function createFormElement(TypedDataInterface $schema) {
       if (!$definition->getLabel()) {
         $definition->setLabel(t('n/a'));
       }
+      $definition->setLabel($definition->getLabel() . ' [' . $schema->getName() . ']');
       $class = $definition['form_element_class'];
       return $class::create($schema);
     } 

to make some sense of it.

TwoD’s picture

@tacituseu Ah, thank you! That makes it so much easier (a proper patch for this would be great, but it should use placeholders as the UI translation string source table now gets polluted with one string per field).

tacituseu’s picture

@TwoD: this is just a development band aid, core/modules/config_translation/src/FormElement/ListElement.php::getGroupTitle() felt/read more like a proper way/place to fix it to me, I'm just not familiar enough with this subsystem to be able to make something usable in reasonable time and hence went with the above.

ckaotik’s picture

Are the actual field widget/formatter labels supposed to be shown here?

For the record, "here" means on e.g. /admin/structure/types/manage/page/translate. I've written a patch using the existing config translation API similarly to what field_ui does, that places the translation UI somewhere closer to what one might expect them: Under /admin/config/regional/config-translation and also as a tab on the actual view/form displays (e.g. /admin/structure/types/manage/page/form-display/default/translate).

Local task to translate currently visible entity display mode
Config list for display modes grouped by entity type

There are still some issues, though:

  1. The nested structure with pretty much useless group labels still persists
  2. The local task on view/form displays applies contextually to the display currently being configured, which is unusual UX within Drupal
  3. The code sometimes relies on specific entity type ids - this is due to EntityDisplayBase's protected displayContext property
  4. The local task on view/form displays only shows up if the display has something translatable
  5. There will be issues if a view/form mode is named translate (wich results in a base path of /some/path/display/translate, identical to the default mode translation path)
  6. I'd love to use config-translation-overview.{$entity_type_id} link templates like config_translation_entity_operation does, but couldn't get it to work. That way we wouldn't need to override in ConfigTranslationEntityDisplayListBuilder::getOperations
  7. I've not fully tested how this behaves with disabled field_ui

Status: Needs review » Needs work

The last submitted patch, 99: 2546212-99.patch, failed testing. View results

ckaotik’s picture

Status: Needs work » Needs review
FileSize
11.31 KB

Rerolled of the proper development version, let's hope it applies cleanly now :)

martin107’s picture

While reviewing I noticed a little error

The issue is so old that code introduced in this patch is using deprecated functions

From entityManager getDefinitions() and getStorage()

It is not effortless to correct this screwup because ConfigEntityMapper only provides the out dated entityManager

So I am splitting off a side issue -- so that this patch no longer has to be railroaded into using out moded calls.

#3031346: Replace EntityManager usage in ConfigEntityMapper

martin107’s picture

a) #3031346: Replace EntityManager usage in ConfigEntityMapper is in so I am updating the patch in the light of #102 and getting rid of references to entityManager in "new code"

b) When adding new code I think it is better to use the '::class' convention as linters will complain if subsequent commits do not uniformly change the namespace of all relevant lurking strings.

-        $class = 'Drupal\config_translation\Controller\ConfigTranslationEntityDisplayListBuilder';
+        $class = ConfigTranslationEntityDisplayListBuilder::class;

c) There are a similar set of arguments in relation to entityTypeManager and the new class

ConfigTranslationEntityDisplayListBuilder extends ConfigTranslationFieldListBuilder

we should not be forced to introduce new code which related to the deprecated entityManager.
So I am going to created another out of scope issue.

Status: Needs review » Needs work

The last submitted patch, 103: interdiff-2546212-101-103.patch, failed testing. View results

martin107’s picture

Status: Needs work » Needs review
Related issues: +#3035383: Replace deprecated usages of entityManager in list builder classes

This, in time, will fix 103 c like errors in core.

martin107’s picture

Assigned: martin107 » Unassigned

I am going to put energy into getting - issues that prevent

#3035383: Replace deprecated usages of entityManager in list builder classes

from being completed ... other many want to work on other aspects of this issue...but
for me this issue is at the end of a long chain.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

martin107’s picture

Reworked patch now that we can remove references to $this->entityManager

penyaskito’s picture

Rerolled

$git commit -m "Patch 2546212 - 108"
[2546212-translate-no-translation-ui d8e4acb284] Patch 2546212 - 108
 3 files changed, 268 insertions(+)
 create mode 100644 core/modules/config_translation/src/ConfigTranslation/EntityDisplayMapper.php
 create mode 100644 core/modules/config_translation/src/Controller/ConfigTranslationEntityDisplayListBuilder.php

$ git rebase 8.8.x
First, rewinding head to replay your work on top of it...
Applying: Patch 2546212 - 108
Using index info to reconstruct a base tree...
M	core/modules/config_translation/config_translation.module
Falling back to patching base and 3-way merge...
Auto-merging core/modules/config_translation/config_translation.module
tedbow’s picture

  1. +++ b/core/modules/config_translation/config_translation.module
    @@ -150,6 +173,34 @@ function config_translation_config_translation_info(&$info) {
    +      $tasks[$id . '.default'] = [
    

    $tasks here probably meant $local_tasks.

    I tried changing to that but then since we are editing inside the from for loop over $local_tasks this doesn't work.

    If I change the for loop to over array_keys($local_tasks) This works but then it creates to 2 links that both point to ../default/translate

    Not sure what is going on there.

  2. The approach with local tasks, that was added in #99, will cause there to be 2x the amount of local tasks. We would also need label differences for each mode.
  3. This is not working at all for me with view displays. I tried to add a number field because I thought that #2546356: Numeric field prefix/suffix settings should be translatable would have provided settings that needed translation. Not sure why
  4. adding #3044993: Allow synced Layout default Translations: translating labels and inline blocks as related issue.

    For defaults we store the layout builder settings under third party settings. Not sure if that issue should actually postponed on this issue.

    For layout builder we have a "Manage Layout" link on each view display page. If you are using layout builder for view mode you don't use the formatter settings for that view mode. So would there be anything else to translate? Maybe other third party settings that are provided by other modules on view display level instead of individual formatter?

    We will need a separate for layout builder translations because we want the translator to use the same Layout Builder form, with different options available.

ckaotik’s picture

  1. I suspect that the $tasks variable was never needed and might have been a development leftover.
  2. The task should only be added once, that's why I used a dynamic link that basically means "translate this" - That's what I meant with 2. in comment #99. If this gets too confusing, maybe removing the local task would be better, there's still the config_translation listing as fallback.

    I had tried before to add the translate task as child of the individual form/view displays, which would be better from a UX standpoint, but those links are not output (only levels 1+2 are).
  3. Tabs will only show up if the config has translatable elements, that have a value in the default translation. Maybe that's what you're missing.

I haven't used the layout_builder module, so unfortunately I can't comment on 4.

bgronek’s picture

@penyaskito I just tried to apply patch #109 to Drupal 8.7.2 and it failed to apply. Does it need to be rerolled again? Thanks!

Neograph734’s picture

@miraclemaxx patches are always against the latest -dev version. If you click the test result in #109, you'll see the last successful test was 28 May 2019 at 04:49 CEST. Try -dev instead of 8.7.2 it should be fine.

kle’s picture

Hello jhodgdon,

time ago I wrote (just) a formatter for myself and I solved the translation in this way: I didnt uses the PHP Timeformat directly - the user could choose from Drupal Date and Time formats. And these are translatable.
Best wishes from Cologne

devkinetic’s picture

Trying to use this patch fails on 8.7.6

    $entity = $this->entityTypeManager
      ->getStorage($this->entityType)
      ->load("{$this->pluginDefinition['base_entity_type']}.{$bundle}.{$mode}");
    $route_match->getParameters()->set($this->entityType, $entity);

    $this->setEntity($entity);

$entity should be a ConfigEntityInterface but is actually an EntityInteface. I'm not seeing an easy way to get the definition for the form/view mode.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

penyaskito’s picture

#109 still applies to 8.9.x

johnpicozzi’s picture

Have the patch from #109 installed and I'm running into this issue https://www.drupal.org/project/drupal/issues/2891227

mbovan’s picture

We had an issue with the patch #109 if there is no default form/view display available.

Steps to reproduce:

  1. Install a clean Drupal 8 with standard profile
  2. Enable config_translation module
  3. Go to admin/structure/taxonomy/manage/tags/overview/display
  4. The page breaks with the following error:
TypeError: Argument 1 passed to Drupal\config_translation\ConfigEntityMapper::setEntity() must implement interface Drupal\Core\Config\Entity\ConfigEntityInterface, null given, called in /usr/local/var/www/d8/web/core/modules/config_translation/src/ConfigTranslation/EntityDisplayMapper.php on line 105 in Drupal\config_translation\ConfigEntityMapper->setEntity() (line 152 of core/modules/config_translation/src/ConfigEntityMapper.php).
Drupal\config_translation\ConfigEntityMapper->setEntity(NULL) (Line: 105)
Drupal\config_translation\ConfigTranslation\EntityDisplayMapper->populateFromRouteMatch(Object) (Line: 85)
Drupal\config_translation\Access\ConfigTranslationOverviewAccess->getMapperFromRouteMatch(Object) (Line: 58)
Drupal\config_translation\Access\ConfigTranslationOverviewAccess->access(Object, Object)
call_user_func_array(Array, Array) (Line: 159)

I am attaching a test-only patch (#109 + tests) and a possible fix.

mbovan’s picture

The interdiff between #119 and #109.

The last submitted patch, 119: 2546212-119-TEST-ONLY.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

johnpicozzi’s picture

Status: Needs review » Needs work

The patch in #119 resolved the problem I posted about on https://www.drupal.org/project/drupal/issues/2891227

Dropa’s picture

Confirming #119 fixing the problem I had with #109 described in 2891227

idebr’s picture

Status: Needs work » Needs review

I'm assuming the #122 status change "Needs review » Needs work" is accidental as it didn't provide any actionable feedback.

johnpicozzi’s picture

@idebr - You are correct! Not sure how that happened. I would event say we could go to RTBC based on #122 and #123

sorlov’s picture

Patch #119 works fine, but I have some thoughts about UI.

Currently, for Manage display for example, Translation link depends on current view mode,
if you're on Default view mode page, you see Translate content view display link to translate it.
So, if you need to Translate Full content view mode, you need to select Full content view mode first and then you'll have correct link to translate it.

I think it is more confusing (cause link is updating depending on selected view/form mode) than having smth like this
configuration UI

davidferlay’s picture

I agree with @sorlov opinion :

if you're on Default view mode page, you see Translate content view display link to translate it.
So, if you need to Translate Full content view mode, you need to select Full content view mode first and then you'll have correct link to translate it.

Much likely agreeing with that idea, but would be great to see a better screenshot exemple to convince everyone)

sorlov’s picture

I mean smth like that
screen

On Translate display and Translate form display pages, you'll see secondary tabs with list of view\form modes to translate.

PS I don't like current way also, cause list of view\form modes in secondary tabs are mixed with Translate link.

davidferlay’s picture

+++ it's the best UI I can think of too atm !

piggito’s picture

@sorlov @davidferlay Please correct me if I'm wrong but as I understand from thread and mentioned at #94 the main plan here is to get this functionality into core as it currently is and to expand UX discussion in follow-up issues.

davidferlay’s picture

Would be great to improve UI as @sorlov suggested in this issue/patch, then review/retest and it would be ready for RTBC imho

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Kristen Pol’s picture

I tried #119 on simplytest.me and get an access denied error for each view mode, e.g. /admin/structure/types/manage/article/display/default/translate?destination=/admin/config/regional/config-translation/node_view_display. I have permission to use configuration translation. I even opened it up to all roles to see if it made any difference and it didn't.

UPDATE: Same issue with #109 so I assume this is a simplytest.me issue.

Kristen Pol’s picture

Issue summary: View changes

I've added a How to test section in the issue summary that will need updating. I wasn't able to see anything on the translation page (get access denied) so I can't fill in other steps.

Martijn de Wit’s picture

Status: Needs review » Needs work
FileSize
29.17 KB
37.62 KB

tested the patch from #119. Using Drupal 8.8.5 and Field group 8.3

We needed this patch to translate some title(legends) and descriptions for field groups at the node edit pages.

At first side everything works as described. People who can use user 1 or have administer rights can edit the translation field.
( remarks on the UX; I feel the same that this is very confusing. If you have multiple display's users don't get the translation tab is changing )

Problem:
We now encounter the problem of translate the description field. At the config part this is a text area without any char limit. At the translation part this is a normal textfield with a max char length of 128 chars. This means we can not enter the whole translation of our description via the new interface the patch introduced.

Expected:
When a config field is a textarea. the translation field should also be a textarea.

field group managed form display

field group translation pane for managed form display

ckaotik’s picture

Issue summary: View changes

I agree that the "Translate" tab can be confusing (as mentioned in #99 and #111 2.). The suggested UI from #128 is neat, but not easily integrated with how config_translation works. I'd suggest simply removing the local task for now and opening a follow-up issue for that new integrated UI.
The fallback UI still exists in the Configuration Translation configuration area, next to fields etc.:
Config list for display modes grouped by entity type

Regarding #134: I've also adjusted the summary test instruction steps 1 & 2. The translation UI is only accessible, if any of the formatters (or widgets) has a config schema that includes translatability. A quick check of the Core schema came up with the "Boolean" formatter, which hopefully does the job. (Can anyone confirm this, I can't test at the moment).

Regarding #135, that's a different issue, because the form element is automatically determined by config_translation, based on the configuration schema in taxonomy.schema.yml. The taxonomy.vocabulary.*: definition for description should be type: text (translated using textarea) instead of type: label (translated using single line textfield). (See the config schema docs for more on this)

Kristen Pol’s picture

Thanks for updating the "How to test" section. I was able to see a config translation form for a boolean field! Woot!

We still need to cover the case where there are no fields that have translatable configuration. It shouldn't show "access denied" in that case.

Kristen Pol’s picture

@ckaotik You wrote:

A quick check of the Core schema came up with the "Boolean" formatter, which hopefully does the job.

I'm looking for where you found this and need help. Would you point me in the right direction please?

I see:

Core/Field/Plugin/Field/FieldFormatter/BooleanFormatter.php

/**
 * Plugin implementation of the 'boolean' formatter.
 *
 * @FieldFormatter(
 *   id = "boolean",
 *   label = @Translation("Boolean"),
 *   field_types = {
 *     "boolean",
 *   }
 * )
 */
class BooleanFormatter extends FormatterBase {

but that is just for the label.

Ditto for:

Core/TypedData/Plugin/DataType/BooleanData.php

/**
 * The boolean data type.
 *
 * The plain value of a boolean is a regular PHP boolean. For setting the value
 * any PHP variable that casts to a boolean may be passed.
 *
 * @DataType(
 *   id = "boolean",
 *   label = @Translation("Boolean")
 * )
 */
class BooleanData extends PrimitiveBase implements BooleanInterface {

I do see this:

Core/Field/Plugin/Field/FieldType/BooleanItem.php

/**
 * Defines the 'boolean' entity field type.
 *
 * @FieldType(
 *   id = "boolean",
 *   label = @Translation("Boolean"),
 *   description = @Translation("An entity field containing a boolean value."),
 *   default_widget = "boolean_checkbox",
 *   default_formatter = "boolean",
 * )
 */
class BooleanItem extends FieldItemBase implements OptionsProviderInterface {

  /**
   * {@inheritdoc}
   */
  public static function defaultFieldSettings() {
    return [
      'on_label' => new TranslatableMarkup('On'),
      'off_label' => new TranslatableMarkup('Off'),
    ] + parent::defaultFieldSettings();
  }

with the TranslatableMarkup for On and Off.

UPDATED:

Ah, maybe this is what you meant?

core/config/schema/core.entity.schema.yml

field.formatter.settings.boolean:
  type: mapping
  mapping:
    format:
      type: string
      label: 'Output format'
    format_custom_false:
      type: label
      label: 'Custom output for FALSE'
    format_custom_true:
      type: label
      label: 'Custom output for TRUE'
joseph.olstad’s picture

The patch #119 still applies cleanly to 8.8.x , 8.9.x , 9.0.x and 9.1.x

I have triggered automated tests for 8.8.x, 9.0.x, and 9.1.x

All tests pass with patch 119 for 8.9.x , 8.9.x, 9.0.x , retriggering the test for 9.1.x

ckaotik’s picture

Ah, maybe this is what you meant?

core/config/schema/core.entity.schema.yml

Exactly :) In there, the on/off labels are of type "label", meaning they are short strings that can be translated. This information is used by the config_translation module to build the translation form and input elements.

Kristen Pol’s picture

Re-reading some of the most recent comments, I agree with #136 that it would be good to break this up so that this issue is only for making the functionality work in the translate form. A different issue can be created for addressing possible tabs like shown in #128.

Some issues I still see that need addressing:

1) Needs tests for different types (e.g. label vs description)

2) Needs to handle the 403 issue mentioned in #137

3) (clarified by @berdir below) I'm not understanding why this test is added and I don't see a mention of it above:

+  /**
+   * Asserts that missing default display does not break the form display page.
+   */
+  public function testDisplayTranslation() {
+    $this->drupalLogin($this->rootUser);
+    // Setup vocabulary.
+    Vocabulary::create([
+      'vid' => 'tags',
+      'name' => 'Tags',
+    ])->save();
+    // Assert there is no default form display.
+    $this->assertNull(EntityFormDisplay::load('taxonomy_term.tags.default'));
+    $this->drupalGet('admin/structure/taxonomy/manage/tags/overview/form-display');
+    $this->assertResponse(200);
+  }
Berdir’s picture

That test was added because before that fix, that scenario resulted in a fatal error, when no form display configuration had been created yet.

Kristen Pol’s picture

Issue tags: +Bug Smash Initiative

@berdir Thanks for the explanation.

Adding tag as I worked on this yesterday for the Bug Smash Initiative.

joseph.olstad’s picture

patch 119 is good enough for us

Phil_b’s picture

Patch #119 works but i have the same problem as mentioned in #135.
When translating a placeholder the 128 character limit is too short. Is there a way to enlarge this field?

Martijn de Wit’s picture

There is a workaround for #135. If you export the config to version control. You can edit the config file and copy/past there the full string.
I you do a config import, the translation is there.

The limitation is only in the interface it self.

Phil_b’s picture

Thank you very much for this hint. Will try it

joseph.olstad’s picture

Roadmap to success for this issue: write test and add test to patch, then RTBC.

It is not crux to this issue but good to know about the workaround for the 128 character limitation, that must mean that increasing the limit in the UI is likely possible/easy without causing regressions. however with that said, I think that increasing the 128 character limitation should be done in a seperate issue as the patch 119 does resolve the issue and we should focus on the task at hand which is to fix this issue as described in the issue summary which is what patch 119 does.

vacho’s picture

As I can resume still needs to work over this issues:

1. Needs to work over UI enhancements mentioned at #126, #136: translate display, form display, secondary tabs.
2. Needs to fix testing problems, for 8.9.x, 9.1.x
3. Needs to fix the issue mentioned at #135: textarea to translate when this is expected.
Limited for translation that has more than 128 character.

ckaotik’s picture

No. 3 (#135) is unrelated and should get its own seperate issue, which I already explained in #136.

Martijn de Wit’s picture

joseph.olstad’s picture

Fixed deprecated assert, see interdiff.

Updated issue summary, removed 'needs tests'

Unrelated issue was openned to deal with other unrelated nit piks.

RTBC! not yet

joseph.olstad’s picture

Assigned: Unassigned » joseph.olstad
Status: Reviewed & tested by the community » Active

on it

joseph.olstad’s picture

Fixed deprecated assert, see interdiff. (153 missing () after assertSession (oops) )

Updated issue summary, removed 'needs tests'

Unrelated issue was openned to deal with other unrelated nit piks.

Testing

joseph.olstad’s picture

Assigned: joseph.olstad » Unassigned
Status: Active » Needs work
Issue tags: +Needs tests

tests only patch passes, need to improve the tests

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Dom.’s picture

Testing patch #155 on a fresh install:

ArgumentCountError: Too few arguments to function Drupal\config_translation\Controller\ConfigTranslationFieldListBuilder::__construct(), 3 passed in \web\core\modules\config_translation\src\Controller\ConfigTranslationEntityDisplayListBuilder.php on line 28 and exactly 4 expected in Drupal\config_translation\Controller\ConfigTranslationFieldListBuilder->__construct() (line 78 of core\modules\config_translation\src\Controller\ConfigTranslationFieldListBuilder.php).

Call to a member function getDefinition() on null in Drupal\config_translation\Controller\ConfigTranslationEntityDisplayListBuilder->getOperations() (line 96 of core\modules\config_translation\src\Controller\ConfigTranslationEntityDisplayListBuilder.php).

Patch included to fix those errors.

Dom.’s picture

Status: Needs work » Needs review
ckaotik’s picture

1. Needs to work over UI enhancements mentioned at #126, #136: translate display, form display, secondary tabs.

I've attached a patch that removes the confusing local task and instead provides actions (see #150, #136). I find this quite intuitive:
Newly added "Translate" action link
The translation is also still accessible via Administration > Configuration > Regional and Language.

I also removed the taxonomy test dependency, as it's already included in the module list.

Edit: Test fail seems to be due to missing use statement.

Dom.’s picture

+1 to get read of confusing local task.

Status: Needs review » Needs work
vsujeetkumar’s picture

Status: Needs work » Needs review
FileSize
14.78 KB

Re-roll patch created.

Status: Needs review » Needs work

The last submitted patch, 163: 2546212_163.patch, failed testing. View results

anmolgoyal74’s picture

Status: Needs work » Needs review
FileSize
15.08 KB
671 bytes

Fixed that failed test case.

penyaskito’s picture

Issue tags: +Needs usability review

I'm not sure if introducing a new UI pattern (see #160) is a good idea, so marking as Needs usability review. All the "translate" links are local tabs, so adding a local task can be confusing.

I don't have any strong opinion however, this patch being committed would leave us in a better position than we are right now, and we can improve that later in follow-ups.

If there's no consensus, maybe we should definitely split this up, as this patch includes the needed APIs (the mappers) for translating any formatter/widget settings, which are even more relevant now than at issue creation as this is were layout builder stores the default layout for an entity type.

johnpicozzi’s picture

FWIW I would tend to agree with @penyaskito. We shouldn't hold up the functionality fix to discuss new UI. The current translation UI can work in the meantime.

ckaotik’s picture

I've split the functionality into two patch files, the main file for the functionality and a separate patch, to be used in addition, for the local action links should one wish to add them on their site.

Edit: That test fail seems unrelated 🤔

Status: Needs review » Needs work

The last submitted patch, 168: 2546212-168.patch, failed testing. View results

cosolom’s picture

Status: Needs work » Needs review
trebormc’s picture

I come from https://www.drupal.org/project/field_group/issues/3114825

The #168 is very useful and works correctly for me.

dgaspara’s picture

#168 works for me. Thanks!

penyaskito’s picture

  1. +++ b/core/modules/config_translation/config_translation.module
    @@ -86,6 +89,10 @@ function config_translation_entity_type_alter(array &$entity_types) {
    +        // @todo Link template is not used but should be set so other logic can apply.
    

    Is this @todo still relevant?

  2. +++ b/core/modules/config_translation/tests/src/Functional/ConfigTranslationListUiTest.php
    @@ -504,4 +505,20 @@ public function testTranslateOperationInListUi() {
    +  public function testDisplayTranslation() {
    

    I'm not sure if we would need more tests for this to be accepted.

parijke’s picture

FileSize
84.15 KB

Applied 2546212-168-actions.patch and 2546212-168.patch but I still cannot see the translate tab
Screenshot

gnuget’s picture

The #168 worked like a charm for me.

Thanks!

ckaotik’s picture

@penyaskito regarding 1., I can't recall what that @todo was for. Probably a leftover from initial development that should probably be removed.
I don't know how much testing we need for this to get accepted.

@parijke Did you rebuild your caches? Both routing (for adding the tab) and the caches for the entity type definitions need to be rebuilt.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Kristen Pol’s picture

Regarding the @todos. I see 3 of them in the patch from #168.

  1. +++ b/core/modules/config_translation/config_translation.module
    @@ -86,6 +89,10 @@ function config_translation_entity_type_alter(array &$entity_types) {
    +        // @todo Link template is not used but should be set so other logic can apply.
    
  2. +++ b/core/modules/config_translation/src/Controller/ConfigTranslationEntityDisplayListBuilder.php
    @@ -0,0 +1,117 @@
    +    // @todo There must be a better way to get this information?
    
  3. +++ b/core/modules/config_translation/src/Controller/ConfigTranslationEntityDisplayListBuilder.php
    @@ -0,0 +1,117 @@
    +    // @todo Use config-translation-overview link template like field_ui does.
    
vacho’s picture

patch #168 works for me however we still need to fix this:

- Just like @kristen says at #178, these @todo(s) looks important to fix.

- Also the GUI to translate configuration show all field configurations as "Field Widget" in the title. Should be better to set the "Field Widget label" to better user experiences at a time to translate.
field widget issues

- Also this new GUI pattern is very questionable. I am suggesting this another for approach

field widget issues

vacho’s picture

Status: Needs review » Needs work
ckaotik’s picture

Regarding the todo remarks highlighted by @kristen-pol: Numbers 1 and 3 are (almost) the same. For number 2, we may be able to compare against Drupal\Core\Entity\Display\EntityFormDisplayInterface or Drupal\Core\Entity\Display\EntityViewDisplayInterface.

Regarding @vacho's suggestions:
1) The common "Field Widget" label is a limitation by config_translation, and should be tackled - in a separate issue, if there is none yet.
2) The patch itself relies on config_translation's UI on /admin/config/regional/config-translation. The action link you'd like to remove is entirely optional (thus a separate patch file) and serves only as a compromise. As of now, config translation only allows a single "Translate" tab for a common base url (in your example, that's the article edit form). So while this is desirable, it should also be handled in a separate issue.

Once the todo remarks are solved, I'd like to focus on getting a working translation UI (using /admin/config/regional/config-translation) committed and improve the experience for more sitebuilder comfort as a follow-up.

danrod’s picture

Patch #168 worked like a charm to me, thanks for all the work involved.

joseph.olstad’s picture

the wxt distribution is now using this patch (from #155 on D9.1.x), not yet using 9.2.x

heiligerbiber’s picture

Patch #168 works for me - thanks a lot!

hodba’s picture

I noticed an issue while exporting and importing the translated configurations.

When exporting the configurations, the translated configuration files have the same name as the original configurations which through the following error in the log while trying to import the config files:

Drupal\language\Exception\DeleteDefaultLanguageException: Can not delete the default language in Drupal\language\Entity\ConfigurableLanguage::preDelete() (line 177 of /data/disk/node/static/dev/alvents-10.3.0/html/core/modules/language/src/Entity/ConfigurableLanguage.php).

Steps to reproduce (as in my case):
- Install field_group module
- Go to "Manage form display"
- Add a new field group (e.g. details)
- Translate the form (translate the newly created field_group)
- Export the configurations (Full archive)
- Import the configurations (Full archive)

If you extracted the configuration (on Mac) you would see duplicate Yaml files of the same view mode with different content; one for the default language and the other for the translated one.

Berdir’s picture

That's how all config translation works, it's always the same file names. That error is something else.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Pasqualle’s picture

The "Field widget" translation missing any kind of field identifier. All just say "Field widget", so it is a guesswork which field is being translated.

The "Field group" (from field_group module) translation shows an addition "A" in all the fieldset labels, like "Media A field group", not sure where that additional letter comes from.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Guido_S’s picture

I had the same problem as #174 and looked into this.
Composer didn't tell me that there is an error when applying the patch but the files that should be created with the patch, weren't created and that's why I couldn't see a translation action link either.
Seems this is a problem with patches that only introduce NEW files.

I added this to my composer file now below the patches section:

"patchLevel": {
  "drupal/core": "-p2"
},

found a hint to this in a github issue where a commit referenced this issue and solved it that way.

But I agree that tabs on the top would be better user experience as config translations always use a tab and no actions links.

immaculatexavier made their first commit to this issue’s fork.

vacho’s picture

@guido_s there is no issue at the patch, this is an issue with your project composer.

Anchal_gupta’s picture

Rerolled patch against 9.5x. Please review

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

vacho’s picture

Issue summary: View changes
vacho’s picture

FileSize
104.29 KB
114.14 KB

Works on my side without any issues.

View display

View display

vacho’s picture

About the observations about the configuration translation

this is the English configuration that drupal export:

uuid: 54dce440-2c6e-49ce-9477-9d1fbf539fd8
langcode: en
status: true
dependencies:
  config:
    - field.field.node.page.body
    - field.field.node.page.field_amount
    - field.field.node.page.field_sex
    - node.type.page
  module:
    - text
    - user
_core:
  default_config_hash: M_Y8L5tfmhx7DR143E05YyZSpvgil6VFvqcfBWykalg
id: node.page.default
targetEntityType: node
bundle: page
mode: default
content:
  body:
    type: text_default
    label: hidden
    settings: {  }
    third_party_settings: {  }
    weight: 100
    region: content
  field_amount:
    type: number_decimal
    label: above
    settings:
      thousand_separator: ''
      decimal_separator: .
      scale: 2
      prefix_suffix: true
    third_party_settings: {  }
    weight: 103
    region: content
  field_sex:
    type: boolean
    label: above
    settings:
      format: custom
      format_custom_false: 'No thanks'
      format_custom_true: 'Yes please'
    third_party_settings: {  }
    weight: 102
    region: content
  links:
    settings: {  }
    third_party_settings: {  }
    weight: 101
    region: content
hidden:
  langcode: true

The translation should be:

langcode: es
content:
  field_sex:
    settings:
      format_custom_false: 'No gracias'
      format_custom_true: 'Si por favor'
smustgrave’s picture

#193 @Anchal_gupta thanks for the patch but 1) Please include an interdiff for what you changed. 2) Since one was not there I started at 168 and that applies fine for 9.5.x so the patch in 193 was not needed.

Starting with patch #168

#173.2 I think more tests will be needed. Label is already there.
#178 the todos should be addressed. or verified they are not needed.

Putting IS update label on to document what steps are still needed.

smustgrave’s picture

#188 with regards to Field Group and the random "A" that's from their schema. Opened https://www.drupal.org/project/field_group/issues/3318031 to fix up the schema work anyway.

For the other section where they all say Field Widget. That is confusing and almost unusable. That's also controlled by cores schema but don't know enough about how to fix it yet.

andypost’s picture

it's coming from core/config/schema/core.entity.schema.yml:115 nut not sure it useful to have labels for sequence and mapping same time

    content:
      type: sequence
      label: 'Field widgets'
      sequence:
        type: mapping
        label: 'Field widget'
        mapping:
smustgrave’s picture

Is there anyway to get to say [Field-name] Field widget

smustgrave’s picture

FileSize
3.59 KB
13.23 KB

Uploading some new tests to check for links and url status code. Have NOT added anything for checking labels on the translation pages.

Writing the tests I found another bug

Go to admin/config/regional/config-translation/node_view_display
Click to translate any view mode
Get an access denied error

Enable layout builder for a content type
Repeat steps
And you get in
Also some weird stuff going on there with nested settings that may need a follow up.

smustgrave’s picture

Status: Needs work » Needs review
FileSize
2.29 KB
3.07 KB
14.09 KB

Added a fix for the "Field widget" display. Not sure if it's the best but works.

As for the error I'm seeing it's because there are no translatable fields so it throws access denied. Not sure if that should be addressed here or in a follow up.

smustgrave’s picture

Issue summary: View changes
Issue tags: -Needs tests, -Needs issue summary update

Tests were added above. If more are needed we can add the Needs tests label back

Updated IS with remaining tasks

The last submitted patch, 203: 2546212-203-tests-only.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 203: 2546212-203.patch, failed testing. View results

smustgrave’s picture

Status: Needs work » Needs review
FileSize
645 bytes
14.1 KB

Fixed the D10 CI error.

Should those others tags be removed?

andypost’s picture

I think it's time to update summary with latest screenshots and ask for #ux review

Jose Reyero’s picture

I've been giving a try to this one and I can say it works well.

The edit/translate UI may need some improvements though. I don't think this can be considered 'usable' without adding proper field / widget names everywhere.

Then, about the implementation, some notes:

1. I think this should be moved, as much as possible to the field_ui namespace. The feature is dependent on field_ui being active, isn't it?

Instead of core/modules/config_translation/src/ConfigTranslation/EntityDisplayMapper
maybe use core/modules/field_ui/src/ConfigTranslation/EntityDisplayMapper
just as core/modules/node/src/ConfigTranslation/NodeTypeMapper - which is the only other example I've found

2. Bit hackish, though I understand there's no easy way of doing this - setting proper names. However, relying on the element title is a no-no IMO.

+/**
+ * Implements hook_preprocess_HOOK() for details element templates.
+ */
+function config_translation_preprocess_details(&$variables) {
+  if (isset($variables['element']) && $variables['element']['#title'] == 'Field widget') {
+    $field = ucwords(str_replace('edit-', '', $variables['attributes']['id']));
+    $variables['title']['#markup'] = $field . ' field widget';
+  }
+}

I mean, we are doing a core patch, right? So we don't really need to hack around. If we need one more hook, we add it, if we need better form element markers, we add them - in field_ui module.

The best option I can think of, so far, instead of adding more hooks is maybe adding some ConfigMapperInterface::massageForm() to the interface... though not sure about it, I'm working on some proof of concept, not there yet...

The other option would be maybe... but same, working on that, not yet there:

/**
 * Implements hook_form_BASE_FORM_ID_alter().
 */
function field_ui_form_config_translation_form_alter(&$form, FormStateInterface $form_state) {
  /** @var \Drupal\config_translation\ConfigMapperInterface $mapper **/
  $mapper = $form_state->get('config_translation_mapper');
  // This is_a(), just to avoid hard dependencies on other modules classes. We wouldn't need it if the mapper was in this module though.
  if (is_a($mapper, 'Drupal\config_translation\ConfigTranslation\EntityDisplayMapper')) { 
     // @todo Add field / widget names, etc, etc...
  }
}

As I said, working on some options ... still haven't figured it out yet... but what I see is the config_translation UI is pretty complex and limited. So anything that makes it easier/better I think will be welcomed...

jlcharette’s picture

Trying to apply #207 on Drupal 9.5.3 and constantly getting error:

Could not apply patch! Skipping. The error was: The process "patch '-p1' --no-backup-if-mismatch -d 'web/core' < '/var/folders/r7/r0x8c0nd55b0zxmrxnxdh2wc0000gp/T/63fa32d91a4f1.patch'" exceeded the timeout of 300 seconds.

I am desperatly trying to find a solution for Field Group translations as the labels DO NOT show up in configuration translations AND there is no interface translation either.

It looks like every field groups translation issues are on hold for a while now.

Martijn de Wit’s picture

@jlcharette patch from #207 is for Drupal 10.1.X only.

Maybe you can try the one from #203 for Drupal 9. Think the feature will only be made ready for Drupal 10.1.X

needs-review-queue-bot’s picture

Status: Needs review » Needs work
FileSize
3.16 KB

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

DuneBL’s picture

I have tried #203 on 9.5.3 but I can't find a place where to translate my group's label.
Should I use tmgmt? If yes which source should be selected?

guilherme-lima-almeida’s picture

Having the same issue as DuneBL.

Jose Reyero’s picture

New patch. Mostly implementing own ideas outlined in #209

Main changes:

- Moved most of the new code - and some of the existing one - to field_ui module. It simplifies a few things, like we don't need to check whether module is enabled or not.
- Naming Fields, Formatters and Widgets using field_ui_form_config_translation_form_alter().

It may be a bit rough yet, may need maybe less comments (?) but this is the working example...

Adding two screenshots, one from the view display, on from form display.

Jose Reyero’s picture

Status: Needs work » Needs review

Forgot to set the status to Needs review...

andypost’s picture

Status: Needs review » Needs work

Looks nice, just needs to fix cs

Akram Khan’s picture

added updated patch try to fixed CCF #215. add accessCheck() because entity queries to check access by default is deprecated so need to check it with true and false.

Jose Reyero’s picture

Thanks @Akram Khan for cleaning up most of the patch.

This is a minor update which:

- Updates failing test, it was not checking for the right string, since latest versions of the patch add proper field names
- Fixes a minor naming issue. We are translating "widget settings", not "format settings" for Form displays. It also adds some better title formats - by using '%placeholders' instead.

smustgrave’s picture

Status: Needs review » Needs work

Still needs issue summary update for #208.

Will ping the #ux channel.

rkoller’s picture

FileSize
104.02 KB

I've applied the patch provided in #219, applied it locally to 10.1.x-dev and followed the steps from the issue summary. Two observations:

  1. The most confusing detail is that the order of the true and false field differs between the boolean field widget on the manage display page for the content type and the translation page. The order should be identical if possible imho.
  2. df

  3. if get onto the page to translate the article default display i only see the bool (the name i've given to the boolean field) detail element collapsed. if i expand it i see the boolean format settings detail element collapsed. so i have to click twice until i reach the fields i initially wanted to get to. i am not sure if it is a requirement from core to have those elements collapsed per default. but as a user i want to see and get to the things directly, in particular in a case where there arent tenth of detail elements like for a view, but here you only have two fields for the boolean states.
Jose Reyero’s picture

Thanks for the review @rkoller,

About 1, it would be really hard, to get the right order here because we are somehow blindly translating properties that have a real form somewhere else, but we cannot use that one... so not sure we can actually get that - order - information from anywhere..

About 2. Agreed, this form sucks... just like all the other config translation forms I guess :D ... anyway the difference here is we are doing some more targetted 'form altering' - to add field names, etc.. - so I'll see about not collapsing all the fieldsets as default...

The form being always small is not a criterium we can use though, as there may be many more fields or properties here, it will just depend on each content type configuration.. and each field type has its own story too.... I'll try with some 'count nested fields < x => do not collapse'..

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

ilya.no’s picture

Status: Needs work » Needs review
FileSize
25.74 KB
12.3 KB

About first point of #221 comment, order of these elements are set directly in formatter, while in other places order is opposite. So I agree, that it's not possible to easily fix it.
About second point I've made a patch, where simply set open state for `formatter` and `settings` form elements. Besides that I've modified logic to include case, when view display uses layout_builder.

smustgrave’s picture

Status: Needs review » Needs work

Did not test. Moving to NW for issue summary update

vasike made their first commit to this issue’s fork.

vasike’s picture

Status: Needs work » Needs review

let's try with a MR so anyone could participate with updates

created a MR for the 11.x branch, using the latest patch available - #224
and make it "green" ..

needs-review-queue-bot’s picture

Status: Needs review » Needs work
FileSize
90 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

vasike’s picture

Status: Needs work » Needs review
joseph.olstad’s picture

Status: Needs review » Reviewed & tested by the community

Justification for RTBC:
merge request has tests
merge request passing tests
Been using a version of this patch since D8.

larowlan’s picture

Status: Reviewed & tested by the community » Needs work

I've not done a thorough review, but I did spot a regression in help text relating to content blocks

srishtiiee made their first commit to this issue’s fork.

srishtiiee’s picture

Status: Needs work » Needs review
srishtiiee’s picture

Issue summary: View changes
tim.plunkett’s picture

Thanks @srishtiiee!

smustgrave’s picture

Great job everyone carrying this forward!

Posted in #ux channel for the usability review

Utkarsh_33’s picture

Status: Needs review » Needs work

Posted a small change.

LOBsTerr’s picture

Status: Needs work » Needs review

I have tested it locally and I didn't find any issues. Great job!

hooroomoo’s picture

Status: Needs review » Reviewed & tested by the community

Tested locally and looks like all the feedback has been addressed.

lauriii’s picture

Status: Reviewed & tested by the community » Needs work

Posted review on the MR

lostcarpark’s picture

Great to see progress being made with this issue.

I'm working on an issue with the Smart Trim module, which adds a new field formatter. This previously used the t() function to translate configuration settings.

Without the patch, the Smart trim settings were not showing in the Config Translation interface.

After applying the patch, I was able to translate Smart Trim settings with the instructions in the description.

Here's hoping this change makes it in core in the not too distant future.

DamienMcKenna’s picture

Two patch for sites still using 9.5.x - the first is a reroll of the patch from #224, the second is a reroll of the merge request as it existed on August 18th; I'm skipping test coverage as I just wanted to share the files in case anyone needed them.

DamienMcKenna’s picture

Here's a D9 version of the merge request; sorry for uploading the same file twice in #243.

joseph.olstad’s picture

Status: Needs work » Needs review

Could the authors of the recent changes please summarize them?

rkoller’s picture

Issue tags: -Needs usability review

We have discussed this issue at #3379289: Drupal Usability Meeting 2023-08-11 and revisited it at #3379289: Drupal Usability Meeting 2023-08-11. Each issue will have a link to a recording of the respective meeting. For the record, the attendees at the first usability meeting were @aaronmchale, @emma-horrell, @rkoller, and @worldlinemine. The attendees at the second usability meeting were @aaronmchale, @benjifisher, @blackbamboo, @rkoller, and @worldlinemine.

During the first meeting we've tested the boolean example from the issue summary plus the link and list (text) field type. While writing up this comment I've noticed when revisiting the example that the patch doesn't affect the link and list (text) field type, the two were already translatable without the patch.

In the context of UX related issues we haven't found anything aside the points already mentioned in #221. In regards of those:

1. Collapsed detail element
We've tested with the latest revision of the patch with the boolean example and all field sets were expanded. That looks great! It would only be interesting to see the behavior for a more complex example @jose-reyero mentioned in #222. Which field type would be a good pick to illustrate that?

2. Field order
It was a lucky coincidence that the ordering issue already surfaced with the boolean field type. There was a clear consensus in both meetings that it would be desirable to have a consistent order between the fields where you add and configure the none translated version and where you add and configure the translated versions.
One example that was articulated which illustrates some of the potential problems when a boolean field is used in a legal context for example to opt-in or out of a legally binding document. In case people would read thoroughly all the time there wouldn't be any issue but usually people just scan. A few potential scenarios:

  • The person who sets up the custom output for the boolean field and wants to translate it afterwards, doesn't read at all and just expects the same order as in the widget on the Manage display page.
  • Someone else is translating the custom output.
  • The custom output sentences for TRUE and FALSE are too similar so things could become a slippery slope.

With more complex fields the number of unexpected ordering issues you could not think of could lead into many more problems down the road could rise . That is just for Core but there also field types added by Contrib or Custom modules.

@jose-reyero already voiced in #222.1 it might be too challenging or close to impossible to fix.

There was a clear consensus not to stall this issue any further. But the recommendation is to open a follow-up issue about the ordering issue. In the first place it would be good to learn why the ordering differs on the boolean field type, as well as does it apply to other field types as well? Based on the outcome the group will try to provide another recommendation how to handle the ordering in case the to be translated fields can't be sorted properly.

And I remove the Needs usability review tag.

smustgrave’s picture

Status: Needs review » Needs work
Issue tags: +Needs followup

Believe this is super close!

There appear to be a few threads on the MR.

#246 mentions a need for a follow up if that could be opened please.

Happy to see this one making it's way to the end.

tedbow’s picture

Issue tags: +Needs tests

I was starting to review this and I found a couple things

  1. If I follow the instructions for "How to test" but don't actually add new field to Article then when I am at admin/config/regional/config-translation/node_view_display then I get an "Access denied" message when I click on any of the Translate links. I am not sure if this expected but I couldn't any of the other config translation to have this behavior. I would expect there to be some page telling there is nothing to translate or better no "translate" link at all
  2. When I edited the Help text of boolean field I added for testing the translated text does not show on the node edit form after I reload it. I have to clear caches first.

Number 1 seems wrong and I would think we need test coverage to prove that this doesn't happen if fix this.

Number 2 I am not sure if this is related to this MR or if this is expected with config translation

omkar.podey made their first commit to this issue’s fork.

omkar.podey’s picture

The next thing would be to create a trait end extract common code snippets from EntityViewDisplayElement and EntityFormDisplayElement

omkar.podey’s picture

Issue summary: View changes
FileSize
162.19 KB
158.82 KB
222.3 KB
132.95 KB

Just pulled what you were trying to test so the EntityViewDisplayElement class is definitely being used without it we lose the additional info.

Without EntityViewDisplayElement


With EntityViewDisplayElement


omkar.podey’s picture

The test definitely needs to be expanded.

tedbow’s picture

I gave it another review. I am bit confused of how the functionality of config_translation is split into different modules. for instance we have ConfigTranslationEntityListBuilderin config_translation but EntityViewDisplayElement in field_ui.

but this presumably both tested in \Drupal\Tests\config_translation\Functional\ConfigTranslationListUiTest::testTranslateOperationInListUi as there are no new tests in the field_ui module

In 11.x is not clear to me either. We have very little use of the Drupal\config_translation namespace outside of the module. Drupal\config_translation\Controller\ConfigTranslationBlockListBuilder lives in config_translation but config_translation_entity_type_alter() show it is for the "block" entity type which defined in \Drupal\block\Entity\Block in the block module but \Drupal\node\ConfigTranslation\NodeTypeMapper lives in the node module.

I bring this up because I recommended to @omkar.podey that he should move the logic in EntityViewDisplayElement dealing with layout_builder into the layout_builder module because it does belong in field_ui. But if we could just put all the config_translation module itself it seems like it would much easier.

omkar.podey’s picture

Issue summary: View changes
FileSize
80.85 KB
80.23 KB

Before

After

tedbow’s picture

  1. #248.1 access problem still is happening. I was testing with user 1 after a standard install just enabling. This happens if you try to translate a view display before you add a field that has translatable setting. It makes sense that there nothing to translate but not that you see the link and get an "Access denied" message.
  2. If I enable layout builder for view mode the access problem goes away. Presumably this because there is at least 1 setting to translation
  3. The modal doesn't scroll so the view display with even a couple layout builder settings because there are nested it becomes impossible to get to the save button. This is probably a general modal problem
  4. The Layout builder settings are "Per-view-mode field layout settings" which doesn't really make sense because you are already looking at the view mode specifically. This comes from core/modules/layout_builder/config/schema/layout_builder.schema.yml. So I think updating should be another issue. But this is probably the first place that this shows up in UI made by core

Added more comments in the MR

omkar.podey’s picture

  1. So the access problem is just the the default behaviour and is caused by \Drupal\config_translation\Access\ConfigTranslationOverviewAccess::doCheckAccess where since we don't have any translatable elements it just denies access to the page.
  2. If we want to then we could change this to return allowed access and then down the line we need to call hasTranslatable() and just say "No translatable elements available....." if there are no elements to translate.
nicoschi’s picture

It seems this patch has a big side issue and it's weird to me anyone has already discovered it (maybe I'm doing something wrong?).

This is a case:
- a series of custom field formatters used on a node view display which original language is english have translatable settings
- translate them through config translation in the other language, say it is Italian
- then visit the entity display edit form in the italian language and save it and all the original setting in english language will be overriden with translations!

Berdir’s picture

What you are describing is what #2910353: Prevent saving config entities when configuration overrides are applied is about, and the same can happen with block overviews, permissions/roles and many other cases. This neither causes nor can it fix that problem.

nicoschi’s picture

@Berdir thanks for the the tip of the other patch to apply, it's the first time it happens to me or maybe it's a big deal in a case of an entity view display with tons of fields and configs

Pilulu changed the visibility of the branch 2546212-entity-viewform-mode to hidden.

Pilulu changed the visibility of the branch 2546212-entity-viewform-mode to active.

ilya.no’s picture

Status: Needs work » Needs review

I've fixed remaining comments, except for note about new class, because I'm not sure, whether this is in scope of this task.
I've removed LB related code from field_ui and recreated it as per first option of this note.
I've also fixed test and locally it passes fine, but on pipeline there is an error.

smustgrave’s picture

Status: Needs review » Needs work

Seems to have a valid test error.

Could we make sure that new functions being added have a typehint return. Even if it's :void. Tests included.

Jonasanne’s picture

Patch rerolled for D10.2

manojbisht_drupal’s picture

Above Patch #264 is not applying, giving error. So I have created a patch based on #168 for 10.2.x, as it was working fine on D9.

Along with it, actions patch can also be applied

sthomen’s picture

The patch in #265 applies but will crash the site, there's a missing use statement for the EntityDisplayMapper in config_translation.module.

Trydoknight’s picture

@sthomen it has in previous patch

ilya.no’s picture

Status: Needs work » Needs review

I've added typehint return to newly added functions. Also tests are ok now.

manojbisht_drupal’s picture

FileSize
11.89 KB

Created new patch : added use statement.

Martijn de Wit changed the visibility of the branch 11.x to hidden.

Martijn de Wit changed the visibility of the branch 2546212-entity-viewform-mode to hidden.

Martijn de Wit’s picture

@manojbisht_drupal please use the merge request.

And if you need a patch, everyone can download a diff/patch from gitlab and load it local with composer.

(hide patches to get focus on merge request.)

artemboiko changed the visibility of the branch 2546212-entity-viewform-mode to active.

artemboiko’s picture

Hi @martijn-de-wit I guess need show this branch as a branch for 10.2.x, but also I think it will be better to rename branch with 10.2.x in name for better visibility and yep create new mr with patch 269 from @manojbisht_drupal

What you think about this next steps?

Martijn de Wit’s picture

thank you for the help artemboiko. Renaming it to 10.2.x will make things clearer.

Drupal V 10.3 will be branched from 11.x. I don't think this will land in 10.2.x because of the size, complexity and changes.
So I think all effort should be at the merge request for the 11.x branch.

I think we need al lot of people testing this, maybe a sign off from a (sub)core maintainer ?

lostcarpark’s picture

I think this has to be seen as a new feature, not a bugfix, so targeting 10.3 makes sense.

I'm interested in this for the Smart Trim module. Currently we have to wrap a config setting in t() because formatter config is not covered by config translation. This is clearly not best practice. Being to use config translation will be a lot cleaner.

Happy to test this change against the Smart Trim config translation change.

artemboiko changed the visibility of the branch 2546212-entity-viewform-mode to hidden.

lolgm changed the visibility of the branch 11.x to active.

lolgm changed the visibility of the branch 11.x to hidden.

sorlov’s picture

rebased with latest changes from 11.x branch, need review

smustgrave’s picture

Status: Needs review » Needs work
Issue tags: -D8MI, -sprint, -SprintWeekend2016, -Needs tests

Cleaned up I believe useful tags that were probably more used in the past.

Ran the test-only feature which results in

1) Drupal\Tests\config_translation\Functional\ConfigTranslationListUiTest::testTranslateOperationInListUi
Behat\Mink\Exception\ExpectationException: No link containing href admin/config/regional/config-translation/node_form_display found.
/builds/issue/drupal-2546212/core/tests/Drupal/Tests/WebAssert.php:575
/builds/issue/drupal-2546212/core/tests/Drupal/Tests/WebAssert.php:389
/builds/issue/drupal-2546212/core/modules/config_translation/tests/src/Functional/ConfigTranslationListUiTest.php:505
/builds/issue/drupal-2546212/core/modules/config_translation/tests/src/Functional/ConfigTranslationListUiTest.php:622
/builds/issue/drupal-2546212/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
ERRORS!
Tests: 1, Assertions: 55, Errors: 1.

So removing tests tag

Believe a follow up is still needed so leaving that tag

Left a few comments on the MR should be easy to resolve. Wonder if the completed threads are able to be closed @sorlov?

Tried testing following the steps in the issue summary but when I get to step
">Click on Translate button for Default view mode for Articlecontent type" at URL
admin/structure/types/manage/article/display/default/translate?destination=/admin/config/regional/config-translation/node_view_display

I get an access denied.

lolgm’s picture

Here's some help if anyone else comes across this problem.

I have a D9.5 website where I have patch #219 installed and Feeds, Field Group modules.

I encountered a very awkward situation when using this patch:

  1. Create a Feed type to import users. (Just create an empty one, the important thing is to add the Feeds item field)
  2. Create a tab component in the users form display.
  3. Create a translation for this previously created component (/admin/config/people/accounts/form-display/default/translate/{LANG}/edit)

When visiting the form display in the original language I can see that the original label remains as expected and by changing the site language to my secondary language the translated element is also correct.
So far everything is as expected, however, when I return to the form display in the original language, the tabs component label, previously translated, remains with the translated label. Exporting configs, you are able to see that it was actually replaced in the original language.

I came to the conclusion that this is happening due to the problem of a missing widget in the Feeds item field.
It can easily be fixed by adding the patch from the issue:
#3389946: Moving "feeds item" field in form display to non-disabled causes PHP error

xmacinfo’s picture

Adding last comment reference to the Related issues.

vasike’s picture

Status: Needs work » Needs review

Updated the MR
- solved conflicts
- addresses latest threads from @smustgrave - (previous) #281
- tried to close/resolve existing threads on MR - i hope i didn't close "still open" threads

It would be nice if the open threads could be checked again by their "initiators": @lauriii, @tedbow, @kekkis

So Needs Review .... temporary.

svenryen made their first commit to this issue’s fork.

svenryen changed the visibility of the branch 2546212-entity-viewform-mode to active.