When the translation is happening on the original config (no changes from shipped config yet), the translations should also be pushed to the locale database, or next time translations are updated from the community, the config translations could get overwritten. This also bridges the gap between "shipped config" and "user provided config" in that they can use the same UI to edit, but shipped config would also save edits to locale. It should save strings as customised.

Marking postponed on #1905152: Integrate config schema with locale, so shipped configuration is translated.

From #39:

A quick example. Say you have

- Anonymous translated to Hungarian as Névtelen based on a .po file imported from the community
- You go update the translation to say Anonymous (which is a word understood in Hungary), now you get your locale storage removed
- You run the locale .po update that is built into core, it finds that you have a "new" string translating Anonymous to Névtelen (because you don't have a record of the translation customized)

Instead, what should happen is we save "Anonymous" as translation and set it to customized, so we protect it from turning back to where we changed it from. So this is the example to remove the delete stuff altogether for locale. We should not delete from locale.

For the update condition wrapping, now that is only reached if you enter something different from the original config. So in the above example, when you want to enter Anonymous, now if we don't delete anymore, what would happen is that the locale stored translation keeps being Névtelen, because the Antonymous word is the same as the source. So locale would not be updated either. That is wrong too.

So to make translation updates properly recorded and protected in the locale table, we should not delete them, we should update and mark as customized at all times. And we should do it even if it was the same as the source string to make sure we have a record of the user wanting to do that.

Files: 
CommentFileSizeAuthor
#46 1936208-save-translation-locale-db-46.patch6.87 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 205 pass(es).
[ View ]
#46 interdiff.txt2.76 KBGábor Hojtsy
#45 1936208-save-translation-locale-db-45.patch6.73 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 207 pass(es).
[ View ]
#45 interdiff.txt1.17 KBGábor Hojtsy
#40 1936208-diff-37-39.txt1.6 KBvijaycs85
#40 1936208-save-translation-locale-db-39.patch6.69 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 206 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#37 1936208-save-translation-locale-db-37-test-only.patch3.42 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 206 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#37 1936208-save-translation-locale-db-37.patch6.97 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 205 pass(es).
[ View ]
#34 interdiff.txt2.91 KBGábor Hojtsy
#34 1936208-save-translation-locale-db-34.patch7 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] 204 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#32 1936208-save-translation-locale-db-32.patch6.3 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 346 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#32 1936208-diff-28-32.txt1.24 KBvijaycs85
#28 1936208-save-translation-locale-db-28.patch6.55 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 318 pass(es), 6 fail(s), and 2 exception(s).
[ View ]
#26 1936208-save-translation-locale-db-26.patch6.51 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1936208-save-translation-locale-db-26.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#26 1936208-diff-24-26.txt963 bytesvijaycs85
#24 1936208-save-translation-locale-db-24.patch6.5 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 314 pass(es), 6 fail(s), and 2 exception(s).
[ View ]
#24 1936208-diff-22-24.txt1.73 KBvijaycs85
#22 1936208-save-translation-locale-db-22-test-only.patch2.92 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 314 pass(es), 6 fail(s), and 0 exception(s).
[ View ]
#22 1936208-save-translation-locale-db-22.patch6.49 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 314 pass(es), 6 fail(s), and 0 exception(s).
[ View ]
#19 1936208-save-translation-locale-db-19.patch3.58 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 297 pass(es).
[ View ]
#19 1936208-diff-15-19.txt1.23 KBvijaycs85
#15 1936208-save-translation-locale-db-15.patch3.5 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 276 pass(es), 16 fail(s), and 16 exception(s).
[ View ]
#15 1936208-diff-9-15.txt1.05 KBvijaycs85
#12 1936208-save-translation-locale-db-12.patch3.52 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 280 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#12 1936208-diff-9-11.txt2.63 KBvijaycs85
#9 1936208-save-translation-locale-db-8.patch3.43 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 278 pass(es), 13 fail(s), and 8 exception(s).
[ View ]
#9 1936208-diff-6-8.txt2.76 KBvijaycs85
#6 1936208-save-translation-locale-db-6.patch3.09 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 264 pass(es), 13 fail(s), and 8 exception(s).
[ View ]

Comments

YesCT’s picture

Gábor Hojtsy’s picture

Title:If config is not edited from shipped config or partially edited, also push translations to locale storage» If config is translated from shipped config or partially edited, also push translations to locale storage
Issue tags:+D8MI, +language-config

Fix title I think :)

YesCT’s picture

Gábor Hojtsy’s picture

