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.

The field settings translation page including the allowed values

This results in:
The English node add form showing English labels for the allowed values of a list field in a select element
vs:
The German node add form showing German labels for the allowed values of a list field in a select element

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

Reference: https://www.drupal.org/core/beta-changes
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?)
CommentFileSizeAuthor
#51 2409701-51.patch14.09 KBtstoeckler
#51 2409701-51--tests-only.patch12.75 KBtstoeckler
#51 2409701-48-51-interdiff.txt870 byteststoeckler
#48 2409701-48.patch14.1 KBtstoeckler
#48 2409701-48--test-only.patch5.05 KBtstoeckler
#40 2409701-38.patch14.46 KBtstoeckler
#40 2409701-38--test-only.patch5.18 KBtstoeckler
#39 2409701-38.patch14.46 KBtstoeckler
#39 2409701-38--test-only.patch.txt5.18 KBtstoeckler
#38 2409701-38.patch14.1 KBtstoeckler
#36 2409701-36.patch7.36 KBtstoeckler
#36 2409701-34-36-interdiff.txt7.36 KBtstoeckler
#34 2409701-34.patch8.62 KBtstoeckler
#30 translate-list-field-values-2409701-30-with-fixed-definitions.patch8.62 KBLeksat
#29 translate-list-field-values-2409701-29-with-fixed-definitions.patch8.75 KBLeksat
#27 translate-list-field-values-2409701-27-with-fixed-definitions.patch8.31 KBLeksat
#27 interdiff-2409701-21-27.txt737 bytesLeksat
#27 translate-list-field-values-2409701-27.patch6.23 KBLeksat
#21 translate-list-field-values-2409701-20.patch6.22 KB_nolocation
#11 list-field-german.png58.32 KBtstoeckler
#11 list-field-english.png57.42 KBtstoeckler
#11 config-translation-after.png34.86 KBtstoeckler
#9 translate-list-field-values-2409701-9.patch6.2 KBLeksat
#9 translate-list-field-values-2409701-9-tests_only.patch4.97 KBLeksat
#9 interdiff-2409701-2-9-no_tests.txt1.43 KBLeksat
#2 translate-list-field-values-2409701-3.patch1.11 KBSchnitzel
Add_German_translation_for_test_field___sb693a37783112a7_s3_simplytest_me.jpg28.92 KBSchnitzel
test___sb693a37783112a7_s3_simplytest_me.jpg90.45 KBSchnitzel
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Schnitzel’s picture

Status: Needs work » Active
Schnitzel’s picture

Status: Active » Needs review
FileSize
1.11 KB

The 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:

settings:
  allowed_values:
    label: lalalafasdfasdasdf234234
    1:
      label: lululuasdfasd34234

If I change the config by hand to:

settings:
  allowed_values:
    0:
      label: lalalafasdfasdasdf234234
    1:
      label: lululuasdfasd34234

then it works. I actually think it should be:

settings:
  allowed_values:
    -
      label: lalalafasdfasdasdf234234
    -
      label: lululuasdfasd34234

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.

Status: Needs review » Needs work

The last submitted patch, 2: translate-list-field-values-2409701-3.patch, failed testing.

Status: Needs work » Needs review
Leksat’s picture

I've debugged this. See #2413481: Sequence translation

saravananr971’s picture

