Files: 
CommentFileSizeAuthor
#61 1797364-61-t-locale.patch813 bytesdcam
PASSED: [[SimpleTest]]: [MySQL] 40,473 pass(es).
[ View ]
#56 interdiff.txt1.88 KBdcam
#56 1797364-56-t-locale.patch51.5 KBdcam
PASSED: [[SimpleTest]]: [MySQL] 40,339 pass(es).
[ View ]
#54 1797364-54-t-locale.patch51.5 KBdcam
PASSED: [[SimpleTest]]: [MySQL] 40,356 pass(es).
[ View ]
#50 1797364-50-t-locale.patch50.86 KBdcam
PASSED: [[SimpleTest]]: [MySQL] 57,445 pass(es).
[ View ]
#47 interdiff.txt1.93 KBdcam
#47 1797364-47-t-locale.patch50.78 KBdcam
PASSED: [[SimpleTest]]: [MySQL] 57,166 pass(es).
[ View ]
#44 1797364-44-t-locale.patch49.78 KBdcam
PASSED: [[SimpleTest]]: [MySQL] 56,787 pass(es).
[ View ]
#41 1797364-41-t-locale.patch49.21 KBdcam
FAILED: [[SimpleTest]]: [MySQL] 56,502 pass(es), 5 fail(s), and 0 exception(s).
[ View ]
#39 1797364-39-t-locale-reroll-do-not-test.patch49.27 KBdcam
#39 interdiff.txt7.23 KBdcam
#39 1797364-39-t-locale.patch49.21 KBdcam
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/locale/lib/Drupal/locale/Tests/LocaleJavascriptTranslation.php.
[ View ]
#35 remove-t-from-locale-asserts-1797364-35.patch50.07 KBlazysoundsystem
PASSED: [[SimpleTest]]: [MySQL] 56,658 pass(es).
[ View ]
#33 remove-t-from-locale-asserts-1797364-33.patch50.12 KBlazysoundsystem
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch remove-t-from-locale-asserts-1797364-33.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#31 cmi-statistics.patch59.83 KBneetu morwani
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/locale/lib/Drupal/locale/Tests/LocaleUninstallTest.php.
[ View ]
#30 cmi-statistics.patch59.83 KBneetu morwani
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/locale/lib/Drupal/locale/Tests/LocaleUninstallTest.php.
[ View ]
#27 1797364-27-t-locale-reroll-do-not-test.patch51.64 KBdcam
#27 interdiff.txt5.03 KBdcam
#27 1797364-27-locale-t.patch49.8 KBdcam
PASSED: [[SimpleTest]]: [MySQL] 58,789 pass(es).
[ View ]
#20 interdiff.txt6.88 KBdcam
#20 1797364-20-t-locale.patch52.37 KBdcam
PASSED: [[SimpleTest]]: [MySQL] 55,385 pass(es).
[ View ]
#14 Screen Shot 2012-10-11 at 21.27.07 .png58.63 KBboran
#10 1797364-10-t-locale.patch58.36 KBLars Toomre
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1797364-10-t-locale.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#4 1797364-4-t-locale.patch60.33 KBLars Toomre
PASSED: [[SimpleTest]]: [MySQL] 41,896 pass(es).
[ View ]
#4 interdiff-1797364-3-5.txt5.55 KBLars Toomre
#3 locale-1797364-3.patch58.54 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 41,695 pass(es).
[ View ]
#3 interdiff.txt967 bytesxjm
#1 locale-1797364-1.patch58.55 KBxjm
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/locale/lib/Drupal/locale/Tests/LocaleJavascriptTranslation.php.
[ View ]

Comments

xjm’s picture

Assigned:xjm» Unassigned
Status:Active» Needs review
StatusFileSize
new58.55 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/locale/lib/Drupal/locale/Tests/LocaleJavascriptTranslation.php.
[ View ]

Includes lots of format_string() replacements.

An assertion in LocaleJavascriptTranslation was trying to win the Fewest Lines of Code Award™ with multiple conditions, a ternary, and two uses of t() with arguments inside the assertion. I factored it out into something somewhat more sane:

<?php
     
// Make sure that the proper context was matched.
     
$message = strlen($context) > 0 ? 'Context for %source is %context' : 'Context for %source is blank';
      if (isset(
$source_strings[$str]) && $source_strings[$str] === $context) {
       
$this->pass(format_string($message, $args));
      }
      else {
       
$this->fail(format_string($message, $args));
      }
    }
?>

Since the string doesn't need to be translated, the rule about using it directly within t() of course does not apply, which is why we can store the message template with placeholders to a variable for readability.

Status:Needs review» Needs work

The last submitted patch, locale-1797364-1.patch, failed testing.

xjm’s picture

Status:Needs work» Needs review
StatusFileSize
new967 bytes
new58.54 KB
PASSED: [[SimpleTest]]: [MySQL] 41,695 pass(es).
[ View ]

I think this is a sign that I should stop doing these now. :)

Lars Toomre’s picture

StatusFileSize
new5.55 KB
new60.33 KB
PASSED: [[SimpleTest]]: [MySQL] 41,896 pass(es).
[ View ]

I reviewed the patch in #3 and found a couple of things further to clean up. Attached is a revised patch and an interdiff.

Lars Toomre’s picture

This is going to conflict with #1808374: Wrong plural for french in locale tests depending on which gets in first.

xjm’s picture

Status:Needs review» Needs work

