Problem/Motivation

The On label and Off label of boolean fields cannot be translated currently.

A screenshot of the settings form of a boolean field including the 'On label' and 'Off label' form elements.

This is because they are marked as type string in the configuration schema instead of type label.

Proposed resolution

Change the schema type to label. This makes the corresponding form elements appear in the translation form.

Remaining tasks

User interface changes

API changes

None.

Data model changes

None.

Comments

tstoeckler created an issue. See original summary.

tstoeckler’s picture

I am pinging @tassilogroeper to comment here, because he discovered the issue and should be credited.

Attached patch fixes the issue.

This results in a pretty weird label in the details element, however.

A picture of the boolean field settings translation form including the 'On label' and 'Off label' form elements inside of a details element with the label  is 'On label in English Boolean settings'. ('On label in English' is the value of the 'On label' form element.)

This is due to following code in Configuration Translation's ListElement (simplified here for readability's sake):

    foreach (array_keys($group_build) as $title_key) {
      if (isset($group_build[$title_key]['source']) && (strpos($title_key, 'title') !== FALSE || strpos($title_key, 'label') !== FALSE)) {
        $title = $group_build[$title_key]['source']['#markup'];
        break;
      }
    }
    return strip_tags($title) . ' ' . $this->t($definition['label']);

The element uses the first element that it finds that contains either title or label in its name, and in this case it finds on_label. This code was intended for things like Views, where you have multiple displays inside of the view, and Configuration Translation wants to display the title of each display instead of just printing Views display for each one, which would make it near impossible to sensibly translate the view. In this case, however, this backfires a little, so not sure if and - if so - how we want to adapt this.

tstoeckler’s picture

Status: Active » Needs review
FileSize
507 bytes

Ahh, here's the patch.

tstoeckler’s picture

Issue tags: +Needs tests

Oh, this, as always, needs tests.

penyaskito’s picture

Status: Needs review » Needs work

Marking as needs work as it needs tests.
I couldn't find a test for the current behavior for "Configuration Translation wants to display the title of each display instead of just printing Views display for each one".
We should probably add one for that so we are sure we don't break that feature.

tassilogroeper’s picture

+1 nice work. thanks @tstoeckler

tstoeckler’s picture

I started some work on fixing the issue with the labels on the config translation form. I had previously thought that the code referenced in #2 is for contrib that does not name its elements title or label directly, but in fact we have that use-case in core already with views: The label/title of views displays is available under a display_title key, which is why that generic/catch-all code is needed, or at the very least outright removing it results in a regression.

I also though the logic in \Drupal\config_translation\FormElement\ListElement::getGroupTitle() could be simplified to always have a title using the schema key as a last-resort fallback, but I realized we cannot do that either, because from the schema we have no way of differentiating between "static" mappings, i.e. where a mapping consists of X known keys, no more, no less, and "dynamic" mappings, i.e. where a list of views displays is contained and any number of keys can appear. We only need the logic of getGroupTitle() in the latter case but because there is no way to differentiate it is called in both cases, so we need the empty handling for the former case.

So no code to show yet...

tstoeckler’s picture

Something like this.

This comes with tests for the views display labels on the config translation form. It still needs tests for the boolean field itself but - more importantly - we need to carefully vet all config translation forms if there are more cases like views where we have keys that contain "title" or "label" in core. Then we also need to get some feedback from a maintainer, whether this minor API change is feasible for 8.0.x. Not tagging "Needs maintainer feedback", though, because we first need to update the issue summary to clearly explain the situation. Therefore tagging "Needs issue summary update" instead.

I think it's of pretty big importance to get this in 8.0.x so in case this is not deemed possible for 8.0.x we should consider a more workaround-y solution that does not involve an API change at all. I.e. have the boolean field schema add a "don't try to be smart here about the labels" flag (just with a better name) that we use in getGroupTitle() to avoid the magic behavior. That would be a pure API addition.

tstoeckler’s picture

