Text and text with summary field default value config does not use the text_format schema type

Created on 24 November 2014, almost 10 years ago
Updated 19 December 2023, 11 months ago

Problem/Motivation

The default values of text fields do not use the text_format schema type which was introduced in #2144413: Config translation does not support text elements with a format . This means that the default value of text fields can not be properly translated currently.

Note that this is not only a UX problem (i.e. no CKEditor for translators) but can also lead to security problems.

Proposed resolution

Use the text_format schema type where appropriate.

The text_with_summary field type has a slightly different structure (i.e. it has an additional summary key) so a dedicated schema type is introduced for that. In order to leverage that, a dedicated Config Translation form element is added, to allow that to be translated. Note that this could be split off and handled in a follow-up.

Remaining tasks

User interface changes

Default values of text fields can be translated using a text editor.

API changes

The schema structure of text fields changes (specifically of the default value declaration). Note that the config structure itself does not change!

Beta phase evaluation

🐛 Bug report
Status

Needs work

Version

11.0 🔥

Component
Text 

Last updated 15 days ago

Created by

🇩🇪Germany tstoeckler Essen, Germany

Live updates comments and jobs are added and updated live.
  • D8MI

    (Drupal 8 Multilingual Initiative) is the tag used by the multilingual initiative to mark core issues (and some contributed module issues). For versions other than Drupal 8, use the i18n (Internationalization) tag on issues which involve or affect multilingual / multinational support. That is preferred over Translation.

  • Needs manual testing

    The change/bugfix cannot be fully demonstrated by automated testing, and thus requires manual testing in a variety of environments.

  • Needs issue summary update

    Issue summaries save everyone time if they are kept up-to-date. See Update issue summary task instructions.

Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • 🇨🇳China jungle Chongqing, China

    First, posting the raw interdiff content of 0 bytes interdiff-32-33.txt

    $ diff 2381147-32.patch 2381147-33.patch
    83c83
    < index 42d54b21ef..d1d7bad78f 100644
    ---
    > index f60c0a56ee..fc053f29f8 100644
    94c94
    < @@ -1108,6 +1109,70 @@ public function testMenuTranslationWithoutChange() {
    ---
    > @@ -1111,6 +1112,70 @@ public function testMenuTranslationWithoutChange() {
    191c191
    < index 08bc9aad87..5a9f3ac4f4 100644
    ---
    > index e0fa5d3d83..ae2ab6ee2f 100644
    194,195c194,195
    < @@ -21,15 +21,8 @@ field.field_settings.text:
    <    label: 'Text (formatted) settings'
    ---
    > @@ -27,15 +27,8 @@ field.field_settings.text:
    >          type: string
    211,212c211,212
    < @@ -40,15 +33,8 @@ field.field_settings.text_long:
    <    type: mapping
    ---
    > @@ -52,15 +45,8 @@ field.field_settings.text_long:
    >          type: string
    228,229c228,229
    < @@ -66,18 +52,8 @@ field.field_settings.text_with_summary:
    <        label: 'Require summary'
    ---
    > @@ -83,18 +69,8 @@ field.field_settings.text_with_summary:
    >          type: string
    
    
  • 🇨🇳China jungle Chongqing, China

    2. review the schema changes

    Posting the schema of text_format from core/config/schema/core.data_types.schema.yml

    # Text with a text format.
    text_format:
    type: mapping
    label: 'Text with text format'
    # We declare the entire mapping of text and text format as translatable. This
    # causes the entire mapping to be saved to the language overrides of the
    # configuration. Storing only the (to be formatted) text could result in
    # security problems in case the text format of the source text is changed.
    translatable: true
    mapping:
    value:
    type: text
    label: 'Text'
    # Mark the actual text as translatable (in addition to the entire mapping
    # being marked as translatable) so that shipped configuration with
    # formatted text can participate in the string translation system.
    translatable: true
    format:
    type: string
    label: 'Text format'
    # The text format should not be translated as part of the string
    # translation system, so this is not marked as translatable.

    +++ b/core/modules/text/config/schema/text.data_types.schema.yml
    @@ -0,0 +1,19 @@
    +# See the 'text_format' type in core.data_types.schema.yml.
    +text_format_with_summary:
    +  label: 'Default'
    +  type: mapping
    +  translatable: true
    +  # Integrate with the Configuration Translation module.
    +  form_element_class: 'Drupal\text\FormElement\TextFormatWithSummary'
    ...
    +    value:
    +      type: text
    +      label: 'Body'
    +      translatable: true
    +    summary:
    +      type: string
    +      label: 'Summary'
    +      translatable: true
    +    format:
    +      type: string
    +      label: 'Text format'
    
    • With the form_element_class key which is good, see hook_config_schema_info_alter below
    • format is not translatable as text_format
    function hook_config_schema_info_alter(&$definitions) {
      // Enhance the text and date type definitions with classes to generate proper
      // form elements in ConfigTranslationFormBase. Other translatable types will
      // appear as a one line textfield.
      $definitions['text']['form_element_class'] = '\Drupal\config_translation\FormElement\Textarea';
      $definitions['date_format']['form_element_class'] = '\Drupal\config_translation\FormElement\DateFormat';
    }
  • Status changed to Needs work over 1 year ago
  • 🇪🇸Spain penyaskito Seville 💃, Spain 🇪🇸, UTC+2 🇪🇺
    +++ b/core/modules/config_translation/src/FormElement/TextFormat.php
    @@ -27,14 +28,15 @@ public function getSourceElement(LanguageInterface $source_language, $source_con
    -    return [
    +    return NestedArray::mergeDeep(parent::getTranslationElement($translation_language, $source_config, $translation_config), [
    ...
    -    ] + parent::getTranslationElement($translation_language, $source_config, $translation_config);
    

    Should we respect the order here? I'm not sure if it's relevant in this case, but could hide bugs.
    PHP arrays addition is not commutative (A+B != B+A).

    I don't see why those css classes are needed now and not before, but being fair I didn't really test it.

    +++ b/core/modules/text/config/schema/text.schema.yml
    @@ -27,15 +27,8 @@ field.field_settings.text:
     field.value.text:
    -  type: mapping
    +  type: text_format
       label: 'Default value'
    -  mapping:
    -    value:
    -      type: label
    -      label: 'Value'
    -    format:
    -      type: string
    -      label: 'Text format'
    

    This is great!

  • 🇨🇳China jungle Chongqing, China
    +++ b/core/modules/config_translation/tests/src/Functional/ConfigTranslationUiTest.php
    @@ -1111,6 +1112,70 @@ public function testMenuTranslationWithoutChange() {
    +   * Test translation of a default value of a text with summary field.
    

    As it set back to NW, here should start with 'Tests', #3133162: Replace the start verb Test with Tests in method comments of tests

    I am trying to test it manually, but I can not get the same result as the test. Leaving this issue temporarily

  • 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
    1. +++ b/core/modules/text/config/schema/text.data_types.schema.yml
      @@ -0,0 +1,19 @@
      +# See the 'text_format' type in core.data_types.schema.yml.
      

      While we're making this consistent, we should move the text_format type out of core.data_types.schema.yml and into this file.

    2. +++ b/core/modules/text/src/FormElement/TextFormatWithSummary.php
      @@ -0,0 +1,41 @@
      +use Drupal\config_translation\FormElement\TextFormat;
      

      Let's move this class into the text module too while we're at it?

  • last update about 1 year ago
    30,388 pass, 10 fail
  • last update about 1 year ago
    30,388 pass, 10 fail
  • 🇺🇸United States smustgrave

    Sorry for taking so long to get back to this.

    #39

    Should we respect the order here? I'm not sure if it's relevant in this case, but could hide bugs.

    that I'm not 100% about, may need a bigger brain then my own

    #40 fixed

    #41.1 moved
    #41.2 moved

    Moved to MR so hiding patches

  • last update about 1 year ago
    30,403 pass, 2 fail
  • last update about 1 year ago
    30,403 pass, 2 fail
  • last update about 1 year ago
    30,403 pass, 2 fail
  • last update about 1 year ago
    30,403 pass, 2 fail
  • last update about 1 year ago
    30,415 pass
  • Status changed to Needs review about 1 year ago
  • 🇺🇸United States smustgrave

    Reverted #41.1 as it caused some failures and may want to handle in a follow up.

  • Status changed to RTBC about 1 year ago
  • 🇧🇪Belgium borisson_ Mechelen, 🇧🇪

    I agree we don't need to do #41.1 in this issue, it can be a followup for later. Can we file that already as a seperate issue? Otherwise this looks good to go imho.

  • last update about 1 year ago
    30,428 pass
  • 🇬🇧United Kingdom longwave UK

    Tagging so we don't forget #41.

  • 🇪🇸Spain penyaskito Seville 💃, Spain 🇪🇸, UTC+2 🇪🇺

    Created 📌 Move TextFormat form element from config_translation to text module Active
    Created 📌 Move text_format data type from core to text module Active

    I think they are independent of each other. Removing "needs followup" tag.

  • Status changed to Needs review about 1 year ago
  • last update about 1 year ago
    Custom Commands Failed
  • last update about 1 year ago
    Custom Commands Failed
  • last update about 1 year ago
    30,429 pass
  • 🇺🇸United States smustgrave

    Reverted the move.

  • Status changed to RTBC about 1 year ago
  • 🇪🇸Spain penyaskito Seville 💃, Spain 🇪🇸, UTC+2 🇪🇺

    We have split the bits that require BC layer into follow-ups. The fix itself was RTBC before, and I think it is, so setting it back to RTBC.

  • last update about 1 year ago
    30,439 pass
  • last update about 1 year ago
    30,444 pass
  • last update about 1 year ago
    30,466 pass
  • last update about 1 year ago
    30,485 pass
  • last update about 1 year ago
    30,487 pass
  • last update about 1 year ago
    30,488 pass
  • last update about 1 year ago
    30,490 pass
  • 29:20
    26:18
    Running
  • last update about 1 year ago
    30,518 pass
  • last update about 1 year ago
    30,530 pass
  • last update about 1 year ago
    30,553 pass
  • last update about 1 year ago
    30,562 pass
  • last update about 1 year ago
    30,604 pass
  • Status changed to Needs work about 1 year ago
  • 🇳🇿New Zealand quietone

    I'm triaging RTBC issues . I read the IS and the comments. I didn't find any unanswered questions or other work to do.

    The diff does not apply to 11.x, there setting to NW for a rebase.

    This may need a CR with a screenshot informing the user of this new capability.

  • First commit to issue fork.
  • Status changed to Needs review about 1 year ago
  • 🇮🇳India ankithashetty Karnataka, India

    Rebased done, hence moving to NR.
    Still need a update on CR as requested in #52.

    Thanks!

  • Status changed to RTBC about 1 year ago
  • 🇺🇸United States smustgrave

    Restoring status after reroll.

  • last update about 1 year ago
    30,607 pass
  • last update 12 months ago
    30,608 pass
  • last update 12 months ago
    30,670 pass
  • last update 12 months ago
    30,677 pass
  • last update 12 months ago
    30,686 pass
  • last update 12 months ago
    30,690 pass
  • last update 12 months ago
    30,690 pass
  • last update 12 months ago
    30,690 pass
  • last update 12 months ago
    30,698 pass
  • last update 12 months ago
    30,700 pass
  • last update 12 months ago
    30,714 pass
  • last update 11 months ago
    30,726 pass
  • last update 11 months ago
    30,766 pass
  • last update 11 months ago
    30,768 pass
  • Status changed to Needs work 11 months ago
  • 🇳🇿New Zealand quietone

    Always nice to see an old issue getting attention, thanks!

    Comment #53 states that this is in need of a CR, therefor setting back to needs work.

    To learn more about what this is fixing I tested on Drupal 11.x today, standard install, with a second language and a Textwithsummary field. I was able to translate the default value field without the patch here. Yet, the issue summary states that this change is to allow, "Default values of text fields can be translated using a text editor." Am I missing something? I don't work with translations so maybe I am.

    I applied that patch and the translation modal was changed so that both the english default value and the translation default value can be edited. I don't think that it correct, only the translation should be editable. In fact the title is "Edit Italian translation for TextWithSummary" so it should only be for the Italian version.

    It is not clear to me what is being fixed here. Tagging for an issue summary update. And since this is changing the UI it should have links to up to date before/after screenshots in the issue summary.

    Looking back at the comments I do not see any manual testing. I am adding a tag for that.

Production build 0.71.5 2024