Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue priority Major because the API changes required to support the integration are not possible later.
Disruption Changes are within config_translation, language and locale modules. Only modules integrating with the typed configuration / configuration translation API are affected and those modules are very limited in number.

Updated: Comment #100

Problem/Motivation

The underlying problem that spawned this issue is:
It is currently not possible to translate values configuration values that consist of text with a text format.

The discussion of this problem and its resolution revealed a number of corollary problems:

  • Because Views allows to place formatted text into Empty/Header/Footer areas and Configuration Translation allows to translate those views you can currently translate the formatted text, but without the context of the actual text format. This is both confusing and - as the source value is simply printed into the HTML - allows XSS injection in the Configuration Translation UI. (This is exploitable for anyone with the administer views permission.) Note that this specific issue will be fixed in #2144505: Views does not use the text format type for formatted text, but this issue will lay the groundwork for that one.
  • Configuration Translation can only handle primitive data types (in other words: strings) and is not capable of translating complex data structures. In this case we want a mapping containing a value and a format needs to be translated.
  • Configuration Translation explicitly invokes the API of Interface Translation to integrate shipped configuration with localize.drupal.org. When updating language overrides of shipped configuration from code, however, Interface Translation does not react properly leaving the system in an inconsistent state where the string translations do not match the configuration translations.
  • LanguageConfigOverride does not dispatch an event when saving or deleting the configuration override.
  • There is no way to retrieve the language code of a LanguageConfigOverride.
  • Instead of traversing the configuration schema Configuration Translation traverses the form values. This is not a bug, but traversing the schema is superior.
  • The form structure of the Configuration Translation UI is repetitive.
  • Form element classes cannot override the display of the source configuration in the Configuration Translation UI.
  • Interface Translation invokes unnecessary deletes on non-existent language overrides.

Proposed resolution

  1. Add a configuration schema type of text_format that is translatable (on the mapping level) so that Configuration Translation saves the entire object into the language override. The mapping consists of a value sub-element that is a translatable string and a format sub-element that is a non-translatable string. This way Interface Translation only synchronizes the value with localize.drupal.org, but not the format.
  2. Adapt ConfigTranslationFormBase to support complex data structures that are translatable and save them wholesale. To facilitate this the form structure is adapted to be less repetitive and to be consistent with the configuration structure. As part of this the traversing of the configuration schema is moved into a ListElement which is used as the form element class for the Mapping and Sequence schema types.
  3. Remove the Interface Translation integration from ConfigTranslationFormBase and into a LocaleConfigSubscriber that reacts to the ConfigEvents::SAVE_OVERRIDE and ConfigEvents::DELETE_OVERRIDE events.
  4. Add a FormElementBase class which provides base logic for simple form elements.
  5. Add a TextFormat form element to the configuration translation system.
  6. Change locale_config_update_multiple() to not (needlessly) delete non-existent language overrides. This is fixed as part of this issue because it is necessary to make tests pass.

Remaining tasks

