Problem/Motivation

When a custom string is created all other strings in form become in custom string

Proposed resolution

verify unchanged strings are not tagged as custom strings

Steps to reproduce

  1. Install the latest Drupal 8.x using the standard profile and built-in English language.
  2. Enable Language, and Interface Translation module.
  3. Go to '/admin/config/regional/language', click 'Add language', and choose 'German'.
  4. Go to '/admin/config/regional/translate/import', Choose German and add translation file 'drupal-7.17.de'. Don't touch that three checkboxes in this page.
  5. Go to '/admin/config/regional/translate/translate', choose language 'German', search in 'Both translated and untranslated', and click filter.
  6. Edit the first translated string at cloumn 'TRANSLATION FOR GERMAN', and click 'Save translations'.
  7. Still this page, Choose 'Translation language' at German, 'Search in' at 'Only translated strings', 'Translation type' at 'Customized translation', click Filter. We get *all* those strings that were on that page as customized. In fact , we edited only *one* string.

Remaining tasks

  • Create instruction to reproduce the problem (thanks! @smiletrl , from #15. )
  • Create an initial patch to verity unchanged string are not tagged as custom string (from @Schnitzel in #13)
  • Create an initial Simple test(from @Schnitzel in #13)
  • (medium) review test-only to see if tests are complete
  • (novice) review patch. Review Task doc: http://drupal.org/node/1488992
  • (novice) manually test the fix/patch. Manually Test Task doc: http://drupal.org/node/1489010
  • (based on reviews and manual test) Improve fix/patch
  • (novice) Coding Standards. Contributor task: Improve patch coding standards and documentation http://drupal.org/node/1487976
  • (ongoing) update Remaining tasks as needed. This is in flux.

User interface changes

Changes in UI aren't required

API changes

Changes in UI aren't required

Original report by [username]

we saw this bug during Gabor's D8MI presentation at BADCamp, will check this myself, just keeping it as a reminder.

Files: 
CommentFileSizeAuthor
#30 drupal-uncustomized_language_strings-1832614-tests.patch3.6 KBsmiletrl
FAILED: [[SimpleTest]]: [MySQL] 50,578 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
#30 interdiff-13-29.txt3.31 KBsmiletrl
#30 drupal-uncustomized_language_strings-1832614-30.patch6.85 KBsmiletrl
PASSED: [[SimpleTest]]: [MySQL] 50,629 pass(es).
[ View ]
#26 drupal-uncustomized_language_strings-1832614-14.patch3.25 KBsmiletrl
PASSED: [[SimpleTest]]: [MySQL] 50,499 pass(es).
[ View ]
#26 interdiff-13-14.txt1.06 KBsmiletrl
#13 1832614-test-only-will-fail.patch3.2 KBSchnitzel
FAILED: [[SimpleTest]]: [MySQL] 50,701 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
#13 1832614-13.patch6.35 KBSchnitzel
FAILED: [[SimpleTest]]: [MySQL] 50,500 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#13 interdiff-1-13.txt3.76 KBSchnitzel
#7 1832614-test.patch2.49 KB-enzo-
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1832614-test.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#2 1832614-1.patch3.21 KBSchnitzel
PASSED: [[SimpleTest]]: [MySQL] 50,647 pass(es).
[ View ]

Comments

Gábor Hojtsy’s picture

Title:Interface translation: all strings are changed from non-customized to customized even only one string is translated» Unchanged strings on page become customized when editing others
Issue tags:+language-ui
Schnitzel’s picture

Title:Unchanged strings on page become customized when editing others» Interface translation: all strings are changed from non-customized to customized even only one string is translated
Status:Active» Needs review
Issue tags:-language-ui
StatusFileSize
new3.21 KB
PASSED: [[SimpleTest]]: [MySQL] 50,647 pass(es).
[ View ]

attached a patch which fixes this issue: it does only save the strings if they are actually changed, not just a translation entered.
refactored the whole thing, as there where pretty confusing variable names.

Schnitzel’s picture

Title:Interface translation: all strings are changed from non-customized to customized even only one string is translated» Unchanged strings on page become customized when editing others
Issue tags:+language-ui

xposting

attiks’s picture

Patch looks good, but don't we need a test?

Gábor Hojtsy’s picture

Issue tags:+Needs tests

Yes, definitely need tests.

Gábor Hojtsy’s picture

+++ b/core/modules/locale/locale.pages.incundefined
@@ -402,31 +402,44 @@ function locale_translate_edit_form_submit($form, &$form_state) {
+    } else if (!$existing_translation && !empty($new_translation_string)) {

As per coding standard this would need to be "elseif" and on a separate line.

-enzo-’s picture

StatusFileSize
new2.49 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1832614-test.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

First initial version of patch for Simple Test

Schnitzel’s picture

Status:Needs review» Needs work

will finish these tests.

Gábor Hojtsy’s picture

Woot, thanks!

-enzo-’s picture

Assigned:Schnitzel» -enzo-

Adding Issue summary template

-enzo-’s picture

Assigned:-enzo-» Unassigned
Schnitzel’s picture

Assigned:Unassigned» Schnitzel
Schnitzel’s picture

Status:Needs work» Needs review
StatusFileSize
new3.76 KB
new6.35 KB
FAILED: [[SimpleTest]]: [MySQL] 50,500 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new3.2 KB
FAILED: [[SimpleTest]]: [MySQL] 50,701 pass(es), 4 fail(s), and 0 exception(s).
[ View ]

new version:
- fixed "else if" issue.
- added Tests to check that strings are only saved and marked as customized if they are actually changed.

Also patch with tests only, which should fail.

Gábor Hojtsy’s picture

Status:Needs review» Needs work

The proposed patch looks good, I only found one condition I think should be tested and some minor code fixes:

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleTranslationTest.phpundefined
@@ -467,4 +468,65 @@ function testStringSearch() {
+   * Test that strings are only saved and marked as customized when they are
+   * actually changed.

Should be shortened to one line if possible :) Eg.

"Tests that only changed strings are saved customized when edited."

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleTranslationTest.phpundefined
@@ -467,4 +468,65 @@ function testStringSearch() {
+    // Queries strings with showing only non-customized strings.

"Query strings..." - no need to talk 3rd person here.

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleTranslationTest.phpundefined
@@ -467,4 +468,65 @@ function testStringSearch() {
+    // Submitting the translations withouth changing the translation, so the

without

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleTranslationTest.phpundefined
@@ -467,4 +468,65 @@ function testStringSearch() {
+    // Submitting the translations with a new the translation, so the
+    // translation will be saved as customized and no result will be returned.
+    $textarea = current($this->xpath('//textarea'));
+    $lid = (string) $textarea[0]['name'];
+    $edit = array(
+      $lid => $this->randomName(100),
+    );
+    $this->drupalPost('admin/config/regional/translate/translate', $edit, t('Save translations'));
+
+    $this->assertText(t('No strings available.'), "No result anymore, so the string has been saved as customized.");

I would add a test that it actually shows up on a separate search for customized strings specifically.

+++ b/core/modules/locale/locale.pages.incundefined
@@ -402,31 +402,45 @@ function locale_translate_edit_form_submit($form, &$form_state) {
+    // compare the new strings with the existing strings the same strings
+    // are created.

"a string in the same format is created" instead of "the same strings are created", since we are only creating one string, right? :)

+++ b/core/modules/locale/locale.pages.incundefined
@@ -402,31 +402,45 @@ function locale_translate_edit_form_submit($form, &$form_state) {
+    $is_changed = FALSE;
+
+    if ($existing_translation && $existing_translation_objects[$lid]->translation != $new_translation_string_delimited) {
+      // If there is an existing translation in the DB and the new translation
+      // is not the same as the existing one.
+      $is_changed = TRUE;
     }
-    if ($has_translation) {
+    elseif (!$existing_translation && !empty($new_translation_string)) {
+      $is_changed = TRUE;
+    }

I can see that this is broken down to an if() and elseif() clause to delineate the two conditions. So maybe adding more docs to the second one, eg. "Newly entered translation." or somesuch would help underline the need for the two blocks of code (instead just one big condition).

smiletrl’s picture

Steps to reproduce

  1. Install the latest Drupal 8.x using the standard profile and built-in English language.
  2. Enable Language, Locale, and Entity Translation module.
  3. Go to '/admin/config/regional/language', click 'Add language', and choose 'German'.
  4. Go to '/admin/config/regional/translate/import', Choose German and add translation file 'drupal-7.17.de'. Don't touch that three checkboxes in this page.
  5. Go to '/admin/config/regional/translate/translate', choose language 'German', search in 'Both translated and untranslated', and click filter.
  6. Edit the first translated string at cloumn 'TRANSLATION FOR GERMAN', and click 'Save translations'.
  7. Still this page, Choose 'Translation language' at German, 'Search in' at 'Only translated strings', 'Translation type' at 'Customized translation', click Filter. We get all strings as customized. In fact , we edited only one string.
smiletrl’s picture

hmmm, did I missing something. After I applied this patch, this problem remains. Maybe I need a refresh install to test this?

Gábor Hojtsy’s picture

Issue tags:+sprint

@smiletrl: well, the tests were not complete above either, so the fix might not be complete either. I don't think you would need a fresh install, but trying again never hurts :) Your steps to reproduce seem spot on.

Gábor Hojtsy’s picture

Assigned:Schnitzel» Unassigned

BTW I believe @Schnitzel does not have capacity to work on this, so unassigning to make it evident that it is free to take. Need people to work on this :)

Gábor Hojtsy’s picture

Issue summary:View changes

update Issue with report template

YesCT’s picture

Issue summary:View changes

Updated issue summary remaining tasks to make it easier for people to find a part of the issue to do

YesCT’s picture

Issue tags:+Novice, +medium

Some of the remaining tasks are novice, some medium (like reviewing the tests to see if they are complete). The tasks do not have to be done in order. This is a good one for a couple people to jump into.

YesCT’s picture

Issue summary:View changes

fix html

YesCT’s picture

Status:Needs work» Needs review
Issue tags:-Needs tests, -Novice, -D8MI, -sprint, -language-ui, -medium

#13: 1832614-13.patch queued for re-testing.

rkjha’s picture

#13: 1832614-13.patch queued for re-testing.

rkjha’s picture

#13: 1832614-test-only-will-fail.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, 1832614-13.patch, failed testing.

rkjha’s picture

Status:Needs work» Needs review

#7: 1832614-test.patch queued for re-testing.

rkjha’s picture

#2: 1832614-1.patch queued for re-testing.

rkjha’s picture

Issue summary:View changes

Updated issue summary to help novice people identify what they can do.

smiletrl’s picture

StatusFileSize
new1.06 KB
new3.25 KB
PASSED: [[SimpleTest]]: [MySQL] 50,499 pass(es).
[ View ]

Since the past patch failed automated test, I guess a better idea is to seperate the test code from fix code.

This attachment is devided from #13 only to fix the problem, not including test code, and it's been updated according to suggestions at #14.

To test the patch, we will need another fresh installment of drupal8, because it won't have effect on existing records in db. That's why I didn't see it work last time.

Let's see will this patch make drupal robot happy:) Will work on test code later.

Gábor Hojtsy’s picture

Well, the earlier tests looked pretty complete, so I think it is better to start looking if the test had issues, or it properly identified a bug with the fix itself. As opposed to starting to write a new test from scratch... Thanks for picking this up!

smiletrl’s picture

Assigned:Unassigned» smiletrl

There're some bugs with the test code.

Firstly, before the last two test assertText, there's no search/filter action, like

<?php
    $this
->drupalPost('admin/config/regional/translate/translate', $edit, t('Save translations'));

   
$source = $this->assertText($translation->getString(), 'Translation still marked as non-customized.');
?>

There should be search/filter action, e.g,

<?php
   $this
->drupalPost('admin/config/regional/translate/translate', $search, t('Filter'));
?>

after saving strings and right before assertion.

Secondly, in this issue's case, we need to create more than one translated and uncustomized strings(manually, we import .po file to do this), update one of them as customized, and then search the customized strings. I think we need assertText to find that customized string and assertNoText to make sure uncustomized strings are not saved as customized. This is also what suggested in #14.

Let me try to fix this:)

smiletrl’s picture

Status:Needs review» Needs work
smiletrl’s picture

StatusFileSize
new6.85 KB
PASSED: [[SimpleTest]]: [MySQL] 50,629 pass(es).
[ View ]
new3.31 KB
new3.6 KB
FAILED: [[SimpleTest]]: [MySQL] 50,578 pass(es), 4 fail(s), and 0 exception(s).
[ View ]

The first patch is the test code, and of course it will fail. It's been updated according to #29/#28, although there're some tweaks.

In this issue, when save unchanged translation strings, these strings are saved as customized. So

+    // Ensure unchanged translation string does appear if searching non-customized translation.
+    $search = array(
+      'string' => $string->getString(),
+      'langcode' => 'de',
+      'translation' => 'translated',
+      'customized' => '0',
+    );
+    $this->drupalPost('admin/config/regional/translate/translate', $search, t('Filter'));
+    $source = $this->assertText($string->getString(), 'Translation is not marked as customized.');

will fail. And the next search will fail too, because this search fails.

interdiff-13-29.txt indicates changes of test code between #13 and #29/#28.

The final patch is the whole patch, including fix code and test code. All these patches work fine in local tests. So, let's see will they satisfy drupal testbot~

Gábor Hojtsy’s picture

Status:Needs work» Needs review

Sending for tests. @smiletrl: thanks!

Gábor Hojtsy’s picture

Status:Needs review» Reviewed & tested by the community

The changes look good to me and testing the fixed functionality properly. Thanks!

Dries’s picture

Status:Reviewed & tested by the community» Fixed

Long variable names, but committed to 8.x. Thanks.

Schnitzel’s picture

@smiletrl thanks for jumping in! awesome :)

Gábor Hojtsy’s picture

Issue tags:-sprint

Woot, thanks!

Gábor Hojtsy’s picture

@smiletrl: thanks again! If you are looking for tasks on a similar scale #1853720: Hide language selection option is backwards or #1869328: Restore simplicity of language list are very good candidates :) Thanks for helping out :)

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

Anonymous’s picture

Issue summary:View changes

Update the reporducing steps with new drupal 8 release.