Problem/Motivation

This is a follow up to #2909444: Migrate D6 i18n custom blocks (boxes). It is a simple task to see if a block of code should move to a trait.

Here is the relevant part from the original comment, In comment #46

The original comment

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

The request is #45 was about the addition of tableExists('i18n_strings') in the issue this is a follow up to. That test was removed because migrate doesn't check for the existence of tables in other source plugins. The assumption is that for Drupal to Drupal migrations if a module is enabled on the source site then the relevant tables exist.

Proposed resolution

Create a trait for the above code.

Remaining tasks

Write a patch
review
commit

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

quietone created an issue. See original summary.

quietone’s picture

Issue tags: +i18n-migrate

Just adding a tag.

quietone’s picture

Status: Active » Postponed

This code is used in MenuLinkTranslation::prepareRow() #2225587: Migrate D6 i18n menu links and BoxTranslation::prepareRow() #2909444: Migrate D6 i18n custom blocks (boxes) but not Block Translation #2225681: Migrate D6 i18n blocks translated strings or VocabularyTranslation.

Postponing until either #2225587: Migrate D6 i18n menu links or #2909444: Migrate D6 i18n custom blocks (boxes) is committed.

quietone’s picture

Issue tags: +migrate-d6-d8

tagging

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

quietone’s picture

quietone’s picture

Version: 8.5.x-dev » 8.6.x-dev
Status: Postponed » Active

No longer postponed, one of the two issues this was postponed on has been committed.

quietone’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
7.41 KB

Made a trait for the shared code in prepareRow of the source plugins MenuLinkTranslation and BoxTranslation.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

