Migrate D6 i18n custom blocks (boxes)

1. Changed label.
2. Added simple documentation and example
3. @todo Can you explain what you mean, please?
4. Chained
5. Combined onto one line.
6. Chained
7. Removed lines and added 'language' migration.
8. Tweaked comment.
9. Moved files from content_translation to block_content

Setting back to Needs Work for someone to address #3.3.

Correct the namespace in the moved files.
Address #3.3.

Added a unit test for the i18n_translation process plugin.

I haven't been able to get to the bottom of the remaining test failure - I can't understand why this fails when MigrateCustomBlockContentTranslationTest passes. My only suspicion is that 'language' should be one of the required migration_dependencies for d6_custom_block_translation, but I am unable to test that on my local build.

migration_dependencies:
   required:
     - block_content_type
+    - language
 provider:
   - block_content
   - migrate_drupal

Passed the test in my local, Will give it a try

Oops, Here we go,

Moved the migration dependency as recommended by @maxocub.

@rakesh.gectcr, @Jo Fitzgerald and @maxocub thanks for getting this to RTBC. It was a goal of mine to get this done in Vienna. And it happened without me doing anything. Love the community.

@phenaproxima says he would like to review this.

1. +++ b/core/modules/content_translation/tests/src/Unit/process/i18nTranslationTest.php
   @@ -0,0 +1,72 @@
   +namespace Drupal\Tests\content_translation\Unit\process;
   

   This should be Drupal\Tests\content_translation\Unit\migrate\process

