Fix confusing UX by merging "Translate configuration" into "Finish translations" task

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: still working on this? I think would be a great improvement!

Yay, thanks!

Needs reroll

Re-roll.

Does not pass anymore unfortunately :/

fix

Patch #9 rerolled.

12: install_merge_translate_tasks-1993452-12.patch queued for re-testing.

Re-rolled patch from #12 still applies successfully.

12: install_merge_translate_tasks-1993452-12.patch queued for re-testing.

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:

This is horror indeed. Needs reroll too.

I will try to reroll this patch

Yay #drupalhugs :)

Attached a new patch rerolled from the latest one.

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

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

applies, so removing the needs reroll tag.

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.

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)

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).

These are minor things, can someone help with them?

This is theoretically that test.

PHP errors.

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?

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 :)

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

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?

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

"General error: 2006 MySQL server has gone away"

Assuming no random fail now.

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

Updated screenshot.

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.

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.

Testbot failed because:

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

Sending for retest.

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.

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.

@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.

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

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.

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

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.

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

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

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.

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