Sir ajax command (',&) this data display in html entity so any soliecion give me.

Schnitzel’s picture

Status: Needs review » Postponed
Leksat’s picture

Assigned: Unassigned » Leksat
Status: Postponed » Needs work
Leksat’s picture

I'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)

The last submitted patch, 9: translate-list-field-values-2409701-9-tests_only.patch, failed testing.

tstoeckler’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
FileSize
34.86 KB
57.42 KB
58.32 KB

Works 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

tstoeckler’s picture

Status: Reviewed & tested by the community » Needs review

I 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.

Gábor Hojtsy’s picture

Title: List field value labels cannot be translated » Field storage configuration is not exposed to config translation UI
Category: Task » Bug report
Issue summary: View changes
Priority: Normal » Major

I 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.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +sprint, +language-config

RTBC pending green testbot result.

tstoeckler’s picture

Hmm... yes, the tabs argument is valid. Let's get this in.

RTBC++

The last submitted patch, 9: translate-list-field-values-2409701-9-tests_only.patch, failed testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 9: translate-list-field-values-2409701-9.patch, failed testing.

_nolocation’s picture

Assigned: Leksat » _nolocation

will reroll tomorrow

_nolocation’s picture

I rerolled the patch

_nolocation’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 21: translate-list-field-values-2409701-20.patch, failed testing.

Gábor Hojtsy’s picture

Looks like some test fixes are still in order:

Drupal\Tests\config_translation\Unit\ConfigEntityMapperTest    6 passes
Drupal\Tests\Component\Utility\XssTest                        59 passes
PHP Fatal error:  Call to undefined method Mock_EntityTypeInterface_eb2807c9::getConfigPrefix() in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/config_translation/src/ConfigEntityMapper.php on line 147
PHP Warning:  Invalid argument supplied for foreach() in /var/lib/drupaltestbot/sites/default/files/checkout/core/scripts/run-tests.sh on line 590

Warning: Invalid argument supplied for foreach() in /var/lib/drupaltestbot/sites/default/files/checkout/core/scripts/run-tests.sh on line 590
Drupal\Tests\config_translation\Unit\ConfigMapperManagerTest  10 passes
Leksat’s picture

Assigned: _nolocation » Leksat
tstoeckler’s picture

+++ b/core/modules/config_translation/tests/src/Unit/ConfigFieldMapperTest.php
@@ -0,0 +1,109 @@
+    $entity_type = $this->getMock('Drupal\Core\Entity\EntityTypeInterface');

Yeah, this should mock "Drupal\Core\Config\Entity\ConfigEntityTypeInterface" now

Leksat’s picture

Assigned: Leksat » Unassigned
Status: Needs work » Needs review
FileSize
6.23 KB
737 bytes
8.31 KB

Thanks @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.

Status: Needs review » Needs work
Leksat’s picture

Status: Needs work » Needs review
FileSize
8.75 KB

Oh, I forgot to also change the mock in the ConfigEntityMapperTest. Fixed now (diff).

Leksat’s picture

I did one small change: replaced the "instanceof" check with @var comment (diff)

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

That makes total sense to me. We should not babysit the code here for potentially wrong entity type names.

xjm’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests, +Usability

Thanks 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.

  1. +++ b/core/modules/config_translation/src/ConfigEntityMapper.php
    @@ -125,13 +125,13 @@ public function populateFromRequest(Request $request) {
    -  public function setEntity(EntityInterface $entity) {
    +  public function setEntity(ConfigEntityInterface $entity) {
    

    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.)
     

  2. +++ b/core/modules/config_translation/src/ConfigFieldMapper.php
    @@ -49,4 +51,22 @@ public function getTypeLabel() {
    +  public function setEntity(ConfigEntityInterface $entity) {
    +    if (parent::setEntity($entity)) {
    +
    +      // Field storage config can also contain translatable values. Add the name
    +      // of the config as well to the list of configs for this entity.
    +      /** @var \Drupal\field\FieldStorageConfigInterface $field_storage */
    +      $field_storage = $this->entity->getFieldStorageDefinition();
    +      /** @var \Drupal\Core\Config\Entity\ConfigEntityTypeInterface $entity_type_info */
    +      $entity_type_info = $this->entityManager->getDefinition($field_storage->getEntityTypeId());
    +      $this->addConfigName($entity_type_info->getConfigPrefix() . '.' . $field_storage->id());
    +      return TRUE;
    +    }
    +    return FALSE;
    

    So the parent implementation has a long comment that reads:

    // Add the list of configuration IDs belonging to this entity. We add on a
      // possibly existing list of names. This allows modules to alter the entity
      // page with more names if form altering added more configuration to an
      // entity. This is not a Drupal 8 best practice (ideally the configuration
      // would have pluggable components), but this may happen as well.
    

    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.
     

  3. @tstoeckler mentioned in #11 that we could add functional tests for this issue in addition to the unit tests, but that the unit tests might be sufficient.

    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.

xjm’s picture

(Adding @tstoeckler and @Gábor Hojtsy to the proposed issue credit for their review work above.)

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
8.62 KB

Needed a reroll, so no changes yet.

Gábor Hojtsy’s picture

Re #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.

tstoeckler’s picture

Status: Needs review » Needs work

The last submitted patch, 36: 2409701-36.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
14.1 KB

This 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.

tstoeckler’s picture

Here's a test-only patch, together with the original patch again.

tstoeckler’s picture

Ahhh #double-fail

The last submitted patch, 40: 2409701-38--test-only.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 40: 2409701-38.patch, failed testing.

Status: Needs work » Needs review

penyaskito queued 40: 2409701-38.patch for re-testing.

The last submitted patch, 40: 2409701-38--test-only.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 40: 2409701-38.patch, failed testing.

Gábor Hojtsy’s picture

Hum, which one is the right one?

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
5.05 KB
14.1 KB

Sorry, 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.

The last submitted patch, 48: 2409701-48--test-only.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 48: 2409701-48.patch, failed testing.

tstoeckler’s picture

Ahh, need to use assertRaw() with random strings in case they contain a quote. Learned something new today! :-)

The last submitted patch, 51: 2409701-51--tests-only.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Yay, looks great, thanks!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

This 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!

  • alexpott committed fea90fb on 8.0.x
    Issue #2409701 by tstoeckler, Leksat, Schnitzel, _nolocation, Gábor...
Gábor Hojtsy’s picture

Issue tags: -sprint

Yay, thanks all!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.