Status: Needs work » Needs review
vijaycs85’s picture

  1. +++ b/core/modules/config_translation/src/FormElement/ListElement.php
    @@ -115,22 +120,23 @@ public function setConfig(Config $base_config, LanguageConfigOverride $config_tr
    +        $prefix = strip_tags($group_build[$key]['source']['#markup']);
    

    Are we sure to strip tags here?

  2. +++ b/core/modules/views/config/schema/views.schema.yml
    @@ -99,6 +99,7 @@ views.view.*:
    +        label_key: 'display_title'
    

    Do we need this here? Looks like not in scope of this issue.

Gábor Hojtsy’s picture

@vijaycs85: re 2, looks like we need to do that change so we can test it (or at least we need to add it to a test schema to test it there, but then we need to fix it in views too as per above).

@tstoeckler: I think the API addition implemented here is entirely backwards compatible, so it should not be considered an API change. API additions should be possible in 8.1, not earlier though AFAIK. We may want to get the schema fix for string => label in ASAP, even if the UI is a bit confusing, at least the translatability would be there and the API addition for 8.1 AFAIS.

badrange’s picture

I just stumbled across this issue in a customer site - this time when adding a boolean to a contact form (using Core contact module). After applying the patch I was able to translate the boolean value labels, export the configuration, move it to a dev server with the patch - and the translation came along. Happy times!

Thanks for making this patch - now I'm hoping it can enter in a future 8.0.x so that also people who don't go hunting in the issue queue for patches can translate their labels..

badrange’s picture

A colleague tipped me about this issue:
https://www.drupal.org/node/1061438

Are these duplicate of one another?

mikeker’s picture

Despite predating this issue by four and a half years, I'm closing #1061438: "On" and "Off" values for boolean fields are not translatable as a dup of this because the patch here has tests and the patch there wasn't approaching the fix correctly.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

@mikeker: well actual tests for the actual fix are still missing. Can you help provide them? :) I think that is mostly what separates us from this being comittable.

Kristen Pol’s picture

Thanks for the patch. I noticed one typo:

