Problem/Motivation

Drupal 8 installer in a non-English language exposes too much translation complexity without really adding value to the end-user. This gives the translation tasks too much weight and adds to a picture that interface translation was something incredibly complex and heavyweight with Drupal. While to some extent this might be true in code, it shouldn't be in the UI.

Proposed resolution

Merge "Translate Configuration" and "Finish Translations" steps.

Remaining tasks

Review, Commit.

User interface changes

One step less in installer in non-English installations.

Before

After

After merge image

API changes

None.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Prioritized changes The main goal of this issue is usability in the installer to make it less intimidating.
Disruption Non disruptive for core/contributed and custom modules/themes. Contributed install profiles may need to change but only if they add tasks relative to the one removed or provide a replacement for the ones modified. We are not aware of such install profiles in existence ATM. There is also an API addition to support a list of batches instead of just one batch in installer tasks to support this change.
Files: 
CommentFileSizeAuthor
#55 interdiff.txt565 bytesGábor Hojtsy
#55 install-merge-translate-tasks-1993452-55.patch8.75 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,382 pass(es). View
#53 interdiff.txt3.54 KBGábor Hojtsy
#53 install-merge-translate-tasks-1993452-53.patch8.67 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
#45 after-merge-Spanish.jpg203.88 KBbannorb
#45 after-merge.jpg218.32 KBbannorb
#44 Telepítési profil kiválasztása | Drupal 2014-12-17 19-31-32.png148.9 KBGábor Hojtsy
#31 interdiff.txt951 bytesGábor Hojtsy
#31 install-merge-translate-tasks-1993452-31.patch9.31 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,373 pass(es). View
#29 interdiff.txt5.06 KBGábor Hojtsy
#29 install-merge-translate-tasks-1993452-29.patch9.3 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/modules/system/src/Tests/Installer/InstallerTranslationMultipleLanguageTest.php. View
#25 interdiff.1993452.23.25.txt1.04 KBYesCT
#25 install-merge-translate-tasks-1993452-25.patch6.21 KBYesCT
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,477 pass(es). View
#23 install-merge-translate-tasks-1993452-23.patch6.16 KBrvilar
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,874 pass(es). View
#21 install_merge_translate_tasks-1993452-21.patch1.28 KBrvilar
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch install_merge_translate_tasks-1993452-21.patch. Unable to apply patch. See the log in the details link for more information. View
#17 Installing Drupal | Drupal 2014-09-05 16-37-40 2014-09-05 17-17-31.png218.04 KBGábor Hojtsy
#12 install_merge_translate_tasks-1993452-12.patch6.03 KBSutharsan
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,285 pass(es). View
#9 install_merge_translate_tasks-1993452-9.patch6.04 KBAlbert Volkman
FAILED: [[SimpleTest]]: [MySQL] 58,326 pass(es), 1 fail(s), and 0 exception(s). View
#2 install_merge_translate_tasks-1993452-2.patch6.32 KBPancho
PASSED: [[SimpleTest]]: [MySQL] 55,374 pass(es). View

Comments

Gábor Hojtsy’s picture

Issue tags: +installer, +D8MI, +language-base

I think at least putting them into a consecutive batch for a start would be great. I don't think reworking the batch API at this point in time is useful.

Pancho’s picture

Status: Active » Needs review
FileSize
6.32 KB
PASSED: [[SimpleTest]]: [MySQL] 55,374 pass(es). View

Okay, so here's the patch.
Quickly tested and seemed to work, but don't know if it's ready for review.

Pancho’s picture

Assigned: Unassigned » Pancho
Status: Needs review » Needs work

Read all documentation and found out that the Batch API should support most everything we want. So we can do better, even if the test says green.

Gábor Hojtsy’s picture

@Pancho: still working on this? I think would be a great improvement!

Pancho’s picture

Thanks for the reminder, @Gábor.
Yes, I think I have a 95% working patch. I'm gonna take another look at it tonight.

Gábor Hojtsy’s picture

Yay, thanks!

Pancho’s picture