It depends. There were non-perfect things committed to core before :D There are many things in this module not fully up to date to D8 yet. Eg. we use no classes for forms, we don't use the new routing system, and so on. Some of this should be fixed before commit (I'll create issues later), but its kind of impossible to tell which. If we hold ourselves to such high standards, then we can look for new ways to do thing all the time (docs changes, form class use, etc), and we'll have stuff to update up to the API freeze date, at which point there is 0% chance to get this in core. So it all depends on what the reviewers want in short. :)

I think this is blocking at least the release of the module in 8.0 (or of itself in contrib).

Gábor Hojtsy’s picture

Priority:Normal» Major
Status:Postponed» Active

#1905152: Integrate config schema with locale, so shipped configuration is translated just landed. We should fix this soon because now the core translation system will overwrite translations submitted with this module :D (For shipped configuration).

vijaycs85’s picture

StatusFileSize
new3.09 KB
FAILED: [[SimpleTest]]: [MySQL] 264 pass(es), 13 fail(s), and 8 exception(s).
[ View ]

Here is the first patch...

Gábor Hojtsy’s picture

Status:Active» Needs review
Issue tags:+Needs tests

Great start, I've had this logic in mind :)

+++ b/config_translation.admin.incundefined
@@ -289,21 +293,41 @@ function config_translation_form_submit(&$form, &$form_state) {
+ * @param bool $has_storage
+ *   Flag to specify whether save translation into locale DB.
+ *

Let's rename this to $shipped_config or something along those lines.

Also document it something like:

"Flag to specify whether the configuration had a shipped version and therefore should also be stored in the locale database."

+++ b/config_translation.admin.incundefined
@@ -289,21 +293,41 @@ function config_translation_form_submit(&$form, &$form_state) {
+          $translation =  $translation ?: locale_storage()->createTranslation($conditions);
+          $translation->setString($value)
+            ->setCustomized()
+            ->save();

We should check if $value is different from what is already in the translation. If it is the same, we should not set it as customised.

Status:Needs review» Needs work
Issue tags:-Needs tests

The last submitted patch, 1936208-save-translation-locale-db-6.patch, failed testing.

vijaycs85’s picture

Status:Needs work» Needs review
StatusFileSize
new2.76 KB
new3.43 KB
FAILED: [[SimpleTest]]: [MySQL] 278 pass(es), 13 fail(s), and 8 exception(s).
[ View ]

Thanks for the quick review @Gábor Hojtsy. Here is the patch that fixes comments in #7

Status:Needs review» Needs work

The last submitted patch, 1936208-save-translation-locale-db-8.patch, failed testing.

Gábor Hojtsy’s picture

Apart from the

+++ b/config_translation.admin.incundefined
@@ -276,6 +278,8 @@ function config_translation_form_submit(&$form, &$form_state) {
+ * @param Language $language
+ *  Language object.

Whitespace issue :)

+++ b/config_translation.admin.incundefined
@@ -292,21 +296,47 @@ function config_translation_form_submit(&$form, &$form_state) {
+ * @param bool $shipped_config
+ *   Flag to specify whether save translation into locale DB.

You did not take my improved doc suggestion here :)

+ the test fails Trying to get property of non-object on line 313 of the admin.inc resulting in the saving not working :)

vijaycs85’s picture

Status:Needs work» Needs review
StatusFileSize
new2.63 KB
new3.52 KB
FAILED: [[SimpleTest]]: [MySQL] 280 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Sorry about the comment miss...

vijaycs85’s picture

Issue tags:+Needs tests

re-storing tag back. wondering how @System Message at #8 can remove tag :)

Status:Needs review» Needs work

The last submitted patch, 1936208-save-translation-locale-db-12.patch, failed testing.

vijaycs85’s picture

Status:Needs work» Needs review
StatusFileSize
new1.05 KB
new3.5 KB
FAILED: [[SimpleTest]]: [MySQL] 276 pass(es), 16 fail(s), and 16 exception(s).
[ View ]

Seems checking isset() caused those test fails.

Status:Needs review» Needs work
Issue tags:-Needs tests, -D8MI, -language-config

The last submitted patch, 1936208-save-translation-locale-db-15.patch, failed testing.

vijaycs85’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work
Issue tags:+Needs tests, +D8MI, +language-config

The last submitted patch, 1936208-save-translation-locale-db-15.patch, failed testing.

vijaycs85’s picture

Status:Needs work» Needs review
StatusFileSize
new1.23 KB
new3.58 KB
PASSED: [[SimpleTest]]: [MySQL] 297 pass(es).
[ View ]

adding isset() and is_object() back to avoid notice and empty object fails...

vijaycs85’s picture

Title:If config is translated from shipped config or partially edited, also push translations to locale storage» SHREK
vijaycs85’s picture

Title:SHREK» If config is translated from shipped config or partially edited, also push translations to locale storage

oops, restoring title back.

vijaycs85’s picture

Issue tags:-Needs tests
StatusFileSize
new6.49 KB
FAILED: [[SimpleTest]]: [MySQL] 314 pass(es), 6 fail(s), and 0 exception(s).
[ View ]
new2.92 KB
FAILED: [[SimpleTest]]: [MySQL] 314 pass(es), 6 fail(s), and 0 exception(s).
[ View ]

Adding test case... consider test-only as interdiff...

Status:Needs review» Needs work

The last submitted patch, 1936208-save-translation-locale-db-22.patch, failed testing.

vijaycs85’s picture

Status:Needs work» Needs review
StatusFileSize
new1.73 KB
new6.5 KB
FAILED: [[SimpleTest]]: [MySQL] 314 pass(es), 6 fail(s), and 2 exception(s).
[ View ]

Updating wrong helper method name...

Status:Needs review» Needs work

The last submitted patch, 1936208-save-translation-locale-db-24.patch, failed testing.

vijaycs85’s picture

Status:Needs work» Needs review
StatusFileSize
new963 bytes
new6.51 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1936208-save-translation-locale-db-26.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

updating container or may be I should stop here and fix my local so that I can run before issue patch.

Status:Needs review» Needs work

The last submitted patch, 1936208-save-translation-locale-db-26.patch, failed testing.

vijaycs85’s picture

Status:Needs work» Needs review
StatusFileSize
new6.55 KB
FAILED: [[SimpleTest]]: [MySQL] 318 pass(es), 6 fail(s), and 2 exception(s).
[ View ]

Wrong commit range...

Status:Needs review» Needs work

The last submitted patch, 1936208-save-translation-locale-db-28.patch, failed testing.

Gábor Hojtsy’s picture

I'm not sure the right things are being tested here. Maybe that will also inform what do we do in the actual code btw :)

The locale storage/system is designed to translate shipped configuration, so when we change translation for the shipped configuration, it should update locale storage. By the point the config differs from shipped (on a string by string basis), we should not fill it back to locale storage. So this is both an issue with the fix and the test :)