Steps to test

  1. Apply patch
  2. Install the Configuration Translation Test (config_translation_test) module. (Note that this requires the $settings['extension_discovery_scan_tests'] = TRUE; trick which is included in the shipped settings.local.php
  3. Add a language on /admin/config/regional/language/add
  4. Go to /admin/config/media/file-system/translate
  5. Ignore the Edit link, and instead Add a translation.
  6. You should see Translation form shows 'Hello World' in a disabled texarea as source and a textarea to change it, which has an input format select element.
  7. For extra credit:

  8. Uninstall the Configuration Translation Test module
  9. Change plain_text to basic_html in core/modules/config_translation/tests/modules/config_translation_test/config/install/config_translation_test.content.yml
  10. Install the Configuration Translation Test module again
  11. Go to /admin/config/media/file-system/translate
  12. Ignore the Edit link, and instead Add a translation.
  13. You should see Translation form shows 'Hello world' in a disabled CKEditor as source and a CKEditor to change it which has an input format select element.
  14. Click on the respective source button on both sides.
  15. You should see Translation form shows 'Hello world' in a disabled CKEditor source view as source and a CKEditor to change it which has an input format select element.

User interface changes

None.

API changes

  • \Drupal\config_translation\FormElement\ElementInterface::getFormElement() is renamed to getTranslationBuild() and is changed in behavior to include the entire subform for this element, i.e. both souce and translation.
  • \Drupal\config_translation\FormElement\ElementInterface::setConfig() is introduced. Because form elements can now control the entire form structure, they are also responsible for setting the configuration values correctly.

API additions

  • The text_format configuration schema type.
  • ConfigEvents::SAVE_OVERRIDE and ConfigEvents::DELETE_OVERRIDE events and a corresponding ConfigOverrideCrudEvent class are introduced.
  • A LanguageConfigOverride::getLangcode() is introduced.
  • The ConfigEvents::SAVE_OVERRIDE and ConfigEvents::DELETE_OVERRIDE events are invoked from LanguageConfigOverride.
  • \Drupal\config_translation\FormElement\FormElementBase and \Drupal\config_translation\FormElement\ListElement are introduced.
Files: 
CommentFileSizeAuthor
#186 2144413-185-config-translation-text-format.patch105.92 KBtstoeckler
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,603 pass(es). View
#124 2144413-124-config-translation-text-format.patch76.84 KBkfritsche
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,297 pass(es). View
#121 2144413-121-config-translation-text-format.patch75.24 KBkfritsche
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,294 pass(es), 0 fail(s), and 2 exception(s). View
#108 2144413-108-config-translation-text-format.patch75.9 KBtstoeckler
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,363 pass(es), 4 fail(s), and 0 exception(s). View
#72 config-translation-text-format-no-editr.png36.94 KBtstoeckler
#70 config_translation_ckeditor_original-sources.png78.07 KBWim Leers
#70 config_translation_ckeditor_original.png74.69 KBWim Leers

Comments

webflo’s picture

FileSize
10.46 KB
FAILED: [[SimpleTest]]: [MySQL] 59,035 pass(es), 6 fail(s), and 0 exception(s). View
Gábor Hojtsy’s picture

Issue tags: +D8MI, +language-config, +sprint
Gábor Hojtsy’s picture

  1. +++ b/core/modules/config_translation/config_translation.module
    @@ -160,6 +160,7 @@ function config_translation_config_translation_type_info_alter(&$definitions) {
       // 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';
    +  $definitions['text_with_format']['form_element_class'] = '\Drupal\config_translation\FormElement\TextFormat';
     }
    
    +++ b/core/modules/config_translation/lib/Drupal/config_translation/Form/ConfigTranslationFormBase.php
    @@ -262,8 +262,23 @@ protected function buildConfigForm(Element $schema, $config_data, $base_config_d
    +      if ($element instanceof Element && !isset($definitions[$element_type]['form_element_class'])) {
    
    @@ -297,17 +312,8 @@ protected function buildConfigForm(Element $schema, $config_data, $base_config_d
    -        if (!isset($definition['translatable']) || !isset($definition['type'])) {
    +        if (!isset($definition['form_element_class']) && (!isset($definition['translatable']) || !isset($definition['type']))) {
    

    Discussed this with @webflo. This would be much cleaner if the type altering would be putting in the form class all the time, so we can remove this stacked logic. The alter could put in the form element class if it has a type and translatable. Then we'd have that one place to look for what is what.

  2. +++ b/core/modules/config_translation/lib/Drupal/config_translation/Form/ConfigTranslationFormBase.php
    @@ -262,8 +262,23 @@ protected function buildConfigForm(Element $schema, $config_data, $base_config_d
    +      if ($element_key == 'display.default.display_options.header.area_1.content') {
    +        $debug = TRUE;
    +      }
    

    debug :D

Status: Needs review » Needs work

The last submitted patch, 1: config-translation-text_format-2144413-1.patch, failed testing.

webflo’s picture

FileSize
23.37 KB

Next try. No interdiff because i forgot a few files in the first patch ...

webflo’s picture

FileSize
14.34 KB
FAILED: [[SimpleTest]]: [MySQL] 59,064 pass(es), 68 fail(s), and 0 exception(s). View

Doh.

webflo’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 6: config-translation-text_format-2144413-3.patch, failed testing.

webflo’s picture

Status: Needs work » Needs review
FileSize
4.73 KB
14.14 KB
PASSED: [[SimpleTest]]: [MySQL] 59,007 pass(es). View

Ok, the logic in config_translation_config_translation_type_info_alter was wrong ...

YesCT’s picture

starting to write how to test steps in the summary.

remaining task: Convert the existing elements in core
needs specifics. are they just the views, and so will be done in that views issue, or are there others? How do we know where they are?

YesCT’s picture

Assigned: Unassigned » YesCT
FileSize
170.98 KB

I'm not sure why the test uses the media file system config form for the test. Makes the test rely on that. Can we make our own test config thing and use that? Maybe we already have one.

Screenshot of that translate page though.

screenshot showing the translate form with the text format'd widgit

But on save I get:

Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[21S01]: Insert value list does not match column list: 1136 Column count doesn't match value count at row 1: INSERT INTO {locales_target} (lid, language, translation, customized) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2_value, :db_insert_placeholder_2_format, :db_insert_placeholder_3); Array ( [:db_insert_placeholder_0] => 215 [:db_insert_placeholder_1] => af [:db_insert_placeholder_3] => 1 [:db_insert_placeholder_2_value] => <strong>Hello World</strong> -AF [:db_insert_placeholder_2_format] => plain_text ) in Drupal\locale\StringDatabaseStorage->dbStringInsert() (line 478 of /Users/ctheys/foo/drupal/core/modules/locale/lib/Drupal/locale/StringDatabaseStorage.php). 

spotted a few little things in the patch to fix. new patch coming.

YesCT’s picture

Issue summary: View changes

added install to steps to test

YesCT’s picture

Assigned: YesCT » Unassigned
Related issues: +#2145633: Use standard discovery facility in TypedConfigManager instead of one-off implementation
FileSize
2.76 KB
14.15 KB
PASSED: [[SimpleTest]]: [MySQL] 59,134 pass(es). View
  1. +++ b/core/modules/config_translation/config_translation.module
    @@ -153,13 +153,28 @@ function config_translation_entity_operation_alter(array &$operations, EntityInt
    + * @todo: Convert this hook into a real typed date alter hook.
    

    making issue for this.
    (and no : in @todo)
    https://drupal.org/node/1354#todo
    updated comment.

  2. +++ b/core/modules/config_translation/config_translation.module
    @@ -153,13 +153,28 @@ function config_translation_entity_operation_alter(array &$operations, EntityInt
    +    if (isset($definition['translatable']) && ($definition['translatable'] == TRUE) && !isset($definitions[$type]['form_element_class'])) {
    +      $definitions[$type]['form_element_class'] = '\Drupal\config_translation\FormElement\Textfield';
    +    }
    

    maybe this should be an else if, and why only if not a form element? Should we set the default to be textfield anyway? what else might definitions be if not type form_element_class?

    I didn't fix anything here. waiting for feedback.

  3. +++ b/core/modules/config_translation/lib/Drupal/config_translation/Form/ConfigTranslationFormBase.php
    @@ -262,8 +262,19 @@ protected function buildConfigForm(Element $schema, $config_data, $base_config_d
           $definition = $element->getDefinition() + array('label' => $this->t('N/A'));
    -      if ($element instanceof Element) {
    +
    +      // Invoke hook_config_translation_type_info_alter() implementations to
    
    @@ -296,27 +307,12 @@ protected function buildConfigForm(Element $schema, $config_data, $base_config_d
    -      else {
    -        $definition = $element->getDefinition();
    ...
    -        if (!isset($definition['translatable']) || !isset($definition['type'])) {
    

    oh, maybe this logic is being moved into the hook.

  4. +++ b/core/modules/config_translation/lib/Drupal/config_translation/FormElement/Element.php
    @@ -6,6 +6,7 @@
     namespace Drupal\config_translation\FormElement;
    +use Drupal\Core\Language\Language;
    

    nit, missing newline. added.

  5. +++ b/core/modules/config_translation/lib/Drupal/config_translation/FormElement/ElementInterface.php
    @@ -29,4 +29,22 @@
    +   * @param $key
    +   *   The key in the configuration object.
    

    adding type, string

  6. +++ b/core/modules/config_translation/lib/Drupal/config_translation/Tests/ConfigTranslationUiTest.php
    @@ -23,7 +23,7 @@ class ConfigTranslationUiTest extends WebTestBase {
    -  public static $modules = array('contact', 'config_translation', 'config_translation_test', 'views', 'views_ui', 'contextual');
    +  public static $modules = array('contact', 'config_translation', 'config_translation_test', 'views', 'views_ui', 'contextual', 'filter');
    

    not sure why we are using the media filter config for this test. Can we use a test config so we dont rely on that? Maybe we have a test config already we can re-use or copy from another test.

  7. +++ b/core/modules/config_translation/tests/modules/config_translation_test/config/config_translation_test.content.yml
    @@ -0,0 +1,7 @@
    +uuid: 7ae02896-32ae-4875-9a00-f252939fa849
    +langcode: en
    

    I dont think we put uuid's in default config. Maybe that was just copied based on an active config file.

    also, it's been a while but I remember langcode: en coming last in config files. Not sure, and doesn't really matter as this is test config.
    removed uuid, didnt move langcode.

Status: Needs review » Needs work

The last submitted patch, 13: config-translation-text_format-2144413-13.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
Gábor Hojtsy’s picture

@YesCT: indeed, we only found text with format in config in views by grepping the code. So no other core is affected as far as we found.

Gábor Hojtsy’s picture

@YesCT: re #11/#13 on why we use the file settings screen, that is also what the other existing test does. We can define our own route and attach our config to that route. This is just a shortcut to not needing to define a route with a valid callback (that would be lots of overhead just so we can attach the config translation stuff to *something*). We just picked an existing route. Again, that is what the existing test already does for the alter hooks.

#13/1: added reasoning on the todo at #2145633: Use standard discovery facility in TypedConfigManager instead of one-off implementation

#13/2: we only set the form element class if not already set. this allows contrib to extend this with other type form classes provided; if we overwrite it, then contrib does not get a chance to extend; in fact !isset($definitions[$type]['form_element_class']) should probably be wrapping the whole contents of the foreach; and yes, the second if() should be an elseif then

#13/3: yes, but we can still improve it, see 2 above.

#13/4-5: thanks

#13/6: we use our own config; we just don't define a base route for our test, because that has no role in the test, so we picked an existing route; the test needs filter module because the text element with filters are only meaningful if you have filter module :)

#13/7: the default config in core is chock full of default uuids, but indeed, they only make sense for config entities, thanks for removing this

So looks like 2 is directly actionable (if logic in the hook) and we can add a route to make it a bit more cleaner, but its just doing what the tests already do. However if the behavior itself is as buggy as you experienced in #11, that sounds concerning. Why does the test pass (which also has locale module enabled and foreign language configured?).

Gábor Hojtsy’s picture

FileSize
2.86 KB
14.94 KB
PASSED: [[SimpleTest]]: [MySQL] 59,237 pass(es). View

Updated with code tidying for the foreach and comments on the route - config mappings that these are just for testing and does not really matter. I think its overkill to define a test route just so we can mostly ignore it and attach a config translation tab on it when we can do it on any existing route just as easily. I also added the comment to the existing core test (on theme translatability, sorry, not alters :).

This should address all code review points. Now is this buggy when translating? :)

Gábor Hojtsy’s picture

Status: Needs review » Needs work

This is said to still be buggy with locale, its getting an array instead of a string.

webflo’s picture

Issue tags: -sprint +SprintWeekend2013, +#D8MA
webflo’s picture

Issue tags: +sprint

Doh.

Gábor Hojtsy’s picture

Fix weekend tag.

webflo’s picture

Issue tags: -#D8MA +D8MA
webflo’s picture

FileSize
14.95 KB
FAILED: [[SimpleTest]]: [MySQL] 63,110 pass(es), 2 fail(s), and 0 exception(s). View

Re-roll.

@Gabor: Can you summarize the problems with your last patch? I applied it and translated a element (with text format). But could not find any errors. Thanks!

webflo’s picture

Status: Needs work » Needs review
prodosh’s picture

Issue summary: View changes
FileSize
269.69 KB
160.62 KB

Status: Needs review » Needs work

The last submitted patch, 24: config-translation-text_format-2144413-24.patch, failed testing.

tstoeckler’s picture

Re @prodosh: I think that is this issue: https://drupal.org/node/2167039 not this one

Gábor Hojtsy’s picture

@webflo: the problem is explained in #11, when translating default config, the locale integration fails due to now getting an array instead of a string when locale saves :/

tstoeckler’s picture

+++ b/core/modules/system/config/schema/system.data_types.schema.yml
@@ -99,3 +99,16 @@ route:
+  translatable: true
...
+      label: 'Content'
...
+      label: 'Text format'
...
+      type: string
...
+    fomrat:
...
+      type: text
...
+    value:
...
+  mapping:
...
+  label: 'Text with format'
...
+text_with_format:
...
+# Human readable string that is associated with a format.
...
+  type: mapping

Discussed this with @webflo on the weekend. The problem can probably be solved by moving the translatable property one level down into 'value'. Having the format translatable is pointless anyway.

We also discussed that we want to hide the text format selector on the translation form and we need #1423244: #allowed_formats property for #type 'text_format' to filter available formats for that. I'm not sure if we want to actually postpone this issue on that, though. We could just as well introduce '#allowed_formats' here, and then it would start working as soon as that issue goes in.

Gábor Hojtsy’s picture

Yeah if we don't allow modification of the format, then we need to work that out ourselves (and still keep the possible WYSIWYG working)

tstoeckler’s picture

Oohh WYSIWYG is another interesting dimension to this. So does that mean you are in favor of (hard) postponing this on #1423244: #allowed_formats property for #type 'text_format' to filter available formats? I would personally be pretty strongly against doing our own form processing until that lands.

Gábor Hojtsy’s picture

Yeah WYSIWYG integration should work out of the box by the nature of using the text format element. I don't think there is a problem with letting translators pick other formats they have access to. The whole formats vs. translators problem is a mess anyway. Eg. i18n has a full settings setup for permissions, since you may have issues translating to target formats that you otherwise don't have access to. I don't think we should solve or think of that problem here, just saying :D

YesCT’s picture

Assigned: Unassigned » YesCT
Issue tags: +Needs reroll

I'm going to reroll this now. (https://drupal.org/patch/reroll)

YesCT’s picture

Assigned: YesCT » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
14.95 KB
FAILED: [[SimpleTest]]: [MySQL] 63,405 pass(es), 5 fail(s), and 0 exception(s). View

That was an easy reroll, no conflicts.
Needs review to see if this gets the same fails as #24.

YesCT’s picture

Issue summary: View changes
FileSize
169.4 KB

I took out the instructions to reproduce the error when translating the body label on a content type.
I was able to do that without an error, and also, that translation form did not use a input filter.

YesCT’s picture

Issue summary: View changes

gr. img tag

Status: Needs review » Needs work

The last submitted patch, 35: config-translation-text_format-2144413-35.patch, failed testing.

YesCT’s picture

Issue summary: View changes

as I summarized irc conversation into pros and cons of different possible solutions, I could see they were almost mirrors. So it's possible to maybe just take out the cons, of each and just list the pros, and assume one pro of one is the con of the other. But I left it spelled out as I think it might be more expressive to be able to word them different and they might change as more people voice their opinion.

YesCT’s picture

Issue summary: View changes
FileSize
228.95 KB
194.5 KB

forgot the screenshots

robertdbailey’s picture

FileSize
37.19 KB
37.19 KB

I was able to reproduce the error per the test steps (attached).

Schnitzel’s picture

Status: Needs work » Needs review
FileSize
14.95 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,611 pass(es), 5 fail(s), and 1 exception(s). View

first a reroll

Status: Needs review » Needs work

The last submitted patch, 42: config-translation-text_format-2144413-42.patch, failed testing.

Schnitzel’s picture

Assigned: Unassigned » Schnitzel
Schnitzel’s picture

Status: Needs work » Needs review
FileSize
10.8 KB
20.9 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,762 pass(es). View

so, fixed all the failing tests, and also implemented a new function in ElementInterface which allows to return the translation string.

I have some strange things with running the hook_config_translation_type_info_alter in the submit handler of the Form.
Because when building the form the patch iterates through all the Config Schema, but while saving the translation I don't see a possibility to iterate through them, because we already iterate through the provided values. So I make some ugly things (in my opinion) to find if there is a schema for some data types.

Schnitzel’s picture

FileSize
2.1 KB
20.82 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,980 pass(es). View

discussed with tstoekler and where not sure if we needed the check for all the keys.

let's see what the Testbot says.

robertdbailey’s picture

FileSize
20.76 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,609 pass(es). View

updated patch to reflect the new location of the core.data_types.schema.yml file.

robertdbailey’s picture

FileSize
985 bytes

Patch #46 did not apply because of the changed file location. So, patch #47 is a manual re-roll. As such, there is no proper interdiff possible, so the attached "interdiff" is just a diff between patch files 46 and 47.

robertdbailey’s picture

Issue summary: View changes

updated summary (steps to test) to include instructions for temporarily moving module config_translation_test into the modules directory because of test modules no longer being discovered (issue https://drupal.org/node/2188661), with a likely long-term solution being a settings.php variable to discover test extensions at runtime (issue https://drupal.org/node/2198713).

robertdbailey’s picture

FileSize
40.74 KB
23.46 KB

Followed the test steps in the issue summary, and all seems to work properly: Step 11's error now does not appear. Before submission:

After submission:

robertdbailey’s picture

FileSize
6.55 KB
23.5 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,941 pass(es), 76 fail(s), and 8 exception(s). View

Reviewing this with @tstoeckler, changed the 'translatable' flag for config_names from the mapping level to the underlying field level. This currently appears to work from the interface side; however, the created yml file still contains the "format" attribute of underlying sub-field "content". This still needs work to remove this from the yml file.

Update 2014.04.21: I have tried to duplicate the testing failures locally, and I have not been able to do so. On my local machine, all tests under Configuration Translation pass. Can anyone think of why this might be? In any case, since I am not able to regenerate the failures using individual testing. The previous failure for the current patch was the following string:

FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,506 pass(es), 76 fail(s), and 0 exception(s).

All failures above were in ConfigTranslationUiTest.php. I will rerun the SimplyTest.Me in case perhaps the failures go through on another pass.

Status: Needs review » Needs work

The last submitted patch, 51: config-translation-text_format-2144413-51.patch, failed testing.

robertdbailey’s picture

Some of the form-field names changed as part of this latest patch, so tests that were referencing the old field names are now failing. I'll update those and then resubmit the patch.

robertdbailey’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 51: config-translation-text_format-2144413-51.patch, failed testing.

Gábor Hojtsy’s picture

Assigned: Schnitzel » Unassigned

@robertdbailey: are you working on that patch update?

robertdbailey’s picture

@Gábor Hojtsy: I got stuck on this as of 4/21, but I will reattempt this as my project for this week!

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
27.71 KB
34.47 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,806 pass(es), 5 fail(s), and 1 exception(s). View

Here we go.

This does the following:
- Adds a bunch of inline comments
- Removes references to ElementInterface::getTranslationString() as it is no longer needed.
- Fixes tests. Most of the changes were due to the element name changes, as 'translation' is no longer a parent, and the form key is no just the sub-key name, and not the entire tree. In other words: Before the name was config.file.name[top-level][second-level][translation][top-level.second-level.third-level] and now it is just config.file.name[top-level][second-level][third-level]. So that's certainly for the better IMO, but required a bunch of updates.
There are not in the interdiff, but were part of the merge with 8.x:
- Moves the configuration into config/install to keep up with 8.x
- Latest 8.x also removed the Element base class in favor of the string translation trait. Instead of re-introducing that here, I moved the getRenderedSource() base method to a SimpleSourceTrait.

Status: Needs review » Needs work

The last submitted patch, 58: 2144413-58-config-translation-text-format.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
788 bytes
34.56 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,832 pass(es). View

This should be green.

Forgot a use statement during the merge.

YesCT’s picture

Issue tags: +Traits
Related issues: +#2134513: [policy, no patch] Trait strategy
sun’s picture

Status: Needs review » Needs work

AFAICS, this is an inappropriate use of traits — the code requires the getRenderedSource() method to exist.

A trait is overkill here. A new FormElementBase class is much more appropriate - provide a default implementation; if you don't like it, override it (or don't use the base class in the first place).

tstoeckler’s picture

Status: Needs work » Needs review

Even though I wouldn't lose sleep over re-introducing the FormElementBase class, I'd like to get some more opinions on this first.

1.

A trait is overkill here.

I don't see how a trait can be "overkill". A base class is nothing but a trait with the additional constraint of (single-)inheritance. So in terms of "kill": base class > trait.

2.

the code requires the getRenderedSource() method to exist.

The code requires $this->t() just as well, so if that is a trait now, I don't see why getRenderedSource() shouldn't be on a trait either.

3. In general I (and not only I) think that traits are superior to base classes because they do not enforce any inheritance chain (see 1.). One could argue about the value of converting all existing base classes, but for new code, I don't really see the point of introducing new base classes.

Wim Leers’s picture

I haven't read through the entire issue, but if the screenshot in the issue summary reflects what this patch does:

Then I have one concern: the original is shown in plain HTML, the translation is shown in the WYSIWYG editor. That's less than great.

Why don't we show both in the same way? You can load CKEditor in read-only state for the original. A quick search tells me that setting the disabled attribute on the <textarea> should do the trick, so setting #disabled = TRUE should do it for us.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

@WimLeers: that is amazing! Woot!

tstoeckler’s picture

Well, the screenshot is inaccurate (sorry!), we're actually showing the filtered markup right now. But showing a disabled CKEditor would be *way* cooler. Thanks @WimLeers!

Jose Reyero’s picture

Taking a look at the code, not sure I get everything -since I am not up to date with all the config translation thing - but just some thoughts:

- (Strongly) Agree with sun #62: adding a trait for this is overkill. Nope, traits are no replacement for base classes, just some device to use when you cannot effectively use simple class inheritance. Call me old fashion...

- I think the 'text_with_format' type should have the 'translatable' property set to TRUE and we should treat it as an object that is translatable by itself (and rearchitect what it takes so we can translate any data type with a simple extension, otherwise contrib modules will need to ditch this and provide their own translation form to translate other types)

- About the widget, the disabled CKEditor sounds good, as long as it allows to see the source. Some translators may prefer the formatted text, some other may prefer the markup and in any case translating the markup will allow more accurate translations. So I think either we show both or we show only the markup. Note also that there may be non-visible language dependent stuff in the markup (lang="xx", links, etc..)

- Btw, are we handling the 'you cannot access this format case' when there is a translation already that the translator cannot edit? (Some message, no submit button, etc...)

- About the 'form_element_class', maybe too late but, can't we add it into the core data types with some other hook that is not 'config translation' specific? There should be some generic way to alter the data types, right?

Overall, this is maybe too complex and I wonder whether needing all this to make one more 'data type' translatable doesn't mean we should improve how the 'configuration translation' feature in general is architected. I mean it should need a plain form element class to make one more different data type translatable and this one is a proof of concept for how extendable is the translation form in that sense. It seems not too much :-/

Jose Reyero’s picture

I need to add some note: Thought my latest comment sounds overly negative, I think this is looking pretty good and the patch is ok -but for the trait :-). Note my concerns are more about the underlying system -which requires all this patch for this feature- than about the patch itself, so half of them may be a bit out of scope here.

Gábor Hojtsy’s picture

@Jose: the proposal to add a generic config type alter in #2100223: Add an alter hook for typed config definitions was won't fixed based on feedback from @alexpott last September, which was directly followed by our need for our own alter which is why we added that.

Wim Leers’s picture

FileSize
74.69 KB
78.07 KB
35.8 KB
4.17 KB

[…] disabled CKEditor sounds good, as long as it allows to see the source […]

As long as you have the "Source" button enabled for a text format, that is possible.

Here's a reroll that adds what I described in #64. I had to modify getRenderedSource() to also allow arrays to be returned. Be ware, it's a hacky reroll; you'll want to clean it up, I just wanted to show in the smallest diff possible how this would work. Updates are still necessary to the base classes/interfaces/…

Notice how all of the source version's CKEditor's buttons are disabled, except the "Source" button (which answers #67).

Finally, I had trouble reproducing this because I didn't have config_translation enabled. Apparently config_translation_test didn't depend on it yet… :) So fixed that too.

(Leaving at NW because I didn't address anything else.)

tstoeckler’s picture

Will take a stab at updating this.

@Jose Reyero: Thanks very much for your review, much appreciated!

Some remarks in response to #67:

I think the 'text_with_format' type should have the 'translatable' property set to TRUE and we should treat it as an object that is translatable by itself (and rearchitect what it takes so we can translate any data type with a simple extension, otherwise contrib modules will need to ditch this and provide their own translation form to translate other types)

Can you elaborate on why you think we should do this, and how Contrib would be affected by this? We're bound fairly strongly to do it that way by the current implementation, but before arguing that I would like to discuss this on a semantical level first.

Currently we do not declare the entire text_with_format element as translatable, but only the contained value property (and not the contained format property). Essentially, we are saying that the text can be translated but the format cannot. And this is exactly how it should be, in my opinion. Translating the format part of a text_with_format is semantically wrong and should not in any way be supported. If a translator does not have access to a particular text format that translator cannot translate text written in that format. It's as simple as that.

I'd love to hear your thoughts on that.

Btw, are we handling the 'you cannot access this format case' when there is a translation already that the translator cannot edit? (Some message, no submit button, etc...)

This will be handled automatically once #1423244: #allowed_formats property for #type 'text_format' to filter available formats is in. We will then just limit the translation form to the one format that is the source format. If that has not landed when this gets to RTBC, though, we should make sure we are not introducing any security regressions so thanks a lot for mentioning that!!! Will keep that in mind.

About the 'form_element_class', maybe too late but, can't we add it into the core data types with some other hook that is not 'config translation' specific? There should be some generic way to alter the data types, right?

In a perfect world, yes, yes, and yes :-). There is no typed data alter hook, though. And while form elements for typed data are in no way specific to config translation, and will have to be re-implemented for Rules / Config Inspector / ... in contrib, that's how things currently stand. I do still plan to improve this for D8 but it's certainly a separate issue.

Overall, this is maybe too complex and I wonder whether needing all this to make one more 'data type' translatable doesn't mean we should improve how the 'configuration translation' feature in general is architected. I mean it should need a plain form element class to make one more different data type translatable and this one is a proof of concept for how extendable is the translation form in that sense. It seems not too much :-/

Hmm... I'll try to justify some of the changes and also the size of the patch but it'd be great if you could be more specific about what you dislike.

In current 8.x everything that ends up in $form_state['values'] under a translatable key will be saved into the language override configuration. We are currently not using the translatable flag in the schema to determine what and what not to save. This is problematic for the text_format form element as its format property has a value, but we do not want to save that in the language override configuration. That is why the patch changes the behavior to only save those values that have the translatable flag set to TRUE. This change however, is worthwhile in its own right and makes the whole form a lot less fragile. I.e. if you have a contrib module altering the config translation form and happens to make some form value appear somewhere in the form state you do not all of a sudden save that value into the configuration file.

In order to traverse the schema and then - when we have a translatable element - grab the correct values out of $form_state['values'] without any arcane magic we adapt the form values so that they resemble the structure of the configuration (schema). Because of $form['#parents'] this is easily possible. Again, this change makes the structure of $form_state['values'] much more sane as it A) removes the pointless source and translation separation (source has no right being in $form_state['values'] in the first place) and B) it allows to remove an unnecessary complexity where the parent of the translation key in $form_state['values'] needed to contain the full dotted "path" to the config value, as this was necessary to set the correct config value.

tstoeckler’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
36.94 KB
16.56 KB
3.07 KB
42.61 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2144413-72-config-translation-text-format.patch. Unable to apply patch. See the log in the details link for more information. View

Here we go.

Thanks a lot @Wim Leers for your input regarding CKEditor and for you initial patch. The fact that the interdiff is relative to #60 and not #70 (and that there are 2) is due to me failing at life, not because I didn't use yours!

This follows through with @Wim Leers' idea above and makes it so that both getTranslationElement() (previously getFormElement()) and getSourceElement() (previously getRenderedSource()) both return a render array. I also moved the appropriate parts into the new FormElementBase (previously SimpleSourceTrait). This allowed to remove a couple of lines from ConfigTranslationFormBaser. I also cleaned up a couple things in the lines that I had to move anyway (i.e. usage of $language->getId() vs. $language->id).

Tested this both with CKEditor and without and it works nicely now (even though it took me a while). @Wim Leers I would like to ping you regarding the @todo about CKEditor.

While working so much with the text_format Form API type, I realized that it is quite weird that we call the schema type text_with_format (even though the latter is more accurate). Do people agree that we should rename this to text_format?

Would love to see some reviews of this, this feels quite ready now. That's barring @Jose Reyero's architectural feedback of course.

Also updated the issue summary.

Status: Needs review » Needs work

The last submitted patch, 72: 2144413-72-config-translation-text-format.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
42.66 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,718 pass(es), 2 fail(s), and 0 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 74: 2144413-74-config-translation-text-format.patch, failed testing.

Gábor Hojtsy’s picture

@tstoeckler: I think text_with_format is more accurate, but if we want to keep in line with the confusing name used in form API then yeah it may be better to use the same confusing name :D Also is the format selector a dropdown even if there is only one item? That seems to be not in scope for this patch, but that looks confusing. Its inviting to do something with it just to figure out you only have one item...

tstoeckler’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
3.64 KB
44.14 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,846 pass(es). View

Wow, Editor module has quite thorough test coverage. This should be green again. Also renamed the schema type to text_format.

Just a note that the editor.module changes are @Wim Leers-approved per IRC. It would be great though if @Wim Leers could take a look at the test fixes, and whether those are OK.

tstoeckler’s picture

+++ b/core/modules/config_translation/lib/Drupal/config_translation/FormElement/FormElementBase.php
@@ -0,0 +1,61 @@
+      ),      '#default_value' => $value,
+      '#attributes' => array('lang' => $language->getId()),

Oops, don't know what happened here. Will fix in next re-roll.

Jose Reyero’s picture

Re @tstoeckler #71:

I think the 'text_with_format' type should have the 'translatable' property set to TRUE and..

Can you elaborate on why you think we should do this, and how Contrib would be affected by this?

When thinking of contrib extending schema elements and providing their own translation widget, we need the ability for the module to define the full widget for the element. Like in this case, we need not only a text field, but a full widget with text format.
If only the text string is translatable, our generic form code would just produce a translatable text field, unless the form code itself is reworked to handle such elements like in this case.
So the fact that we need to patch the form generating code means that it is not really reusable to add arbitrary translatable elements and widgets.

Essentially, we are saying that the text can be translated but the format cannot. And this is exactly how it should be..

It's just how you call it, but the fact is we are "translating" the text format too. Or call it what you want "to allow a different value for each language", that won't make a difference.

tstoeckler’s picture

If only the text string is translatable, our generic form code would just produce a translatable text field, unless the form code itself is reworked to handle such elements like in this case.
So the fact that we need to patch the form generating code means that it is not really reusable to add arbitrary translatable elements and widgets.

Sorry, but I do not really understand this. In current 8.x you are correct, that this is not possible. But the current patch adapts it to work for arbitrary elements which have one or more translatable elements within them. I don't see why the fact that it wasn't like that before - i.e. that we need to patch the form generating code - can say anything about how it is reusable after the patching. I'm probably missing something...

It's just how you call it, but the fact is we are "translating" the text format too. Or call it what you want "to allow a different value for each language", that won't make a difference.

No, that is not true! There are no different text formats per language. And it would be both very confusing and semantically incorrect if it were the case, at least that's my (strong) opinion. The fact that there is currently still a format selector is due to #1423244: #allowed_formats property for #type 'text_format' to filter available formats not being committed yet - i.e. it's impossible to render a '#type' => 'text_format' element without a text format selector. But we do not actually save the format into the language overrides. (Sorry if I was unclear about this detail above.)

Jose Reyero’s picture

But we do not actually save the format into the language overrides. (Sorry if I was unclear about this detail above.)

Don't we? Then I may have missed that part sorry, I thought we were allowing the translator to use other of the formats they have access too.

However, if we are using the same format for all the translations, how do we handle when the source text's format changes? Are we just applying the same text format to existing translations? (This would be a security issue I'll explain if it is the case).

tstoeckler’s picture

Status: Needs review » Needs work

However, if we are using the same format for all the translations, how do we handle when the source text's format changes? Are we just applying the same text format to existing translations? (This would be a security issue I'll explain if it is the case).

Yes, that is how it currently is. And, yes, that needs to be fixed.

That's why it's so great to have you back! :-) I had never thought of that complexity.

You're right that just saving the text while the format can be changed is a security issue which we need to prevent. We need to somewhere store the format with which the translated text was saved in at the time of translating. And the only sane place to store that is next to the text in the language override. So in effect we do need to make the format translatable. Because we still do not want to allow people to change the text format, that would make #1423244: #allowed_formats property for #type 'text_format' to filter available formats a hard blocker of this now.

So the question then remains whether we want to allow to mark the entire text_format translatable or whether we want to mark the value and format properties translatable separately. The former might (arguably) be considered more semantically correct, but the latter would be easier to implement, for the following reason:

To be able to translate shipped configuration on http://localize.drupal.org we need to integrate with the Interface translation (locale) module. That, however, is only capable of storing strings. It cannot cope with array('value' => 'foo', 'format' => 'plain_text'). So we would need to refactor the locale integration in order for that to work.

Also we need tests to ensure that the format makes it into the locale override as this is security-related.

Gábor Hojtsy’s picture

I agree we should store the format on the translation as well. And then there is still the question that translations may not have access to the same format the source uses. What happens then? Is that already covered?

tstoeckler’s picture

And then there is still the question that translations may not have access to the same format the source uses. What happens then? Is that already covered?

I assume you mean translators, not translations. In that case (at least with #1423244: #allowed_formats property for #type 'text_format' to filter available formats) the translator will not be able to translate that particular piece of text, i.e. it will be a disabled textarea. That's the only sensible thing we can do in this case.

That's why this is now blocked on that issue. I'd like to get this done - in terms of reviews and architectural reviews - first though, before actually marking the issue postponed. I.e. I'd like to go straight from postponed to RTBC, so that we don't lose more time.

Gábor Hojtsy’s picture

@tstoeckler (#84): that default behaviour makes sense to me for formats without access.

Jose Reyero’s picture

@tsoeckler,
Now I think I get it, thanks for the explanation, sorry for not doing my homework and being up to date with all that background info.

Here are my (now up to date :-) thoughts:

- First of all, I think we need the whole thing (text + format) to be marked as 'translatable'. Though you can argue about this specific case, I think we want to implement some generic mechanism for configuration 'things' to be translatable. I mean now it's text + format, but a contrib module may want to handle more complex stuff like 'price + currency' or a map ('image', 'cooordinates', 'zoom level'...). Let't not get lost into the detail (because arguably you can always translate piece by piece) but let's think of objects that need to be translated as a whole ,that is, they need it to be editable in a single widget, and this would be the case: 'need a specific widget for editing/translating it'.

- Moreover, that 'objects' cannot rely on custom forms because they always can be a single config object or they may be part of a bigger one, a mapping, a list, etc... And here comes the need of mapping 'configuration name' and 'translation widget' for the full object.

- About how locale, I don't think we need to change too much because default strings can be translated without the format, that should be the same. However we may need to extend LocaleTypedConfig to get 'translatable objects' not only translatable strings. This is how I see it working:

This would be the schema definition:

text_with_format:
  type: mapping
  label: 'Text with format'
  translatable: true   ***
  mapping:
    value:
      type: text
      label: 'Content'
    format:
      type: string
      label: 'Text format'

That 'schema', when 'compiled' (text element inherits translatable = TRUE) will look like this (only relevant parts):

text_with_format:
  type: mapping
  translatable: true   ***
  mapping:
    value:
      type: text
      translatable: true ***
    format:
      type: string      

Now when the LocaleTypedConfig parser gets to the object, it should check:
a) The object is 'translatable':
a1) If the (full) object doesn't match the default, pull out (format may have changed, it is not safe anymore to blindly translate strings)
a2)) If it does (object is default config, thus format too), get into the object and translate the translatable strings in it
b) The object is not 'translatable': Recurse into the object's elements and find translatable strings in it.

On the other side, our form/element code, since the object is translatable as a whole will just handle it as a whole widget/subform. Our form element class, as it is specific to the object type, will 'know' how to handle it.

Gábor Hojtsy’s picture

@JoseReyero: I think in your setup of two translatable elements nested code in Locale and localize.drupal.org can just skip if the translatable element is a container (not scalar) and move on. Only deal with translatable scalars. That would be the only change needed, no? Sounds like you explain some more complex logic, which I am not sure why is needed?

Wim Leers’s picture

+++ b/core/modules/editor/editor.module
@@ -257,9 +257,8 @@ function editor_pre_render_format($element) {
-  // element. Skip this text format widget, if it contains no 'format' or when
-  // the current user does not have access to edit the value.
-  if (!isset($element['format']) || !empty($element['value']['#disabled'])) {
+  // element. Skip this text format widget, if it contains no 'format'.
+  if (!isset($element['format'])) {

Looks good!

+++ b/core/modules/config_translation/lib/Drupal/config_translation/FormElement/TextFormat.php
@@ -32,11 +32,21 @@ public function getTranslationElement(array $definition, LanguageInterface $lang
+    $element['#attributes']['readonly'] = 'readonly';
+    // @todo CKEditor does not support the 'readonly' attribute.
+    $element['#attributes']['disabled'] = 'disabled';

What exactly is missing here? When the disabled attribute is set, CKEditor enables its read-only state automatically. See http://docs.ckeditor.com/#!/api/CKEDITOR.config-cfg-readOnly.

Wow, Editor module has quite thorough test coverage. This should be green again. Also renamed the schema type to text_format.

:)

Glad to see that independently verified :D

No, that is not true! There are no different text formats per language. And it would be both very confusing and semantically incorrect if it were the case, at least that's my (strong) opinion.

I strongly agree. Imagine that you have a media filter with tokens for e.g. a video. If you switch to another text format, that video may no longer work.

We need to somewhere store the format with which the translated text was saved in at the time of translating.

Indeed.

And then there is still the question that translations may not have access to the same format the source uses. What happens then? Is that already covered?

Considering what I replied to two quotes above this one: the only possible solution is to tell the current user that (s)he does not have sufficient permissions to translate this piece of content.

Jose Reyero’s picture

@ Gábor Hojtsy,
Yes for localize.drupal.org, I don't think so for locale module.

I mean we'd need to check whether the container as a whole has changed before happily translating contained strings and a very good case is this 'text_with_format' container. Once the 'format' has been changed, I wouldn't risk translating the 'text' in it even if it's the default text.

Of course we could check also whether the translation's 'format' has been changed or not but that's a type-specific check (it assumes we are dealing with text+format), so not generic enough to implement in locale module.

This is my -yes, I know, twisted and stupid and that- but I think valid use case:

0. Original text_with_format, not translation yet:

- text: 'Look how text filtering stops XSS! <script>alert('Hello');</script>'
- format: plain_text

1. Now I am the site admin playing around with texts and formats, and change:

- text: 'Look how text filtering stops XSS! <script>alert('Hello');</script>';
- format: 'full_html' 

(Oh, cool, the filtering works... I prefer the nice popup though....)

2. Site gets updated translations (automated, po import form somewhere, etc..), and the translation is:

- text: 'Look <script>..[bad guy's script here]..</script>'
- format: 'full_html'

* Since the source text is not changed, this is suppossed to be a safe automated translation
** The format is inherited from the source text. (full_html)
*** No one really looked at the translation because it *was* being filtered as plain text so why should they care?

Well, you can tell me translations will be somehow validated but, how do you validate translations for text+format?. How do you automate it for any contrib module with any hypothetical text format?

So I think translating texts with format is somehow risky, but at least you can rely on a known format (or skip translation if unknown). But what about translating texts with known format that may land on a system with a different format?

tstoeckler’s picture

Before I get into the heavy discussion:

I prefer the nice popup though...

Literally laughed out loud at that. That will be my response from now on when people tell me about XSS. It's a feature, not a bug! :-) (Sorry, now back on topic...)

What exactly is missing here? When the disabled attribute is set, CKEditor enables its read-only state automatically. See http://docs.ckeditor.com/#!/api/CKEDITOR.config-cfg-readOnly.

So there's the disabled attribute which "greys out" the textarea and then there's the readonly attribute which still allows focus, and to highlight the text in the textarea (but not change it). I think in this case the latter is preferable, and it is in fact the behavior that CKEditor performs in its readOnly state: You can still highlight the text. However, CKEditor does not react to setting the readonly attribute, but only the disabled attribute. That's why we have to set both attributes, when readonly should in fact suffice (and it does - when not using CKEditor). So there are two options for this as far as I see it, from the CKEditor side:

  1. Make the readonly attribute have the same effect as the disabled attribute currently has. This shouldn't be too hard, looking at line 170 of core_editor.js.
  2. Introduce an actual disabled state which is distinct from readOnly and completely greys out the textarea and does not allow focus. As this would be an API/behavorial change I assume the CKEditor folks will not be too excited about this.

Let's discuss this in a separate issue or preferably in IRC, (because I'm not yet sure that it will end up as a d.o issue), but I wanted to justify why I put the @todo there.

That said if you have suggestions both for the code and for the @todo comment, please enlighten me! :-)

Regarding where to put translatable and how config translation and locale interact:
So what you propose is the following (please correct me if I'm wrong:)

  • Upon submittion config_translation's translation form we traverse the config schema.
    • config_translation stops at the highest level element that has translatable: true and saves the entire "container" object into the language override
    • locale traverses down to the very end and only then checks whether the final element has translatable: true or not. If so (and if the source string matches a shipped string of course), it saves the string into the string translation system. (To be precise config_translation does this on behalf of locale, but that's an implementation detail.)

    This allows for the simple case of the first translatable element already being the final one in the tree and thus both config_translation and locale act on the same element. But it also allows for the more complex case where a container element is translatable as a whole but only some components of it are eligible for the string translation process.

    If this is in fact the proposed solution I agree 100%. I think it rather elegantly solves this complex problem. BUT this will need extensive documentation as it will be impossible to get right otherwise for contrib maintainers. Perhaps we should add a whole config_translation @defgroup. Of course, that should be a follow-up.

  • localize.drupal.org parses the entire schema tree but only cares for the scalar values, i.e. the final elements of the tree. It has no notion of "containers" that are translatable as a whole.

I think this would make sense.

The scenario given in #89 is both valid an problematic, however I don't see a way around it with the current system. I think the shortfall is that we are bending localize.drupal.org - which is at its heart a string management service - to work with aggregated data structures such as configuration. If we were building l.d.o from scratch with the D8 architecture in mind, we would allow to translate config entities in their entirety (which is totally possible, as the form depends only on the schema) and would export the language override YAML files directly. Then you could never enter

- text: 'Look <script>..[bad guy's script here]..</script>'
- format: 'full_html'

because you wouldn't be able to change the text format as a translator.
Also note that this problem exists already for views empty/footer/header texts, etc. which can be translated, but their format is changed independently currently. So I think we can safely make this a follow-up issue, even though this is security-related.

Wim Leers’s picture

#90: RE CKEditor supporting readonly — I'm talking to them about this. But don't expect this to be supported overnight. In the mean time, we could support this ourselves by modifying core/modules/ckeditor/js/ckeditor.js — which contains the CKEditor-specific attaching/detaching logic that the Text Editor module API requires.

Gábor Hojtsy’s picture

@tstoeckler:

locale traverses down to the very end and only then checks whether the final element has translatable: true or not. If so (and if the source string matches a shipped string of course), it saves the string into the string translation system. (To be precise config_translation does this on behalf of locale, but that's an implementation detail.)

Well the way Jose proposed, locale needs to be aware if the text format was changed from the install source and not load in the translation if it did. So locale needs to handle the whole object as well, but look up translation only for the text translatable value. For the original ==/!= comparison, it would look at the container, for the string translation lookup it would only look at the string. Does that make sense?

tstoeckler’s picture

Just FTR, the CKEditor fun is now happening in #2275491: CKEditor does not support the "readonly" attribute.

Re #92, yes I think that makes sense. If we consider the container objects to be the ones that are semantically translated as whole objects then it is fine for locale to be aware of that. That doesn't seem like it is an implementation detail of the text_format type. However, I think the comparison logic needs to be a little bit more involved. a simple == on the container object is not sufficient, unless I am missing something. I think the following example code should do it:

foreach ($schema as $element) {
  if (empty($element->isTranslatable()) {
    // Traverse further into the schema tree.
    if ($element instanceof ArrayElement) {
    }
    // We have reached the end of the tree and the
    // element is not translatable. Do nothing.
    else {
      continue;
    }
  }
  // We have a translatable element.
  else {
    // Get the respective source language values.
    $element_base_config = ...
    // Get the respective language override values.
    // Because config_translation translates container
    // objects wholesale these include all properties,
    // translatable or not.
    $element_language_config = ...
    // This probably needs to be a recursive iteration, but whatever...
    foreach ($element as $property_name => $property) {
      if (!$property->isTranslatable() && $element_base_config[$property_name] !== $element_language_config[$property_name]) {
        // A non-translatable property has changed, which means the source
        // object has been changed manually. It is not safe to override any translations
        // of properties in this case.
        continue;
      }
    }
    // We are still here, so let's update strings now!
    foreach ($element as $property_name => $property) {
      $string = $this->localeStorage->getTranslationString(array(
        'source' => $element_base_config[$property_name],
        'langcode' => $langcode,
      );
      if ($element->isTranslatable() && $translation_string != $element_language_config[$property_name]) {
        // The string translation has been updated on l.d.o. To be
        // really safe (and also cool) we would have the isCustomized()
        // capability that locale has itself for configuration overrides as well
        // as it might very well be that people want to keep custom translations.
        $element_language_override_config->set($property_name) = $translation_string;
      }
    }
  }
}

?!

tstoeckler’s picture

Assigned: tstoeckler » Unassigned

Just realized this was still assigned to me, sorry. I do plan to work on this next week, but that shouldn't stop anyone from beating me to it.

Gábor Hojtsy’s picture

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
59.58 KB

Here's a first start. This is not 100% ready set, but I want to see what breaks. I will update the issue summary once this is green.

tstoeckler’s picture

FileSize
59.58 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,060 pass(es), 68 fail(s), and 58 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 97: 2144413-96-test.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
67.79 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,181 pass(es). View

Here we go. Will update the issue summary and provide some details on the patch later.

tstoeckler’s picture

Priority: Normal » Major
Issue summary: View changes
Issue tags: -Needs issue summary update +security

Updated the issue summary.

One note on introducing LocaleConfigSubscriber: Even though I believe it is an improvement regardless of this issue I wouldn't have gone ahead and introduced it here if it wasn't necessary. However - as a result of the above discussion - the config saving and the string translation saving have a different traversing logic. And embedding both of those into ConfigTranslationFormBase would have been terrible for readability.

I re-read #93 and I am in fact incorrect (who knew! :-). We cannot ignore values if non-translatable properties change because then it would be impossible to re-translate formatted text when the source text format changes. We just need to document that it is the responsibility of the form element class to make sure that invalid/unsafe combinations of values cannot be submitted. In our case the text_format form element already ensures that for us. I will add documentation and tests for this, though.

I'm very much reluctant to do this, but raising to major because of the security impact here. The scenario depicted by @José Reyero in #86 is already exploitable with the empty/header/footer areas from views. In fact the text that is entered in to the textarea is simply printed into the Config Translation UI so you can create an XSS injection there fairly easily, which makes #2144505: Views does not use the text format type for formatted text major as well.

If you're wondering about the change in LocaleConfigTranslationTest, it is - strictly speaking - unnecessary, but it failed at some point and the current assertion there is just impossible to debug, so I split it up into multiple ones.

Some notes from self-review:

  1. +++ b/core/config/schema/core.data_types.schema.yml
    @@ -311,3 +311,33 @@ condition.plugin:
    +      # with http://localize.drupal.org. Because it only handles strings and
    ...
    +      # http://localize.drupal.org
    

    I realized it's also https://localize.drupal.org now.

  2. +++ b/core/lib/Drupal/Core/Config/ConfigEvents.php
    @@ -9,6 +9,8 @@
    + * @see \Drupal\Core\Config\ConfiguCrudEvent
    

    Stray "u" (ConfiguCrudEvent).

  3. +++ b/core/modules/config_translation/src/FormElement/DateFormat.php
    @@ -11,33 +11,34 @@
    +    if (class_exists('intlDateFormatter')) {
    +      $description = $this->t('A user-defined date format. See the <a href="@url">PHP manual</a> for available options.', array('@url' => 'http://userguide.icu-project.org/formatparse/datetime'));
    +    }
    +    else {
    +      $description = $this->t('A user-defined date format. See the <a href="@url">PHP manual</a> for available options.', array('@url' => 'http://php.net/manual/function.date.php'));
    +    }
    

    Oops, faulty merge.

  4. +++ b/core/modules/config_translation/src/FormElement/FormElementBase.php
    @@ -0,0 +1,61 @@
    +        array(
    +          '!label' => $this->t($definition['label']),
    +          '!source_language' => $language->getName(),
    +        )
    +      ),      '#default_value' => $value,
    +      '#attributes' => array('lang' => $language->getId()),
    

    Missing trailing comma and weird formatting.

  5. +++ b/core/modules/config_translation/src/Tests/ConfigTranslationUiTest.php
    @@ -93,7 +93,7 @@ public function setUp() {
    -  public function testSiteInformationTranslationUi() {
    +  public function _testSiteInformationTranslationUi() {
    
    @@ -159,7 +159,7 @@ public function testSiteInformationTranslationUi() {
    -  public function testSourceValueDuplicateSave() {
    +  public function _testSourceValueDuplicateSave() {
    
    @@ -236,7 +236,7 @@ public function testSourceValueDuplicateSave() {
    -  public function testContactConfigEntityTranslation() {
    +  public function _testContactConfigEntityTranslation() {
    
    @@ -367,7 +367,7 @@ public function testContactConfigEntityTranslation() {
    -  public function testDateFormatTranslation() {
    +  public function _testDateFormatTranslation() {
    
    @@ -432,7 +432,7 @@ public function testDateFormatTranslation() {
    -  public function testAccountSettingsConfigurationTranslation() {
    +  public function _testAccountSettingsConfigurationTranslation() {
    
    @@ -462,7 +462,7 @@ public function testAccountSettingsConfigurationTranslation() {
    -  public function testSourceAndTargetLanguage() {
    +  public function _testSourceAndTargetLanguage() {
    
    @@ -510,7 +510,7 @@ public function testSourceAndTargetLanguage() {
    -  public function testViewsTranslationUI() {
    +  public function _testViewsTranslationUI() {
    
    @@ -553,16 +553,16 @@ public function testViewsTranslationUI() {
    -  public function testLocaleDBStorage() {
    +  public function _testLocaleDBStorage() {
    
    @@ -610,7 +610,7 @@ public function testLocaleDBStorage() {
    -  public function testSingleLanguageUI() {
    +  public function _testSingleLanguageUI() {
    
    @@ -637,7 +637,7 @@ public function testSingleLanguageUI() {
    -  public function testAlterInfo() {
    +  public function _testAlterInfo() {
    

    Oops, sorry.

  6. +++ b/core/modules/locale/src/LocaleConfigSubscriber.php
    @@ -0,0 +1,161 @@
    +use Drupal\Core\Config\TypedConfigManagerInterface;
    

    Unused.

  7. +++ b/core/modules/locale/src/LocaleConfigSubscriber.php
    @@ -0,0 +1,161 @@
    +    // when the configuration override gets deleted so we can re-use the same
    

    A comma before the "so" would make the sentence less ambiguous.

  8. +++ b/core/modules/locale/src/LocaleConfigSubscriber.php
    @@ -0,0 +1,161 @@
    +
    

    Remove.

tstoeckler’s picture

FileSize
44.21 KB
5.52 KB
24.21 KB
74.77 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,230 pass(es). View

Here we go. This feels ready from my point of view, but I'd love to hear some thoughts on the architecture. The added test coverage showed that the #allowed_formats integration did not work in the above patch. Fixed that now (tests++).

Also noticed that I didn't provide an interdiff for the previous patch, sorry for that! Here they are (two, because I merged 8.x in between).

tstoeckler’s picture

tstoeckler’s picture

Related issues: +#2277945: Typed configuration implements TypedDataInterface improperly
FileSize
75.33 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,313 pass(es), 14 fail(s), and 9 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 103: 2144413-103-config-translation-text-format.patch, failed testing.

Jose Reyero’s picture

Hi, I was in the middle of a review but this is not applying anymore, since this one got committed #2277945: Typed configuration implements TypedDataInterface improperly

So if you don't mind I'll re-roll the patch for you, then add some comments after I post the update.

(It looks great to me so far)

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
3.58 KB
75.42 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,231 pass(es). View

Here we go. The getSourceElement() didn't conflict in the merge, because it is introduced here, but we need to use DataDefinitionInterface there as well, of course.

Edit: Crosspost. I beat you with the re-roll. :-)

Jose Reyero’s picture

The patch looks great to me, but since it's big and complex, I have some questions, suggestions, etc... None of this should be considered a blocking issue though.

1. Mostly curiosity, but:

+++ b/core/lib/Drupal/Core/Config/TypedConfigManagerInterface.php
@@ -23,7 +23,7 @@
    * @param string $name
    *   Configuration object name.
    *
-   * @return \Drupal\Core\Config\Schema\Element
+   * @return \Drupal\Core\Config\Schema\ArrayElement

I think ok, it is a more specific type that happens to be true, but just wondering, why do we need this?
Could we use ComplexDataInterface instead? (may open up some doors to future schema definitions by contrib modules)

2.

-- a/core/config/schema/core.data_types.schema.yml
+++ b/core/config/schema/core.data_types.schema.yml
@@ -312,3 +312,33 @@ condition.plugin:
       label: 'Context assignments'
       sequence:
         - type: string
+
+# Human readable string that is associated with a format.
+text_format:
+  type: mapping
+  label: 'Text with text format'
+  # Even though it is not sensible to translate the text format of a formatted
.....................

The long explanation here is great, but I think it should be somewhere else, where we document all options, especially the 'translatable' one, for config schemas (I don't remember where..)

3.

+  foreach ($definitions as $type => &$definition) {
+    if (!isset($definition['form_element_class'])) {
+      if (isset($map[$definition['type']])) {
+        $definition['form_element_class'] = $map[$definition['type']];
+      }
+      elseif (!empty($definition['translatable'])) {
+        $definition['form_element_class'] = '\Drupal\config_translation\FormElement\Textfield';
+      }
+    }
+  }

We may be making a way too broad assumption here, that is adding our TextField element for any other unknown translatable element. This may override better suited form elements set by other contrib modules for their data types.
I think we should add form elements only for the types we "know".

4.

-  protected function buildConfigForm(Element $schema, $config_data, $base_config_data, $open = TRUE, $base_key = '') {
+  protected function buildConfigForm($name, ArrayElement $schema, $config_data, $base_config_data, $open = TRUE, $base_key = NULL) {

Wondering whether we need so many parameters here. From $chema we could get
$config_data = $schema->getValue()
$base_config_data = $schema->getRoot()/getParent()->getValue()
Or maybe they are config objects, then forget about this but better remove the '_data' suffix which makes them confusing.

5.

+      // Invoke hook_config_translation_type_info_alter() implementations to
+      // alter the configuration types.
+      $definitions = array(
+        $definition['type'] => &$definition,
+      );
+
+      $this->moduleHandler->alter('config_translation_type_info', $definitions);

Since it seems likely this hook goes through some change, could we encapsulate the whole thing with something like getFormElementFromDefinition(..) ? (that also may use some static caching in the future if we don't eventually fix the hook)

6.

+      // If this element is translatable, show a translation form. This may be a
+      // simple element, as is the case for unformatted text or date formats, or
+      // an array element, as is the case for formatted text. In the latter case
+      // the form element class is responsible for handling input for all
+      // configuration values contained in the element.
+      if (!empty($definition['translatable'])) {
+        /** @var \Drupal\config_translation\FormElement\ElementInterface $form_element */
+        $form_element = new $definition['form_element_class']();
+
+        $build[$element_key] = array(
+          '#theme' => 'config_translation_manage_form_element',
+        );
+        $build[$element_key]['source'] = $form_element->getSourceElement($definition, $this->sourceLanguage, $base_config_data[$key]);
+        $build[$element_key]['translation'] = $form_element->getTranslationElement($definition, $this->language, $base_config_data[$key], $config_data[$key]);
+        // For accessibility we make source and translation appear next to each
+        // other in the source for each element, which is why we utilize the
+        // 'source' and 'translation' sub-keys for the form. The form values,
+        // however, should mirror the configuration structure, so that we can
+        // traverse the configuration schema and still access the right
+        // configuration values in ConfigTranslationFormBase::setConfig().
+        // Therefore we make the 'source' and 'translation' keys the top-level
+        // keys in $form_state['values'].

+        $parents = array_merge(array('config_names', $name), explode('.', $element_key));
+        $build[$element_key]['source']['#parents'] = array_merge(array('source'), $parents);
+        $build[$element_key]['translation']['#parents'] = array_merge(array('translation'), $parents);
+
+      }

Since this is a custom made class, could we move the logic into ElementBase so it could at some point produce really more complex elements?
I mean $build[element_key] = $form_element->getElementSubform(....);

The issue is both 'Source' and 'Translatable' methods being overridable but not the subform itself (source + translation) and since we have a base class now, it would be equally cheap, and maybe better encapsulation.

6b. Also I think we could either make them static methods, so we don't need to instantiate the class or optionally, pass the data when creating the form element object. Needing to instantiate it without data doesn't make too much sense I think.
• I don't know if there is any pattern for element building classes somewhere else...?

7.

+  /**
    * Returns the translation form element for a given configuration definition.
    *
+   * For complex data structures (such as mappings) that are translatable
+   * wholesale but contain non-translatable properties, the form element is
...
...

The explanation is right but I'd say if such a long explanation is required, there's something wrong somewhere else...

Could we make it shorter, maybe move the text-format specifics to that class... ?

My main concern is people jumping over this too much text and missing the security implications.

8. Also wondering, what happens when there's a translation but it cannot be edited by current user (because of text format) and the the form is saved. Does existing translation get deleted?
(Sorry but I don't find where to manual test the feature with durrent d8)
Do we have any place in Drupal core btw where this is actually used?

9.

 /**
  * Implements hook_config_translation_type_info_alter().
+ *
+ * @todo Convert this hook into a real typed data alter hook,
+ *   https://drupal.org/node/2145633.
  */

Have some issues with that but not for this patch so I'll follow up on that other case. I understand this patch only builds on what is implemented atm for the hook...

10.
About the new events and changes in LanguageConfigSubscriber, code looks good, but I'm afraid I'm not the one to review them atm, not up to date on config language / locale overrides... Will try in the next days though.

Still, I'd appreciate some better comments on

	+/**
+ * Updates corresponding string translation when language overrides change.
+ */
+class LocaleConfigSubscriber implements EventSubscriberInterface {

Yes, but how does it update them? What happens when?

(updated just to add some numbering)

tstoeckler’s picture

FileSize
15.79 KB
75.9 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,363 pass(es), 4 fail(s), and 0 exception(s). View

Here we go. Replies with the same numbering as in #107.

1.
- In SchemaCheckTrait we check that each config object is an ArrayElement so the more specific check is safe and is in fact what is considered "valid" schema.
- The suggestion to use ComplexDataInterface is awesome, but unfortunately ArrayElement does not implement that (#fail). Also we should update SchemaCheckTrait then as mentioned above. I will open a follow-up for that.
- Thus left this as is.

2.
- I agree. :-)
- I don't really have an idea where to put the information if not there, however.
- In the long run I think it makes sense to have a dedicated @topic/landing-page for config translation where such things can be documented and which we could reference from core.data_types.yml
- Barring any concrete ideas for improvement, I left this as is for now.

3.
- This was how it was previously. But I did change it as I tend to agree.

4.
- Yeah, those are not even config objects (despite the docs), they are config values, i.e. the result of $config_object->get();
- I thus changed the names to $source_config and $translation_config.
- I also changed the order to be more consistent with ElementInterface::getTranslationElement and as I think it makes more sense that way. :-)

5.
- Since that will change soon with #2145633: Use standard discovery facility in TypedConfigManager instead of one-off implementation anyway I left this as is for now. Is that OK?

6.
a. I'm not sure how that would work because we rely on the exact structure (including the #parents trick) in the submit handler. Thus, if would off-load that entirely into the form class, we would also need to let the form class handle the value submitting or something like that.

b. Awesome idea! I changed FormElementBase to have the definition injected. We could also inject even more, but wasn't sure. Happy for more suggestions.

7.
Again, I'm happy to incorporate concrete suggestions. I personally don't think leaving out the text format parts (i.e. the "for example" parts) would be a good idea, as that makes it hard to understand for people that don't already what we're talking about.

Example:

   * In addition, the form element is responsible for checking whether the
   * value of such non-translatable properties in the translated configuration
   * is equal to the corresponding source values. If not, that means that the
   * source value has changed after the translation was added.

Is that understandable on its own, i.e. without an example? I would say no, but if you think it is, then - you're right - we can vastly shorten that entire section.

Didn't change anything as of yet.

8.
- No, the existing configuration is not overwritten. This working correctly depends on the form element returning the correct value in that case (by setting #value). I added a little bit of text to that effect to the (already overlong...) explanation in ElementInterface::getTranslationElement().
- We have tests for that behavior.
- Manually testing is also possible, but you will have to enable the config_translation_test module (with the patch applied) and then visit the Media > File system page, which should have a translation tab then.

9.
Left that for #2145633: Use standard discovery facility in TypedConfigManager instead of one-off implementation to fix.

10.
Added a comment to LocaleConfigSubscriber.

tstoeckler’s picture

Oops, missed one part of the interdiff (it's in the patch, though).

Status: Needs review » Needs work

The last submitted patch, 108: 2144413-108-config-translation-text-format.patch, failed testing.

Jose Reyero’s picture

Taking a look at patch #108

I think it has some minor issue -maybe also why tests are failing, I don't know, in config_translation.module

 if (!isset($definition['form_element_class']) && isset($map[$definition['type']])) {

That $definition['type'] may not be set, better use the $type foreach key or fallback to it.

About #107, ok with almost everything, I think important issues have been addressed and about the rest, I don't have better ideas either.

Only about the access control part (previous 7th point):
- It should be really clear this may need access control
- Usability related, but user doesn't get any clue why elements are grayed out.

I'm thinking that FormElement maybe should implement the AccessibleInterface and that way:
a) It is stated clearly that it needs access control and it is done in a single point.
b) The access check can be done from FormElementBase, user warned, if it is an access issue
c) We don't need to save forms/elements that may not have any editable element at all (Because right now if I got it right, the parent form doesn't know whether the element is actually editable, does it?)

$element_class->access('translate') ?

Jose Reyero’s picture

I guess this may need an update after we've got a brand new hook_config_schema_info_alter(). That should simplify this one a little bit.
See #2145633: Use standard discovery facility in TypedConfigManager instead of one-off implementation

@tstoeckler, are you still working on this one? I may give it a try if not.

tstoeckler’s picture

@Jose Reyero: Feel free to tackle this one. I do intend to come back to this eventually but I can't promise anything right now.

robertdbailey’s picture

Status: Needs work » Needs review
FileSize
75.24 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,123 pass(es), 12 fail(s), and 1,395 exception(s). View

Rerolled patch from #108

robertdbailey’s picture

Some comments on this current patch:

  1. +++ b/core/modules/config_translation/src/Form/ConfigTranslationFormBase.php
    @@ -43,11 +42,8 @@
       /**
    -   * The string translation storage object.
        *
    -   * @var \Drupal\locale\StringStorageInterface
        */
    -  protected $localeStorage;
    

    This comment block was removed, but the beginning of the comment block (/**) remains. This needs to be fixed in an updated patch.

  2. +++ b/core/modules/config_translation/src/Form/ConfigTranslationFormBase.php
    @@ -240,24 +233,29 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
    +   *   (optional) The base key that the schema and the configuration values
    +   *   belong to. This should be NULL for the top-level configuration object and
    

    Text should read: "The base key to which the schema and configuration values belong." (not important)

  3. +++ b/core/modules/config_translation/src/Form/ConfigTranslationFormBase.php
    @@ -266,10 +264,47 @@ protected function buildConfigForm(Element $schema, $config_data, $base_config_d
    +      // Invoke hook_config_translation_type_info_alter() implementations to
    +      // alter the configuration types.
    +      $definitions = array(
    +        $definition['type'] => &$definition,
    +      );
    +
    +      $this->moduleHandler->alter('config_translation_type_info', $definitions);
    

    Lines should be removed, as the equivalent moduleHandler code was removed in HEAD.

Status: Needs review » Needs work

The last submitted patch, 114: 2144413-114-config-translation-text-format.patch, failed testing.

tstoeckler’s picture

While @robertdbailey is happily ploughing away at this I opened #2346129: Introduce a TraversableTypedDataInterface and use that for typehinting instead of ArrayElement to address #107.1

I will try to move as much as possible into separate issues so that we can maybe finally get this sucker finished this week.

tstoeckler’s picture

robertdbailey’s picture

Status: Needs work » Needs review
FileSize
75.24 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,294 pass(es), 4 fail(s), and 2 exception(s). View
3.33 KB

Made the fixes from the comments in #115, and fixed an issue with #114 causing the fields to disappear.

The points of conversation in #107 and #108 have not been addressed here.

Please review.

tstoeckler’s picture

Issue tags: +Amsterdam2014
kfritsche’s picture

FileSize
1.66 KB
75.24 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,294 pass(es), 0 fail(s), and 2 exception(s). View

Manually tested this with the steps provided above.

Fixed an issue where Rob and me noticed that the source and translation is flipped, when editing an existing translation. Also a minor spelling mistake.

Works great now also with CKEditor.

Seems fine for me.

The last submitted patch, 119: 2144413-119-config-translation-text-format.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 121: 2144413-121-config-translation-text-format.patch, failed testing.

kfritsche’s picture

FileSize
76.84 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,297 pass(es). View
1.6 KB

Fixed the two failing tests.

kfritsche’s picture

Status: Needs work » Needs review
Jose Reyero’s picture

FileSize
83.17 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,459 pass(es). View
15.17 KB

I think this is looking good, but still, I am missing really reusable/replaceable form elements.

This patch builds on the previous code, but aiming to provide reusable form elements:
- All element building logic moved from ConfigTranslationFormBase to ElementInterface classes.
- New form element type, ListElement that will handle mappings and sequences like any other element.

The difference is the whole subform, included nested elements is handled by element classes now, which means we could code any form element structure into the Element classes. No ArrayElement nor other element-specific logic into the ConfigTranslationFormBase class.

Posting mostly for feedback, it may need some more cleanup yet (like dropping most of ElementInterface)

Paul Broon’s picture

Status: Needs review » Reviewed & tested by the community

Followed the instructions closely and got the exact output as expected for steps 7, 13, and 15.
Patch applied without issues or error messages.

webflo’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
10.96 KB
85.35 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,356 pass(es). View

We (jose, tobias, and me) did another in depth review during DrupalCon code sprint. We decided to move the setConfig method to the ListElement. This decouples the submit action for form elements from ConfigTranslationFormBase.

tstoeckler’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/config_translation/src/Form/ConfigTranslationFormBase.php
    @@ -285,7 +284,7 @@ public static function createFormElement(TypedDataInterface $schema) {
    -        $definition->setLabel(t('N/A'));
    +        $definition->setLabel(t('n/a'));
    

    Checked that 'n/a' is in fact widely used in core, in contrast to 'N/A'. So ++

  2. +++ b/core/modules/config_translation/src/FormElement/ElementInterface.php
    @@ -108,4 +110,33 @@ public function getSourceElement(LanguageInterface $source_language, $source_con
    +   *   A simple one dimensional or recursive array:
    

    one dimensional -> one-dimensional
    recursive -> nested

  3. +++ b/core/modules/config_translation/src/FormElement/ElementInterface.php
    @@ -108,4 +110,33 @@ public function getSourceElement(LanguageInterface $source_language, $source_con
    +   *     - simple:
    +   *        array(name => array('translation' => 'French site name'));
    +   *     - recursive:
    +   *        cancel_confirm => array(
    +   *          cancel_confirm.subject => array('translation' => 'Subject'),
    +   *          cancel_confirm.body => array('translation' => 'Body content'),
    +   *        );
    

    I'm pretty sure this is not correct. The whole idea with the #parents trick is that 'translation' does not show up at that place. Also the nesting with the '.' in the keys should be gone now.

  4. +++ b/core/modules/config_translation/src/FormElement/FormElementBase.php
    @@ -95,4 +98,37 @@ public function getTranslationElement(LanguageInterface $translation_language, $
    +    foreach ($this->element as $key => $element) {
    ...
    +      if ($element instanceof Element && empty($definition['translatable'])) {
    ...
    +        $this->setConfig($base_config, $config_translation, $value, $element_key);
    ...
    +      else {
    ...
    +        if ($base_config->get($element_key) !== $value) {
    +          $config_translation->set($element_key, $value);
    +        }
    +        else {
    +          $config_translation->clear($element_key);
    +        }
    

    It would be awesome if the traversing logic could be moved into ListElement. That way we would have true parity between the form generation and the value submission.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
22.08 KB
82.17 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,021 pass(es), 30 fail(s), and 6 exception(s). View

Here we go.

This implements what I meant in #129. I think it neatly puts the traversing into ListElement while the other elements no longer care about that and receive the same $config_values in setConfig() as in get(Translation|Source)Element().

Looked through the entire patch again and also tried it out manually. I feel pretty confident that this is ready.

webflo’s picture

Just two nitpicks.

  1. +++ b/core/modules/config_translation/src/FormElement/ElementInterface.php
    @@ -7,8 +7,10 @@
     use Drupal\Core\TypedData\DataDefinitionInterface;
    

    DataDefinitionInteface is not used.

  2. +++ b/core/modules/config_translation/src/FormElement/FormElementBase.php
    @@ -0,0 +1,183 @@
    +  public function getSourceElement(LanguageInterface $source_language, $source_config) {
    ...
    +  public function getTranslationElement(LanguageInterface $translation_language, $source_config, $translation_config) {
    

    These methods are not part of the Interface and should be protected.

  • Docs does not match in FormElementBase::getTranslationElement
tstoeckler’s picture

FileSize
2.43 KB
82.18 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,023 pass(es), 30 fail(s), and 6 exception(s). View

Here you go.

The last submitted patch, 130: 2144413-130-config-translation-text-format.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 132: 2144413-132-config-translation-text-format.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
15.58 KB
83.29 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,084 pass(es). View

This should be green.

I also tested this with CKEditor module again and it works fine.

Jose Reyero’s picture

This is looking great, I love specially how the Element classes can get to process their own submission, though I think there are still a few things we could improve, and still save some more code.

1. Could we get rid completely of ConfigTranslationFormBase::buildConfigForm() ? These few lines of code need a lot of overhead for passing and documenting parameters. Also there are some parameters not used there, like $open and $base_key.

2. We are using a constructor for ElementInterface classes that is not part of the interface. We should replace it by a create() method and make it part of the interface. That method could take some more parameter, like base_key, that is used both for form building and submission, maybe 'parents' too.

3. There is some duplication in ListElement and FormElementBase. I'd rather have one more base class that could be extended by both.

4. I have some doubts about the form submission. What happens when a plugin that was previously there is disabled now? (I think the config translation may stay?). Still I don't know what is supposed to happen with configuration itself, will 'disabled configuration' stay there in case we enable it again?

5. For this last part (instead of setConfig) I'd rather have a callback that returns a plain config values array, and let ConfigTranslationFormBase handle the full LanguageConfigOverride set/clear methods. That way we'd save passing the config objects to the form element, a lot of intermediate set/clear, and still it would be able to do some processing on submitted form values.

Anyway, I think the only important points in this list are 1. (not used parameters) and 2., and maybe assess 4., the others are just some ideas for improvement/simplification.

tstoeckler’s picture

Will fix 1. and 2.

3. That's really just the constructor that is being duplicated. I don't see what a sensible base class for that would be and I also don't think we gain much with adding another level of inheritance just to save ~20 LOC (incl. docs). The new class would certainly be an increase in pure LOC.

4. So are you talking about form element classes that were provided by a module and then the module gets uninstalled? Since we now moved the form submission logic into the form element classes the config will just stay around until a form element class for that element exists again. So there's nothing to do here?! This also validated my concern above, as this use-case is still a problem in #128.

5. I will look into this. I assume you mean that we can save passing the $base_key into FormElementBase::setConfig()? That would indeed be nice, although I'm not sure this will actually work as - per 4. - we now actually have a reason why the form submission *must* be handled by the form element classes. So we would still need to track the difference between a form element class explitly deleting the value and no value being there due to a missing form element class. I'm not sure how to do that off-hand.

Jose Reyero’s picture

@tstoeckler,

3. Ok

4. Yes, that was my concern, now I think that should be handled by a different layer, maybe LocaleConfigSubscriber -if it ever needs to be done-, so fine with that too.

5. Well, I was thinking more about proper encapsulation, more consistent parameters. While in the form building we are passing config arrays, then we use config objects for form submission, and I think they're almost the same (config objects contain the config arrays), so it is mostly about reducing the different types of objects form elements need to handle (let ConfigTranslationFormBase handle config objects, and form elements handle array values). Not a major concern though.

But if you are re-rolling the patch, just one more request. Could we use some better (translation related) name instead of getElements(), like getTranslationSubform() or similar?.
Just in case you are wondering, I'm thinking of some contrib module that would build on this to actually autogenerate the configuration forms, it looks to me that it could very well build on these form elements, adding some more interface while still implementing the current ElementInterface, i...

tstoeckler’s picture

FileSize
8.34 KB
83 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2144413-139-config-translation-text-format.patch. Unable to apply patch. See the log in the details link for more information. View

Here we go. Did not fix 5. due to

So we would still need to track the difference between a form element class explitly deleting the value and no value being there due to a missing form element class.

I wouldn't be offended if someone were to RTBC this, just sayin' ;-)

Status: Needs review » Needs work

The last submitted patch, 139: 2144413-139-config-translation-text-format.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
83.03 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,467 pass(es), 89 fail(s), and 96 exception(s). View

Re-roll after #2340667: Protect Drupal\Core\Language\Language::id, and use getId().

Because I already updated the deprecated usages where we were changing code nothing else should fail. So much for things being out of scope...

Status: Needs review » Needs work

The last submitted patch, 141: 2144413-141-config-translation-text-format.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
903 bytes
83.03 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,486 pass(es). View

I hereby request to be slapped in the face unexpectedly at the next DrupalCon.

Status: Needs review » Needs work

The last submitted patch, 143: 2144413-143-config-translation-text-format.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 143: 2144413-143-config-translation-text-format.patch, failed testing.

Status: Needs work » Needs review

Jose Reyero’s picture

This looks great to me, all important issues have been addressed. Also, tested manually, everything seems to work.

Just minor code/documentation issues before this can be RTBC:

1. I guess we should be using the brand new TraversableTypedDataInterface instead of ArrayElement in a few places #2346129: Introduce a TraversableTypedDataInterface and use that for typehinting instead of ArrayElement

2. Method Documentation in ListElement, should be ListElement instead of FormElementBase:

/**
 * Defines the list element for the configuration translation interface.
 */
class ListElement implements ElementInterface {

.....

  /**
   * Constructs a FormElementBase.
   *
   * @param \Drupal\Core\Config\Schema\ArrayElement $element
   *   The schema element this form element is for.
   */
  public function __construct(ArrayElement $element) {
...  

3. I don't think this comment applies here for ListElement

class ListElement {
  public function getTranslationBuild(LanguageInterface $source_language, LanguageInterface $translation_language, $source_config, $translation_config, $parents, $base_key = NULL) {
...

      if ($form_element = ConfigTranslationFormBase::createFormElement($element)) {
        // For accessibility we make source and translation appear next to each
        // other in the source for each element, which is why we utilize the
        // 'source' and 'translation' sub-keys for the form. The form values,
        // however, should mirror the configuration structure, so that we can
        // traverse the configuration schema and still access the right
        // configuration values in ConfigTranslationFormBase::setConfig().
        // Therefore we make the 'source' and 'translation' keys the top-level
        // keys in $form_state['values'].

I mean this 'source and translation appear next to each other etc...' is ok for FormElementBase, not for ListElements...

Gábor Hojtsy’s picture

Title: Add config translation support for Text elements with a filter format » Config translation does not support text elements with a format
Category: Task » Bug report

Just noting that #2351991: Add a config entity for a configurable, topic-based help system introduces a format supporting text field for help topics, which will need this missing integration as well.

Also retitling and reclassifying as bug, which it is.

tstoeckler’s picture

FileSize
6.84 KB
82.13 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2144413-151-config-translation-text-format.patch. Unable to apply patch. See the log in the details link for more information. View

Conflicted with #2028109: Convert hook_stream_wrappers() to tagged services. but only because both issues add stuff to locale.services.yml so very easy merge.

Fixes #149, thanks for catching that!

Could this be the last one?

tstoeckler’s picture

Issue summary: View changes

Updated the issue summary. Was mostly correct already but not quite up to date with the latest patch.

Jose Reyero’s picture

Status: Needs review » Reviewed & tested by the community

I think this is ready, nice work!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 151: 2144413-151-config-translation-text-format.patch, failed testing.

YesCT’s picture

Status: Needs work » Needs review
FileSize
82.18 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/modules/config_translation/src/FormElement/DateFormat.php. View

Status: Needs review » Needs work

The last submitted patch, 155: 2144413.155.patch, failed testing.

YesCT’s picture

Status: Needs work » Needs review
FileSize
821 bytes
82.15 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,560 pass(es). View

sorry, missed a conflict.

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
3.7 KB
82.15 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View

Thanks for the reroll, looks perfect.

In verifying that everything looks as it should I found some minor docs problems, which the attached patch fixes:

  1. +++ b/core/modules/config_translation/src/Form/ConfigTranslationFormBase.php
    @@ -238,169 +234,25 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
    +    // such as the ListElement form element which used for for Mapping and List
    

    "which used for for Mapping" -> "which is used for Mapping"

    Also the thing is called "Sequence" not "List" (that was my bad)

  2. +++ b/core/modules/config_translation/src/FormElement/TextFormat.php
    @@ -0,0 +1,45 @@
    +      // @see \Drupal\config_translation\Element\ElementInterface::getTranslationElement()
    

    This was moved to \Drupal\config_translation\Element\FormElement::getTranslationElement()

  3. +++ b/core/modules/locale/locale.module
    @@ -320,10 +320,10 @@ function locale_modules_installed($modules) {
    - * Implements hook_modules_uninstalled().
    + * Implements hook__module_preuninstall().
    

    This should be reverted.

  4. +++ b/core/modules/locale/src/LocaleConfigSubscriber.php
    @@ -0,0 +1,167 @@
    +          // If we have a new translation or different from what is stored in
    +          // locale before, save this as an updated customize translation.
    

    This comment could use some love and I think should be placed a bit lower where the string translation is actually updated.

Marking RTBC. (The patch was RTBC before + I've verified the reroll + This patch is only doc fixes)

YesCT’s picture

that interdiff looks good.

tstoeckler’s picture

Issue summary: View changes
tstoeckler’s picture

Added some draft change notices.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

I reviewed this in irc with @tstoeckler - general points are:

  1. +++ b/core/lib/Drupal/Core/Config/ConfigEvents.php
    @@ -21,6 +23,18 @@
       /**
    +   * Name of event fired when saving the configuration override.
    +   *
    +   * This event is not used by the configuration system itself but should be
    +   * used by implementors of configuration overrides. See Language module's
    +   * implementation for an example.
    +   *
    +   * @see \Drupal\Core\Config\ConfigOverrideCrudEvent
    +   * @see \Drupal\language\Config\LanguageConfigOverride::save()
    +   */
    +  const SAVE_OVERRIDE = 'config.save_override';
    
    @@ -28,6 +42,18 @@
       /**
    +   * Name of event fired when deleting the configuration override.
    +   *
    +   * This event is not used by the configuration system itself but should be
    +   * used by implementors of configuration overrides. See Language module's
    +   * implementation for an example.
    +   *
    +   * @see \Drupal\Core\Config\ConfigOverrideCrudEvent
    +   * @see \Drupal\language\Config\LanguageConfigOverride::delete()
    +   */
    +  const DELETE_OVERRIDE = 'config.delete_override';
    

    These events should be specific to language overrides - since they are an implementation detail of the language override. Different overrides may choose to fire events or not.

  2. +++ b/core/modules/language/src/Config/LanguageConfigOverride.php
    @@ -56,8 +80,19 @@ public function delete() {
    +  /**
    +   * Returns the language code of this language override.
    +   *
    +   * @return string
    +   *   The language code.
    +   */
    +  public function getLangcode() {
    +    return $this->langcode;
    +  }
    

    This should get the langcode from the storage collection name. In order to make this simple a couple of protected methods on the override factory should be turned into a trait a used here.

  3. +++ b/core/modules/locale/src/LocaleConfigSubscriber.php
    @@ -0,0 +1,167 @@
    +class LocaleConfigSubscriber implements EventSubscriberInterface {
    

    I'm concerned that we don't have test coverage of all the code pathways through the logic contained in this event. Do we have coverage of a delete where the config is shipped and where it is not. And similar coverage for an update. And there are a number of conditions in saveStrings which I can't determine if we have coverage (without doing a lot of work).

The last submitted patch, 158: 2144413-158-config-translation-text-format.patch, failed testing.

tstoeckler’s picture

Assigned: Unassigned » tstoeckler
tstoeckler’s picture

Assigned: tstoeckler » Unassigned
Status: Needs work » Needs review

Please take a look at this patch, it should address the remaining concerns. Here's the commit log from my local branch:

267c4e0 DEV-2144413: Refactor LocaleConfigSubscriber and add tests for LocaleConfigManager saving and deleting configuration
95d4346 DEV-2144413: Add LocaleConfigSubscriberTest
a74f3ce DEV-2144413: Make locale_test.no_translation.test config translatable
9baddd1 DEV-2144413: User override-free configuration in LocaleConfigSubscriber
5b6c679 DEV-2144413: Make the test configuration in locale_test translatable in the schema
b9f2a7a DEV-2144413: Do not invoke LocaleConfigSubscriber when importing community translations.
a16f215 DEV-2144413: Remove leftover usage of ConfigOverrideCrudEvent
8fbfbe8 DEV-2144413: Get the language code from the storage in LanguageConfigOverride
9e2cad0 DEV-2144413: Move the OVERRIDE_UPDATE and OVERRIDE_DELETE events to Language module

Apart from the above I found straight up bug in LocaleConfigSubscriber where we were not disabling the override state before fetching the source config. That did not surface yet, because ConfigTranslationFormBase explicitly disables that itself.

In writing the tests I furthermore fixed a few more things which were lacking and which forced me to refactor LocaleConfigSubscriber a fair bit:

  • We were explicitly marking the strings as customized so that they do not get overwritten by community translations, but we were also saving language overrides from LocaleConfigManager::saveTranslationData(). In that case the strings are the community translations and thus should not be marked as customized.
  • Similarly, LocaleConfigManager::deleteTranslationData() deletes language overrides. Again in this case, the logic we do otherwise - we save a customized string with the value of the source, so that it doesn't get overridden - does not apply. In this case the string should actually be deleted.

Because the traversing logic is the same in each case but there are 4 different cases what to do at the end of the traversion the code became a little bit more complex, but I split each logical unit into a separate method and added documentation to each detail, so I hope it makes the code fairly readable.
The same goes for the test, which should serve as a good reference for the intended behavior. There were different combinations of setting up the system and performing tasks that were identical but put together in different ways so that I split them in a bunch of methods to make the actual test methods really lean.
As always, I am open to suggestions on pretty much everything.

This is not a re-write in terms of the actual code that is being run but it is a little bit of an architectural change so I would like to have a review both from the Config system side as well as the Interface Translation side. I.e. I would love if @alexpott and @Gábor Hojtsy or @Sutharsan could take a look at this, specifically LocaleConfigSubscriber and LocaleConfigSubscriberTest.

tstoeckler’s picture

FileSize
41.31 KB
106.08 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,185 pass(es), 62 fail(s), and 0 exception(s). View

Also had to merge 8.0.x.

Status: Needs review » Needs work

The last submitted patch, 166: 2144413-165-config-translation-text-format.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
815 bytes
710 bytes
106.1 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,244 pass(es). View
YesCT’s picture

FileSize
7.39 KB
106.13 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,728 pass(es). View

I'm not the architectural review being looked for, that is still needed.

Read up to ListElement. Just a first read to get oriented to it. Fixed a few little things as I read. (Hope they are only fixes and didn't break anything.)

  1. +++ b/core/modules/config_translation/src/FormElement/FormElementBase.php
    @@ -0,0 +1,190 @@
    +   * In addition, the form element is responsible for checking whether the
    +   * value of such non-translatable properties in the translated configuration
    +   * is equal to the corresponding source values. If not, that means that the
    +   * source value has changed after the translation was added. In this case -
    +   * again - the translation of this element must be disabled if the translator
    +   * does not have access to the source value of the non-translatable property.
    +   * For example, if a formatted text element, whose source format was plain
    +   * text when it was first translated, gets changed to the full HTML format,
    +   * simply changing the format of the translation would lead to an XSS
    +   * vulnerability as the translated text, that was intended to be escaped,
    +   * would now be displayed unescaped. Thus, if the translator does not have
    +   * access to the Full HTML format, the translation for this particular element
    +   * may not be updated at all (the textarea must be disabled). Only if access
    +   * to the Full HTML format is granted, an explicit translation taking into
    +   * account the updated source value(s) may be submitted.
    

    Note to myself, check when the permissions are checked. At the beginning when building the form, *and* when submitting it?

  2. +++ b/core/modules/config_translation/src/FormElement/ListElement.php
    @@ -0,0 +1,143 @@
    +        // Build sub-structure and include it with a wrapper in the form
    +        // if there are any translatable elements there.
    +        $build[$key] = array();
    +        if ($element instanceof TraversableTypedDataInterface) {
    

    if they are traversable, they are translatable?

tstoeckler’s picture

Re 1. The flow is that during form building access is checked so that only the correct text formats are given as options. Then during submit Form API checks whether the received value is one of the values specified in #options.

Re 2. Yes, that comment is no longer accurate, I meant to remove/update that. Thanks!

Jose Reyero’s picture

Partly review, just some mixed notes:

- LanguageConfigCollectionNameTrait. I don't think we need a trait to introduce two api methods. Couldn't they be static methods in any of the classes or api methods anywhere else? Though maybe I'm wrong because it looks like current Drupal pattern may be 'add traits everywhere for everything'... :-/ But really you don't need a trait to provide reusable functions that don't access any of the class properties or internals.

- LocaleConfigManager::isUpdating ? I don't like too much this kind of 'internal state' of LocaleConfigManager and certainly it doesn't help to much for code readability or to understand what is going on.
Also we end up with this kind of methods, btw, missing documentation, but my question: is this an event callback or what?

class LocaleConfigSubscriber implements EventSubscriberInterface {
...
  /**
   * Updates the translation strings of shipped configuration.
   *
   * @param \Drupal\language\Config\LanguageConfigOverrideCrudEvent $event
   */
  protected function onUpdate(LanguageConfigOverrideCrudEvent $event, $callable) {
...

If this variable is anywhere I think it should be in the event itself.

- LanguageConfigOverride::getLangcode()... When I see the depths we need to get into to get the language for a 'language configuration override' I'm wondering whether 'language' shouldn't be a straight parameter/property of this class.

Overall, this is already a really complex patch and when I read the issue summary it looks like we are trying to fix everything configuration & language related in this one. Could we consider letting some of the events/subscribers/etc.. -mainly the ones that according to @alexpott #162 are not clear enough yet- out of this patch and for another one?

tstoeckler’s picture

- LanguageConfigCollectionNameTrait. I don't think we need a trait to introduce two api methods. Couldn't they be static methods in any of the classes or api methods anywhere else? Though maybe I'm wrong because it looks like current Drupal pattern may be 'add traits everywhere for everything'... :-/ But really you don't need a trait to provide reusable functions that don't access any of the class properties or internals.

This was specifically requested by @alexpott, so I hope we can defer that discussion for now. If you feel strongly maybe we can discuss that in a follow-up.
I am personally totally in the add traits everywhere for everything camp, just for the record. :-)

- LocaleConfigManager::isUpdating ? I don't like too much this kind of 'internal state' of LocaleConfigManager and certainly it doesn't help to much for code readability or to understand what is going on.

Yes, I don't very much like that either. But I couldn't think of a different way to solve this problem. Basically we have to differentiate between locale saving language overrides and everyone else saving language overrides. But I have added documentation to why that is needed, so please be specific on the parts that are unclear.

Also we end up with this kind of methods, btw, missing documentation, but my question: is this an event callback or what?

Again, can you specify which methods are missing documentation? I thought I went through all of them again before posting the patch but maybe I missed some. And I don't understand your question, sorry.

class LocaleConfigSubscriber implements EventSubscriberInterface {
...
  /**
   * Updates the translation strings of shipped configuration.
   *
   * @param \Drupal\language\Config\LanguageConfigOverrideCrudEvent $event
   */
  protected function onUpdate(LanguageConfigOverrideCrudEvent $event, $callable) {
...

If this variable is anywhere I think it should be in the event itself.

Again, I don't really know what you mean, sorry.

- LanguageConfigOverride::getLangcode()... When I see the depths we need to get into to get the language for a 'language configuration override' I'm wondering whether 'language' shouldn't be a straight parameter/property of this class.

That's how it was in the previous iteration. But @alexpott (IMO correctly) pointed out a problem with that: The langcode needs to be persisted to the storage, so we have it in two places then (even though - granted - one of them is bit "hidden") and it is not at all clear what happens when one of them changes. They certainly aren't synched anywhere explicitly so the system would be in an inconsistent state. I personally think that's more of an edge case so I could live with re-introducing the langcode property, but I don't see any reason to: Now that we have the getLangcode() method the internal storage logic is hidden away from consumers and whether it's a property or not should not matter.

Overall, this is already a really complex patch and when I read the issue summary it looks like we are trying to fix everything configuration & language related in this one. Could we consider letting some of the events/subscribers/etc.. -mainly the ones that according to @alexpott #162 are not clear enough yet- out of this patch and for another one?

As I tried to explain above and in the issue summary there is a hard reason why all of this is included in the patch. It is unfortunate that this patch has grown over time the way it did but at this point I don't think we gain much by splitting the locale stuff out as we would have to postpone this issue on that. And at this point I would just like to get this one in, no matter what. Also, 100KB patches are not that rare these days. I think that's more a criticism of our issue queue process in general than a defense of this patch but it's a fact nevertheless.

The reason why the locale stuff cannot be split out and handled separately is the following: Currently ConfigTranslationFormBase marks the strings as customized when you save the form. As part of this issue we are changing the traversing logic of ConfigTranslationFormBase to support complex elements containing multiple sub-elements. Because locale - by nature - needs to retain the old traversing logic, however, keeping it inside of ConfigTranslationFormBase would require traversing the config schema in two distinct ways, one of which would have nothing at all to do with config translation. That's why it was split off in the first place.

As I said, I would do pretty much everything at this point to get this patch in, so if people feel LocaleConfigSubscriber is too controversial to introduce here, I can also put all that code back into ConfigTranslationFormBase and open a follow-up for the event subcriber. But - to be blunt - I would rather someone just mark this one RTBC.

Jose Reyero’s picture

@tstoecker,
This is where I mean there's missing documentation, for the $callbable parameter.
And the function name itself is confusing, because it resembles an event callback.

+
+  /**
+   * Updates the translation strings of shipped configuration.
+   *
+   * @param \Drupal\language\Config\LanguageConfigOverrideCrudEvent $event
+   */
+  protected function onUpdate(LanguageConfigOverrideCrudEvent $event, $callable) {
+    $translation_config = $event->getLanguageConfigOverride();
+    $name = $translation_config->getName();

About LocaleConfigManager::isUpdating what I mean is this property and the associated method 'isUpdatingConfigTranslations()' could make more sense in the event itself (LanguageConfigOverrideCrudEvent), or maybe if it is a really different kind of event, we can add another different event class. Otherwise we are breaking encapsulation because we need some other variable to determine what should be the event's behavior.

For the rest, ok, you've provided valid reasons for all my objections. Nothing wrong with the patch.

The reason why the locale stuff cannot be split out and handled separately...

Thanks for the explanation, I think I knew... but I had forgotten... :D

Also I'd like to get this in asap, so I was just suggesting to split some parts in case it was helping getting the big part in faster... Now I see it all makes sense together.

tstoeckler’s picture

FileSize
2.1 KB
106.4 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,133 pass(es). View

Ok, so this fixed the missing docs and renames the method.

About LocaleConfigManager::isUpdating what I mean is this property and the associated method 'isUpdatingConfigTranslations()' could make more sense in the event itself (LanguageConfigOverrideCrudEvent)

I agree that it is sub-optimal that we require LocaleConfigSubscriber to check into internal state of LocaleConfigManager. I don't see any other way to solve that problem, however. Because we need to trigger the state change in LocaleConfigManager::saveTranslations() and ::deleteTranslations() maintaining the state in LocaleConfigSubscriber would mean LocaleConfigManager would have to call into LocaleConfigSubscriber at the appropriate times. This, however, is not possible because LocaleConfigSubscriber depends on LocaleConfigManager so that would mean a circular dependency.

or maybe if it is a really different kind of event, we can add another different event class.

We are already adding a new event class (LanguageConfigOverrideCrudEvent), so I don't know what you mean which this proposal.

Otherwise we are breaking encapsulation because we need some other variable to determine what should be the event's behavior.

Totally agreed, but I really don't see any way around it, to be honest.

Jose Reyero’s picture

FileSize
6.6 KB

About the onUpdate() method rename it looks ok to me.

About the isUpdating, I've been looking into where to put it so it can be checked from the event, attached diff is just an idea, not really tested. But the only place I can think of without reworking half of the interfaces is the LanguageConfigOverride object.
You tell me whether it makes sense, it may need still some polishing if so...

jhodgdon’s picture

The previous comment was about documentation so I started looking at this patch and got hung up on the first bit:

+++ b/core/config/schema/core.data_types.schema.yml
+
+# Human readable string that is associated with a format.
+text_format:
+  type: mapping
+  label: 'Text with text format'
+  # Even though it is not sensible to translate the text format of a formatted
+  # string, we conceive of the text and its date format as a single composite
+  # object and declare that object (or in other words the entire mapping) 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

That first line is confusing to me. It's not just "associated" with a format, it "has" a format, and the mapping here has both the string and the format in it?

And the rest of the documentation seems overly verbose for a schema file. Starting any documentation with "Even though..." seems wrong, and saying "we conceive"... Can't we just skip the philosophy and document what it is and how it's stored?

And:

+      # locale.module integrates the language overrides of shipped configuration
+      # with https://localize.drupal.org. Because it only handles strings and
+      # cannot deal with complex data structures, it parses the configuration
+      # schema until it reaches a primitive and only then checks whether the
+      # element is translatable.
+      translatable: true
+    format:
+      type: string
+      label: 'Text format'
+      # Even though the entire 'text_format' is marked as translatable for the
+      # sake of language configuration overrides, the ID of the text format of
+      # texts in shipped configuration should not be exposed to
+      # https://localize.drupal.org

Can't these be combined in some way? Again, this seems like too much information.

I also glanced through the rest of the patch... it seems like there are several changes to base form elements and element interfaces that may not be captured in the change records? You've added, removed, and changed methods on several interfaces. Interfaces are implied contracts... I don't think the new create() method on FormElementBase(), for instance, has been covered in a change notice? Even changing the type of a parameter or return value on an interface, I think, needs a change notice at this stage?

tstoeckler’s picture

Issue summary: View changes
FileSize
2.39 KB
105.95 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,077 pass(es). View

Re #175: The problem I have with that approach is that the distinction of "update mode" is really specific to locale module. language module should not know about it. It should just dispatch the event and be done with it.

Re #176:
1. I had a go at making the documentation in core.data_types.schema.yml more concise. I kept the security-relevant note as that is important information and I also did not combine the latter two blocks of documentation. The point is that we have translatable: true has slightly different semantics depending on where it is declared and, thus, I feel it's important to elaborate not only where we're putting translatable: true but also where we're omitting it as this is sort of intricate.
2. Updated the corresponding change notice to mention ElementInterface::create(), that was in fact missing. As far as I know, everything else is covered by change notices, please correct me if you find anything.

jhodgdon’s picture

Status: Needs review » Needs work

Thanks - I like the docs in the schema file much better.

Regarding change notices, here are some changes that I don't see in the 3 change notices for this issue:
- TypedConfigManagerInterface - @return type changed for some method (can't tell what it is in the patch)
- ConfigTranslationFormBase - several changes... does that need a change notice or is this considered an internal-use base class for within Config Translation module only? Probably the latter, so OK.

I also took the time to read through all of the API docs in the patch, except for in test classes, and they really look great! I had a few nitpicks:

a) ConfigTranslationFormBase::buildConfigForm() docs:

   /**
-   * Formats configuration schema as a form tree.
+   * Create form element builder.

Should be "Creates "... actually ... that's just wrong. I don't think it creates a form element builder? Doesn't it just build the form?

b) ElementInterface::getTranslationBuild()

+   * @param string $base_key
+   *   (optional) Base key to be used for the elements in the form. NULL for
+   *   top-level form elements.

Should be type string|null I think?

c) On that same method, the first-line doc says:

Returns the translation build for a given configuration definition.

But ... what it returns is an array. I am not familiar with the term "translation build" but didn't understand it... Maybe it should say something like "Builds render arrays for the source and translation form elements"?

d) Same interface:

+   * @return array
+   *   Translation configuration override data.
+   */
+  public function setConfig(Config $base_config, LanguageConfigOverride $config_translation, $config_values, $base_key = NULL);

The implementation of this method on the base class doesn't seem to have a return value. Is this right?

e) Same setConfig() method... This comment in the base implementation really concerns me because this behavior is not documented in the interface that I can see:

+    ... If the form values are the same as the
+    // original configuration, remove the override.

f) LanguageConfigCollectionNameTrait - Several places in the doc the word "langcode" is used. This is not a word. Please use "language code" when writing documentation.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
5.08 KB
106 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,132 pass(es). View

Thanks for your attention to detail @jhodgdon, this should fix everything mentioned. Some comments:

- TypedConfigManagerInterface - @return type changed for some method (can't tell what it is in the patch)

That's not actually an API change, that's just making the docs conform with what's already the case to benefit from type-hinting. Of course if there were other implementations of that interface which were strictly abiding by the interface that would be an API change for them, but IMHO that's going a bit far.

- ConfigTranslationFormBase - several changes... does that need a change notice or is this considered an internal-use base class for within Config Translation module only? Probably the latter, so OK.

Yes, I agree.

Should be "Creates "... actually ... that's just wrong. I don't think it creates a form element builder? Doesn't it just build the form?

No it actually creates an ElementInterface instance which then gets called to actually build the forum. Changed to "Creates a ...".

jhodgdon’s picture

Docs look much better, thanks! I haven't reviewed or tested the code in the patch.

Jose Reyero’s picture

@tstoeckler #177

Re #175: The problem I have with that approach is that the distinction of "update mode" is really specific to locale module. language module should not know about it. It should just dispatch the event and be done with it.

Yes, you are right, I don't have a better idea either.

dawehner’s picture

Disclaimer: by knowledge of config translation/config overrides/config is limited, but we should move this issue forward, so here is a review.

  1. +++ b/core/lib/Drupal/Core/Config/ConfigEvents.php
    @@ -9,6 +9,8 @@
     /**
      * Defines events for the configuration system.
    + *
    + * @see \Drupal\Core\Config\ConfigCrudEvent
      */
     final class ConfigEvents {
    

    Good idea in general.

  2. +++ b/core/modules/config_translation/config_translation.module
    @@ -173,10 +173,22 @@ function config_translation_entity_operation(EntityInterface $entity) {
    +    if (!isset($definition['form_element_class']) && isset($map[$type])) {
    

    in an ideal world I would swap that if statement, because the second if, would imply the, from my point of view, more important condition.

  3. +++ b/core/modules/config_translation/src/Form/ConfigTranslationFormBase.php
    @@ -179,13 +165,21 @@ public function buildForm(array $form, FormStateInterface $form_state, Request $
    +    // Even though this is a nested form, we do not set #tree to TRUE because
    +    // the form value structure is generated by using #parents for each element.
    +    // @see \Drupal\config_translation\FormElement\FormElementBase::getElements()
    

    Well, I am glad that not many people or noone woudl have to write a custom translation form element, but yeah having to deal with #parent for yourself is worse in terms of DX

  4. +++ b/core/modules/config_translation/src/Form/ConfigTranslationFormBase.php
    @@ -179,13 +165,21 @@ public function buildForm(array $form, FormStateInterface $form_state, Request $
    -      $form['config_names'][$name] += $this->buildConfigForm($this->typedConfigManager->get($name), $config_factory->get($name)->get(), $this->baseConfigData[$name]);
    ...
    +        $form['config_names'][$name] += $form_element->getTranslationBuild($this->sourceLanguage, $this->language, $source_config, $translation_config, $parents);
    

    I don't see a point why this method was renamed, but well, no big deal here.

  5. +++ b/core/modules/config_translation/src/FormElement/DateFormat.php
    @@ -12,34 +12,29 @@
    -  public function getFormElement(DataDefinitionInterface $definition, LanguageInterface $language, $value) {
    +  public function getTranslationElement(LanguageInterface $translation_language, $source_config, $translation_config) {
         $description = $this->t('A user-defined date format. See the <a href="@url">PHP manual</a> for available options.', array('@url' => 'http://php.net/manual/function.date.php'));
    -    $format = $this->t('Displayed as %date_format', array('%date_format' => \Drupal::service('date.formatter')->format(REQUEST_TIME, 'custom', $value)));
    +    $format = $this->t('Displayed as %date_format', array('%date_format' => \Drupal::service('date.formatter')->format(REQUEST_TIME, 'custom', $translation_config)));
    +
    

    Well, we already passed in a different variable, so renaming is less problematic.

  6. +++ b/core/modules/config_translation/src/FormElement/DateFormat.php
    @@ -12,34 +12,29 @@
           '#description' => $description,
    

    Is there a reason $description could not be part of the config schema directly? Maybe worth to investigate in a follow up.

  7. +++ b/core/modules/config_translation/src/FormElement/ElementInterface.php
    @@ -16,19 +18,56 @@
       /**
    -   * Returns the translation form element for a given configuration definition.
    +   * Creates a form element instance from a schema definition.
        *
    -   * @param \Drupal\Core\TypedData\DataDefinitionInterface $definition
    -   *   Configuration schema type definition of the element.
    -   * @param \Drupal\Core\Language\LanguageInterface $language
    -   *   Language object to display the translation form for.
    -   * @param string $value
    -   *   Default value for the form element.
    +   * @param \Drupal\Core\TypedData\TypedDataInterface $schema
    +   *   The configuration schema.
    +   *
    +   * @return static
    +   */
    +  public static function create(TypedDataInterface $schema);
    +
    

    Is there a simple reason why this is not just passed to the constructor?

  8. +++ b/core/modules/config_translation/src/FormElement/ListElement.php
    @@ -0,0 +1,143 @@
    +      // Do not bother traversing schema elements for which no values have been
    +      // submitted.
    +      if (!isset($config_values[$key])) {
    +        continue;
    +      }
    

    Are you sure we should not remove those elements from $config_translation ? Maybe document why it should not be done.

  9. +++ b/core/modules/config_translation/src/FormElement/ListElement.php
    @@ -0,0 +1,143 @@
    +    if (isset($group_build['title']['source'])) {
    +      $title = $group_build['title']['source']['#markup'];
    +    }
    +    elseif (isset($group_build['label']['source'])) {
    +      $title = $group_build['label']['source']['#markup'];
    +    }
    

    It is super confusing that we support both "title" and "label", but well it seems to be like that.

  10. +++ b/core/modules/config_translation/src/Tests/ConfigTranslationUiTest.php
    @@ -660,6 +673,104 @@ public function testAlterInfo() {
    +    // Assert that changing the text format is not possible, even for an
    +    // administrator.
    +    $this->assertNoFieldByName('translation[config_names][config_translation_test.content][content][format]');
    +
    

    It is great that there is a dedicated test coverage for that!

  11. +++ b/core/modules/language/src/Config/LanguageConfigOverrideEvents.php
    @@ -0,0 +1,33 @@
    +final class LanguageConfigOverrideEvents {
    

    Just a note: #2264049: Create an Events topic

  12. +++ b/core/modules/locale/src/LocaleConfigSubscriber.php
    @@ -0,0 +1,275 @@
    +      $conditions = [
    +        'lid' => $source_string->lid,
    +        'language' => $langcode,
    +      ];
    +      $translations = $this->stringStorage->getTranslations($conditions + ['translated' => TRUE]);
    

    Nitpick: You could just put translated => TRUe in the $conditions directly.

  13. +++ b/core/modules/locale/src/LocaleConfigSubscriber.php
    @@ -0,0 +1,275 @@
    +        return $translation = $this->stringStorage->createTranslation($conditions);
    

    There is no point in storing the variable first.

  14. +++ b/core/modules/locale/src/Tests/LocaleConfigSubscriberTest.php
    @@ -0,0 +1,397 @@
    +class LocaleConfigSubscriberTest extends KernelTestBase {
    

    I tried to find a testcase for the following scenario, but could not:

    * You start with a translated string
    * You override that translated string locally
    * You update translations from remote, ensure that the local override wins.

  15. +++ b/core/modules/locale/src/Tests/LocaleConfigSubscriberTest.php
    @@ -0,0 +1,397 @@
    +  /**
    +   * Tests updating translations of shipped configuration.
    +   */
    ...
    +  /**
    +   * Tests updating translations of shipped configuration.
    +   */
    ...
    +  /**
    +   * Tests deleting translations of shipped configuration.
    +   */
    ...
    +  /**
    +   * Tests deleting translations of shipped configuration.
    +   */
    

    It is a bit odd, that both have the same text.

tstoeckler’s picture

Related issues: +#2376693: Consider adding descriptions to config schema
FileSize
3.45 KB
105.88 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2144413-183-config-translation-text-format.patch. Unable to apply patch. See the log in the details link for more information. View

@dawehner: Thanks for the review!!!

1. -
2. Agreed. Fixed.
3. Yes, see also the discussions above. Until #126 the #parents magic was handled by ConfigTranslationFormBase so that form elements did not have to deal with it. That also made things less flexible, though, so by making the form elements fully self-contained (i.e. they output the form and submit the data now) we had to move that logic to the form elements. Because it is implemented in the base class I don't think it matters much practically.
4. Since it now also returns the render array for the source element, getFormElement() was no longer correct. If that were to hold this up, though, in any way, we could surely rename it back or introduce a BC-helper.
5. Yes.
6. I like that idea a lot, so opened #2376693: Consider adding descriptions to config schema.
7. This was requested explicitly in #136 to make the instantiation transparent (as in part of the interface). I think this could be seen as consistent with e.g. DataDefinition::create().
8. Wow, you're right, that that could happen in theory. It does not actually happen now because we do not have any translation elements that use checkboxes or radios (not that that would make much sense, either...). The downside is that since we now can generate a form element class for any mapping (or sequence) we do quite a lot of extra processing because of removing that check. I.e. currently we only traverse into the elements in setConfig() which have an actual config value. Removing the check means traversing the entire config schema, including all elements which do not have a translatable "tip"/"leaf" element. This is consistent with what we do during the form building anyway, so removed that check. Thanks for your attention to detail!
9. Yes, that is brought over from the current code (at least now it's in its own method!). I don't like it much either, but Views translation UI is horrible enough already, so we can't get away with removing the more meaningful labels and I also don't have an idea how to make that more generic off-hand.
10. -
11. Thanks, subscribed.
12. No, because $conditions is used below. (See your 13.)
13. Fixed, thanks!
14. This is covered in testUpdateTranslation(). Note that we don't actually test what happens when you update translations (that would be fairly complex), we just assert that the string is marked customized. This is also a little bit more unit-test-y because LocaleConfigSubscriber simply marks the string as customized, it does not deal with the import directly, either. Needless to say, feel free to suggest improvements to the test.
15. Fixed, thanks!

Gábor Hojtsy’s picture

Assigned: Unassigned » alexpott
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

As per https://groups.drupal.org/node/448068 the impact needs to be bigger than the disruption (API changes incurred) based on this being a major bug. While the patch is huge, the changes are limited to mostly config_translation and then some in language and locale modules.

Addresses @alexpott's concern since it was last RTBC. It has also been reviewed to death by a wide array of people since then and co-worked on by several area experts.

Assigning to @alexpott because of the topic of the issue and his earlier concerns.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 183: 2144413-183-config-translation-text-format.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
105.92 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,603 pass(es). View

Don't what failed to apply here, but Git was able to resolve this automatically.

Back to RTBC per #184.

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. Thanks for all the change records and addressing all my feedback from #162. Committed 1a8858a and pushed to 8.0.x. Thanks!

diff --git a/core/modules/locale/src/Tests/LocaleConfigManagerTest.php b/core/modules/locale/src/Tests/LocaleConfigManagerTest.php
index f87ced5..4cc9fb2 100644
--- a/core/modules/locale/src/Tests/LocaleConfigManagerTest.php
+++ b/core/modules/locale/src/Tests/LocaleConfigManagerTest.php
@@ -8,7 +8,6 @@
 namespace Drupal\locale\Tests;
 
 use Drupal\language\Entity\ConfigurableLanguage;
-use Drupal\simpletest\DrupalUnitTestBase;
 use Drupal\simpletest\KernelTestBase;
 
 /**

Out-of-scope changes are bad :) ... fixed this on commit.

  • alexpott committed 1a8858a on 8.0.x
    Issue #2144413 by tstoeckler, YesCT, robertdbailey, webflo, Schnitzel,...
Gábor Hojtsy’s picture

Issue tags: -sprint

Folks, I cannot thank you enough for bringing this issue to a reality. I think this is a huge milestone to the usability of configuration translation and a great testament to the team spirit in the multilingual team. Thanks all for the reviews, testing and feedback all! Let's all pour a $favoriteBeverage and celebrate :) Well deserved.

Gábor Hojtsy’s picture

Status: Fixed » Closed (fixed)

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