Fix string translation in MigrateUpgradeRunBatch

Patch seems good and applies correctly. Is it there any way to test it manually?

@rteijeiro: To test manually, in a fresh Drupal 8 install go to /upgrade, provide the credentials of a Drupal 6 or 7 database, and execute the upgrade process. Watch the batch messages, and also review the log messages after the process, to verify they are properly formatted (paying particular perception to the plurals).

Going to do that myself right now...

So, a long time problem (thought there had been an existing issue in the migrate_upgrade queue, but not now finding it there or in the core queue) is sometimes "1" being rendered as plural:

I attempted to debug this several months ago and couldn't figure it out. This time I did notice something even weirder - you see those top few lines that say "1 item total"? After the migration moves on to the next thing, those same migrations show "1 items total". Finally figured it out after all that time, we need to use the magic @count placeholder instead of an explicit placeholder for the count (I'm not *exactly* sure why this fails). But... oh... now I'm getting stuff like "Upgraded URL aliases (processed items total)". So, presumably the problem here is the list of messages we're accumulating are instances of the PluralTranslatableMarkup class, which are saved to the sandbox, and when retrieved for display in a separate batch request the count has evaporated (is the sandbox not serialized?). The upshot as I see it is that we need to make sure the string is instantiated (i.e., PluralTranslatableMarkup objects cast to string) when saving it to the sandbox.

The attached patch works properly for me.

Are there tests that we can update to ensure the right message gets displayed with the proper proper pluralization?

I am personally not attached to needing tests, but if people believe so then we need to figure out a way.

I re-rolled the last patch attached to the current version of 8.2.x . It didn't apply because of the else cases of :

