Migrate D6 i18n blocks translated strings

Blocked by #2594263: Add translation data to Migrate Drupal's test fixtures.

Unblocked

Just a beginning. This migrates the block title from the i18n_strings table.

Needs the source data from #2670170: Add i18n string & variable data to d6_dump and destination plugins in #2225717: Add config translation support to migrations and implement for Drupal 6 user profile fields.

Consider this a first draft. It does migrate the translations for the title and the block content, using the existing block migrations as models. It may not be the best or most efficient method.

Forgot to add language manager to the other destination entities.

Have no idea why $language_manager was private. Especially when tests are passing locally.

Fix class name.

Let's test this.

Reroll, and the interdiff fails.

Thanks for working on this :)

1. index e3582c5..8701631 100644
   --- a/core/modules/comment/src/Plugin/migrate/destination/EntityComment.php
   
   --- a/core/modules/comment/src/Plugin/migrate/destination/EntityComment.php
   +++ b/core/modules/comment/src/Plugin/migrate/destination/EntityComment.php
   
   +++ b/core/modules/comment/src/Plugin/migrate/destination/EntityComment.php
   @@ -6,6 +6,7 @@
   
   @@ -6,6 +6,7 @@
    use Drupal\Core\Entity\EntityStorageInterface;
    use Drupal\Core\Entity\Query\QueryFactory;
    use Drupal\Core\Field\FieldTypePluginManagerInterface;
   +use Drupal\Core\Language\LanguageManagerInterface;
    use Drupal\Core\State\StateInterface;
   

   Why do comments need to change here?

2. +++ b/core/modules/comment/src/Plugin/migrate/destination/EntityComment.php
   --- /dev/null
   +++ b/core/modules/config_translation/migration_templates/d6_i18n_block.yml
   

   Unfortunately the migration system was rearchitected to not use migration templates anymore, this needs to be modified.

3. +++ b/core/modules/config_translation/migration_templates/d6_i18n_block.yml
   @@ -0,0 +1,70 @@
   +      map:
   +        book:
   +          0: book_navigation
   +        comment:
   +          0: views_block:comments_recent-block_1
   +        forum:
   +          0: forum_active_block
   +          1: forum_new_block
   +        locale:
   +          0: language_block
   +        node:
   +          0: node_syndicate_block
   +        search:
   +          0: search_form_block
   +        statistics:
   +          0: statistics_popular_block
   +        system:
   +          0: system_powered_by_block
   +        user:
   +          0: user_login_block
   +          1: system_menu_block:tools
   +          2: views_block:who_s_new-block_1
   +          3: views_block:who_s_online-who_s_online_block
   

   Are we supposed to only support a fixed set of blocks? :/ Can we take these from the existing block migration or do we need to list them again?

4. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
   @@ -122,6 +136,19 @@ public function getIds() {
      protected function updateEntity(EntityInterface $entity, Row $row) {
   +    if ($entity instanceof TranslatableInterface) {
   +      $property = $this->storage->getEntityType()->getKey('langcode');
   +
   +      if ($row->hasDestinationProperty($property)) {
   +        $language = $row->getDestinationProperty($property);
   +
   +        if (!$entity->hasTranslation($language)) {
   +          $entity->addTranslation($language);
   +        }
   +        $entity = $entity->getTranslation($language);
   +      }
   +    }
   +
   

   This change is already made in #2225775: Migrate Drupal 6 core node translation to Drupal 8 or at least similar :)

@quietone: are you still working on this?

Needs also an reroll against 8.2.x.

$ curl https://www.drupal.org/files/issues/2225681-18.patch | patch -p1
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 47918  100 47918    0     0  72006      0 --:--:-- --:--:-- --:--:-- 72057
patching file core/modules/comment/src/Plugin/migrate/destination/EntityComment.php
patching file core/modules/config_translation/migration_templates/d6_i18n_block.yml
patching file core/modules/config_translation/src/Plugin/migrate/process/I18nBlockSettings.php
patching file core/modules/config_translation/src/Plugin/migrate/process/Substr.php
patching file core/modules/config_translation/src/Plugin/migrate/source/d6/I18nBlock.php
patching file core/modules/config_translation/tests/src/Kernel/Migrate/d6/MigrateI18nBlockTest.php
patching file core/modules/config_translation/tests/src/Unit/Plugin/migrate/source/d6/I18nBlockSourceTest.php
patching file core/modules/content_translation/migration_templates/d6_i18n_custom_block.yml
patching file core/modules/content_translation/src/Plugin/migrate/source/d6/I18nBox.php
patching file core/modules/content_translation/tests/src/Kernel/Migrate/d6/MigrateI18nBlockContentTest.php
patching file core/modules/content_translation/tests/src/Unit/Plugin/migrate/source/d6/I18nBoxTest.php
patching file core/modules/file/src/Plugin/migrate/destination/EntityFile.php
Hunk #1 FAILED at 8.
Hunk #2 FAILED at 38.
Hunk #3 FAILED at 53.
Hunk #4 FAILED at 72.
4 out of 4 hunks FAILED -- saving rejects to file core/modules/file/src/Plugin/migrate/destination/EntityFile.php.rej
patching file core/modules/migrate/src/Plugin/migrate/destination/EntityConfigBase.php
patching file core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
Hunk #1 FAILED at 7.
Hunk #2 succeeded at 23 (offset 1 line).
Hunk #3 succeeded at 58 (offset 1 line).
Hunk #4 succeeded at 83 (offset 1 line).
Hunk #5 FAILED at 137.
2 out of 5 hunks FAILED -- saving rejects to file core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php.rej
patching file core/modules/migrate/tests/src/Unit/Plugin/migrate/destination/EntityContentBaseTest.php
Hunk #1 succeeded at 12 (offset 1 line).
Hunk #2 succeeded at 43 (offset 1 line).
Hunk #3 succeeded at 58 (offset 1 line).
Hunk #4 succeeded at 72 (offset 1 line).
Hunk #5 succeeded at 102 (offset 1 line).
patching file core/modules/migrate/tests/src/Unit/destination/EntityRevisionTest.php
patching file core/modules/migrate_drupal/src/Plugin/migrate/destination/EntityFieldStorageConfig.php
patching file core/modules/user/src/Plugin/migrate/destination/EntityUser.php
Hunk #3 FAILED at 49.
Hunk #4 succeeded at 56 (offset -2 lines).
Hunk #5 succeeded at 73 (offset -2 lines).
1 out of 5 hunks FAILED -- saving rejects to file core/modules/user/src/Plugin/migrate/destination/EntityUser.php.rej

Gave it a go at the reroll, but failed miserably... this isn't a reroll someone without good knowledge of the latest changes and what this patch is trying to achieve can do, meaning to say the reroll will have to make some changes to the original patch was doing because of what is in core nowadays.

Re-roll.

Updated version to 8.3.x.

Well this is just getting embarrassing...

Awesome @Jo Fitzgerald thanks for working on this!

I was checking some coding and comment standards out of this. All of this should be novice.

1. +++ b/core/modules/config_translation/src/Plugin/migrate/process/I18nBlockSettings.php
   @@ -0,0 +1,35 @@
   +  public function transform($value, MigrateExecutableInterface $migrate_executable, Row $row, $destination_property) {
   +    list($plugin, $delta, $old_settings, $title) = $value;
   +    $settings = array();
   +    $settings['label'] = $title;
   +    if ($title) {
   +      $settings['label_display'] = BlockInterface::BLOCK_LABEL_VISIBLE;
   

   Use short array syntax.

2. +++ b/core/modules/config_translation/src/Plugin/migrate/process/I18nBlockSettings.php
   @@ -0,0 +1,35 @@
   +    list($plugin, $delta, $old_settings, $title) = $value;
   +    $settings = array();
   +    $settings['label'] = $title;
   +    if ($title) {
   +      $settings['label_display'] = BlockInterface::BLOCK_LABEL_VISIBLE;
   

   Use short array syntax

3. +++ b/core/modules/config_translation/src/Plugin/migrate/process/Substr.php
   @@ -0,0 +1,35 @@
   +
   +/**
   + * Gets a substring.
   + *
   + * @MigrateProcessPlugin(
   + *   id = "substr"
   + * )
   + */
   +
   +class Substr extends ProcessPluginBase {
   +
   +  public function transform($value, MigrateExecutableInterface $migrate_executable, Row $row, $destination_property) {
   +    $start = isset($this->configuration['start']) ? $this->configuration['start'] : 0;
   

   Unnecessary extra newline. Shouldn't have any. See "Plugin discovery annotations" from comment standards.

4. +++ b/core/modules/config_translation/src/Plugin/migrate/source/d6/I18nBlock.php
   @@ -0,0 +1,99 @@
   +use Drupal\migrate\Row;
   +use Drupal\migrate_drupal\Plugin\migrate\source\DrupalSqlBase;
   +/**
   + * Gets i18n string block data from source database.
   + *
   + * @MigrateSource(
   

   Needs a blank line between use declarations and comment block. See "Identing" from coding standards.

5. +++ b/core/modules/config_translation/src/Plugin/migrate/source/d6/I18nBlock.php
   @@ -0,0 +1,99 @@
   +    $query = $this->select('i18n_strings', 'i18n')
   +      ->fields('i18n', array('lid', 'type', 'property', 'objectid'))
   +      ->fields('lt', array('lid', 'language', 'translation'))
   +      ->fields('b', array('bid', 'module', 'delta', 'theme'))
   +      ->condition('i18n.type', 'block');
   +    $query->leftJoin($this->blockTable, 'b', 'b.bid = i18n.objectid');
   

   Use short array syntax.

6. +++ b/core/modules/config_translation/src/Plugin/migrate/source/d6/I18nBlock.php
   @@ -0,0 +1,99 @@
   +  public function fields() {
   +    return array(
   +      'bid' => $this->t('The block numeric identifier.'),
   +      'module' => $this->t('The module providing the block.'),
   +      'delta' => $this->t('The block\'s delta.'),
   +      'theme' => $this->t('Which theme the block is placed in.'),
   +      'language' => $this->t('Language for this field.'),
   +      'property' => $this->t('Name of property being translated.'),
   +      'translation' => $this->t('Translation of either the title or explanation.'));
   +  }
   +
   

   Use short array syntax here and check indenting that it closes in its own line.

7. +++ b/core/modules/config_translation/src/Plugin/migrate/source/d6/I18nBlock.php
   @@ -0,0 +1,99 @@
   +    $ids['theme']['type'] = 'string';
   +    $ids['language']['type'] = 'string';
   +    return $ids;
   +  }
   +  /**
   +   * {@inheritdoc}
   +   */
   

   Needs a blank line.

8. +++ b/core/modules/content_translation/src/Plugin/migrate/source/d6/I18nBox.php
   @@ -0,0 +1,102 @@
   +      ->fields('i18n', array('lid', 'type', 'property', 'objectid'))
   +      ->fields('lt', array('lid', 'language', 'translation'))
   +      ->fields('b', array('bid', 'body', 'info', 'format'))
   

   Use short array syntax.

9. +++ b/core/modules/content_translation/src/Plugin/migrate/source/d6/I18nBox.php
   @@ -0,0 +1,102 @@
   +    return array(
   +      'bid' => $this->t('The block numeric identifier.'),
   +      'language' => $this->t('Language for this field.'),
   +      'body' => $this->t('The block/box content'),
   +      'info' => $this->t('Admin title of the block/box.'),
   +      'format' => $this->t('Input format of the custom block/box content.')
   

   Use short array syntax

10. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityConfigBase.php
    @@ -21,6 +26,53 @@
    +   * @var \Drupal\Core\Language\LanguageManagerInterface
    +   */
    +  protected $languageManager;
    +
    +  /**
    +  /**
    +   * Construct a new entity.
    +   *
    +   * @param array $configuration
    +   *   A configuration array containing information about the plugin instance.
    +   * @param string $plugin_id
    

    Double comment block starting.

Working in this now!

the new changes for standard commentaries and arrays were added.

I have removed extra indents

+++ b/core/modules/config_translation/src/Plugin/migrate/process/I18nBlockSettings.php
@@ -0,0 +1,35 @@
diff --git a/core/modules/config_translation/src/Plugin/migrate/process/Substr.php b/core/modules/config_translation/src/Plugin/migrate/process/Substr.php

Since this issue began the Substr process has been added to core. So this can be removed.

now the core/modules/config_translation/src/Plugin/migrate/process/Substr.php was removed.

This and a couple of other i18n issues that make changes to EntityConfigBase destintations. Of those I believe #2225717: Add config translation support to migrations and implement for Drupal 6 user profile fields has received the most feedback from Gábor Hojtsy and a review from phenaproxima (who has asked for a test for the changes to the destination).

Therefor, I am marking this as Postponed, until the destination is finalized in #2225717: Add config translation support to migrations and implement for Drupal 6 user profile fields.

Yeah it would be important to commit this at one place instead of working on the same thing in parallel, thanks @quietone, was about to say the same :)

#2225717: Add config translation support to migrations and implement for Drupal 6 user profile fields is in so no longer postponed.

Removed core/modules/config_translation/src/Plugin/migrate/process/Substr.php.

Oh silly me, i18n user profile isn't in. It is just RBTC.

Still, I did some cleanup and converted I18nBoxTest and I18nBockSourceTest from MigrateSqlSourceTestCase to MigrateSqlSourceTestBase.

Yay, thanks! Let's hope the dependency lands soon.

Cross-referencing the issue that's blocking this one.

#2225717: Add config translation support to migrations and implement for Drupal 6 user profile fields is in, no loner postponed.

reroll

Thanks, here is an initial review:

1. +++ b/core/modules/comment/src/Plugin/migrate/destination/EntityComment.php
   @@ -47,6 +55,8 @@ class EntityComment extends EntityContentBase {
   +   * @param LanguageManagerInterface $language_manager
   +   *   The language manager.
       * @param \Drupal\Core\Entity\EntityManagerInterface $entity_manager
       *   The entity manager service.
       * @param \Drupal\Core\Field\FieldTypePluginManagerInterface $field_type_manager
   @@ -54,9 +64,11 @@ class EntityComment extends EntityContentBase {
   
   @@ -54,9 +64,11 @@ class EntityComment extends EntityContentBase {
       * @param \Drupal\Core\State\StateInterface $state
       *   The state storage object.
       */
   -  public function __construct(array $configuration, $plugin_id, $plugin_definition, MigrationInterface $migration, EntityStorageInterface $storage, array $bundles, EntityManagerInterface $entity_manager, FieldTypePluginManagerInterface $field_type_manager, StateInterface $state) {
   -    parent::__construct($configuration, $plugin_id, $plugin_definition, $migration, $storage, $bundles, $entity_manager, $field_type_manager);
   +  public function __construct(array $configuration, $plugin_id, $plugin_definition, MigrationInterface $migration, EntityStorageInterface $storage, array $bundles, LanguageManagerInterface $language_manager, EntityManagerInterface $entity_manager, FieldTypePluginManagerInterface $field_type_manager, StateInterface $state, QueryFactory $entity_query) {
   +    parent::__construct($configuration, $plugin_id, $plugin_definition, $migration, $storage, $bundles, $language_manager, $entity_manager, $field_type_manager);
        $this->state = $state;
   +    $this->entityQuery = $entity_query;
   +    $this->languageManager = $language_manager;
      }
   
   +++ b/core/modules/user/src/Plugin/migrate/destination/EntityUser.php
   @@ -50,8 +53,8 @@ class EntityUser extends EntityContentBase {
   -  public function __construct(array $configuration, $plugin_id, $plugin_definition, MigrationInterface $migration, EntityStorageInterface $storage, array $bundles, EntityManagerInterface $entity_manager, FieldTypePluginManagerInterface $field_type_manager, PasswordInterface $password) {
   -    parent::__construct($configuration, $plugin_id, $plugin_definition, $migration, $storage, $bundles, $entity_manager, $field_type_manager);
   +  public function __construct(array $configuration, $plugin_id, $plugin_definition, MigrationInterface $migration, EntityStorageInterface $storage, array $bundles, LanguageManagerInterface $language_manager, EntityManagerInterface $entity_manager, FieldTypePluginManagerInterface $field_type_manager, PasswordInterface $password) {
   +    parent::__construct($configuration, $plugin_id, $plugin_definition, $migration, $storage, $bundles, $language_manager, $entity_manager, $field_type_manager);
        $this->password = $password;
      }
   

   Why do we need to make these changes to comments, users, etc?

2. +++ b/core/modules/config_translation/migration_templates/d6_i18n_block.yml
   @@ -0,0 +1,70 @@
   +    - d6_block
   +
   +
   

   Minor: too much whitespace.

3. +++ b/core/modules/config_translation/tests/src/Kernel/Migrate/d6/MigrateI18nBlockTest.php
   @@ -0,0 +1,67 @@
   +    'views',
   +    'comment',
   +    'menu_ui',
   ...
   +    'locale',
   

   No locale needed here I believe.

   Also why do we need all of views, comment and menu UI as well?

4. +++ b/core/modules/content_translation/src/Plugin/migrate/source/d6/I18nBox.php
   @@ -0,0 +1,103 @@
   + * Gets Drupal 6 i18n block body translation from database.
   + *
   + * @MigrateSource(
   + *   id = "d6_i18n_box"
   + * )
   + */
   +class I18nBox extends DrupalSqlBase {
   

   This is for custom content blocks, that would be a better explanation IMHO, not just bodies, they also have titles.

5. +++ b/core/modules/content_translation/tests/src/Kernel/Migrate/d6/MigrateI18nBlockContentTest.php
   @@ -0,0 +1,75 @@
   +    'locale',
   

   I am not sure we need locale module for this one? Do we?

6. +++ b/core/modules/migrate_drupal_ui/src/Form/MigrateUpgradeForm.php
   @@ -66,6 +66,10 @@ class MigrateUpgradeForm extends ConfirmFormBase {
   +    'd6_i18n_block' => [
   +      'source_module' => 'translation',
   +      'destination_module' => 'config_translation',
   +    ],
        'd7_block' => [
          'source_module' => 'block',
          'destination_module' => 'block',
   @@ -82,6 +86,10 @@ class MigrateUpgradeForm extends ConfirmFormBase {
   
   @@ -82,6 +86,10 @@ class MigrateUpgradeForm extends ConfirmFormBase {
          'source_module' => 'block',
          'destination_module' => 'block_content',
        ],
   +    'd6_i18n_custom_block' => [
   +      'source_module' => 'translation',
   +      'destination_module' => 'content_translation',
   +    ],
   

   source should be i18n module (or i18nstrings or the respective i18n module) IMHO

Also finally the issue summary needs updating to explain how this covers both custom blocks (title/body) and arbitrary blocks (title).

Fixes for everything in #56.

As for the issue summary update, I think this only covers custom blocks title and body. Adding a translation for the title on the navigation block created content in the i18n_block table, which is not used in the source plugins here. If I am correct then the various changes here should be renamed by adding 'custom' to the names. And then, the new migration for block titles would use d6_i18n_block. Do I have that right?

Fix namespace errors.

As for the issue summary update, I think this only covers custom blocks title and body. Adding a translation for the title on the navigation block created content in the i18n_block table, which is not used in the source plugins here. If I am correct then the various changes here should be renamed by adding 'custom' to the names. And then, the new migration for block titles would use d6_i18n_block. Do I have that right?

Yeah in Drupal 6 these are called boxes, in Drupal 8 they are block_content. Depending on what naming convention would the migration use, we should name them after that version's name. I think box.

OK. I took a fresh look at these block translations. Also, a good idea because the first patch I made for this was 11 months ago when I knew a lot less about migration. Not that there isn't much more to learn.

What needs to happen is migrate the block title field and the custom block title and body fields. The source tables for these are:

Blocks - 'blocks' and 'i18n_block'.
Custom blocks - 'boxes' and 'i18n_strings'.

If that is correct the rest is simpler. The source plugins are d6_i18n_block and d6_i18n_box, matching the existing d6_block and d6_box. There is no need to change any destinations, so that has been removed. And everything is in content_translation.

Probably a good idea to ignore the interdiff, it is larger than the patch.

Some of those errors are in tests unrelated to the patch and there is "The /var/www/html/core/modules/content_translation/migration_templates/d6_i18n_block.yml contains invalid YAML". Which it doesn't or the tests would not pass locally, so maybe the patch was corrupted?

Anyway, found a space error and a namespace error.

Yes, there really was an error in the d6_i18n_block.yml file but it didn't stop the local tests from passing.

This fixes the yml file and also changes it to use the migration process plugin to get the id. It seems better to do it this way since the d6_block migration uses dedupe on the id. Other i18n migration might need to do this as well. Also, fixed
MigrateBlockTest since the source not includes a translation of the title.

Assigning for review.

I think this looks generally good but it would be really nice to clean up some class naming and docs because its confusing which class/migration/test does what.

1. +++ b/core/modules/content_translation/migration_templates/d6_i18n_block.yml
   @@ -0,0 +1,64 @@
   +id: d6_i18n_block
   +label: i18n Blocks
   
   +++ b/core/modules/content_translation/migration_templates/d6_i18n_custom_block.yml
   @@ -0,0 +1,27 @@
   +id: d6_i18n_custom_block
   +label: Custom blocks
   

   Its hard to tell which specific things do these migrations support, can we be more specific?

2. +++ b/core/modules/content_translation/src/Plugin/migrate/source/d6/I18nBlock.php
   @@ -0,0 +1,104 @@
   +/**
   + * Gets i18n block data from source database.
   
   +++ b/core/modules/content_translation/src/Plugin/migrate/source/d6/I18nBox.php
   @@ -0,0 +1,103 @@
   +/**
   + * Gets Drupal 6 i18n custom block translations from database.
   

   Cleaner / more similarly structured (data vs. translation for example) descriptions would be nice to make it easier to tell which one is which one.

3. +++ b/core/modules/content_translation/src/Plugin/migrate/source/d6/I18nBlock.php
   @@ -0,0 +1,104 @@
   +      'title' => $this->t("Title translation for this language.."),
   

   Minor: ..

4. +++ b/core/modules/content_translation/src/Plugin/migrate/source/d6/I18nBox.php
   @@ -0,0 +1,103 @@
   +   * Merge the block body and info translations into a single row.
   +   *
   +   * @return array
   +   *   An array of ...
   +   */
   +  protected function values() {
   

   Maybe: "An array of result rows with the info keys merged in to (...)"

5. +++ b/core/modules/content_translation/tests/src/Kernel/Migrate/d6/MigrateI18nBlockContentTest.php
   @@ -0,0 +1,54 @@
   +/**
   + * Tests migration of i18n block translations.
   
   +++ b/core/modules/content_translation/tests/src/Kernel/Migrate/d6/MigrateI18nCustomBlockContentTest.php
   @@ -0,0 +1,73 @@
   +/**
   + * Tests migration of i18n custom block strings.
   

   Cleaner description of which one is doing what would be useful.

6. +++ b/core/modules/content_translation/tests/src/Kernel/Migrate/d6/MigrateI18nBlockContentTest.php
   @@ -0,0 +1,54 @@
   +class MigrateI18nBlockContentTest extends MigrateDrupal6TestBase {
   

   It says block content test, but it tests the title translation, this is confusing :/

7. +++ b/core/modules/content_translation/tests/src/Kernel/Plugin/migrate/source/d6/I18nBlockTest.php
   @@ -0,0 +1,100 @@
   +/**
   + * Tests i18n block title source plugin.
   
   +++ b/core/modules/content_translation/tests/src/Kernel/Plugin/migrate/source/d6/I18nBoxTest.php
   @@ -0,0 +1,143 @@
   +/**
   + * Tests i18n block title source plugin.
   

   The descriptions are the same but the functionality is different.

Also I think similarly to other i18n migrations, this does not ensure the block fields themselves are marked translatable (for UI purposes). While translating the content on the API works without that, users will not be able to change or add new translations. Not 100% sure if we should do that here or not, but it needs to get done somewhere :)

This also needs renaming. The migrations be in the form basicmigration_variation. The same pattern should be used for the related files like migration plugins and tests. See #26 and #27

First, rename migrations to use _translation as per #69.

Still need to do everything in #67.

Made the corrections in MigrateUpgradeForm. I think I will throw a party when #2569805: For Drupal migration, identify the source module is Fixed.

Also I think similarly to other i18n migrations, this does not ensure the block fields themselves are marked translatable (for UI purposes).

Let's do that here.

The source plugin BoxTranslation uses the same method as a version of menu link translation source plugin that alexpott kicked back with concerns about the scalability. See #39 of that issue.

Back to Needs work to rework the source plugin.

Postponing this because the source plugin test is returning false positives. #2862006: MigrateSourceTestBase returns false positives for most plugin tests

No longer blocked.

Bumping priority while triaging the queue in the weekly migrate meeting. Getting this done is blocking completion of d6 migration path.

Reworked the source plugin.

Update the source plugin test and the migration to suit the new source plugin.

Need to rename the migration as per #2861383: Rename i18n migrations to _translation.

Ah, silly me, the renaming is already done, in #70.

Removed redundant (debugging?) line that was highlighted as a coding standards infringement.

Assigning for review

This is not a full review, I'm just trying to understand the scope of the issue since the IS seems to be outdated.

I'm have no experience with the D6 i18n module, but if I understand correctly, if you select a language for a block, the block will only appear if you are viewing the site in the selected language. Maybe this is not the scope of this issue, but I don't see this behavior being migrated here.

+++ b/core/modules/block/tests/src/Kernel/Migrate/d6/MigrateBlockTest.php
@@ -116,9 +116,9 @@ public function testBlockMigration() {
     $visibility = [];
     $settings = [
       'id' => 'system_menu_block',
-      'label' => '',
+      'label' => 'zu - Test Title 02',
       'provider' => 'system',
-      'label_display' => '0',
+      'label_display' => 'visible',
       'level' => 1,
       'depth' => 0,
     ];

This block is supposed to be only displayed when the site is in Zulu, so the $visibility settings should not be empty.

+++ b/core/modules/content_translation/migration_templates/d6_block_translation.yml
@@ -0,0 +1,64 @@
+id: d6_block_translation
+label: Blocks translation
+migration_tags:
+  - Drupal 6
+source:
+  plugin: d6_block_translation
+  constants:
+    dest_label: 'settings/label'

This should provide a process pipeline for the visibility settings.

@maxocub, thanks for the review. I've been learning D6 internationalization as I work on these issues so don't have much experience.

I'll try to explain what happens when I translate a block, in this case the navigation block. On the block config page, I can choose a language for the block and the various visibility settings. If say, I choose Zulu, the label field in the blocks table is overwritten with the Zulu title and so are the visibility settings. If I then choose French, the French title and visibility settings overwrites the Zulu versions and the block can only be for French. It seems that the block can only be in one language (or all languages). The block migration will migrate the possibly translated title and the visibility settings. Then d6_block_translation, will make sure the title has the correct language code.

Make sense?

@quietone: I think I understand what you're doing, that is migrating the string translations of the block labels?

What I understood and what I tried was:
1) Fresh D6 install
2) Enable i18nblocks
3) Add a language, say French
4) Enable path prefix for language selection
5) Go to the Blocks page
6) Configure a block, say Navigation
7) Chose French in the languages selection
8) Now, if you are seeing the site in English, you don't see the Navigation block, but if you see it in French (/fr) you see it.
9) Then if you migrate, on the D8 site the Navigation block is visible on both English and French, which is not the same behavior as on the D6 site.

If on the D8 site I configure the Main Navigation block and select French, then the visibility configuration of the block will be:

visibility:
  language:
    id: language
    langcodes:
      fr: fr
    negate: false
    context_mapping:
      language: '@language.current_language_context:language_interface'

But in the above migration scenario, the visibility configuration of the block is empty.

Am I thinking out of the scope of this issue?

@maxocub, thanks! Now I understand the problem.

Am I thinking out of the scope of this issue?

I think we should move visibility to a follow up. Even if we make the changes to migrate the visibility settings of a translated block the visibility settings for the non-translated block will be incorrect. And It doesn't seem obvious how to solve this. At least to me. Because, if I understand correctly, when migrating a french language navigation block, we also have to modify the visibility settings on the english version navigation block.

And, In favour of a followup is that this does migrate the translation and the user can fix this visibility in the UI.

So, yes, let's move migrating visibility settings on i18n blocks to a follow up.

Picking up for review

I'd like to see an issue summary update here, to make it easier for reviewers and eventually commiters.

had a go at updating the IS.
A follow up has been created as discussed in #90, #91. #2903579: Migrate translated block visibility settings

Thanks @quietone!
Assigned for review.

1.

+++ b/core/modules/content_translation/migration_templates/d6_block_translation.yml
 
+++ b/core/modules/content_translation/migration_templates/d6_custom_block_translation.yml

+++ b/core/modules/content_translation/src/Plugin/migrate/source/d6/BlockTranslation.php

+++ b/core/modules/content_translation/src/Plugin/migrate/source/d6/BoxTranslation.php

+++ b/core/modules/content_translation/tests/src/Kernel/Migrate/d6/MigrateBlockContentTranslationTest.php

+++ b/core/modules/content_translation/tests/src/Kernel/Migrate/d6/MigrateCustomBlockContentTranslationTest.php

+++ b/core/modules/content_translation/tests/src/Kernel/Plugin/migrate/source/d6/BlockTranslationTest.php

+++ b/core/modules/content_translation/tests/src/Kernel/Plugin/migrate/source/d6/BoxTranslationTest.php

Why do we put this in content_translation and not block and block content? Like d6_node_translation being in the node module?

2.

+++ b/core/modules/content_translation/tests/src/Kernel/Migrate/d6/MigrateCustomBlockContentTranslationTest.php
@@ -0,0 +1,74 @@
+    'block',
...
+    'config_translation',
...
+    'statistics',

This is not needed.

3.

+++ b/core/modules/content_translation/tests/src/Kernel/Migrate/d6/MigrateCustomBlockContentTranslationTest.php
@@ -0,0 +1,74 @@
+    ConfigurableLanguage::createFromLangcode('en')->save();
+    ConfigurableLanguage::createFromLangcode('fr')->save();
+    ConfigurableLanguage::createFromLangcode('zu')->save();

Instead of creating each languages, we can just add 'language' to the executed migrations.

4.
I'm not certain if it would accelerate this issue getting in, but personnaly I would have find a lot easier to understand this patch if it was split in two, one for block translation ans one for custom block translation.

Oh, and it needs a re-roll, simply needs to remove the changes in MigrateUpgradeForm.php

Just the reroll.

Needed to add the source_module to the source plugin annotations. Hopefully that will fix the tests and complete the reroll.

Changed the source module for BoxTranslation.

Removed the redundant migrations from MigrateCustomBlockContentTranslationTest (#97.2).

Ok, so I took the liberty to split this patch in two for easier reviews & commit, one problem/solution per issue. This one now deals with block translated string migration, and the other one, #2909444: Migrate D6 i18n custom blocks (boxes), deals with custom blocks (boxes) translations migration.
No change has been made to the remaining code.

1.

+++ b/core/modules/content_translation/migration_templates/d6_block_translation.yml
@@ -0,0 +1,64 @@
+label: Blocks translation

Nit: Block translations?

2.

+++ b/core/modules/content_translation/src/Plugin/migrate/source/d6/BlockTranslation.php
@@ -0,0 +1,104 @@
+      ->fields('b', ['bid', 'module', 'delta', 'theme', 'title']);
+    $query->addField('b', 'module', 'block_module');
+    $query->addField('b', 'delta', 'block_delta');

These can be chained.

3.

+++ b/core/modules/block/tests/src/Kernel/Migrate/d6/MigrateBlockTest.php
--- /dev/null
+++ b/core/modules/content_translation/migration_templates/d6_block_translation.yml

+++ b/core/modules/content_translation/migration_templates/d6_block_translation.yml
--- /dev/null
+++ b/core/modules/content_translation/src/Plugin/migrate/source/d6/BlockTranslation.php

+++ b/core/modules/content_translation/src/Plugin/migrate/source/d6/BlockTranslation.php
--- /dev/null
+++ b/core/modules/content_translation/tests/src/Kernel/Migrate/d6/MigrateBlockContentTranslationTest.php

+++ b/core/modules/content_translation/tests/src/Kernel/Migrate/d6/MigrateBlockContentTranslationTest.php
--- /dev/null
+++ b/core/modules/content_translation/tests/src/Kernel/Plugin/migrate/source/d6/BlockTranslationTest.php

I'm not sure that content_translation is the right place for this code. I think it should go in block, like the node translation migrations are in the node module.

4.

+++ b/core/modules/content_translation/tests/src/Kernel/Plugin/migrate/source/d6/BlockTranslationTest.php
@@ -0,0 +1,100 @@
+ * Tests i18n block title source plugin.

This comment needs to be updated.

1. Label changed.
2. These cannot be chained because addField() returns "The unique alias that was assigned for this field".
3. Moved files from content_translation to block.
4. Updated the comment.

• Moved files to the correct location (block, not block_content).
• Update namespace in files.

Put d6_block_translation source plugin in a d6 sub-directory.

1. +++ b/core/modules/block/src/Plugin/migrate/source/d6/BlockTranslation.php
   @@ -0,0 +1,104 @@
   +    if ($this->getModuleSchemaVersion('system') >= 7000) {
   +      $this->blockTable = 'block';
   +    }
   +    else {
   +      $this->blockTable = 'blocks';
   +    }
   

   We are putting this in the d6 namespace and we are checking if we are dealing with D6 or D7. We should do one or the other. I think this source plugin should also work with D7, but we should confirm that before.

2. +++ b/core/modules/block/tests/src/Kernel/Plugin/migrate/source/BlockTranslationTest.php
   @@ -0,0 +1,100 @@
   +namespace Drupal\Tests\block\Kernel\Plugin\migrate\source\d6;
   

   The namespace doesn't match the file location. This will depend on where we put the source plugin.

Re #112:

1. I checked and it seems like the source plugin won't work with D7. There's no i18n_blocks table in D7, the translations are stored in i18n_string. So the plugin should stay in the d6 namespace but we should remove the getModuleSchemaVersion() call and just hardcode the blocks table name.
2. We should move BlockTranslationTest in a d6 folder to match the namespace. This should fix the failing test.

1. Hard-coded blocks table name and removed all (now-redundant) references to blockTable.
2. Moved BlockTranslationTest to a d6 sub-folder.

Every concerns have been addressed, I think this is ready to be RTBCed.

@Jo Fitzgerald and @maxocub thanks for getting this to RTBC. Wanting to get this done in Vienna. And you all did it!

This patch looks very, very good overall. So nice and clear, and good test coverage. I did find a few concerns, though.

1. +++ b/core/modules/block/src/Plugin/migrate/source/d6/BlockTranslation.php
   @@ -0,0 +1,90 @@
   + *   source_module = "block"
   

   This needs to be i18n_block, or whichever module it is that actually creates the i18n_blocks table. Pretty sure it ain't the block module, though. =\

2. +++ b/core/modules/block/src/Plugin/migrate/source/d6/BlockTranslation.php
   @@ -0,0 +1,90 @@
   +      ->fields('b', ['bid', 'module', 'delta', 'theme', 'title']);
   +    $query->addField('b', 'module', 'block_module');
   +    $query->addField('b', 'delta', 'block_delta');
   

   Nit: Let's take module and delta out of the call to fields(), since we're including them again in the addField() calls.

3. +++ b/core/modules/block/tests/src/Kernel/Migrate/d6/MigrateBlockTest.php
   @@ -116,9 +116,9 @@ public function testBlockMigration() {
   -      'label' => '',
   +      'label' => 'zu - Test Title 02',
          'provider' => 'system',
   -      'label_display' => '0',
   +      'label_display' => 'visible',
   

   I don't understand why these changed, in this test.

4. +++ b/core/modules/migrate_drupal/tests/fixtures/drupal6.php
   @@ -746,7 +746,7 @@
   -  'title' => '',
   +  'title' => 'zu - Test Title 02',
   

   Why this change in the block table? That seems incorrect.

#117.4) I was originally surprised when learning about translated blocks how the data is stored in the block table. When making changes to the source db, I use the UI and watch what happens to the database tables. When adding the Zulu translation the translation was put in a row in the block table. This change reflects that.
#117.3) The translation was updated so, update the test string. I think the visibility can be restored to the original, there is a follow up issue for visibility settings.

This fixes Observion #1 & #2 from comment #117.

However, #3 & #4 are a little tricky.

If we change the Block title in the fixture from empty value to a string. Tests in MigrateBlockTest.php expect the label to be visible. Don't know why... I am hoping that the answer is "Blocks are weird"....

And thus if we have a non-empty title for the block in fixture, we need the test to expect the lable to be visible. ¯\_(ツ)_/¯

Forgot to put this in review.... :(

Regrarding #117.3) There has been some discussion and questioning the change to 'label_display' in the test. That setting is migrated in d6_block, specifically the process plugin block_settings. In that process, if the title field is populated in the d6 source then the label_display will be 'visible'. The relevant code is:

if ($title) {
      $settings['label_display'] = BlockPluginInterface::BLOCK_LABEL_VISIBLE;
    }
    else {
      $settings['label_display'] = '0';
    }

So, the change to the test is correct.

1. +++ b/core/modules/block/src/Plugin/migrate/source/d6/BlockTranslation.php
   @@ -0,0 +1,90 @@
   +class BlockTranslation extends DrupalSqlBase {
   

   Is there any reason this class could not extend the Block source plugin, and add i18n stuff to query()? This would reduce the amount of code we need here...

2. +++ b/core/modules/block/tests/src/Kernel/Plugin/migrate/source/d6/BlockTranslationTest.php
   @@ -0,0 +1,100 @@
   +class BlockTranslationTest extends MigrateSqlSourceTestBase {
   

   If we make BlockTranslation extend Block, this test could maybe extend Block's test too.

@phenaproxima I have made the changes you suggested, but I'm not entirely convinced of the benefits - can you persuade me?

@Jo Fitzgerald, thanks! I was working on this but then webchick came by to do some commits.

Aww, I screwed that up, didn't I! @quietone Feel free to take over, I'm calling it a day and going home for the weekend!

quiteone's original patch had a query in BlockTranslation class that did a left join from i18n_blocks table to blocks table to get all the translated blocks.

jo-fitzgerald's patch in #128 made a change to base BlockTranslation on Block, but this changed the query and the new query had a left joind from blocks table to i18n_blocks table which resulted in null values for some blocks.

I've updated the code to do an innerJoin between blocks & i18n_blocks to get only the required translated blocks.

Patch in 128 failed to apply because I created the patch on 8.5.x version and somehow managed to get unwanted code in the diff.
Patch in 128 should be completely disregarded.

Changing patch to 8.4.x.

Changes mentioned in 128 still apply though.

One more thing! I think that the intent of #123.2, extending the source plugin test, hasn't been addressed.

+++ b/core/modules/block/tests/src/Kernel/Plugin/migrate/source/d6/BlockTranslationTest.php
@@ -0,0 +1,95 @@
+    $tests = parent::providerSource();

This is the first time, that I can think of, that a source plugin test extends from the parent. That is ok, but this is not clear at all what is the difference between this test and the parent test. And looking at the parent it seems that currently every array in $tests is overwritten, which means this line is not doing anything except confusing the reader. How about we keep this line but just change those array values that are different. That has the advantage that we can quickly see the necessary changes for this test.

From #105.3,

I'm not sure that content_translation is the right place for this code. I think it should go in block, like the node translation migrations are in the node module.

This has been niggling at me for a while. And now back at home it is clear to me why I put in content_translation.

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.

@quietone: Thanks for the clarification, I had not realized that, so I agree that this should go in content_trnaslation.

Changes in response to #133 and #134:

• Moved back to content_translations.
• Tweaked BlockTranslationTest to make better use of the data from the parent.

I would appreciate opinions on a change I made in the name of extension: previously the query() function of BlockTranslation contained a leftJoin from i18n_blocks to block. In trying to extend Block I switched the tables which of course did not work. @dipakmdhrm found a solution by replacing the leftJoin with innerJoin, but I'm not comfortable with this because it is not a like-for-like replacement. Ideally it would be a rightJoin, but that has been deprecated. Do we have to drop the call to parent::query() in BlockTranslation::query() (which would make life complicated because it also seta a few properties) or is the innerJoin acceptable?

I've struggled to even put this question into words let alone answer it so hopefully someone else will understand what I'm talking about and will have an answer!

This looks quite good. Only a few minor things.

1. +++ b/core/modules/content_translation/migration_templates/d6_block_translation.yml
   @@ -0,0 +1,64 @@
   +  id:
   +    plugin: migration
   +    migration: d6_block
   +    source:
   +      - module
   +      - delta
   

   This should be using the migration_lookup plugin, not migration. Additionally, should we skip the row if the migration_lookup comes back empty?

2. +++ b/core/modules/content_translation/tests/src/Kernel/Migrate/d6/MigrateBlockContentTranslationTest.php
   @@ -0,0 +1,60 @@
   +  public static $modules = [
   

   This should be protected.

3. +++ b/core/modules/content_translation/tests/src/Kernel/Migrate/d6/MigrateBlockContentTranslationTest.php
   @@ -0,0 +1,60 @@
   +    $config = \Drupal::service('language_manager')->getLanguageConfigOverride('zu', 'block.block.user_1');
   

   Let's use $this->container rather than \Drupal::service().

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

   Should be protected.

Do we have to drop the call to parent::query() in BlockTranslation::query() (which would make life complicated because it also seta a few properties) or is the innerJoin acceptable?

If innerJoin() is not ideal, then we should use whatever way is most ideal. It seems to me that dropping the call to parent::query(), and copy-pasting the code from the parent method, is the way to go so that we can use the non-deprecated leftJoin(), which is the documented replacement for rightJoin(). As long as we have a comment explaining the copypasta, I'm OK with that.

A couple of changes and a question back to @phenaproxima:

1. Changed the plugin, skip the row if it comes back empty.
2. Why should it be protected? Many (29) occurrences are protected, but most (100+) are public.
3. Used $this->container.
4. See 2.

@Jo Fitzgerald, thanks. And here is an update to the source plugin query.

Ah, silly thing. I was uploading patch but Jo Fitzgerald beat me to it. Patch -139 has the changes to the source plugin query.

Why should it be protected? Many (29) occurrences are protected, but most (100+) are public.

It was changed in the base classes (KernelTestBase et. al.), so we should change it down the line. There is no reason for the $modules property to be publicly exposed. That said, in the context of this issue, it's a nitpick we don't need to bother with.

I did find one superdupernit, fixable on commit.

+++ b/core/modules/content_translation/src/Plugin/migrate/source/d6/BlockTranslation.php
@@ -0,0 +1,53 @@
+    // Let the parent set the block table to use but do not use the parent query.

This line should be 80 characters long. :)

If the tests pass, I think this one is ready for RTBC.

Okay. All seems legit here. RTBC for patch-139 in #139.

It only takes a moment to resolve those minor nits.

The changes are so minor that this can be safely returned to RTBC.

So it seems that making $modules protected is a little more involved - it needs to be changed 'all the way up the chain' on all of its ancestor classes too otherwise an error occurs.

As @phenaproxima said this is "a nitpick we don't need to bother with" (and for simplicity's sake) I have removed this change. Perhaps we could have a follow-up ticket that changes all instances of $modules to protected?

Surely this change can be safely set back to RTBC... (famous last words)