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
- Use core version applicable to the patch and apply the patch
- Configure
Article
node type with aboolean
field, displayed inDefault
view mode using custom texts - Enable
Configuration Translation
module (/admin/modules
) - Add at least one language (
/admin/config/regional/language
) - Go to configuration translation page (
/admin/config/regional/config-translation
) - Click on
List
button forContent view display
- Click on
Translate
button forDefault
view mode forArticle
content type - See what is on the configuration translation form for the
Article
Default
view mode - @todo - Add more steps for translating specific configuration
Proposed resolution
- 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.
- 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.
- 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
Comment | File | Size | Author |
---|
Issue fork drupal-2546212
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:
Comments
Comment #2
Gábor HojtsyComment #3
mpdonadioComment #4
tstoecklera) 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.
Comment #5
jhodgdonI 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?]
Comment #6
jhodgdonRegarding the "prefix" and "suffix" fields on the integer fields, in core/config/schema/core.data_types.schema.yml I find
So it looks like those are marked as type "string" which I am pretty sure means translatable, right?
Comment #7
jhodgdonEditing issue summary to clarify form mode vs. form display, whoops.
Comment #8
tstoecklerRe #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:
So in short:
A) I will open issues for
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.
Comment #9
BerdirAFAIK, 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 ;)
Comment #10
tstoecklerIn 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
totype: string
withtranslatable: true
just like #9 says all over core, becauselabel
is just very confusing (and incorrect). So as of now, we're stuck withtype: label
. We should open a bug report against Config Translation, however, that it doesn't support settingtype: translatable
manually.Comment #11
jhodgdonOK 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.
Comment #12
jhodgdonRegarding 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.
Comment #13
Gábor Hojtsy@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?
Comment #14
jhodgdonOK...
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
???
Comment #15
Gábor HojtsyField 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.
Comment #16
Gábor HojtsyOk, I wanted to look into this. How do we tell the location of the UI for the parent entity of an entity display?
Comment #17
jhodgdonField 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?
Comment #18
tstoecklerRe #16:
So that's a pretty good question: So
Node
declares afield_ui_base_route
which points toentity.node_type.edit_form
. So from there we can get to theNodeType
entity type and find it's edit form. I think this proves, however, that this is architecturally not a very sound approach. Because thefield_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 byfield_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) ??Comment #19
Gábor Hojtsy@tstoeckler: but we already have a translate tab and so many other tabs :/
Comment #20
tstoecklerRe #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.
Comment #21
tstoecklerFound #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.
Comment #22
Gábor Hojtsy@tstoeckler why would we need to alter the forms? We can add the additional config keys to the mappers with:
Comment #23
tstoecklerWorking on this with @jan.stoeckler and @mmaihoff at Barcelona now...
Comment #24
tstoecklerOh, 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...
Comment #25
Gábor HojtsyYeah 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.
Comment #26
tstoeckler@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()
andsetEntity()
. That whole logic cannot be reached from other modules, however, so we need to add another mechanism there.Not really sure what to do.
Comment #27
Gábor HojtsyThat sounds like is where #2565031: Expose $entity in ConfigEntityMapper would in fact come in handy! I praise your visionary mind :)
Comment #28
tstoecklerRe #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
Comment #29
tstoeckler#2295255: Convert ConfigMapperInterface::populateFromRequest() to use RouteMatch landed, but opened #2577761: We need a way to dynamically alter the list of config names for config mappers for the issue mentioned above, so PP-2 is still correct.
Comment #30
tstoeckler#2295255: Convert ConfigMapperInterface::populateFromRequest() to use RouteMatch landed, but opened #2577761: We need a way to dynamically alter the list of config names for config mappers for the issue mentioned above, so PP-2 is still correct.
Comment #31
tstoecklerDon'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
Comment #32
tstoecklerSince 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.
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!
Comment #33
jhodgdonThe 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!
Comment #34
tstoeckler1. 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...)
Comment #35
jhodgdonI'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.
Comment #36
jhodgdonHm. 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.
Comment #37
tstoecklerHmm.. sorry about that, will roll a combined patch then
Comment #38
tstoecklerNot 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.
Comment #40
maxocub CreditAttribution: maxocub commentedI 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.
Comment #42
maxocub CreditAttribution: maxocub commentedI 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.
Comment #43
maxocub CreditAttribution: maxocub commentedI 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:
One way to avoid that, I think, would be to check if the base route name is 'entity.node_type.edit_form'
Comment #44
Gábor Hojtsy@maxocub: that looks unintended / unfortunate indeed.
Comment #45
Gábor HojtsyI 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:
Should the bundle be the entity id? That sounds confusing...
Should it be _field_ui_base_route_for or something like that? The value is not really a route.
Not the same name as above. Also not a route name(?)
Comment #46
maxocub CreditAttribution: maxocub commentedAbout 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?
Comment #47
tstoecklerSo 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 infield_ui
module which dynamically registers the event subscriber if theconfig_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.Comment #48
maxocub CreditAttribution: maxocub commentedI 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!
Comment #49
maxocub CreditAttribution: maxocub commentedForgot the files...
Comment #50
maxocub CreditAttribution: maxocub commentedComment #51
tstoecklerI know me saying this has gotten old at this point, but you are quite awesome @maxocub, thanks a lot!
One minor quibble:
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.But this is only a minor point. Let's see if the patch is green - if so, we are really close here, I think.
Comment #52
maxocub CreditAttribution: maxocub commentedI see your point, it's more intuitive this way, let's do that.
Comment #53
maxocub CreditAttribution: maxocub commentedComment #54
maxocub CreditAttribution: maxocub commented#2577761: We need a way to dynamically alter the list of config names for config mappers is 8.1, so this one will be too.
Comment #55
maxocub CreditAttribution: maxocub commented@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)
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)
What about
$entity_route->setOption('_entity_type_id', $entity_type_id);
since we are assigning it the entity type id?3)
Same comment as in 2)
Comment #56
jhodgdonRE #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).
Comment #57
jhodgdonIn other words, an in-code // comment stating this might clear up some confusion. ;)
Comment #59
maxocub CreditAttribution: maxocub commentedHere 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
Comment #60
tstoecklerThe 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:for the base routes and
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?
Comment #61
maxocub CreditAttribution: maxocub commentedI 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?
Comment #62
tstoecklerYeah, I think that was the idea. Or for contrib to use, so that it does not have to perform the same checks.
Comment #63
maxocub CreditAttribution: maxocub commentedHere are the changes suggested in #60.
Comment #64
BerdirAs 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?
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.
Comment #66
vijaycs85How about this? Just checked few sample pages and seem to be OK.
Comment #68
vijaycs85Wrong patch...
Comment #70
vijaycs85all 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.
Comment #71
ckaotikI'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()
causesNotice: 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:
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 thelabel
of a schema definition, instead of only fortype
keys.Comment #73
idebr CreditAttribution: idebr at iO commentedComment #74
pk188 CreditAttribution: pk188 at OpenSense Labs commentedRe rolled.
Comment #75
tacituseu CreditAttribution: tacituseu commentedCould 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()
.Comment #76
tacituseu CreditAttribution: tacituseu commentedThere is an issue already for #71.2, unfortunately not much in it.
Comment #78
dawehner🙃 Let's get rid of that
🙃 Let's get rid of that
🙃 Could you use EntityDisplaySubscriber::class instead?
I like the namespacing here
Given that
$entity_route
is an object already, can't we skip the overriding completely? It will just change?Comment #79
tstoecklerClosed #2925584: Custom boolean field is not translatable as duplicate.
Also noticed this is still assigned to me, changing that now.
Comment #81
Anybody#2980929: Link text is not translatable is also an example where a translation is needed.
Comment #82
johnchqueAddressing suggestions from #78.
Comment #83
martin107 CreditAttribution: martin107 as a volunteer commentedIn #82 .. looking at the patch
There are more @file tags, that need to be removed.
Comment #84
johnchqueRebasing 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.
Comment #85
BerdirConfirmed 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 :)
Comment #87
tacituseu CreditAttribution: tacituseu commented#2577761: We need a way to dynamically alter the list of config names for config mappers is in :).
Comment #88
andypostHere's re-roll
Comment #89
andypostA bit of clean-up
Comment #90
Wim LeersThis new route option is necessary 👍
So is this one 👍
Pointless comment?
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?
Let's create an issue for this and link to it.
Comment #91
tstoecklerThanks 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()
.Comment #92
Wim Leers👌 Of course!
P.S.: #2571371: Editor settings cannot be translated is a sister issue of this, and is now also unblocked!
Comment #93
TwoDI 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?
Comment #94
tstoecklerRe #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... ;-)
Comment #95
Gábor HojtsyHm, why exactly is that not possible? First this needs an issue summary update so we are clear on scope and what things are deferred.
Comment #96
tacituseu CreditAttribution: tacituseu commentedRe #93: Looked into it some time ago (see #75), ended up adding:
to make some sense of it.
Comment #97
TwoD@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).
Comment #98
tacituseu CreditAttribution: tacituseu commented@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.Comment #99
ckaotikFor 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
).There are still some issues, though:
EntityDisplayBase
's protected displayContext propertytranslate
(wich results in a base path of/some/path/display/translate
, identical to the default mode translation path)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::getOperationsComment #101
ckaotikRerolled of the proper development version, let's hope it applies cleanly now :)
Comment #102
martin107 CreditAttribution: martin107 as a volunteer commentedWhile 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
Comment #103
martin107 CreditAttribution: martin107 as a volunteer commenteda) #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.
c) There are a similar set of arguments in relation to entityTypeManager and the new class
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.
Comment #105
martin107 CreditAttribution: martin107 as a volunteer commentedThis, in time, will fix 103 c like errors in core.
Comment #106
martin107 CreditAttribution: martin107 as a volunteer commentedI 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.
Comment #108
martin107 CreditAttribution: martin107 as a volunteer commentedReworked patch now that we can remove references to $this->entityManager
Comment #109
penyaskitoRerolled
Comment #110
tedbow$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/translateNot sure what is going on there.
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.
Comment #111
ckaotik$tasks
variable was never needed and might have been a development leftover.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).
I haven't used the layout_builder module, so unfortunately I can't comment on 4.
Comment #112
bgronek CreditAttribution: bgronek at Igility commented@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!
Comment #113
Neograph734@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.
Comment #114
kle CreditAttribution: kle commentedHello 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
Comment #115
devkinetic CreditAttribution: devkinetic commentedTrying to use this patch fails on 8.7.6
$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.
Comment #117
penyaskito#109 still applies to 8.9.x
Comment #118
johnpicozziHave the patch from #109 installed and I'm running into this issue https://www.drupal.org/project/drupal/issues/2891227
Comment #119
mbovan CreditAttribution: mbovan at MD Systems GmbH commentedWe had an issue with the patch #109 if there is no default form/view display available.
Steps to reproduce:
I am attaching a test-only patch (#109 + tests) and a possible fix.
Comment #120
mbovan CreditAttribution: mbovan at MD Systems GmbH commentedThe interdiff between #119 and #109.
Comment #122
johnpicozziThe patch in #119 resolved the problem I posted about on https://www.drupal.org/project/drupal/issues/2891227
Comment #123
Dropa CreditAttribution: Dropa at Mediamaisteri Oy commentedConfirming #119 fixing the problem I had with #109 described in 2891227
Comment #124
idebr CreditAttribution: idebr at iO commentedI'm assuming the #122 status change "Needs review » Needs work" is accidental as it didn't provide any actionable feedback.
Comment #125
johnpicozzi@idebr - You are correct! Not sure how that happened. I would event say we could go to RTBC based on #122 and #123
Comment #126
sorlov CreditAttribution: sorlov at Skilld commentedPatch #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
Comment #127
davidferlay CreditAttribution: davidferlay at Skilld commentedI agree with @sorlov opinion :
Much likely agreeing with that idea, but would be great to see a better screenshot exemple to convince everyone)
Comment #128
sorlov CreditAttribution: sorlov at Skilld commentedI mean smth like that
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.
Comment #129
davidferlay CreditAttribution: davidferlay at Skilld commented+++ it's the best UI I can think of too atm !
Comment #130
piggito CreditAttribution: piggito as a volunteer and at Skilld commented@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.
Comment #131
davidferlay CreditAttribution: davidferlay commentedWould be great to improve UI as @sorlov suggested in this issue/patch, then review/retest and it would be ready for RTBC imho
Comment #133
Kristen PolI 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.
Comment #134
Kristen PolI'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.Comment #135
Martijn de Wittested 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.
Comment #136
ckaotikI 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.:
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 fordescription
should betype: text
(translated using textarea) instead oftype: label
(translated using single line textfield). (See the config schema docs for more on this)Comment #137
Kristen PolThanks 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.
Comment #138
Kristen Pol@ckaotik You wrote:
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
but that is just for the label.
Ditto for:
Core/TypedData/Plugin/DataType/BooleanData.php
I do see this:
Core/Field/Plugin/Field/FieldType/BooleanItem.php
with the
TranslatableMarkup
forOn
andOff
.UPDATED:
Ah, maybe this is what you meant?
core/config/schema/core.entity.schema.yml
Comment #139
joseph.olstadThe 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
Comment #140
ckaotikExactly :) 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.
Comment #141
andypostRe #140 there's #2545730: Misuse of formatPlural() in Numeric field prefix/suffix and already linked ds issue #3011528: Expert field prefix & suffix cannot be translated
Comment #142
Kristen PolRe-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:
Comment #143
BerdirThat test was added because before that fix, that scenario resulted in a fatal error, when no form display configuration had been created yet.
Comment #144
Kristen Pol@berdir Thanks for the explanation.
Adding tag as I worked on this yesterday for the Bug Smash Initiative.
Comment #145
joseph.olstadpatch 119 is good enough for us
Comment #146
Phil_bPatch #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?
Comment #147
Martijn de WitThere 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.
Comment #148
Phil_bThank you very much for this hint. Will try it
Comment #149
joseph.olstadRoadmap 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.
Comment #150
vacho CreditAttribution: vacho at Skilld commentedAs 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.
Comment #151
ckaotikNo. 3 (#135) is unrelated and should get its own seperate issue, which I already explained in #136.
Comment #152
Martijn de WitOk created a separate issue for that. #3173509: When a config field is a textarea. the translation field should also be a textarea.
Comment #153
joseph.olstadFixed deprecated assert, see interdiff.
Updated issue summary, removed 'needs tests'
Unrelated issue was openned to deal with other unrelated nit piks.
RTBC!not yetComment #154
joseph.olstadon it
Comment #155
joseph.olstadFixed 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
Comment #156
joseph.olstadtests only patch passes, need to improve the tests
Comment #158
Dom. CreditAttribution: Dom. as a volunteer and at ACINO commentedTesting 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.
Comment #159
Dom. CreditAttribution: Dom. as a volunteer and at ACINO commentedComment #160
ckaotikI've attached a patch that removes the confusing local task and instead provides actions (see #150, #136). I find this quite intuitive:
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.
Comment #161
Dom. CreditAttribution: Dom. as a volunteer and at ACINO commented+1 to get read of confusing local task.
Comment #163
vsujeetkumar CreditAttribution: vsujeetkumar at Srijan | A Material+ Company for Drupal India Association commentedRe-roll patch created.
Comment #165
anmolgoyal74 CreditAttribution: anmolgoyal74 at OpenSense Labs for DrupalFit commentedFixed that failed test case.
Comment #166
penyaskitoI'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.
Comment #167
johnpicozziFWIW 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.
Comment #168
ckaotikI'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 🤔
Comment #170
cosolom CreditAttribution: cosolom commentedComment #171
trebormcI come from https://www.drupal.org/project/field_group/issues/3114825
The #168 is very useful and works correctly for me.
Comment #172
dgaspara CreditAttribution: dgaspara at NTT DATA commented#168 works for me. Thanks!
Comment #173
penyaskitoIs this @todo still relevant?
I'm not sure if we would need more tests for this to be accepted.
Comment #174
parijke CreditAttribution: parijke commentedApplied 2546212-168-actions.patch and 2546212-168.patch but I still cannot see the translate tab
Comment #175
gnugetThe #168 worked like a charm for me.
Thanks!
Comment #176
ckaotik@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.
Comment #178
Kristen PolRegarding the
@todo
s. I see 3 of them in the patch from #168.Comment #179
vacho CreditAttribution: vacho at Skilld commentedpatch #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.
- Also this new GUI pattern is very questionable. I am suggesting this another for approach
Comment #180
vacho CreditAttribution: vacho at Skilld commentedComment #181
ckaotikRegarding 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
orDrupal\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.Comment #182
danrodPatch #168 worked like a charm to me, thanks for all the work involved.
Comment #183
joseph.olstadthe wxt distribution is now using this patch (from #155 on D9.1.x), not yet using 9.2.x
Comment #184
heiligerbiber CreditAttribution: heiligerbiber commentedPatch #168 works for me - thanks a lot!
Comment #185
hodba CreditAttribution: hodba commentedI 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.
Comment #186
BerdirThat's how all config translation works, it's always the same file names. That error is something else.
Comment #188
PasqualleThe "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.
Comment #190
Guido_SI 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:
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.
Comment #192
vacho CreditAttribution: vacho at Skilld commented@guido_s there is no issue at the patch, this is an issue with your project composer.
Comment #193
Anchal_gupta CreditAttribution: Anchal_gupta at Srijan | A Material+ Company for Drupal India Association commentedRerolled patch against 9.5x. Please review
Comment #195
vacho CreditAttribution: vacho at Skilld commentedComment #196
vacho CreditAttribution: vacho at Skilld commentedWorks on my side without any issues.
Comment #197
vacho CreditAttribution: vacho at Skilld commentedAbout the observations about the configuration translation
this is the English configuration that drupal export:
The translation should be:
Comment #198
smustgrave CreditAttribution: smustgrave at Mobomo commented#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.
Comment #199
smustgrave CreditAttribution: smustgrave at Mobomo commented#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.
Comment #200
andypostit's coming from
core/config/schema/core.entity.schema.yml:115
nut not sure it useful to have labels for sequence and mapping same timeComment #201
smustgrave CreditAttribution: smustgrave at Mobomo commentedIs there anyway to get to say [Field-name] Field widget
Comment #202
smustgrave CreditAttribution: smustgrave at Mobomo commentedUploading 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.
Comment #203
smustgrave CreditAttribution: smustgrave at Mobomo commentedAdded 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.
Comment #204
smustgrave CreditAttribution: smustgrave at Mobomo commentedTests were added above. If more are needed we can add the Needs tests label back
Updated IS with remaining tasks
Comment #207
smustgrave CreditAttribution: smustgrave at Mobomo commentedFixed the D10 CI error.
Should those others tags be removed?
Comment #208
andypostI think it's time to update summary with latest screenshots and ask for #ux review
Comment #209
Jose Reyero CreditAttribution: Jose Reyero at Cambrico commentedI'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.
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:
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...
Comment #210
jlcharette CreditAttribution: jlcharette commentedTrying 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.
Comment #211
Martijn de Wit@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
Comment #212
needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commentedThe 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.
Comment #213
DuneBLI 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?
Comment #214
guilherme-lima-almeidaHaving the same issue as DuneBL.
Comment #215
Jose Reyero CreditAttribution: Jose Reyero at Cambrico commentedNew 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.
Comment #216
Jose Reyero CreditAttribution: Jose Reyero at Cambrico commentedForgot to set the status to Needs review...
Comment #217
andypostLooks nice, just needs to fix cs
Comment #218
Akram Khanadded 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.
Comment #219
Jose Reyero CreditAttribution: Jose Reyero at Cambrico commentedThanks @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.
Comment #220
smustgrave CreditAttribution: smustgrave at Mobomo commentedStill needs issue summary update for #208.
Will ping the #ux channel.
Comment #221
rkollerI'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:
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.Comment #222
Jose Reyero CreditAttribution: Jose Reyero at Cambrico commentedThanks 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'..
Comment #224
ilya.no CreditAttribution: ilya.no as a volunteer and at Skilld for Skilld commentedAbout 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.
Comment #225
smustgrave CreditAttribution: smustgrave at Mobomo commentedDid not test. Moving to NW for issue summary update
Comment #228
vasikelet'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" ..
Comment #229
needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commentedThe 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.
Comment #230
vasikeComment #231
joseph.olstadJustification for RTBC:
merge request has tests
merge request passing tests
Been using a version of this patch since D8.
Comment #232
larowlanI've not done a thorough review, but I did spot a regression in help text relating to content blocks
Comment #234
srishtiiee CreditAttribution: srishtiiee at Acquia commentedComment #235
srishtiiee CreditAttribution: srishtiiee at Acquia commentedComment #236
tim.plunkettThanks @srishtiiee!
Comment #237
smustgrave CreditAttribution: smustgrave at Mobomo commentedGreat job everyone carrying this forward!
Posted in #ux channel for the usability review
Comment #238
Utkarsh_33 CreditAttribution: Utkarsh_33 as a volunteer and at Acquia commentedPosted a small change.
Comment #239
LOBsTerr CreditAttribution: LOBsTerr at European Commission and European Union Institutions, Agencies and Bodies commentedI have tested it locally and I didn't find any issues. Great job!
Comment #240
hooroomooTested locally and looks like all the feedback has been addressed.
Comment #241
lauriiiPosted review on the MR
Comment #242
lostcarpark CreditAttribution: lostcarpark as a volunteer commentedGreat 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.
Comment #243
DamienMcKennaTwo 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.
Comment #244
DamienMcKennaHere's a D9 version of the merge request; sorry for uploading the same file twice in #243.
Comment #245
joseph.olstadCould the authors of the recent changes please summarize them?
Comment #246
rkollerWe 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 thelink
andlist (text)
field type. While writing up this comment I've noticed when revisiting the example that the patch doesn't affect thelink
andlist (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:boolean
field and wants to translate it afterwards, doesn't read at all and just expects the same order as in the widget on theManage display
page.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.
Comment #247
smustgrave CreditAttribution: smustgrave at Mobomo commentedBelieve 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.
Comment #248
tedbowI was starting to review this and I found a couple things
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
Comment #250
omkar.podey CreditAttribution: omkar.podey at Acquia commentedThe next thing would be to create a trait end extract common code snippets from
EntityViewDisplayElement
andEntityFormDisplayElement
Comment #251
omkar.podey CreditAttribution: omkar.podey at Acquia commentedJust 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
Comment #252
omkar.podey CreditAttribution: omkar.podey at Acquia commentedThe test definitely needs to be expanded.
Comment #253
tedbowI gave it another review. I am bit confused of how the functionality of
config_translation
is split into different modules. for instance we haveConfigTranslationEntityListBuilder
in config_translation butEntityViewDisplayElement
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 moduleIn 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 inconfig_translation
butconfig_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.Comment #254
omkar.podey CreditAttribution: omkar.podey at Acquia commentedBefore
After
Comment #255
tedbowcore/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 coreAdded more comments in the MR
Comment #256
omkar.podey CreditAttribution: omkar.podey at Acquia commented\Drupal\config_translation\Access\ConfigTranslationOverviewAccess::doCheckAccess
where since we don't have any translatable elements it just denies access to the page.hasTranslatable()
and just say "No translatable elements available....." if there are no elements to translate.Comment #257
nicoschi CreditAttribution: nicoschi commentedIt 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!
Comment #258
BerdirWhat 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.
Comment #259
nicoschi CreditAttribution: nicoschi commented@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
Comment #262
ilya.no CreditAttribution: ilya.no as a volunteer and at Skilld for Skilld commentedI'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.
Comment #263
smustgrave CreditAttribution: smustgrave at Mobomo commentedSeems 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.
Comment #264
Jonasanne CreditAttribution: Jonasanne at Calibrate commentedPatch rerolled for D10.2
Comment #265
manojbisht_drupal CreditAttribution: manojbisht_drupal as a volunteer and commentedAbove 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
Comment #266
sthomen CreditAttribution: sthomen commentedThe patch in #265 applies but will crash the site, there's a missing use statement for the EntityDisplayMapper in config_translation.module.
Comment #267
Trydoknight CreditAttribution: Trydoknight commented@sthomen it has in previous patch
Comment #268
ilya.no CreditAttribution: ilya.no as a volunteer and at Skilld for Skilld commentedI've added typehint return to newly added functions. Also tests are ok now.
Comment #269
manojbisht_drupal CreditAttribution: manojbisht_drupal as a volunteer and commentedCreated new patch : added use statement.
Comment #272
Martijn de Wit@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.)
Comment #274
artemboikoHi @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?
Comment #275
Martijn de Witthank 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 ?
Comment #276
lostcarpark CreditAttribution: lostcarpark as a volunteer commentedI 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.
Comment #280
sorlov CreditAttribution: sorlov at Skilld commentedrebased with latest changes from 11.x branch, need review
Comment #281
smustgrave CreditAttribution: smustgrave at Mobomo commentedCleaned up I believe useful tags that were probably more used in the past.
Ran the test-only feature which results in
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 forDefault
view mode forArticle
content type" at URLadmin/structure/types/manage/article/display/default/translate?destination=/admin/config/regional/config-translation/node_view_display
I get an access denied.
Comment #282
lolgm CreditAttribution: lolgm at ADJ 3 Sistemas commentedHere'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:
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
Comment #283
xmacinfoAdding last comment reference to the Related issues.
Comment #284
vasikeUpdated 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.