+++ b/core/modules/config_translation/src/Tests/ConfigTranslationUiTest.php
@@ -580,6 +580,13 @@ public function testViewsTranslationUI() {
+    // Make sure that the details element for each display has a proper (unqiue)

Nitpick (typo): uniqiue => unique.

tstoeckler’s picture

Per #11 here's a patch with just the schema change + test coverage. Will open a new issue for the label improvements including the views test coverage.

@Kristen Pol: Thanks for the review! I will make sure it does not get lost when I open the new issue.

tstoeckler’s picture

Issue tags: -Needs tests

The last submitted patch, 17: 2609874-17--tests-only.patch, failed testing.

Kristen Pol’s picture

Thanks! I looked at the new patch and have one question:

+++ b/core/modules/config_translation/src/Tests/ConfigTranslationUiTest.php
@@ -734,6 +735,42 @@ public function testFieldConfigTranslation() {
+    $this->assertText(Html::escape($on_label) . ' Boolean settings');
+    $this->assertEscaped($on_label);
+    $this->assertEscaped($off_label);

Is there supposed to be an assertText for $off_label too?

Kristen Pol’s picture

Oh... I see, it's the fieldset so I guess not. That is a bit confusing.

Kristen Pol’s picture

+++ b/core/modules/config_translation/src/Tests/ConfigTranslationUiTest.php
@@ -734,6 +735,42 @@ public function testFieldConfigTranslation() {
+    $this->assertText(Html::escape($on_label) . ' Boolean settings');
+    $this->assertEscaped($on_label);
+    $this->assertEscaped($off_label);

I think it would be useful to add some comments to explain what is being checked, particularly for the assertText part. Thanks.

tstoeckler’s picture

Thanks for the review! #22 is a very good point. Will do that once I have opened the other issue, so we can add a @todo to that in the code.

Kristen Pol’s picture

I have tested the patch and it is working as expected.

Kristen Pol’s picture

For better UX though, I'm wondering if it would be make sense to get rid of the fieldset altogether?

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs issue summary update

@Kristen: for config translation, the fieldset is merely there because the two labels are contained within a parent element. This is automated and cannot be overridden in core at least. I think this is already a big enough improvement either way?

Looking at the summary, it seems to be updated according to current patch.

Kristen Pol’s picture

@Gábor Hojtsy - Ok, no worries. Yes, this is a great improvement! :) RTBC++

  • alexpott committed 96dc18e on 8.2.x
    Issue #2609874 by tstoeckler, Kristen Pol, tassilogroeper: Boolean field...

  • alexpott committed b96fc99 on 8.1.x
    Issue #2609874 by tstoeckler, Kristen Pol, tassilogroeper: Boolean field...
alexpott’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +String change in 8.1.0, +String change in 8.0.5

Committed 5830c2f and pushed to 8.0.x, 8.1.x and 8.2.x. Thanks!

  • alexpott committed 5830c2f on 8.0.x
    Issue #2609874 by tstoeckler, Kristen Pol, tassilogroeper: Boolean field...
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

  • alexpott committed eeab24a on 8.2.x
    Revert "Issue #2609874 by tstoeckler, Kristen Pol, tassilogroeper:...

  • alexpott committed ca99efc on 8.1.x
    Revert "Issue #2609874 by tstoeckler, Kristen Pol, tassilogroeper:...

  • alexpott committed 7ce5da5 on 8.0.x
    Revert "Issue #2609874 by tstoeckler, Kristen Pol, tassilogroeper:...
alexpott’s picture

Status: Fixed » Needs work

This patch caused a random fail... https://www.drupal.org/pift-ci-job/214169

I don't think there is any reason to use ->randomString() here... how about just hardcoding values?

alexpott’s picture

tstoeckler’s picture

That's fine with me. I had copied the method from the test method above and adapted it. That is the only reason why there was randomString() in there.

alexpott’s picture

Here's 20 runs of the test... the first string is the on label and the second is the off label.

string(8) "3&):>&sX"
string(8) "*mkY>&W\"
Drupal\config_translation\Tests\ConfigTranslationUiTest::tes  20 passes
string(8) "*y]P>&@B"
string(8) "|0.y>& _"
Drupal\config_translation\Tests\ConfigTranslationUiTest::tes  20 passes
string(8) "R0]A>&pR"
string(8) ">3==>&&="
Drupal\config_translation\Tests\ConfigTranslationUiTest::tes  20 passes
string(8) "^3,$>&x_"
string(8) "T5Gb>&._"
Drupal\config_translation\Tests\ConfigTranslationUiTest::tes  20 passes
string(8) "BL[U>&z8"
string(8) "HS_V>&NU"
Drupal\config_translation\Tests\ConfigTranslationUiTest::tes  20 passes
string(8) "_RpN>&<F"
string(8) "BSX >&Rg"
Drupal\config_translation\Tests\ConfigTranslationUiTest::tes  19 passes   1 fails
- Found database prefix 'simpletest745070' for test ID 19.
string(8) "`T(N>&]~"
string(8) ">z4W>&`p"
Drupal\config_translation\Tests\ConfigTranslationUiTest::tes  20 passes
string(8) "T&og>&:w"
string(8) "Pk%U>&F&"
Drupal\config_translation\Tests\ConfigTranslationUiTest::tes  20 passes
string(8) "z;xd>&]?"
string(8) "0},X>&+H"
Drupal\config_translation\Tests\ConfigTranslationUiTest::tes  20 passes
string(8) ")w]^>&\`"
string(8) "7X;A>&:a"
Drupal\config_translation\Tests\ConfigTranslationUiTest::tes  20 passes
string(8) "Dg"l>&r\"
string(8) "Sky/>&o@"
Drupal\config_translation\Tests\ConfigTranslationUiTest::tes  20 passes
string(8) "~mDR>&|2"
string(8) "XLhv>&5>"
Drupal\config_translation\Tests\ConfigTranslationUiTest::tes  20 passes
string(8) "0PY\>&:\"
string(8) "q#>L>&cN"
Drupal\config_translation\Tests\ConfigTranslationUiTest::tes  20 passes
string(8) "|HcM>&K8"
string(8) "D:$O>&N)"
Drupal\config_translation\Tests\ConfigTranslationUiTest::tes  20 passes
string(8) "7-j`>&>J"
string(8) "0yR_>&Gx"
Drupal\config_translation\Tests\ConfigTranslationUiTest::tes  20 passes
string(8) "i?{,>&}3"
string(8) "H$iX>&)Y"
Drupal\config_translation\Tests\ConfigTranslationUiTest::tes  20 passes
string(8) "MwXX>&Tb"
string(8) "^=it>&Sd"
Drupal\config_translation\Tests\ConfigTranslationUiTest::tes  20 passes
string(8) "Gmv%>&!7"
string(8) "DbF+>&8/"
Drupal\config_translation\Tests\ConfigTranslationUiTest::tes  20 passes
string(8) "pG_L>&$8"
string(8) "M35)>&>U"
Drupal\config_translation\Tests\ConfigTranslationUiTest::tes  20 passes
string(8) ".PL6>&co"
string(8) "JNoq>&s@"
Drupal\config_translation\Tests\ConfigTranslationUiTest::tes  20 passes

So the problem seems to be related to spaces...

tstoeckler’s picture

Wow, that's very useful, thanks a lot! Will look into it...

mikeker’s picture

Status: Needs work » Needs review
FileSize
2.51 KB
1.27 KB

Changed the on/off labels to be fixed strings instead of randomString(). I still can't figure out what about those strings was causing the failure, though. Also added two comments to better describe what's being tested, as per #22.

Kristen Pol’s picture

Thanks for the update. I think it's useful that you added more comments.

+++ b/core/modules/config_translation/src/Tests/ConfigTranslationUiTest.php
@@ -734,6 +735,45 @@ public function testFieldConfigTranslation() {
+    // Checks the details label.

Nitpick: For this one, maybe something like the following would be more clear?

// Checks the fieldset text.

The other ones look good to me.

tstoeckler’s picture

Thanks for the patch!

Can we add some markup such as & and < > to the test strings? I *think* those will force to go back to using assertEscaped(), but we'll see about that.

mikeker’s picture

@tstoeckler: Sure, that's a good idea. I've added some basic HTML and an ampersand to the labels. Doing that does raise an interesting point: in \Drupal\config_translation\FormElement\ListElement::getGroupTitle we strip_tags() the title, which is probably a bit heavy handed. (Summary elements can contain a fair bit of HTML.) Thus, we strip_tags() the expected value in the test.

But that's a different issue for a different day...

From #42:

// Checks the fieldset text.

Except that these are no longer fieldset but details elements. Regardless, I've updated the wording -- let me know if that clarifies things.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +String change in 8.1.0, +String change in 8.0.5

Committed 891e57e and pushed to 8.0.x, 8.1.x and 8.2.x. Thanks!

  • alexpott committed 3479333 on 8.2.x
    Issue #2609874 by tstoeckler, mikeker, Kristen Pol, tassilogroeper:...

  • alexpott committed acb8752 on 8.1.x
    Issue #2609874 by tstoeckler, mikeker, Kristen Pol, tassilogroeper:...

  • alexpott committed 891e57e on 8.0.x
    Issue #2609874 by tstoeckler, mikeker, Kristen Pol, tassilogroeper:...
Gábor Hojtsy’s picture

Issue tags: -sprint

Superb, thanks!

Gábor Hojtsy’s picture

Would be a string change in 8.0.6 then I guess.

Status: Fixed » Closed (fixed)

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