It unfortunately wasn't 95% working, and digging through the quite messy code of the locale subsystem is a bit cumbersome, but I'm gonna finish it ASAP.

jair’s picture

Issue tags: +Needs reroll

Needs reroll

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
6.04 KB
FAILED: [[SimpleTest]]: [MySQL] 58,326 pass(es), 1 fail(s), and 0 exception(s). View

Re-roll.

Status: Needs review » Needs work

The last submitted patch, install_merge_translate_tasks-1993452-9.patch, failed testing.

Gábor Hojtsy’s picture

Does not pass anymore unfortunately :/

Gábor Hojtsy’s picture

Issue summary: View changes

fix

Sutharsan’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
6.03 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,285 pass(es). View

Patch #9 rerolled.

Status: Needs review » Needs work

The last submitted patch, 12: install_merge_translate_tasks-1993452-12.patch, failed testing.

Sutharsan’s picture

Status: Needs work » Needs review
Alumei’s picture

Issue tags: -Needs reroll

Re-rolled patch from #12 still applies successfully.

Alumei’s picture

Gábor Hojtsy’s picture

So I wanted to post an even more horror screenshot but that turns out it is actually due to more steps added in the multilingual demo profile :D This would have been it:

Sutharsan’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

This is horror indeed. Needs reroll too.

basvanderveen’s picture

Issue tags: +Amsterdam2014

I will try to reroll this patch

Gábor Hojtsy’s picture

Yay #drupalhugs :)

rvilar’s picture

Assigned: Pancho » rvilar
Status: Needs work » Needs review
FileSize
1.28 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch install_merge_translate_tasks-1993452-21.patch. Unable to apply patch. See the log in the details link for more information. View

Attached a new patch rerolled from the latest one.

Installation tested and completed, but the steps are not merged.

Status: Needs review » Needs work

The last submitted patch, 21: install_merge_translate_tasks-1993452-21.patch, failed testing.

rvilar’s picture

Status: Needs work » Needs review
FileSize
6.16 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,874 pass(es). View

Sorry, I've attached the wrong patch. Now I attach the correct one.

YesCT’s picture

Issue tags: -Needs reroll

applies, so removing the needs reroll tag.

YesCT’s picture

Assigned: rvilar » Unassigned
FileSize
6.21 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,477 pass(es). View
1.04 KB

I read through this and it makes sense.

I noticed two nits: some trailing whitespace and a comment that got changed and was over 80 chars. just fixed those two things.

Did not yet try this manually.

bwinett’s picture

I installed Drupal on simplytest.me both without the patch and with the patch. I selected Spanish as the language.