Thanks Lars. The changes in #4 look mostly correct as well (though the group message part wasn't part of the original scope). One issue:

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocalePluralFormatTest.phpundefined
@@ -121,12 +121,24 @@ class LocalePluralFormatTest extends WebTestBase {
+          format_string('Computed plural index for %langcode for count %count is %expected', array(
...
+            '%expect' => $expected_plural_index

Bug here. The placeholder is %expected but the array key is %expect.

I also had to think twice about these messages, since they include t()-style placeholders in them, but they should default to displaying the placeholder as part of the message when it's not set in the array of substitutions.

Lars Toomre’s picture

Thanks for the review @xjm. Good catch on '%expect' where it should be '%expected'.

Regarding t() around the group parameter, in one of the sub-issues of this initiative (sorry, I cannot recall which one), @jhodgdon stated that she would welcome any patches that also remove t() from around the group parameter. Hence, since then, where appropriate, I have added that change as well.

I will try to re-roll a patch for the issue you noted in #6.

Lars Toomre’s picture

My limited git skills are failing me. Apparently since #4 was created, an independent commit was made to LocaleUninstallTest.php. Hence, I am not able to resolve what is going wrong with git apply.

Perhaps someone else can re-roll the patch in #4 that includes the comment for #6. Thanks in advance.

xjm’s picture

@Lars Toomre, there are two things you can try. One would be to reroll the patch with rebase and resolve any merge conflicts. The other would be to apply the patch with git apply --reject, which would apply all the lines but the ones that no longer work, and then re-add those changes manually if appropriate.

Lars Toomre’s picture

StatusFileSize
new58.36 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1797364-10-t-locale.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Thanks @xjm. The second option in #9 worked and I simply went through and made all of the changes again in LocaleUninstallTest.php. Hence, that file too needs to reviewed carefully again in the attached patch. I am unable to create an interdiff at present.

Lars Toomre’s picture

Status:Needs work» Needs review

Grr.. Need to remember to change the status.

xjm’s picture

Glad to see that worked.

I reviewed #10 carefully both locally and in Dreditor (gotta love the new patch format from .gitattributes) and didn't find any additional errors. (Several things to be careful of in this patch since it's from the module that tests t() itself!)

I can't RTBC this because I created the original patch.

Lars Toomre’s picture

I obviously cannot RBTC this patch either. Perhaps we can recruit someone (beside @jhodgdon) to do a review so that @jhodgdon can do a further review before commit?

boran’s picture

StatusFileSize
new58.63 KB

I looked though he sources changes, and didn't notice any problems. The basic change involved removing t() from the second argument to assertRaw().

There is the issue of "1 heure" to "@count heure" as noted in a separated issue #1808374.
$this->assertRaw("msgid \"1 hour\"\nmsgid_plural \"@count hours\"\nmsgstr[0] \"1 heure\"\nmsgstr[1] \"@count heures\"", 'Plural translations exported properly.');

So, did a fresh d8 install from git, enabled locale and added the french language.
Ran the locale tests from admin/config/development/testing
Applied the patch in #10, and reran the tests and verified that the results are exactly the same (see attached screenshot).
==> Is this the testing required? Please advise what you'd like done.
Now, I'm pretty new to units tests, so I'm not sure if this review i enough. I presume the idea was to see if the lines where t() was removed contained syntax errors or cause tests to fail?

Lars Toomre’s picture

Issue tags:-needs backport to D7

#10: 1797364-10-t-locale.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+needs backport to D7

The last submitted patch, 1797364-10-t-locale.patch, failed testing.

Lars Toomre’s picture

Issue #1809962: Move some locale updates to update.inc for a safe language upgrade. was committed so this will need to re-rolled.

xjm’s picture

Thanks @boran for the review! Yep, for this issue, it's basically just a matter of ensuring that each change is transparent (i.e. that it only has the effect of removing the coupling to translation, without altering the actual assertion displayed at all or accidentally altering different parameters).

git diff --color-words --unified=0 with the patch applied is a good way to scan the changes, and for trickier bits running the test locally and checking the assertions is a good idea too, though in most cases that's not necessary. Testbot takes care of checking for failed tests and syntax errors for us.

xjm’s picture

dcam’s picture

Status:Needs work» Needs review
StatusFileSize
new52.37 KB
PASSED: [[SimpleTest]]: [MySQL] 55,385 pass(es).
[ View ]
new6.88 KB

Rerolled #10. interdiff.txt shows additional t() functions removed after the reroll.

izus’s picture

#20: 1797364-20-t-locale.patch queued for re-testing.

izus’s picture

Status:Needs review» Reviewed & tested by the community

hi,
the patch seems good.
Thanks

jhodgdon’s picture

I was taking a look at committing this, but really I don't think the changes to some of the tests' syntax in several places are very good. They make the code less clear IMO. I'll leave this for someone else to commit if they want to.

I don't really get why we need to use a really complicated format_string() in place of what was a fairly simple string concatenation in a test assertion?

alexpott’s picture

Status:Reviewed & tested by the community» Needs work
+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocalePluralFormatTest.phpundefined
@@ -121,12 +121,24 @@ function testGetPluralFormat() {
-        $this->assertIdentical(locale_get_plural($count, $langcode), $expected_plural_index, 'Computed plural index for ' . $langcode . ' for count ' . $count . ' is ' . $expected_plural_index);
+        $this->assertIdentical(locale_get_plural($count, $langcode), $expected_plural_index,
+          format_string('Computed plural index for %langcode for count %count is %expected', array(
+            '%langcode' => $langcode,
+            '%count' => $count,
+            '%expected' => $expected_plural_index
+          ))
+        );
         // Assert that the we get the right translation for that. Change the
         // expected index as per the logic for translation lookups.
         $expected_plural_index = ($count == 1) ? 0 : $expected_plural_index;
         $expected_plural_string = str_replace('@count', $count, $plural_strings[$langcode][$expected_plural_index]);
-        $this->assertIdentical(format_plural($count, '1 hour', '@count hours', array(), array('langcode' => $langcode)), $expected_plural_string, 'Plural translation of 1 hours / @count hours for count ' . $count . ' in ' . $langcode . ' is ' . $expected_plural_string);
+        $this->assertIdentical(format_plural($count, '1 hour', '@count hours', array(), array('langcode' => $langcode)), $expected_plural_string,
+          format_string('Plural translation of 1 hours / @count hours for count %count in %langcode is %expected', array(
+            '%count' => $count,
+            '%langcode' => $langcode,
+            '%expected' => $expected_plural_string
+          ))
+        );

These changes are out-of-scope... we're not removing a call to t() here.

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUninstallTest.phpundefined
@@ -83,7 +83,7 @@ function testUninstallProcess() {
-    $this->assertTrue($result = file_exists($js_file), t('JavaScript file created: %file', array('%file' => $result ? $js_file : t('none'))));
+    $this->assertTrue($result = file_exists($js_file), format_string('JavaScript file created: %file', array('%file' => $result ? $js_file : 'none')));

@@ -109,31 +109,31 @@ function testUninstallProcess() {
-    $this->assertTrue($result = !file_exists($js_file), t('JavaScript file deleted: %file', array('%file' => $result ? $js_file : t('found'))));
+    $this->assertTrue($result = !file_exists($js_file), format_string('JavaScript file deleted: %file', array('%file' => $result ? $js_file : 'found')));

The messages here look bogus... surely we the creation of deletion is unsuccessful we want to know the name of the file!... the following looks much simpler to me...

  $this->assertTrue(file_exists($js_file), format_string('JavaScript file created: %file', array('%file' => $js_file)));
...
  $this->assertFalse(file_exists($js_file), format_string('JavaScript file deleted: %file', array('%file' => $js_file)));
+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleJavascriptTranslation.phpundefined
@@ -90,12 +90,18 @@ function testFileParsing() {
       // Make sure that the proper context was matched.
-      $this->assertTrue(isset($source_strings[$str]) && $source_strings[$str] === $context, strlen($context) > 0 ? t("Context for %source is %context", $args) : t("Context for %source is blank", $args));
+      $message = strlen($context) > 0 ? 'Context for %source is %context' : 'Context for %source is blank';
+      if (isset($source_strings[$str]) && $source_strings[$str] === $context) {
+        $this->pass(format_string($message, $args));
+      }
+      else {
+        $this->fail(format_string($message, $args));
+      }

This change is not logically equivalent... something like the following is..

      $test = isset($source_strings[$str]) && $source_strings[$str] === $context;
      if (strlen($context) > 0) {
        $this->assertTrue($test, format_string('Context for %source is %context', $args));
      }
      else {
        $this->assertTrue($test, format_string('Context for %source is blank', $args));
      }
jhodgdon’s picture

Bump! There are only a few of these "remove t()" issues left. Does someone want to make a new patch addressing the comments in #24 so we can get this one done?

dcam’s picture

I'm camping this weekend. I probably won't be able to fix it until Tuesday. Maybe someone else will get to it first. It would be nice to finally get these reviewed and out of my issue queue.

dcam’s picture

Status:Needs work» Needs review
StatusFileSize
new49.8 KB
PASSED: [[SimpleTest]]: [MySQL] 58,789 pass(es).
[ View ]
new5.03 KB
new51.64 KB

Rerolled #20, then made the changes from #24. Changes are shown in interdiff.txt.

lazysoundsystem’s picture

Status:Needs review» Reviewed & tested by the community

At last I think we're going to get to the end of this issue... RTBC.

jhodgdon’s picture

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs reroll

Sorry, but the patch does not apply today.

neetu morwani’s picture

Status:Needs work» Needs review
StatusFileSize
new59.83 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/locale/lib/Drupal/locale/Tests/LocaleUninstallTest.php.
[ View ]

rerolled patch in the tests for the locale module.

neetu morwani’s picture

StatusFileSize
new59.83 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/locale/lib/Drupal/locale/Tests/LocaleUninstallTest.php.
[ View ]

rerolled patch in the tests for the locale module.

Status:Needs review» Needs work

The last submitted patch, cmi-statistics.patch, failed testing.

lazysoundsystem’s picture

Status:Needs work» Needs review
StatusFileSize
new50.12 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch remove-t-from-locale-asserts-1797364-33.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

The cmi statistics patch in #31 involves quite a few other changes. Re-rolling the patch in #27 has one change that needed to be removed, and removal of another remaining t().

These from the patch in #31

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUninstallTest.php
@@ -68,7 +68,11 @@ function testUninstallProcess() {
+<<<<<<< HEAD
     $this->assertEqual(language(Language::TYPE_INTERFACE)->id, $this->langcode, t('Current language: %lang', array('%lang' => language(Language::TYPE_INTERFACE)->id)));
+=======

This change should probably go in a different issue.

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUninstallTest.php
@@ -115,31 +119,35 @@ function testUninstallProcess() {
+<<<<<<< HEAD
     $this->assertEqual(language(Language::TYPE_INTERFACE)->id, 'en', t('Language after uninstall: %lang', array('%lang' => language(Language::TYPE_INTERFACE)->id)));
+=======
+    $this->assertEqual(language(Language::TYPE_INTERFACE)->langcode, 'en', format_string('Language after uninstall: %lang', array('%lang' => language(Language::TYPE_INTERFACE)->langcode)));
+>>>>>>> Rerollong the patch for the locale module https://drupal.org/node/1797364#comment-7573647

     // Check JavaScript files deletion.
-    $this->assertTrue($result = !file_exists($js_file), t('JavaScript file deleted: %file', array('%file' => $result ? $js_file : t('found'))));
+    $this->assertFalse(file_exists($js_file), format_string('JavaScript file deleted: %file', array('%file' => $js_file)));

and this too.

And the many changes to LocaleUpdateTest.php

Status:Needs review» Needs work

The last submitted patch, remove-t-from-locale-asserts-1797364-33.patch, failed testing.

lazysoundsystem’s picture

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new50.07 KB
PASSED: [[SimpleTest]]: [MySQL] 56,658 pass(es).
[ View ]

Sorry for the noise, there was another change, from '->langcode' to '->id'. This one applies.

jhodgdon’s picture

THIS IS THE LAST ONE OF THESE plain-vanilla remove t() from test assert issues!!!! (at least, I think it is)... Can someone please review it so we can get it into 8.x, then backport to 7.x, and be DONE with the meta-issue? :)

Lars Toomre’s picture

Status:Needs review» Reviewed & tested by the community

I do not have a local D8 installation to apply the patch in #35 against. Hence, I am unable to confirm that the resulting patched file has caught all of the appropriate t() calls.

However, what is changed in the patch I can confirm is all correct. Hence, I think that this final pain-vanilla remove t() patches is ready to go!

jhodgdon’s picture

Status:Reviewed & tested by the community» Needs work

There are still problems with this patch:

a) This segment of LocaleJavascriptTranslation.php is not quite right:

      // Make sure that the proper context was matched.
-      $this->assertTrue(isset($source_strings[$str]) && $source_strings[$str] === $context, strlen($context) > 0 ? t("Context for %source is %context", $args) : t("Context for %source is blank", $args));
+     $test = isset($source_strings[$str]) && $source_strings[$str] === $context;
+      if (strlen($context) > 0) {
+        $this->assertTrue($test, format_string('Context for %source is %context', $args));
+      }
+      else {
+        $this->assertTrue($test, format_string('Context for %source is blank', $args));
+      }

For one thing, the indentation of the first line is wrong. For another thing, let's just keep the code as it was and remove t() rather than making completely new code. Out of scope for this issue. Thanks!

b) Also, on another issue we were asked to stop using format_string():
#2035077-2: removing t() from asserts - remaining changes.
so I guess I cannot commit this until that is addressed for lines changed in this patch. :(

c) Don't change the code around in tests -- out of scope -- such as this segment of LocaleUninstallTest.php:

-    $this->assertTrue($result = !file_exists($js_file), t('JavaScript file deleted: %file', array('%file' => $result ? $js_file : t('found'))));
+    $this->assertFalse(file_exists($js_file), format_string('JavaScript file deleted: %file', array('%file' => $js_file)));
dcam’s picture

Status:Needs work» Needs review
StatusFileSize
new49.21 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/locale/lib/Drupal/locale/Tests/LocaleJavascriptTranslation.php.
[ View ]
new7.23 KB
new49.27 KB

Here's a reroll and the changes from #38.

Status:Needs review» Needs work

The last submitted patch, 1797364-39-t-locale.patch, failed testing.

dcam’s picture

Status:Needs work» Needs review
StatusFileSize
new49.21 KB
FAILED: [[SimpleTest]]: [MySQL] 56,502 pass(es), 5 fail(s), and 0 exception(s).
[ View ]

Let's try this again. A '}' was accidentally deleted.

Status:Needs review» Needs work

The last submitted patch, 1797364-41-t-locale.patch, failed testing.

lazysoundsystem’s picture

Thanks @dcam for taking this back to basics, it had started to get out of control. The patch applies fine and looks good, but it needs use Drupal\Component\Utility\String; at the top of each file that uses String::format.

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleJavascriptTranslation.php
@@ -96,12 +96,12 @@ function testFileParsing() {
+      $this->assertTrue(isset($source_strings[$str]), String::format("Found source string: %source", $args));

here,

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleTranslationUiTest.php
@@ -261,14 +261,14 @@ function testJavaScriptTranslation() {
+    $this->assertTrue($result = file_exists($js_file), String::format('JavaScript file created: %file', array('%file' => $result ? $js_file : 'not found')));

and here,

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUninstallTest.php
@@ -89,7 +89,7 @@ function testUninstallProcess() {
+    $this->assertTrue(file_exists($js_file), String::format('JavaScript file created: %file', array('%file' => $js_file)));

and here.

dcam’s picture

Status:Needs work» Needs review
StatusFileSize
new49.78 KB
PASSED: [[SimpleTest]]: [MySQL] 56,787 pass(es).
[ View ]

Oops. I guess I expected the namespace was already loaded or maybe I just wasn't thinking.

lazysoundsystem’s picture

Status:Needs review» Reviewed & tested by the community

Thanks - this is now complete. RTBC.

jhodgdon’s picture

Status:Reviewed & tested by the community» Needs work

Ummm... This change in LocaleUninstallTest still doesn't look right to me:

-    $this->assertTrue($result = file_exists($js_file), t('JavaScript file created: %file', array('%file' => $result ? $js_file : t('none'))));
+    $this->assertTrue(file_exists($js_file), String::format('JavaScript file created: %file', array('%file' => $js_file)));

I think the message used to say "none" if the test failed and the file did not exist. That won't happen with the patch.

Everything else looks OK.

dcam’s picture

Status:Needs work» Needs review
StatusFileSize
new50.78 KB
PASSED: [[SimpleTest]]: [MySQL] 57,166 pass(es).
[ View ]
new1.93 KB

I checked for other problems, but obviously wasn't diligent enough. I also made certain to check for newer t() functions and found one.

lazysoundsystem’s picture

Status:Needs review» Reviewed & tested by the community

Well, this issue has already been RTBC four times (twice by me), so I'm a bit hesitant to try once more. RTBC.

alexpott’s picture

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs reroll

Needs a reroll...

git ac https://drupal.org/files/1797364-47-t-locale.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 51995  100 51995    0     0  15942      0  0:00:03  0:00:03 --:--:-- 27409
error: patch failed: core/modules/locale/lib/Drupal/locale/Tests/LocaleContentTest.php:125
error: core/modules/locale/lib/Drupal/locale/Tests/LocaleContentTest.php: patch does not apply
error: patch failed: core/modules/locale/lib/Drupal/locale/Tests/LocaleExportTest.php:110
error: core/modules/locale/lib/Drupal/locale/Tests/LocaleExportTest.php: patch does not apply
error: patch failed: core/modules/locale/lib/Drupal/locale/Tests/LocalePathTest.php:112
error: core/modules/locale/lib/Drupal/locale/Tests/LocalePathTest.php: patch does not apply
error: patch failed: core/modules/locale/lib/Drupal/locale/Tests/LocaleUninstallTest.php:89
error: core/modules/locale/lib/Drupal/locale/Tests/LocaleUninstallTest.php: patch does not apply
dcam’s picture

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new50.86 KB
PASSED: [[SimpleTest]]: [MySQL] 57,445 pass(es).
[ View ]

Ugh.... rerolled #47.

lazysoundsystem’s picture

Status:Needs review» Reviewed & tested by the community

For what it's worth, once more: RTBC.

jhodgdon’s picture

Version:8.x-dev» 7.x-dev
Status:Reviewed & tested by the community» Patch (to be ported)

THANKS! Committed to 8.x (before it needs another reroll). :)

7.x should require much less rerolling and anguish... anyone up for a backport?

dcam’s picture

Oh, thank goodness... I wasn't looking forward to the possibility of rerolling it yet again for D8. I'll backport it later unless someone beats me to it.

dcam’s picture

Status:Patch (to be ported)» Needs review
StatusFileSize
new51.5 KB
PASSED: [[SimpleTest]]: [MySQL] 40,356 pass(es).
[ View ]

Backport of #50.

lazysoundsystem’s picture

+++ b/modules/locale/locale.test
@@ -1492,44 +1492,44 @@ class LocaleUninstallFunctionalTest extends DrupalWebTestCase {
-    $this->assertTrue($language_negotiation, t('Interface language negotiation: %setting', array('%setting' => t($language_negotiation ? 'none' : 'set'))));
+    $this->assertTrue($language_negotiation, format_string('Interface language negotiation: %setting', array('%setting' => t($language_negotiation ? 'none' : 'set'))));
     $language_negotiation = language_negotiation_get(LANGUAGE_TYPE_CONTENT) == LANGUAGE_NEGOTIATION_DEFAULT;
-    $this->assertTrue($language_negotiation, t('Content language negotiation: %setting', array('%setting' => t($language_negotiation ? 'none' : 'set'))));
+    $this->assertTrue($language_negotiation, format_string('Content language negotiation: %setting', array('%setting' => t($language_negotiation ? 'none' : 'set'))));
     $language_negotiation = language_negotiation_get(LANGUAGE_TYPE_URL) == LANGUAGE_NEGOTIATION_DEFAULT;
-    $this->assertTrue($language_negotiation, t('URL language negotiation: %setting', array('%setting' => t($language_negotiation ? 'none' : 'set'))));
+    $this->assertTrue($language_negotiation, format_string('URL language negotiation: %setting', array('%setting' => t($language_negotiation ? 'none' : 'set'))));

This all looks good except for an inconsistency with t()s inside 'format_string()' calls. All have been removed, except these three.

dcam’s picture

StatusFileSize
new51.5 KB
PASSED: [[SimpleTest]]: [MySQL] 40,339 pass(es).
[ View ]
new1.88 KB

Here's #54 + the changes from #55.

lazysoundsystem’s picture

Status:Needs review» Reviewed & tested by the community

Thanks, RTBC.

David_Rothstein’s picture

Status:Reviewed & tested by the community» Fixed
-    $this->drupalPost(NULL, $edit, t('Save translations'));
...
+    $this->drupalPost(NULL, $edit, 'Save translations');

I'm not sure that one is right, but there's no way I'm holding up what is apparently the last one of these "remove t()" issues over that!!! (I think it doesn't matter much in practice, and we could always have a followup to add it back.)

Therefore, committed to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/e516351

If this is really the last one of these issues, someone should throw a very large party. Great work everyone.

hass’s picture

This 'Save translations' looks wrong to me as it fails in other site languages.

jhodgdon’s picture

Status:Fixed» Needs work

Yes, that one was wrong. (#58). Follow-up please?

dcam’s picture

Status:Needs work» Needs review
StatusFileSize
new813 bytes
PASSED: [[SimpleTest]]: [MySQL] 40,473 pass(es).
[ View ]

Here's the follow-up.

David_Rothstein’s picture

Status:Needs review» Reviewed & tested by the community

Looks good - thanks!

FYI, the reason I don't think this matters much in practice is that this test (like almost all tests) runs in English. It doesn't matter if the parent site is in another language; this code runs against the site that is being tested so it should pass regardless.

But it still makes sense to fix this anyway, for consistency etc.

@jhodgdon, you can certainly feel free to commit this whenever you want (including right before the 7.23 release - this patch is definitely not subject to "code freeze").

jhodgdon’s picture

Status:Reviewed & tested by the community» Fixed

Thanks! Follow-up committed to 7.x

And, this was the *****LAST ONE*****

(cheers!)

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

Anonymous’s picture

Issue summary:View changes

Removing myself from the author field to unfollow the issue. --xjm