Problem/Motivation
Field storage settings, such as values in list fields have labels which will contain localized strings and should therefore be translated. But the translate UI doesn't show them as translatable because it only exposes field config settings (as opposed to field storage config settings):
It works when the configuration language overwrite file is generated by hand. So it's only an UI issue.
Proposed resolution
Add field storage configuration files to the UI mapper we have for field configuration.
This results in:
vs:
Remaining tasks
Review. Commit.
User interface changes
Field storage settings will be translatable on a screen integrated with field config.
API changes
None.
Beta phase evaluation
Issue category | Bug because field storage settings, such as allowed values cannot be translated |
---|---|
Issue priority | Major because its a whole set of config entities not exposed for translation. |
Not unfrozen | |
Disruption | No disruption, because no data structure change (let alone API change). Not even cache needs to be cleared (can you believe it?) |
Comment | File | Size | Author |
---|---|---|---|
#51 | 2409701-51.patch | 14.09 KB | tstoeckler |
#51 | 2409701-51--tests-only.patch | 12.75 KB | tstoeckler |
Comments
Comment #1
Schnitzel CreditAttribution: Schnitzel commentedComment #2
Schnitzel CreditAttribution: Schnitzel commentedThe problem is, that the values (called field settings) are not in the field instance, they are on the field config (or also called field storage).
The UI though only searches for translatable elements of the field instance.
This first patches fixes the issue with adding the config name of the field storage to
ConfigTranslationFormBase
which is responsible for going through the configs and show the translatable elements. It adds that in ConfigFieldMapper, so it happens only for Fields.The UI works now, it shows the values. The problem is, they are saved wrong in the language overwrite config. This is how it looks after translating it:
If I change the config by hand to:
then it works. I actually think it should be:
as this is how the default config for the field storage saves the settings.
Don't have time right now to look into it, let's see what the testbot says about this change overall.
Comment #5
Leksat CreditAttribution: Leksat commentedI've debugged this. See #2413481: Sequence translation
Comment #6
saravananr971 CreditAttribution: saravananr971 commentedSir ajax command (',&) this data display in html entity so any soliecion give me.
Comment #7
Schnitzel CreditAttribution: Schnitzel commentedSo this is postponed till #2395627: Do not remove 0 from config translation data lands
Comment #8
Leksat CreditAttribution: Leksat commentedComment #9
Leksat CreditAttribution: Leksat commentedI've made small improvements to the Schnitzel's patch, added unittest for the ConfigFieldMapper class, and extended the ConfigEntityMapperTest::testSetEntity() to also check if configuration name is added to the mapper.
(commits)
Comment #11
tstoecklerWorks great, awesome!
Added some screenshots and a beta evaluation.
We could add some functional tests for this but for me the unit tests are sufficient. Let's let the committers decide.
-> RTBC
Comment #12
tstoecklerI just saw #2446869: Convert the "Field storage edit" form to an actual entity form and now I'm thinking whether it wouldn't make sense for the field storage settings form to have its own "translate" page separate from the field settings form.
That would mean we have to add a new mapper for field storages.
It would be more "correct" in terms of the data model, but I'm not sure in terms of usability which would probably trump the data model in this case.
Not tagging "Needs usability review" yet, but will when I post some screenshots of the two alternatives.
Comment #15
Gábor HojtsyI think the patch makes sense. The UI to edit field storage config is a tab on the field config UI, integrated with editing field config basically. So we could put two translate tabs on this screen (one for the edit tab and one for the field storage edit tab), but that sounds like pretty painful.
I went to look if we expose field storage config in config translation mappers already. Looking at config_translation_config_translation_info(), we don't because field storage configs don't have an edit-form link template. (Field config does not have one directly either, but they get dynamically set in \Drupal\field\Entity\FieldConfig::linkTemplates()).
I think its totally fine to integrate the field storage config translation with the field config translation.
So elevating to major bug and changing title.
Comment #16
Gábor HojtsyRTBC pending green testbot result.
Comment #17
tstoecklerHmm... yes, the tabs argument is valid. Let's get this in.
RTBC++
Comment #20
_nolocation CreditAttribution: _nolocation at Amazee Labs commentedwill reroll tomorrow
Comment #21
_nolocation CreditAttribution: _nolocation at Amazee Labs commentedI rerolled the patch
Comment #22
_nolocation CreditAttribution: _nolocation at Amazee Labs commentedComment #24
Gábor HojtsyLooks like some test fixes are still in order:
Comment #25
Leksat CreditAttribution: Leksat at Amazee Labs commentedComment #26
tstoecklerYeah, this should mock "Drupal\Core\Config\Entity\ConfigEntityTypeInterface" now
Comment #27
Leksat CreditAttribution: Leksat at Amazee Labs commentedThanks @tstoeckler! Fixed this.
In translate-list-field-values-2409701-27-with-fixed-definitions.patch I additionally fixed definition of the setEntity() method (diff), but I'm not sure whether this is correct.
Comment #29
Leksat CreditAttribution: Leksat at Amazee Labs commentedOh, I forgot to also change the mock in the ConfigEntityMapperTest. Fixed now (diff).
Comment #30
Leksat CreditAttribution: Leksat at Amazee Labs commentedI did one small change: replaced the "instanceof" check with @var comment (diff)
Comment #31
Gábor HojtsyThat makes total sense to me. We should not babysit the code here for potentially wrong entity type names.
Comment #32
xjmThanks all! The issue summary is very clear and helped me understand the change quickly even though I'm not really familiar with this UI at all. This is definitely a good usability improvement.
So technically, this would break any implementation that passed a non-config-entity. But if you passed a non-config-entity to ConfigEntityMapper, your code is broken. So making the method more explicit makes sense.
However, the protected
$entity
property documentation still references only EntityInterface. Someone can also can fix that on commit though since it is a very minor docs issue. (Edit: Though actually since I've marked this NW for an additional test case it can be added to the patch now as well.)So the parent implementation has a long comment that reads:
I wasn't exactly sure what the comment meant, but it sounded like it could be a different explanation for why these fields were not being included for translation. I checked and the comment dates back to the original introduction of the translation UI in #1952394: Add configuration translation user interface module in core, as part of #2083231: Avoid making up custom terminology: group => names in the sandbox.
I asked @Gábor Hojtsy what this was about, and he suggested that it might be because this code predates third-party settings, since things using third-party settings would be automtically integrated with config translation.
I'm not sure if that's relevant to this issue, which is just meant to merge the base fields and configurable fields into one translatable UI, because from the translator's perspective the distinction between them being stored in the entity type config or the field config is irrelevant and confusing. @Gábor Hojtsy also said that there is no existing UI for the base fields at all.
The patch obviously works nicely as it is, so I'll just ask @alexpott to provide feedback on this point to confirm the current implementation still makes sense.
After having reviewed the unit tests and the new UI, I think we actually should add that functional test coverage for the full user interaction since the situation in HEAD is kind of a WTF for translators. So marking NW for that addition.
Comment #33
xjm(Adding @tstoeckler and @Gábor Hojtsy to the proposed issue credit for their review work above.)
Comment #34
tstoecklerNeeded a reroll, so no changes yet.
Comment #35
Gábor HojtsyRe #32/2: that comment is indeed referring to 3rd party settings that may be stored independently (not using the official 3rd party settings). In this case, this is two config entity instances that make most sense separately. The point of the config mappers is to provide a UI mapping, and having multiple tabs for the translation of field config and storage would be very poor UX.
Comment #36
tstoecklerI'm not going to tell you how long it took to complete this test, but here we go. Should be ready from my perspective.
Also fixes #32.
Comment #38
tstoecklerThis should apply.
Interdiff is still valid except that I removed the accidental added ues statement in
ConfigEntityStorage
, but not providing an extra interdiff for that.Comment #39
tstoecklerHere's a test-only patch, together with the original patch again.
Comment #40
tstoecklerAhhh #double-fail
Comment #47
Gábor HojtsyHum, which one is the right one?
Comment #48
tstoecklerSorry, the patch had some windows encoding / line-endings...
So that makes it a #triple-fail? #quadruple-fail? I'm losing track...
Let's see if this is better.
Comment #51
tstoecklerAhh, need to use
assertRaw()
with random strings in case they contain a quote. Learned something new today! :-)Comment #53
Gábor HojtsyYay, looks great, thanks!
Comment #54
alexpottThis issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed fea90fb and pushed to 8.0.x. Thanks!
Comment #56
Gábor HojtsyYay, thanks all!