phenaproxima’s picture

  1. +++ b/core/modules/content_translation/src/Plugin/migrate/source/d6/I18nQueryTrait.php
    @@ -0,0 +1,65 @@
    +    $language = $row->getSourceProperty('language');
    +    $objectId = $row->getSourceProperty($objectIdName);
    

    These variables should be $snake_case, not $camelCase, and we need to assert that they're both non-empty (i.e., throw an exception if they're empty).

  2. +++ b/core/modules/content_translation/src/Plugin/migrate/source/d6/I18nQueryTrait.php
    @@ -0,0 +1,65 @@
    +    if ($this->idMap->lookupDestinationIds([$objectIdName => $objectId, 'language' => $language])) {
    

    We'll need a method in this trait to get the ID map from the migration; we can't assume $this->idMap is set.

quietone’s picture

1. All the renaming complete. And added MigrateSkipRowExceptions with simple error message.
2. I must be missing something, Why would it not be set? Instead of a new method why not just, $this->migration->getIdMap()?

heddn’s picture

I don't also see how we can assume the $thhis->migration is set either. So I just passed in the map to the function. I mean, we can be pretty sure, but this is safer.

phenaproxima’s picture

--- a/core/modules/block_content/src/Plugin/migrate/source/d6/BoxTranslation.php
+++ b/core/modules/block_content/src/Plugin/migrate/source/d6/BoxTranslation.php

+++ b/core/modules/block_content/src/Plugin/migrate/source/d6/BoxTranslation.php
@@ -4,6 +4,7 @@

@@ -4,6 +4,7 @@
 
 use Drupal\migrate\Row;
 use Drupal\migrate_drupal\Plugin\migrate\source\DrupalSqlBase;
+use Drupal\content_translation\Plugin\migrate\source\d6\I18nQueryTrait;

This is introducing a dependency on content_translation. Not sure we should be doing that. Maybe we should just put the trait in Migrate Drupal...

heddn’s picture

On the surface, yes. I did some research though and the migration yamls that use those two source plugins exist in content_translation. So no hidden dependency exists.

phenaproxima’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/content_translation/src/Plugin/migrate/source/d6/I18nQueryTrait.php
    @@ -0,0 +1,74 @@
    +   * other in prepareRow(). This will save both translations to the row.
    

    Can we mention what the names of the translated properties will be?

  2. +++ b/core/modules/content_translation/src/Plugin/migrate/source/d6/I18nQueryTrait.php
    @@ -0,0 +1,74 @@
    +   * @throws \Exception
    +   */
    +  protected function getPropertyNotInRowTranslation(Row $row, $property_not_in_row, $object_id_name, MigrateIdMapInterface $id_map) {
    

    The @throws needs to be improved; we should actually throw MigrateException if the properties are not there. Also, let's mention in the documentation for $row that the 'language' and 'objectid' properties need to be there.

quietone’s picture

Status: Needs work » Needs review
FileSize
8.18 KB
2.22 KB

1. The comment is expanded to explain that the property names vary.
2. Changed to MigrateException. Documentation for $row improved as suggested.

I'd like more informative exception messages, this is quite specific to i18n maybe that should in the message? 'The i18n translation requires a language' and 'The i18n translation requires and objectid'?

maxocub’s picture

Status: Needs review » Needs work

Nice to see this code in a reusable trait!
Just found a few nits:

  1. +++ b/core/modules/block_content/src/Plugin/migrate/source/d6/BoxTranslation.php
    @@ -48,34 +51,12 @@ public function query() {
       public function prepareRow(Row $row) {
    +    $property_in_row = $row->getSourceProperty('property');
    +    // Get the translation for the property not already in the row and save it
    +    // in the row.
    +    $property_not_in_row = ($property_in_row === 'title') ? 'body' : 'title';
    +    return $this->getPropertyNotInRowTranslation($row, $property_not_in_row, 'bid', $this->idMap);
       }
    

    Maybe this is out of scope since the original code didn't have it, but I think we should call parent::prepareRow() here.

  2. +++ b/core/modules/content_translation/src/Plugin/migrate/source/d6/I18nQueryTrait.php
    @@ -0,0 +1,80 @@
    +   * @throws \Exception
    

    The exception being thrown is a MigrateException, we should specify it here.

  3. +++ b/core/modules/content_translation/src/Plugin/migrate/source/d6/I18nQueryTrait.php
    @@ -0,0 +1,80 @@
    +    $propertyInRow = $row->getSourceProperty('property');
    +    $row->setSourceProperty($propertyInRow . '_translated', $row->getSourceProperty('translation'));
    

    Leftover camelCase variable that should be converted to snake_case.

jofitz’s picture

Status: Needs work » Needs review
FileSize
8.21 KB
1.97 KB

Corrected the nits.

maxocub’s picture

Status: Needs review » Needs work

Thanks @Jo Fitzgerald, just one thing left:

+++ b/core/modules/content_translation/src/Plugin/migrate/source/d6/I18nQueryTrait.php
@@ -0,0 +1,80 @@
+   * @throws \MigrateException

It should be the full class name: \Drupal\migrate\MigrateException

quietone’s picture

Status: Needs work » Needs review
FileSize
8.24 KB
762 bytes

Fixing the @throws.

maxocub’s picture

Status: Needs review » Reviewed & tested by the community

This now looks ready to me, thanks everyone!

alexpott’s picture

I've backported this to 8.6.x because multilingual migrations are still not stable.

FILE: ...tent_translation/src/Plugin/migrate/source/d6/I18nQueryTrait.php
----------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
----------------------------------------------------------------------
 28 | ERROR | [x] Parameter comment indentation must be 3 spaces,
    |       |     found 1 spaces
 29 | ERROR | [x] Parameter comment indentation must be 3 spaces,
    |       |     found 1 spaces
----------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------
diff --git a/core/modules/content_translation/src/Plugin/migrate/source/d6/I18nQueryTrait.php b/core/modules/content_translation/src/Plugin/migrate/source/d6/I18nQueryTrait.php
index aceb5ebae5..a5bc9494f2 100644
--- a/core/modules/content_translation/src/Plugin/migrate/source/d6/I18nQueryTrait.php
+++ b/core/modules/content_translation/src/Plugin/migrate/source/d6/I18nQueryTrait.php
@@ -25,8 +25,8 @@
    *
    * @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_strings table.
+   *   and an 'objectid' property. The 'objectid' is the value for the
+   *   'objectid' field in the i18n_strings table.
    * @param string $property_not_in_row
    *   The name of the property to get the translation for.
    * @param string $object_id_name

Fixed coding standards on commit.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed e22910b9d9 to 8.7.x and 36aed27964 to 8.6.x. Thanks!

  • alexpott committed e22910b on 8.7.x
    Issue #2918295 by quietone, heddn, Jo Fitzgerald, phenaproxima, maxocub...

  • alexpott committed 36aed27 on 8.6.x
    Issue #2918295 by quietone, heddn, Jo Fitzgerald, phenaproxima, maxocub...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.