2. +++ b/core/modules/content_translation/tests/src/Unit/process/i18nTranslationTest.php
   @@ -0,0 +1,72 @@
   +  public function testI18nTranslationTest($value, $expected) {
   

   Should have parameter definitions for $value and $expected in the doc block

3. +++ b/core/modules/content_translation/tests/src/Unit/process/i18nTranslationTest.php
   @@ -0,0 +1,72 @@
   +  public function testI18nTranslationInvalidTest($value) {
   

   Ditto.

4. +++ b/core/modules/content_translation/tests/src/Unit/process/i18nTranslationTest.php
   @@ -0,0 +1,72 @@
   \ No newline at end of file
   

   Add a newline.

This needs to be moved back to content_translation. It was split in #105 of #2225681: Migrate D6 i18n blocks translated strings.

The node translation migrations can be in the node module because it uses the same source plugin as non-translated node. But for blocks, a new source plugin is needed because both the block table and the i18n_blocks table are required.

When block is enabled on the destination site this block translation migration will be discovered but the source site may not have translations. Without the existence of all the source tables the migration will generate an error about 'base table or view not found'. And for those with a single language site this will be confusing and just plain wrong. This can be avoided by moving the block translation code to content_translation. That way the block translation migration is only discovered when content_translation is enabled, which is needs to be to translate blocks.

Therefor this should be moved back to content_translations.

Made the changes above, without moving the files to content_translation. Just want to wait to see if someone disagrees.

1. +++ b/core/modules/block_content/migration_templates/d6_custom_block_translation.yml
   @@ -0,0 +1,40 @@
   +  constants:
   +    type: basic
   

   This completely assumes that the destination site has installed Standard. It's a safe enough assumption, but I wonder if we shouldn't resolve this dynamically in a hook_migration_plugins_alter() implementation, and set the constant value to the first available block content type that has a body field. If none are available, the entire migration should be made unavailable. That way, this migration will work with profiles that are not Standard.

2. +++ b/core/modules/block_content/migration_templates/d6_custom_block_translation.yml
   @@ -0,0 +1,40 @@
   +    plugin: i18n_translation
   

   So...this migration is in block_content, but it's relying on a plugin that exists in content_translation? That seems dicey.

3. +++ b/core/modules/block_content/src/Plugin/migrate/source/d6/BoxTranslation.php
   @@ -0,0 +1,94 @@
   +    // Add in the property, which is either title or body.
   +    $query->leftJoin('i18n_strings', 'i18n', 'i18n.objectid = b.bid');
   +    $query->isNotNull('i18n.lid');
   

   We are assuming that the i18n_strings table exists here. We should probably be smarter about that.

   Maybe one option is to override initializeIterator() and return an empty ArrayIterator if these tables are not existing in the source database.

4. +++ b/core/modules/block_content/src/Plugin/migrate/source/d6/BoxTranslation.php
   @@ -0,0 +1,94 @@
   +    $query->leftJoin('locales_target', 'lt', 'lt.lid = i18n.lid');
   

   I'm not sure we should assume that locales_target exists.

5. +++ b/core/modules/block_content/src/Plugin/migrate/source/d6/BoxTranslation.php
   @@ -0,0 +1,94 @@
   +    if (empty($this->idMap->lookupDestinationIds(['bid' => $bid, 'language' => $language]))) {
   +      $row->setSourceProperty('migrate', TRUE);
   +    }
   

   What's happening here? Can it be documented in a comment?

6. +++ b/core/modules/block_content/src/Plugin/migrate/source/d6/BoxTranslation.php
   @@ -0,0 +1,94 @@
   +    $row->setSourceProperty('body_untranslated', Unicode::truncate($row->getSourceProperty('options/attributes/title'), 255));
   

   Er...why is the body being derived from options/attributes/title?

7. +++ b/core/modules/block_content/src/Plugin/migrate/source/d6/BoxTranslation.php
   @@ -0,0 +1,94 @@
   +    return [
   +      'bid' => $this->t('The block numeric identifier.'),
   +      'language' => $this->t('Language for this field.'),
   +      'title' => $this->t('Block title translation.'),
   +      'body' => $this->t('Block body translation.'),
   +    ];
   

   There are several interesting properties being defined in prepareRow() that we should document here...

8. +++ b/core/modules/block_content/tests/src/Kernel/Plugin/migrate/source/d6/BoxTranslationTest.php
   @@ -0,0 +1,134 @@
   +    // The expected results.
   +    $tests[0]['expected_results'] = [
   

   Nit: No need for "The expected results" comment, since the array key makes it pretty obvious :)

9. +++ b/core/modules/content_translation/src/Plugin/migrate/process/d6/i18nTranslation.php
   @@ -0,0 +1,48 @@
   +    if ($translated) {
   +      return $translated;
   +    }
   +    else {
   +      return $untranslated;
   +    }
   

   Nit: We could condense this to return $translated ?: $untranslated

10. +++ b/core/modules/content_translation/tests/src/Unit/migrate/process/i18nTranslationTest.php
    @@ -0,0 +1,81 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  protected function setUp() {
    +    parent::setUp();
    +  }
    

    We can remove this.

11. +++ b/core/modules/content_translation/tests/src/Unit/migrate/process/i18nTranslationTest.php
    @@ -0,0 +1,81 @@
    +   * @expectedException \Drupal\migrate\MigrateException
    +   * @expectedExceptionMessage You need to specify both the translated and untranslated values on the i18n_translation plugin.
    

    Apparently we no longer use annotations for these; we need to call $this->setExpectedException() in the test method.

26.2. See my comment in #24 explaining why these files should be moved back to content_translation.

This patch only moves the files to content_translation.

This patch only moves the files to content_translation.

Er...no patch is attached to #28?

Here is the patch, mysteriously missing, from #28.

Oh, and even with the wrong comment number. Did I not get enough sleep?

5. Removed this, looks like leftover from a different approach.
7. Improved.
8. Fixed
9. Sure can, fixed.
10. Fixed
11. Fixed
And added some documentation to i18nTranslation.php process plugin.

Still to do: 1, 3, 4, and 6.

1. Actually, this isn't needed. This migration is only adding translated strings of certain fields. The block itself exists and is not being changed.
3 and 4. If InitializeIterator returns an empty interator then nothing is migrated and the user has no idea why. Because of that I tried something different. The source plugin checks if the tables exist, and if not, throws an exception.
6. Where did that come from? Fixed to get the body from, well, the body.

Also, while working on this I stepped through the source plugin and found a bug. This line, $property2 = ($property == 'title') ? 'body' : 'title'; assumes that the property will always be title or body. But there was a row where the property was 'name' and the translation was a taxonomy string. Because of that the query has a new condition, that 'type'='block'.

I think that finishes all the items in #26. Now, lets see what the testbot finds.

Check the migrateMessages rather than expecting an exception (inspired by Drupal\Tests\migrate_drupal\Kernel\dependencies\MigrateDependenciesTest).

I found some relatively minor things, but overall this looks good. Looooovely test coverage!

1. +++ b/core/modules/content_translation/migrations/d6_custom_block_translation.yml
   @@ -0,0 +1,39 @@
   +  constants:
   +    type: basic
   

   type is no longer used, so we can remove it from here.

2. +++ b/core/modules/content_translation/migrations/d6_custom_block_translation.yml
   @@ -0,0 +1,39 @@
   +  id:
   +    plugin: migration
   +    migration: d6_custom_block
   +    source:
   +      - bid
   

   The plugin should be migration_lookup, not migration, and we should skip the row if it returns null.

3. +++ b/core/modules/content_translation/migrations/d6_custom_block_translation.yml
   @@ -0,0 +1,39 @@
   +  info:
   +    plugin: i18n_translation
   +    source:
   +      - title_untranslated
   +      - title_translated
   

   I don't know if we actually need to change this, but we don't need i18n_translation to be its own plugin. We could do something like this:

   -
     plugin: callback
     source:
       - title_translated
       - title_untranslated
     callable: array_filter
   -
     plugin: callback
     callable: reset
   

   This will filter out the values that don't exist, and return the first one that does. Same thing that i18n_translation does.

   The advantage being that we don't have to write and test yet another process plugin to do simple one-line logic. (This is why I think declarative programming is a mistake in a system like Migrate, but that's another discussion.)

4. +++ b/core/modules/content_translation/migrations/d6_custom_block_translation.yml
   @@ -0,0 +1,39 @@
   +  'body/format':
   +    plugin: migration
   +    migration: d6_filter_format
   +    source: format
   

   We should use the migration_lookup plugin, not migration. And should we skip the row, or set a default value, if it fails?

5. +++ b/core/modules/content_translation/src/Plugin/migrate/source/d6/BoxTranslation.php
   @@ -0,0 +1,107 @@
   +    if (($this->getDatabase()->schema()->tableExists('i18n_strings')) &&
   +      $this->getDatabase()->schema()->tableExists('locales_target')) {
   

   Can we call $this->getDatabase()->schema() once and reuse the return value, just to clean this up and make it more readable?

6. +++ b/core/modules/content_translation/src/Plugin/migrate/source/d6/BoxTranslation.php
   @@ -0,0 +1,107 @@
   +    else {
   +      throw new MigrateException("Missing one or both translation tables, 'i8n_strings' and 'locales_target'");
   +    }
   

   This is more elegant -- I like it.

7. +++ b/core/modules/content_translation/src/Plugin/migrate/source/d6/BoxTranslation.php
   @@ -0,0 +1,107 @@
   +    $property2 = ($property == 'title') ? 'body' : 'title';
   

   I don't like "$property2" as a variable name. Is there any chance we could rename it to something a little more descriptive?

1. Fixed
2. Fixed
3. I'd like to not have that process plugin too. But I didn't see a way to do it in the pipeline. And the suggested code, in the patch, fails. Any other idea?
4. Fixed but didn't add a skip row because since block migration has already fun, I figure, this will already exist.
5. Fixed
6. Yes, I agree.
7. Yes, it is a yucky name. Changed to $translation.

+++ b/core/modules/content_translation/tests/src/Kernel/Plugin/migrate/source/d6/BoxTranslationTest.php
--- /dev/null
+++ b/core/modules/content_translation/tests/src/Unit/migrate/process/i18nTranslationTest.php

Oh, this needs to be removed too.

But still the process pipeline modified as suggest in #35.3, with this error.

1) Drupal\Tests\content_translation\Kernel\Migrate\d6\MigrateCustomBlockContentTranslationTest::testCustomBlockContentTranslation
Parameter 1 to reset() expected to be a reference, value given

Huh, yeah. I can see why that might have failed. How about this, then?

# Filter out the empty values, preferring the translated version if it exists.
-
  plugin: callback
  callable: array_filter
  source:
    - body_translated
    - body_untranslated
# Re-key the array to guarantee that, if it has any values left, index 0 exists.
-
  plugin: callback
  callable: array_values
# Extract index 0.
-
  plugin: extract
  index: [0]

Could we add an optional configuration to the callback process plugin so we can pass variables by reference?

I don't believe so; I don't think call_user_func() supports passing by reference. call_user_func_array() might, but reference-ness is a messy business anyway and I'd prefer to avoid it, as I can imagine it having unintended side effects.

I don't see how this will work when both the translated and untranslated values for a given property exist in the row.

The key is the array_filter.

Given this array: ["the translated value", "the untranslated value"]

If I call array_filter() on that, it will look exactly the same. The translated value will be first. So the extract plugin will pluck that value from the array.

But if there's no translated value: ["", "The untranslated value"]...

Then array_filter() will knock out the first (empty) item, and extracting the first item in the array will give me the untranslated one. Which is what we want, since there was no translated value.

I don't see how it won't work, personally.

Removed the now unused process plugin test. The pipeline now uses array_filter and current.

    -
      plugin: callback
      source:
        - title_translated
        - title_untranslated
      callable: array_filter
    -
      plugin: callback
      callable: current

RE my comment in #42, I completely misread the suggestion in #40, missing the use of array_values. So was confused about the array indices, which I then totally failed to convey in my comment.

+++ b/core/modules/content_translation/src/Plugin/migrate/source/d6/BoxTranslation.php
@@ -0,0 +1,106 @@
+    if ($schema->tableExists('i18n_strings') && $schema->tableExists('locales_target')) {

If this is being added here, there should be a follow up to add this to any existing i18n translation source plugin that uses these tables.

This keeps looking better and better to me. One final round of fixes, and then we can mark this RTBC.

1. +++ b/core/modules/content_translation/src/Plugin/migrate/source/d6/BoxTranslation.php
   @@ -0,0 +1,106 @@
   +class BoxTranslation extends DrupalSqlBase {
   +  /**
   +   * {@inheritdoc}
   +   */
   

   Nit: There should be an empty line after the opening brace of the class.

2. +++ b/core/modules/content_translation/src/Plugin/migrate/source/d6/BoxTranslation.php
   @@ -0,0 +1,106 @@
   +    // Set untranslated values as defaults.
   +    $row->setSourceProperty('title_untranslated', $row->getSourceProperty('info'));
   +    $row->setSourceProperty('body_untranslated', $row->getSourceProperty('body'));
   

   Rather than do this, we should just add the fields in query(): $query->addField('b', 'body', 'body_untranslated');

3. +++ b/core/modules/content_translation/src/Plugin/migrate/source/d6/BoxTranslation.php
   @@ -0,0 +1,106 @@
   +    $query = $this->select('i18n_strings', 'i18n')
   +      ->fields('i18n', ['lid'])
   +      ->condition('i18n.property', $translation)
   +      ->condition('i18n.objectid', $bid);
   +    $query->leftJoin('locales_target', 'lt', 'i18n.lid = lt.lid');
   +    $query->condition('lt.language', $language)
   +      ->addField('lt', 'translation');
   

   This seems potentially quite useful for other i18n migrations. Can we maybe move this into a trait, or new base class extending DrupalSqlBase, as a helper method? This could also fulfill @quietone's request in #45, and spare us a follow-up issue.

4. +++ b/core/modules/content_translation/src/Plugin/migrate/source/d6/BoxTranslation.php
   @@ -0,0 +1,106 @@
   +      'body' => $this->t('Block body.'),
   +      'info' => $this->t('Admin title of the block/box.'),
   

   As noted before, let's get rid of these.

5. +++ b/core/modules/content_translation/tests/src/Kernel/Migrate/d6/MigrateCustomBlockContentTranslationMissingTest.php
   @@ -0,0 +1,47 @@
   +  public static $modules = [
   

   Should be protected.

6. +++ b/core/modules/content_translation/tests/src/Kernel/Migrate/d6/MigrateCustomBlockContentTranslationMissingTest.php
   @@ -0,0 +1,47 @@
   +   * Tests the Drupal 6 i18n custom block strings to Drupal 8 migration when the
   +   * source translation tables are missing.
   

   This violates coding standards. The short description needs to be one line. We can add more detail in subsequent paragraphs.

7. +++ b/core/modules/content_translation/tests/src/Kernel/Migrate/d6/MigrateCustomBlockContentTranslationMissingTest.php
   @@ -0,0 +1,47 @@
   +    $this->assertEquals($this->migrateMessages['error'], [new FormattableMarkup('Migration failed with source plugin exception: @e', ['@e' => "Missing one or both translation tables, 'i8n_strings' and 'locales_target'"])]);
   

   Nit: Can we assert $this->migrateMessages['error'][0], rather than wrap the FormattableMarkup in an array?

8. +++ b/core/modules/content_translation/tests/src/Kernel/Migrate/d6/MigrateCustomBlockContentTranslationTest.php
   @@ -0,0 +1,68 @@
   +  public static $modules = [
   

   Should be protected.

9. +++ b/core/modules/content_translation/tests/src/Kernel/Migrate/d6/MigrateCustomBlockContentTranslationTest.php
   @@ -0,0 +1,68 @@
   +    /** @var BlockContent $block */
   

   Nit: Needs to be the fully qualified class name.

10. +++ b/core/modules/content_translation/tests/src/Kernel/Plugin/migrate/source/d6/BoxTranslationTest.php
    @@ -0,0 +1,133 @@
    +  public static $modules = ['content_translation', 'migrate_drupal'];
    

    Should be protected.

1. Done.
2. Done (and edited the test appropriately).
3. I have not exported the potentially useful code into a trait or base class - perhaps that could be a follow-up?
4. Done.
5. Why should this be protected?
6. Done.
7. Done (and added assertCount() to maintain equivalent tests).
8. See 5.
9. Done.
10. See 5.

3. Moved that to a followup, #2918295: Move i18n query to a trait
5, 8, 10. Was discuss in #2225681: Migrate D6 i18n blocks translated strings, #137->#149. It is being done in a follow up created, #2918296: Change all instances of $modules to protected.

That covers all the todos here.

Full steam ahead.

Just noting that the migrations are in the correct directory, 'migrations'.

+++ b/core/modules/content_translation/tests/src/Kernel/Migrate/d6/MigrateCustomBlockContentTranslationMissingTest.php
@@ -0,0 +1,50 @@
+    ]);
+    $this->assertEquals($this->migrateMessages['error'][0], new FormattableMarkup('Migration failed with source plugin exception: @e', ['@e' => "Missing one or both translation tables, 'i8n_strings' and 'locales_target'"]));
+    $this->assertCount(1, $this->migrateMessages['error']);

One thing with this. If the source tables are missing, then i18n_strings probably isn't installed on the source site, in which case there's nothing to migrate. So why do we treat this as an error?

In general, we don't check if the tables exist (although there are a few cases, and I don't recall which ones). Why is this on special?

Edit: I misread your comment, so this answer doesn't make much sense.

This is blocking #2918295: Move i18n query to a trait.

Yes, it should be i18nblocks but that is separate from testing the existence of the translation tables.

However, removed the test for the existence of the translation tables. That isn't done in any of the other translation migrations that use those tables. If anyone wants to discuss that can be done in a separate issue and not hold up any of the i18n issues?

NR for testing

This is failing because it no longer checks if the source tables exists and when I removed that I didn't remove the corresponding test. Note that adding checks for the existence of the tables was suggested in #28.3

Why remove this? If the source is Drupal it is assumed that if the relevant source_module is installed on the source then all the necessary tables are installed. In this case, when i18nblocks is installed on Drupal 6 several tables are created. They are i18n_blocks, i18n_strings, i18n_variable, languages, locales_source and locales_target.

Move i18nblocks to the available upgrade paths.

remove unused use statement.

Renamed the the source properties to be a bit simpler.

this needs the code to prevent processing duplicate rows.

Added the code to prevent processing duplicate rows.

i18nmenulink broke Postgres and was fixed by alexpott here. Those changes will most likely need to be added here, and to the other i18n issues

Kicked off a Postgres test, OOI.

Patch no longer applies. Re-rolling before doing anything else.

Added a similar Postgres fix to #2225587: Migrate D6 i18n menu links.

@Jo Fitzgerald, awesome, thx. Seems you are a star too!

I am not finding very much to complain about with this patch. It's dense, but I see why it needs to be dense. Ideally we'd be able to reduce that "density", but there's only so much we can do, really.

1. +++ b/core/modules/content_translation/migrations/d6_custom_block_translation.yml
   @@ -0,0 +1,48 @@
   +    -
   +      plugin: callback
   +      callable: current
   

   This should probably be 'reset', to reset the internal pointer to the first item in the array.

2. +++ b/core/modules/content_translation/migrations/d6_custom_block_translation.yml
   @@ -0,0 +1,48 @@
   +  'body/format':
   +    plugin: migration_lookup
   +    migration: d6_filter_format
   +    source: format
   

   Do we want to skip the row if lookup fails?

3. +++ b/core/modules/content_translation/src/Plugin/migrate/source/d6/BoxTranslation.php
   @@ -0,0 +1,101 @@
   +    // Build a query based on 18n_strings table where each row has the
   

   Typo in 'i18n_strings' :)

4. +++ b/core/modules/content_translation/src/Plugin/migrate/source/d6/BoxTranslation.php
   @@ -0,0 +1,101 @@
   +    $query->addField('b', 'info', 'title');
   

   Can we have a comment as to why this is being done? I understand it, but I'd like a comment for posterity.

5. +++ b/core/modules/content_translation/src/Plugin/migrate/source/d6/BoxTranslation.php
   @@ -0,0 +1,101 @@
   +    // so PostgreSQL can make the join
   

   Needs a period at the end of the sentence.

6. +++ b/core/modules/content_translation/src/Plugin/migrate/source/d6/BoxTranslation.php
   @@ -0,0 +1,101 @@
   +    // If this row has been migrated it is a duplicate then skip it.
   +    if ($this->idMap->lookupDestinationIds(['bid' => $bid, 'language' => $language])) {
   +      return FALSE;
   +    }
   

   This is very "heavy". Is there any way we could do this in the process pipeline? If not, no big deal...but ideally we could do it there, rather than here.

7. +++ b/core/modules/content_translation/src/Plugin/migrate/source/d6/BoxTranslation.php
   @@ -0,0 +1,101 @@
   +    $translation = ($property == 'title') ? 'body' : 'title';
   

   Should use === here.

8. +++ b/core/modules/content_translation/src/Plugin/migrate/source/d6/BoxTranslation.php
   @@ -0,0 +1,101 @@
   +      ->fields('i18n', ['lid'])
   

   We are never using the lid except as a join condition, so do we need to retrieve it?

9. +++ b/core/modules/content_translation/tests/src/Kernel/Migrate/d6/MigrateCustomBlockContentTranslationTest.php
   @@ -0,0 +1,68 @@
   +    $this->assertTrue(REQUEST_TIME <= $block->getChangedTime() && $block->getChangedTime() <= time());
   

   This should be two separate calls to the assertGreaterOrEqual() (or similar) methods.

10. +++ b/core/modules/content_translation/tests/src/Kernel/Migrate/d6/MigrateCustomBlockContentTranslationTest.php
    @@ -0,0 +1,68 @@
    +    $block = BlockContent::load(1)->getTranslation('zu');
    

    No need to reload the block; I think you could just do $block = $block->getTranslation('zu').

1. Reset doesn't work here. Changing results in:

1) Drupal\Tests\content_translation\Kernel\Migrate\d6\MigrateCustomBlockContentTranslationTest::testCustomBlockContentTranslation
Parameter 1 to reset() expected to be a reference, value given