1. So in the test, *do not change* the default config, just translate and check that locale storage was changed.

2. Then the test can change some of the config, translate that and some other things that are not changed and check that locale storage only changed for the config strings that was kept intact in the shipped state and not changed. (I think this will need functionality changes in the patch too?)

YesCT’s picture

Yeah, I was thinking of trying to write out detailed steps in words the other day but got confused about what to actually do where.

vijaycs85’s picture

Status:Needs work» Needs review
StatusFileSize
new1.24 KB
new6.3 KB
FAILED: [[SimpleTest]]: [MySQL] 346 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Updating the URL in #28
UPDATE: please ignore interdiff... it doesn't contain all changes.

Status:Needs review» Needs work

The last submitted patch, 1936208-save-translation-locale-db-32.patch, failed testing.

Gábor Hojtsy’s picture

Status:Needs work» Needs review
StatusFileSize
new7 KB
FAILED: [[SimpleTest]]: [MySQL] 204 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new2.91 KB

Added a few more asserts to prove the locations are found and the source string is found. It works for me. I did add a new custom language here to ensure the config locations end up in the DB. Earlier versions of the test used this up in setUp() but we sped that up for other tests. This test needs the full web flow to add a language so the config locations are added.

Status:Needs review» Needs work

The last submitted patch, 1936208-save-translation-locale-db-34.patch, failed testing.

Gábor Hojtsy’s picture

[12:46pm] GaborHojtsy: vijaycs85: yeah looking at the logic it would never save a new translation
[12:46pm] GaborHojtsy:         if ($shipped_config && isset($translation) && is_object($translation)) {
[12:46pm] GaborHojtsy:           $translation = $translation ?: locale_storage()->createTranslation($conditions);
[12:46pm] GaborHojtsy: vijaycs85: ^^^^ it does not have a change to get into $translation ?: …. because the condition above will not let the code in here if there was no prior translation
[12:47pm] GaborHojtsy: vijaycs85: so it can currently be used to update translations that are already in locale, but not to add new ones
[12:47pm] GaborHojtsy: vijaycs85: so legitimate test fail
vijaycs85’s picture

Status:Needs work» Needs review
StatusFileSize
new6.97 KB
PASSED: [[SimpleTest]]: [MySQL] 205 pass(es).
[ View ]
new3.42 KB
FAILED: [[SimpleTest]]: [MySQL] 206 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Fixing issue mentioned in #36.

Gábor Hojtsy’s picture

Status:Needs review» Needs work

Yay, we can work a bit on refining the conditions then :)

