Regression: No interface translation in the first installer steps

regression means critical
thanks for opening this Pancho

this should fix it...my bad, tags are useless in first steps of installer:/

Bring on D8MI sprint, add ui tag.

That did it :)

Is it possible for a test to be written for this? I think some things have been committed to HEAD in the last month that make testing the installer possible, at least in theory, but maybe not fully in practice? If it's not possible, then a comment explaining why and linking to what issue is needed to make it possible would help. Thanks.

i cant make it work...i can make a test fail because of it, but i cant make it pass, the semaphore table blows up, any help appreciated

I have partially fixed the problem with the test in #7, the semaphore exception issue doesn't happen anymore, but there is still a @todo in the code - I'm temporarily using hardcoded translated string for "Save and continue" submit button, because the t() is not available early in the installation. This is not very nice because the translation can change and if that happens the test would fail.

I have discussed this with ParisLiakos, we will try to fix this tomorrow.

The attached patch now includes both fix and the test. It's not 100% ready yet, but let's see if it passes.

Let's test this.

#10: drupal-installation_translation-2022549-10.patch queued for re-testing.

Fixed the issue mentioned in #10 - now we use xpath to get the submit button's value. Please review.

There is also another change in the test compared to the parent InstallerTest class. The installer form submission test now follows the user experience - in first step the language is selected and submitted, then profile is selected on the next page, which already uses the selected translation.

Diffs attached:
interdiff-2022549-7-14.txt - git diff between applied #7 and #14
diff-InstallerTranslationTest-InstallerTest.txt - diff between new test InstallerTranslationTest and its parent class InstallerTest.

+++ core/modules/system/lib/Drupal/system/Tests/Installer/InstallerTranslationTest.php	2013-06-26 16:20:02.806807053 +0200
@@ -2,23 +2,23 @@
+ * Tests installer translation detection.
...
+      'name' => 'Installer translation tests',
+      'description' => "Tests the installation UI's translations.",

This seems not really to describe what the test is doing.

The test is just checking that on the page right after selecting a language, that the second page is not in english (kind of like, that it's in the language picked)

+++ core/modules/system/lib/Drupal/system/Tests/Installer/InstallerTranslationTest.php	2013-06-26 16:20:02.806807053 +0200
@@ -78,8 +82,24 @@
+    // On the following page where installation profile is being selected,
+    // the interface should be already translated.
+    $this->assertNoText('Set up database', '"Set up database" string successfully translated.');
+    $this->assertNoText('Save and continue', '"Save and continue" string successfully translated.');
+
+    // Get the "Save and continue" submit button translated value from
+    // the translated interface.

A.

The assert is actually just checking that those English phrases are not there.

So if another change, changes the phrasing of those english phrases, this test will fail.

It's ok for now I think. But maybe we should point out that we are not actually checking if it's translated to German, we are checking that it's not in english.

B. Why are we asserting two things? I think checking that one thing is not in English is enough to show the page was translated.

C. Why do we continue with the tests after the assertion?

We talked about this, and because if we take out the rest of the test, there is an exception. I think the tests expect the installation to succeed?

D. Since we are continuing, I suggest adding a comment to explain why we go on clicking continue even after we know the the pages are not in English.

Implemented suggested changes.

I just noticed one german was lowercase and one German uppercase.
This fixes that.
I also changed a contraction to be the two words so it is clearer. (I think we try and do that.)

This has tests, removing the needs tests tag.

I have reviewed the patch, not tested it. Only one comment:

+++ b/core/modules/system/lib/Drupal/system/Tests/Installer/InstallerTranslationTest.phpundefined
@@ -0,0 +1,137 @@
+    $variable_groups = array(
+      'system.file' => array(
+        'path.private' =>  $this->private_files_directory,
+        'path.temporary' => $this->temp_files_directory,
+      ),
+      'locale.settings' => array(
+        'translation.use_source' => 'local',
+        'translation.path' => $this->translation_files_directory,
+      ),
+    );

Using 'translations.use_source' is not required here. The installer uses the local file, and otherwise will try to download the remote. This setting is not used by the installer.

I have removed 'translations.use_source' as per Sutharsan's remark.

There was also a bug in the test which just appeared, because of which the "drupal-installation_translation-2022549-19-justtests-shouldfail.patch" passed but was supposed to fail. The "Save and continue" text is the text value of the submit button and this was obviously not properly asserted using the assertNoText() method. I have replaced back with the "Set up database" string as in the original patch.

I have fixed and updated both, patch test with fix and patch test without fix.

This works, makes sense, and the test bot agrees and the code looks good.

well lets get this in, i dont think we can have a better test than that

Committed 2a993a8 and pushed to 8.x. Thanks!

Yay!