Before the patch, the steps were (I include translations in parentheses):

  1. Elegir un idioma (Select a language)
  2. Elegir perfil (Choose profile)
  3. Verificar requisitos (Check requirements)
  4. Configurar base de datos (Configure database)
  5. Instalar sitio (Install site) - note: after this step was done, the installation went directly into the next step, without intervention
  6. Configurar traducciones (Configure translations)
  7. Configurar sitio (Configure site) - when I clicked on the button here, it seemed to go directly into processing the final step (maybe the following step completed so quickly that I didn't see it?)
  8. Terminar traducciones (Complete translations)
  9. Traducir configuración (Translate configuration) - note that this is the only step that was not displayed after the patch

After the patch, the steps were (I include translations in parentheses):

  1. Elegir un idioma (Select a language)
  2. Elegir perfil (Choose profile)
  3. Verificar requisitos (Check requirements)
  4. Configurar base de datos (Configure database)
  5. Instalar sitio (Install site) - note: after this step was done, the installation went directly into the next step, without intervention
  6. Configurar traducciones (Configure translations)
  7. Configurar sitio (Configure site)
  8. Terminar traducciones (Complete translations)
Gábor Hojtsy’s picture

Status: Needs review » Needs work
Issue tags: +sprint, +Needs tests

Nice and simple solution that it changes the batch return API slightly to allow for multiple batches. This may have been useful at other translation tasks before, but oh well :) Good now too. Except:

  1. +++ b/core/includes/install.core.inc
    @@ -1497,7 +1493,7 @@ function install_bootstrap_full() {
    + *   An array containing the batch definition.
    

    Batch definition*s*, no? (This seems to use good grammar at other instances).

  2. +++ b/core/includes/install.core.inc
    @@ -1720,30 +1720,19 @@ function install_import_translations_remaining(&$install_state) {
    -    if ($batch = locale_translation_batch_update_build(array(), array_keys($languages), $options)) {
    ...
    +    if ($batch = locale_translation_batch_update_build(array(), array($install_state['parameters']['langcode']), $options)) {
    

    Let's not loose this advantage from before that we processed all languages not just the active language used for translation.

  3. +++ b/core/includes/install.core.inc
    @@ -1720,30 +1720,19 @@ function install_import_translations_remaining(&$install_state) {
    -  $languages = \Drupal::languageManager()->getLanguages();
    -  return locale_config_batch_update_components(array(), array_keys($languages));
    +  $batches[] = locale_config_batch_update_components(array(), array($install_state['parameters']['langcode']));
    

    Same here. Keep processing all languages not just the install state language. The reason this is not failing is because the test coverage does not have a config string translataed. See InstallerTranslationMultipleLanguageTest and InstallerTranslationMultipleLanguageForeignTest. If the .po file includes a config value translation as well (from something the system module installs, eg. name of anonymous user), then that would fail on this change. That would actually test this step well too. Adding needs tests tag for this. Should be easy by adding a couple lines to those two tests (to the sample .po file and the assertion to assert with the config API).

Gábor Hojtsy’s picture

These are minor things, can someone help with them?

Gábor Hojtsy’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
9.3 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/modules/system/src/Tests/Installer/InstallerTranslationMultipleLanguageTest.php. View
5.06 KB

This is theoretically that test.

Status: Needs review » Needs work

The last submitted patch, 29: install-merge-translate-tasks-1993452-29.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
9.31 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,373 pass(es). View
951 bytes

PHP errors.

penyaskito’s picture

In terms of code and accuracy to the issue summary, this looks good to me. Tested it and works fine.
However, the installations steps still look confusing:

  • Configurar traducciones (Configure translations)
  • Configurar sitio (Configure site)
  • Terminar traducciones (Finish translations)

Finish translations? Maybe we can find a better copy?

Gábor Hojtsy’s picture

Is that translation configuration step the very first one? I mean there is only a configuration step for languages :)

I agree the 'Finish translations' step could be named better but that is an existing name with or without this patch. All this patch does is it folds one more step into the finish step :)

penyaskito’s picture

Status: Needs review » Reviewed & tested by the community

Not the first one, those were the three last ones.
Yep, the naming is probably another issue on its own. Let's move forward.

penyaskito’s picture

Issue summary: View changes
penyaskito’s picture

Issue summary: View changes
vijaycs85’s picture

Looks good. +1 to RTBC

+++ b/core/includes/install.core.inc
@@ -1763,41 +1761,32 @@ function _install_prepare_import($langcodes, $server_pattern) {
-function install_update_configuration_translations(&$install_state) {

Hope we don't need a CR as it is not 7.x?

Gábor Hojtsy’s picture

Title: Merge "Translate configuration" into "Finish translations" task » Fix confusing UX by merging "Translate configuration" into "Finish translations" task
Category: Task » Bug report

@vijaycs85: No, its a bugfix modifying only internal functions. These are not an API.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 31: install-merge-translate-tasks-1993452-31.patch, failed testing.

Gábor Hojtsy’s picture

"General error: 2006 MySQL server has gone away"

Gábor Hojtsy’s picture

Status: Needs work » Reviewed & tested by the community

Assuming no random fail now.

YesCT’s picture

Issue summary: View changes
Issue tags: +Usability, +Needs screenshots

a recent before and after in the summary would be nice.

Gábor Hojtsy’s picture

Updated screenshot.

bannorb’s picture

Issue summary: View changes
FileSize
218.32 KB
203.88 KB

I tested this through the simplytest.me in Hungarian (Magyar). I noticed the translation for numbers 6 and 8 are the same. I tested in Spanish and matched the results of @bwinett in comment #26.

After merge in Spanish

This seems to fine to me. I don't think it's a problem that the Hungarian translates into the same words for numbers 6 and 8.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 31: install-merge-translate-tasks-1993452-31.patch, failed testing.

bannorb’s picture

Status: Needs work » Needs review

Testbot failed because:

FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: failed to create checkout database.

Sending for retest.

bannorb’s picture

Status: Needs review » Reviewed & tested by the community

This was already rtbc in comment #34, something happened with the testbot, retested and the test came back pass/green. I'm marking it rtbc again.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update

Looks like hook_install_tasks() documentation needs to be updated. But I'm not sure that the solution is correct - it seems odd to return an array of batches. Is there no way of combining install_import_translations_remaining and install_update_configuration_translations in a way that does not result in an install API change?

Non disruptive for core/contributed and custom modules/themes.

From the beta evaluation is not correct. Any install profile that adds a batch step would need to change.

Gábor Hojtsy’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

@alexpott: it is an install API change either way because what the returned steps do would change. Also the list of steps would change, so those adding steps inbetween them would need to be aware as well. We tried above to just make the last language step hidden, but then no progress information is displayed on the tasks so this is not useful for long running things.

The issue summary mentioned modules and themes. They cannot add batch steps in the installer. It is true the issue summary did not mention install profiles, and they would need to adapt.

Updated the issue summary. I think the code will be considerably uglier if we are to flatten the batch but will look into that one.

Gábor Hojtsy’s picture

Sorry I had an outdated memory of the API changes proposed in this patch. Duh. Looking into flattening that batch then.

Gábor Hojtsy’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs issue summary update
FileSize
8.67 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
3.54 KB

Ok, so looking at the batches to flatten them out, the problem is manyfold. Both batches have their unique callbacks, temporary data and finished callbacks. We could write a post-processor to process those batch arrays into one and create a finished callback that calls back other finished callbacks but not sure implementing that for the protection of the installer API is sane? Also, we can modify the locale batches themselves, but then an installer UX fix spills into locale API changes (IMHO worse than installer API changes).

Let's shoot for allowing arrays of batches in the installer then, which is an API addition for the installer API. As mentioned before we cannot avoid removing an installer step or still returning the same thing from this step that we modify.

Also updated the summary again, should now be correct with the current patch.

Status: Needs review » Needs work

The last submitted patch, 53: install-merge-translate-tasks-1993452-53.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
8.75 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,382 pass(es). View
565 bytes

Of course the batch is an array, we need to detect a batch vs. a list of batches instead.

penyaskito’s picture

Patch works correctly, and reviewed the code and didn't find anything to complain about.
I'm wondering if we need a change record for the API addition.

Gábor Hojtsy’s picture

Added a mini change notice for the mini API addition. Not sure it is a good idea to document API additions on this small scale, but it will not hurt to have it ready just in case :) https://www.drupal.org/node/2401825

penyaskito’s picture

Status: Needs review » Reviewed & tested by the community

RTBC then. Leaving the publication or not of the change record at committer discretion.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 0410129 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation for to the issue summary.

diff --git a/core/includes/install.core.inc b/core/includes/install.core.inc
index 02a7a47..1ec434d 100644
--- a/core/includes/install.core.inc
+++ b/core/includes/install.core.inc
@@ -562,7 +562,7 @@ function install_run_task($task, &$install_state) {
         // there is nothing more to do here.
         return;
       }
-      // Simulate a one item list of batches if only one batch was provided.
+      // Create a one item list of batches if only one batch was provided.
       if (isset($batches['operations'])) {
         $batches = array($batches);
       }

We are not simulating anything. Fixed on commit.

  • alexpott committed 0410129 on 8.0.x
    Issue #1993452 by Gábor Hojtsy, YesCT, bannorb, rvilar, Pancho,...
Gábor Hojtsy’s picture

Issue tags: -sprint

Superb, thanks a lot! Published the change record at https://www.drupal.org/node/2401825

Status: Fixed » Closed (fixed)

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