+++ b/config_translation.admin.incundefined
@@ -292,21 +296,46 @@ function config_translation_form_submit(&$form, &$form_state) {
       // Save value, if different from original configuration. If same as
       // original configuration, remove override.
       if ($base_config->get($key) !== $value) {
         $translation_config->set($key, $value);
+        // Store into locale DB, only on string change or new translation.
+        if ($shipped_config && isset($conditions)) {
+          $translation = $translation ?: locale_storage()->createTranslation($conditions);
+          if ($translation->isNew() || $translation->getString() != $value) {
+            $translation->setString($value)
+              ->setCustomized()
+              ->save();
+          }
+        }
       }
       else {
         $translation_config->clear($key);
+        if ($shipped_config && isset($translation) && is_object($translation)) {
+          $translation->delete();
+        }
       }
     }
   }

I think we should refine this logic. If the translation value is the same as the original value, we may not want to delete the translation. It would mean a translation update would restore the community translation. So we should save a customized value with the string coming from the form (same as the above logic).

In the upper logic, again, we update only if the value is not the original... I think the locale conditions should be independent entirely of the $base_config conditions, not nested inside them.

If the new value is different from *the locale stored value*, then we should save it there, regardless of how it is different from the original (English) base config.

So all the added lines in this hunk should be outside of the conditions for $base_config and have their own condition base on ($translation->isNew() || $translation->getString() != $value) only. If its not new and the same as the old one *in locale*, we should not do anything, otherwise we should update the translation in locale.

This will affect the tests a little.

Gábor Hojtsy’s picture

A quick example. Say you have

- Anonymous translated to Hungarian as Névtelen based on a .po file imported from the community
- You go update the translation to say Anonymous (which is a word understood in Hungary), now you get your locale storage removed
- You run the locale .po update that is built into core, it finds that you have a "new" string translating Anonymous to Névtelen (because you don't have a record of the translation customized)

Instead, what should happen is we save "Anonymous" as translation and set it to customized, so we protect it from turning back to where we changed it from. So this is the example to remove the delete stuff altogether for locale. We should not delete from locale.

For the update condition wrapping, now that is only reached if you enter something different from the original config. So in the above example, when you want to enter Anonymous, now if we don't delete anymore, what would happen is that the locale stored translation keeps being Névtelen, because the Antonymous word is the same as the source. So locale would not be updated either. That is wrong too.

So to make translation updates properly recorded and protected in the locale table, we should not delete them, we should update and mark as customized at all times. And we should do it even if it was the same as the source string to make sure we have a record of the user wanting to do that.

vijaycs85’s picture

StatusFileSize
new6.69 KB
FAILED: [[SimpleTest]]: [MySQL] 206 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new1.6 KB

Thanks for the sample and code help over IRC @Gábor Hojtsy. here is the update.

vijaycs85’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 1936208-save-translation-locale-db-39.patch, failed testing.

Gábor Hojtsy’s picture

So this proved my hunch right that the test would fail, since we don't remove the translation anymore. The test should check that the translation is still there AND that the value is now 'Anonymous'. It does not hurt to check also that the prior update properly updated it to 'Anonyme' as well.

Gábor Hojtsy’s picture

Assigned:Unassigned» Gábor Hojtsy

I'm looking into this.

Gábor Hojtsy’s picture

Status:Needs work» Needs review
StatusFileSize
new1.17 KB
new6.73 KB
PASSED: [[SimpleTest]]: [MySQL] 207 pass(es).
[ View ]

So this :)

Gábor Hojtsy’s picture

StatusFileSize
new2.76 KB
new6.87 KB
PASSED: [[SimpleTest]]: [MySQL] 205 pass(es).
[ View ]

Also looking at the code, I found the conditions for if ($shipped_config etc.) being duplicate now that they are right under each other, so that we can just wrap them in one. Also added some more comments and removed a TODO from the test. The .po file upload / custom string retention scenario in locale itself is tested within the locale module already, so no need to duplicate that here.

Gábor Hojtsy’s picture

Status:Needs review» Fixed

Superb, committed this one. Heroic effort @vijaycs85!

YesCT’s picture

#39 was a great example. added it to summary.
removing needs tag since this is committed. :)

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

Anonymous’s picture

Issue summary:View changes

added awesome example