2. I don't know filters well at all but other migrations that use d6_filter_format do not include a skip on empty after it in the process pipeline and that includes d6_custom_block, on which this is based.
3, 4, 5, Fixed
6. Yes, this is a bit unusual but it is difficult to determine if this is a duplicate row and I would just as soon do it as early as possible. This is the same technique used in MenuLinkTranslation. Do you have a better idea?
7. Fixed
8. Needs testing
9. Fixed
10. Fixed

Edit: Update 1.

Forgot to retest after the changes made to #72.8. the intention here was to only get rows where lt.lid was not NULL instead i18n_strings was being used. That is now corrected and the 'isNotNull' is moved to where it is easier to see.

It's a happy-maker. Complicated, but a happy-maker. Fasten seat belts and prepare for landing.

Retested and added a test for 8.6.x

Sorry, I think this needs a pause and a double check that the source plugins and migrations are in the desired directory. This has a different structure than the menu links translation it needs to be double checked.

Looks like quietone wants to review this. Assigning.

This moves the files so that all files that were in content_translation are now in block_content, except the migration itself. This make it the same as the existing menu_links_translation, with the addition of destination_module to the migration so that the UI shows content_translation as the destination module.

Ready for review

@phenaproxima set this to RTBC in #76.

According to the interdiff in #81, the only change since #76 is the change from content_translation to block_content and related namespace changes. Arguments in #81 for this change make sense to me.