if ($results['operation'] == 'import') {
....

are not at 8.2.x

I run the test locally and it seems is passing. Let's try again.

this issue is still tagged as "beta target" although 8.1 is already released
which is strange since we are already in 8.2.x beta phase
according to https://www.drupal.org/core/release-cycle-overview#minor

I was looking at the patch and it's completely removing the getTranslation() method
which would not necessarily be a problem since this is an experimental module, thus it has no BC promise, according to https://www.drupal.org/core/d8-allowed-changes#experimental and https://www.drupal.org/core/experimental

my questions are:
1. is this something that should stay 8.1.x without the "beta target" tag
or it should be moved to move it to 8.2.x and keep the "beta target"?
2. after figuring out the exact version, what is exactly the task that remains to solve this issue?

Removing old "beta target" tag.

I'm perfectly comfortable removing the protected getTranslation() method - this is not a class that's likely to be overridden.

I see no harm in committing this to 8.1.x, but given there's only one 8.1 patch release to go I'm fine with it going into 8.2.x (and 8.3.x) only.

As far as I can see, there's no task left here other than someone who did not contribute to the patch (as I did) reviewing and testing so it can move to RTBC.

YesCT brought up the subject of tests in IRC. It should be noted that this issue is mainly about properly using the translation API - the only effect here on the actual rendered output is fixing the plural problem ("1 items total"), which is only visible when displaying strings from a previous batch request in a subsequent batch request. Unless someone has some idea how to test the output from a following batch request, I think we can go in without adding tests here.

based on my review with YesCT @Drupalaton and with mikeryan1 on IRC I'm updating the issue summary with details on what's affected

Patch seems good and applies correctly to all active 8.x branches.

thx, patch applies and works as expected. But I don't know enough about translations to do a review.

TranslatableMarkup (http://cgit.drupalcode.org/potx/tree/potx.inc#n274) and PluralTranslatableMarkup (http://cgit.drupalcode.org/potx/tree/potx.inc#n324) are found with potx, so this seems to be fine as a "fix". There is not really a difference for translation because formatPlural() as a method on anything is identified the same way and global t() is also still found. So it is a fix for D8 practices basically.

That said, what other classes do is they use the StringTranslationTrait, which provides a translation service and the formatPlural() and t() methods imported to this class.

Also:

+++ b/core/modules/migrate_drupal_ui/src/MigrateUpgradeRunBatch.php
@@ -136,35 +138,35 @@ public static function run($initial_ids, $operation, $config, &$context) {
         case MigrationInterface::RESULT_STOPPED:
-          $context['sandbox']['messages'][] = t('Operation stopped by request');
+          $context['sandbox']['messages'][] = new TranslatableMarkup('Operation stopped by request');
           break;
 
         case MigrationInterface::RESULT_FAILED:
-          $context['sandbox']['messages'][] = t('Operation on @migration failed', ['@migration' => $migration_name]);
+          $context['sandbox']['messages'][] = (string) new TranslatableMarkup('Operation on @migration failed', ['@migration' => $migration_name]);
           $context['results']['failures']++;
           static::logger()->error('Operation on @migration failed', ['@migration' => $migration_name]);
           break;

Why are some direct cast to a string, while others are not?! Do they need to be cast to string at all? I don't expect so, given t() does not return a string directly either.

regarding the cast to string, see mikeryan's comment #6

Ok I see. Let's do it consistently for all message strings in the batch then and document it with a code comment.

If I understood correctly, the next step is using a cast to string when new TranslatableMarkup is used in the batch in a consistent manner, so we force the message to be rendered before the @count variable is not valid anymore in the batch process. I think this is a good task for the Novice tag.

addressing #23

+++ b/core/modules/migrate_drupal_ui/src/MigrateUpgradeRunBatch.php
@@ -138,11 +137,11 @@
-          $context['sandbox']['messages'][] = (string) $message;
+          $context['sandbox']['messages'][] = $message;

@@ -156,17 +155,17 @@
-          $context['sandbox']['messages'][] = new TranslatableMarkup('Operation stopped by request');
+          $context['sandbox']['messages'][] = t('Operation stopped by request');
...
-          $context['sandbox']['messages'][] = (string) new TranslatableMarkup('Operation on @migration failed', ['@migration' => $migration_name]);
+          $context['sandbox']['messages'][] = t('Operation on @migration failed', ['@migration' => $migration_name]);
...
-          $context['sandbox']['messages'][] = (string) new TranslatableMarkup('Operation on @migration skipped due to unfulfilled dependencies', ['@migration' => $migration_name]);
+          $context['sandbox']['messages'][] = t('Operation on @migration skipped due to unfulfilled dependencies', ['@migration' => $migration_name]);

@@ -183,7 +182,7 @@
-        $context['sandbox']['messages'][] = (string) $message;
+        $context['sandbox']['messages'][] = $message;

@@ -204,7 +203,7 @@
-          $context['message'] = new TranslatableMarkup('Currently upgrading @migration (@current of @max total tasks)', [
+          $context['message'] = t('Currently upgrading @migration (@current of @max total tasks)', [

@@ -250,28 +249,28 @@
-        drupal_set_message(new TranslatableMarkup('Congratulations, you upgraded Drupal!'));
+        drupal_set_message(t('Congratulations, you upgraded Drupal!'));
...
-      drupal_set_message(Link::fromTextAndUrl(new TranslatableMarkup('Review the detailed upgrade log'), $url), $failures ? 'error' : 'status');
+      drupal_set_message(Link::fromTextAndUrl(t('Review the detailed upgrade log'), $url), $failures ? 'error' : 'status');

@@ -335,7 +334,7 @@
-    $message = new TranslatableMarkup('Source ID @source_id: @message', ['@source_id' => $source_id_string, '@message' => $event->getMessage()]);
+    $message = t('Source ID @source_id: @message', ['@source_id' => $source_id_string, '@message' => $event->getMessage()]);

Why are you undoing all the global t() removal? Did someone indicate this direction will be better? What I said above is instead of using the classes directly, other implementations just use the StringTranslationTrait and so they have local methods to invoke for these things, which would be the standard Drupal approach.

So today I learned what string cast is from @penyaskito :) This patch is based on #18.
I added the string cast to the translated strings that were missing it. I added a comment explaining why it's needed, based on #6.

Thanks @zsofi.major for the string casts yay! The question remains: why are we not using StringTranslationTrait here to make the code more unified with the rest of core? @penyaskito any reasons? :)

@alexpott's first patch included that change, as potx extractor couldn't find those strings.
All methods of the class are static, so IMHO last patch it's the best we can do until we evolve the batch API.

Maybe the situation around the potx extraction changed? I don't think so.

All right, since all methods on this class are static in service of the batch API, it makes sense that StringTranslationTrait cannot be used because that is not static. So looked at the message again and there are some things I spotted:

1. +++ b/core/modules/migrate_drupal_ui/src/MigrateUpgradeRunBatch.php
   @@ -137,35 +139,35 @@ public static function run($initial_ids, $operation, $config, &$context) {
   -          $context['sandbox']['messages'][] = t('Operation stopped by request');
   +          $context['sandbox']['messages'][] = new TranslatableMarkup('Operation stopped by request');
   

   This one is not yet cast to a string.

2. +++ b/core/modules/migrate_drupal_ui/src/MigrateUpgradeRunBatch.php
   @@ -203,7 +205,7 @@ public static function run($initial_ids, $operation, $config, &$context) {
   -          $context['message'] = t('Currently upgrading @migration (@current of @max total tasks)', [
   +          $context['message'] = new TranslatableMarkup('Currently upgrading @migration (@current of @max total tasks)', [
   

   Should this cast as well?

Addressing feedback in #30

Thanks Juan!

+++ b/core/modules/migrate_drupal_ui/src/MigrateUpgradeRunBatch.php
index 2635aaa..43ac545 100644
--- a/core/modules/taxonomy/src/Plugin/migrate/source/d6/TermNode.php

--- a/core/modules/taxonomy/src/Plugin/migrate/source/d6/TermNode.php
+++ b/core/modules/taxonomy/src/Plugin/migrate/source/d6/TermNode.php

+++ b/core/modules/taxonomy/src/Plugin/migrate/source/d6/TermNode.php
index 26f069c..57fcc2d 100644
--- a/core/modules/taxonomy/tests/src/Kernel/Migrate/d6/MigrateTermNodeTest.php

--- a/core/modules/taxonomy/tests/src/Kernel/Migrate/d6/MigrateTermNodeTest.php
+++ b/core/modules/taxonomy/tests/src/Kernel/Migrate/d6/MigrateTermNodeTest.php

These are unrelated changes. We need to remove those from the patch.

@juancasantito: your patch contains changes other than the ones posted in the interdiff, namely two new files with unrelated changes in term node migration. Removing those, the patch should be green. (Not making any other changes here, just removing the unrelated changes).

Looks all good now. I only removed the unrelated changes that somehow ended up in the patch in #31, so should still be eligible to RTBC.

Sorry about the errors these are my first patches and interdiff . And thanks for the tips and support Gabor and Penyaskito. Improving in process.

Let's not hide the current patch :) @juancasantito I posted and even committed my fair share of bad patches, including ones with parse errors, so no reason to feel bad :)

Removing note from summary about 8.1.x since that is not updated anymore (apart from a last security fix).

Removing from sprint, thanks all again!

Huh, removing accidental typed tag.