Config translation does not support text elements with a format

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

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?

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.

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.

added install to steps to test

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.

13: config-translation-text_format-2144413-13.patch queued for re-testing.

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

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

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? :)

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

Fix weekend tag.

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

@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 :/

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

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)

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.

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

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

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

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.

gr. img tag

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.

forgot the screenshots

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

first a reroll

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.

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

let's see what the Testbot says.

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

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.

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

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:

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.

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.

51: config-translation-text_format-2144413-51.patch queued for re-testing.

@robertdbailey: are you working on that patch update?

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

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.

This should be green.

Forgot a use statement during the merge.

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

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.

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.

@WimLeers: that is amazing! Woot!

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!

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

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.

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

[…] 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.)

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.

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.

Conflicted due to #2224887: Language configuration overrides should have their own storage. (Yes, I had a old 8.x...)

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

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.

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

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.

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

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

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.

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?

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.

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

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

@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?

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

@ 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?

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.

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

@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?

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;
      }
    }
  }
}

?!

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.

#1423244: #allowed_formats property for #type 'text_format' to filter available formats landed, this needs some updates.

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.

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

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.

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

Re-roll after #2277945: Typed configuration implements TypedDataInterface improperly

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)

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

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)

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.

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

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

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.

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

Rerolled patch from #108

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.

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.

Opened #2346195: Document configuration schema with a dedicated @config_schema_api @defgroup for #107.2

No patch there yet, though.

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.

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.

Fixed the two failing tests.

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)

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

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.

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.

Here you go.

This should be green.

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

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.

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.

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

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

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

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