Back to RTBC.

Cheers,
Markus

Thanks all! I was reviewing the code and "The translation" struck me as an odd field. It looks like the same as the title translation(?) Do we need that and if so, can we give it a more specific title?

+      'translation' => $this->t('The translation.'),
+      'title_translated' => $this->t('Block title translation.'),
+      'body' => $this->t('Block body translation.'),

assigning to myself.

I discussed #84 with @Gábor Hojtsy in Slack to understand his concern.

1. The point here is that the locales_target.translation contains the translated string value, which is either translated title or translated body in this case. We have both those as separate fields so the field 'translation' seems to be redundant.

2. Second point that Gábor was pointing out was that the field names are not consistent as we have 'title_translated' and 'body', should probably be 'title_translated' and 'body_translated'.

I was thinking to do this today but I'm prioritizing the investigation of the i18n taxonomy term migration issue so unassigning myself from this.

Cheers,
Markus

Thanks all.

So this updates prepareRow() to ensure that the property for both translation always exists on the row (title_translated and body_translated). One will always be NULL but the property will be set, if someone needs it. The fields() method is adjusted for that change. The 'translation' property is still on the row because I haven't thought of a better name and I am about to go out. (I hope my haste hasn't produced a bad patch).

1. Body

@@ -83,10 +88,10 @@
       'body' => $this->t('Block body translation.'),
