Migrate D7 i18n custom blocks

I suggest that d6_box_translation should not be deprecated (at least not while d7_block_custom_translation is created in exactly the same manner)

I read the patch and found a couple of nits.

1.

+++ b/core/modules/block_content/src/Plugin/migrate/source/BlockCustomTranslation.php
@@ -1,32 +1,50 @@
 /**
- * Gets Drupal 6 i18n custom block translations from database.
+ * Drupal custom block source from database.
  */

Class descriptions should start with a third person verb.

2.

   public function query() {
-    // Build a query based on i18n_strings table where each row has the
+    // Get the name of the i18n string table.
+    if ($this->getModuleSchemaVersion('system') >= 7000) {
+      $this->i18nStringTable = 'i18n_string';
+      $this->blockCustomTable = 'block_custom';
+    }
+    else {
+      $this->i18nStringTable = 'i18n_strings';
+      $this->blockCustomTable = 'boxes';
+    }

Can we improve the inline comments a bit, for example like this: "Drupal 6 and 7 used stored the data in different tables. Determine which table names to use."

3.

+    // Build a query based on i18n_string table where each row has the
     // translation for only one property, either title or description. The
     // method prepareRow() is then used to obtain the translation for the
     // other property.
-    $query = $this->select('boxes', 'b')
+    $query = $this->select($this->blockCustomTable, 'b')

Build a query based on blockCustomTable, not i18n_string table.

4.

     // Use 'title' for the info field to match the property name in the
-    // i18n_strings table.
+    // i18n_string table.
     $query->addField('b', 'info', 'title');

... to match the property name in i18nStringTable.

5.

+++ b/core/modules/block_content/src/Plugin/migrate/source/d7/BlockCustomTranslation.php
@@ -0,0 +1,15 @@
+<?php
+/**
+ * Drupal 7 custom block source from database.

Class descriptions should start with a third person verb.

6.

+++ b/core/modules/content_translation/src/Plugin/migrate/source/I18nQueryTrait.php
@@ -0,0 +1,87 @@
+  /**
+   * Get the translation for the property not already in the row.
+   *
[...]
+   * Since these values are stored in separate rows of the i18n_string
+   * table we get them individually, one in the source plugin query() and the
[...]
+   * @param \Drupal\migrate\Row $row
+   *   The current migration row which must include both a 'language' property
+   *   and an 'objectid' property. The 'objectid' is the value for the
+   *   'objectid' field in the i18n_string table.
[...]
+   * @param string $object_id_name
+   *   The value of the objectid in the i18n table.
+   */
+  protected function getPropertyNotInRowTranslation(Row $row, $property_not_in_row, $object_id_name, MigrateIdMapInterface $id_map) {

Method descriptions should start with a third person verb as well. And were are referring to i18n_string table here like in the previous comments.

7. @quietone, could you please comment on the deprecation comment by Jo Fitzgerald in #10?

Cheers,
Markus

Queued for PostgreSQL and SQLite

NW for a duplicate I18nQueryTrait and sqlite failures.

1. +++ b/core/modules/block_content/src/Plugin/migrate/source/BlockCustomTranslation.php
   @@ -1,45 +1,65 @@
   +    // Drupal 6 and Drupal 7 use different tables for custom blocks and i18n
   +    // strings. Determine which table names to use.
   +    // Get the name of the i18n string table.
   +    if ($this->getModuleSchemaVersion('system') >= 7000) {
   +      $this->i18nStringTable = 'i18n_string';
   +      $this->blockCustomTable = 'block_custom';
   +    }
   +    else {
   +      $this->i18nStringTable = 'i18n_strings';
   +      $this->blockCustomTable = 'boxes';
   +    }
   +    // Build a query based on blockCustomTable table where each row has the
   

   Instead of having a single base class, can we use one each for d6 and d7 and use a static constant that has the table names?

   maybe move the bulk of the code into the d7 plugin and have the d6 plugin extend that with a new static member. then d6 can just be pulled into contrib later (if we want to).

2. +++ b/core/modules/menu_link_content/src/Plugin/migrate/source/d6/MenuLinkTranslation.php
   @@ -33,9 +33,18 @@ public function query() {
   +    $query->leftJoin($this->i18nStringTable, 'i18n', 'CAST(ml.mlid as CHAR(255)) = i18n.objectid');
   

   I'm mildly interested why casting to CHAR(255), we picked 255. Does that mean we don't let more than 255 characters? Do we have any values stored in our test cases that are longer than 255?

Perfect. Onward and upward.

Yay great! Looks good except this one:

+++ b/core/modules/menu_link_content/src/Plugin/migrate/source/d6/MenuLinkTranslation.php
@@ -33,9 +33,18 @@ public function query() {
+    // Drupal 6 and Drupal 7 use different tables for i18n strings. Determine
+    //  which table names to use.  Get the name of the i18n string table.
+    if ($this->getModuleSchemaVersion('system') >= 7000) {
+      $this->i18nStringTable = 'i18n_string';
+    }
+    else {
+      $this->i18nStringTable = 'i18n_strings';
+    }

Why does D6 menu translation class has this code for D7? That looks confusing.

IMHO same CONST solution could apply as for the block translation class suggested above by @heddn.

Feedback addressed, tests green. Back to rtbc.

Attempted to commit but phpsniff found these:

FILE: ..._content/src/Plugin/migrate/source/d7/BlockCustomTranslation.php
----------------------------------------------------------------------
FOUND 4 ERRORS AFFECTING 4 LINES
----------------------------------------------------------------------
  3 | ERROR | [x] There must be one blank line after the namespace
    |       |     declaration
  8 | ERROR | [x] There must be one blank line after the last USE
    |       |     statement; 0 found;
 97 | ERROR | [x] Expected 1 blank line after function; 0 found
 98 | ERROR | [x] The closing brace for the class must have an empty
    |       |     line before it

Per interdiff, style issues have been addressed, RTBC for me again, pending passing tests.

Superb, thanks!