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
Comment | File | Size | Author |
---|---|---|---|
#20 | interdiff.txt | 762 bytes | quietone |
#20 | 2918295-19.patch | 8.24 KB | quietone |
#18 | interdiff-2918295-16-18.txt | 1.97 KB | jofitz |
#18 | 2918295-18.patch | 8.21 KB | jofitz |
#16 | interdiff-12-16.txt | 2.22 KB | quietone |
Comments
Comment #2
quietone CreditAttribution: quietone at Acro Commerce commentedJust adding a tag.
Comment #3
quietone CreditAttribution: quietone at Acro Commerce commentedThis 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.
Comment #4
quietone CreditAttribution: quietone at Acro Commerce commentedtagging
Comment #6
quietone CreditAttribution: quietone at Acro Commerce commentedComment #7
quietone CreditAttribution: quietone as a volunteer commentedNo longer postponed, one of the two issues this was postponed on has been committed.
Comment #8
quietone CreditAttribution: quietone as a volunteer commentedMade a trait for the shared code in prepareRow of the source plugins MenuLinkTranslation and BoxTranslation.
Comment #10
phenaproximaThese 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).
We'll need a method in this trait to get the ID map from the migration; we can't assume $this->idMap is set.
Comment #11
quietone CreditAttribution: quietone as a volunteer commented1. 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()?
Comment #12
heddnI 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.
Comment #13
phenaproximaThis is introducing a dependency on content_translation. Not sure we should be doing that. Maybe we should just put the trait in Migrate Drupal...
Comment #14
heddnOn 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.
Comment #15
phenaproximaCan we mention what the names of the translated properties will be?
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.
Comment #16
quietone CreditAttribution: quietone as a volunteer commented1. 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'?
Comment #17
maxocub CreditAttribution: maxocub as a volunteer commentedNice to see this code in a reusable trait!
Just found a few nits:
Maybe this is out of scope since the original code didn't have it, but I think we should call parent::prepareRow() here.
The exception being thrown is a MigrateException, we should specify it here.
Leftover camelCase variable that should be converted to snake_case.
Comment #18
jofitz CreditAttribution: jofitz at ComputerMinds commentedCorrected the nits.
Comment #19
maxocub CreditAttribution: maxocub as a volunteer commentedThanks @Jo Fitzgerald, just one thing left:
It should be the full class name: \Drupal\migrate\MigrateException
Comment #20
quietone CreditAttribution: quietone as a volunteer commentedFixing the @throws.
Comment #21
maxocub CreditAttribution: maxocub as a volunteer commentedThis now looks ready to me, thanks everyone!
Comment #22
alexpottI've backported this to 8.6.x because multilingual migrations are still not stable.
Fixed coding standards on commit.
Comment #23
alexpottCommitted and pushed e22910b9d9 to 8.7.x and 36aed27964 to 8.6.x. Thanks!