-      'body_untranslated' => $this->t('Block body.'),
+      'body_translated' => $this->t('Block body.'),

Are the field name and description correct here? Should 'body' be 'Block body' and 'body_translated' be 'Block body translation'?

2. Regarding the 'translation'.
Answering to myself and Gábor in #84 and #87. The query first reads the value from 'translation' and then we do the voodoo to determine if it was the title or body translation and based on this, populate the value to the correct field. So the answer is no, we can't remove this field from the query. However, finding descriptive names is always difficult and 'The translation' is prone to WTF moments. I propose that we have this description for it: Translation for title or body, depending on context. Use title_translated or body_translated.

3. Nit, not related to the content of the patch: It would be nice if the interdiff file name would indicate the issue number and the patch numbers that it interdiffs. The URL for this interdiff for example is https://www.drupal.org/files/issues/2018-06-16/interdiff_4.txt i.e. it doesn't give any indication which interdiff this was. When reviewing multiple issues, it would make it easier for reviewer if the file name in this case would be interdiff-2909444-81-88.txt

Cheers,
Markus

89.1 Fixed
89.2 "Translation for title or body, depending on context. Use title_translated or body_translated." is accurate but long. Perhaps too long? And we know the context, the translation is the 'The translation of the value of "property"'. Would that shorter version be sufficient? Still seems difficult to be succinct. I've opted to use the shorter version in this patch but it is only a suggestion.
89.3 I used to always number the interdiff as you suggest, I guess I got lazy.

Hi!

This is ready once the tests pass. I added PostgreSQL and SQLite tests.

Markus

And the tests turned greencas expected.

We have test coverage, I have tested this manually, several contributors have reviewed the code and all feedback has been addressed. RTBC.

Markus

Helps if I actually change the status as well...

Does not apply anymore.

Needs reroll

And here is the reroll. It was simple, the changes in the MigrateUpgrade tests have already been committed in #2225681: Migrate D6 i18n blocks translated strings.

NR for testbot.

I was working off 8.6.x so the patch doesn't apply to 8.5.x and I think this should be 8.6 anyway.

Restoring RTBC.

Adjusting credits.

Committed e64090e and pushed to 8.6.x. Thanks!