Migrate D7 i18n field option translations

Thanks for working on this. Only found a few things:

1. +++ b/core/modules/config_translation/migrations/d7_field_instance_option_translation.yml
   @@ -0,0 +1,59 @@
   +#class: Drupal\migrate_drupal\Plugin\migrate\FieldMigration
   +#field_plugin_method: alterFieldInstanceMigration
   

   Why did you keep these here?

2. +++ b/core/modules/config_translation/migrations/d7_field_instance_option_translation.yml
   @@ -0,0 +1,59 @@
   +      index: [1]
   ...
   +      index: [0]
   
   +++ b/core/modules/config_translation/migrations/d7_field_option_translation.yml
   @@ -0,0 +1,47 @@
   +      index: [1]
   ...
   +      index: [0]
   

   Is number indexing results the best approach? Are there precedents elsewhere? Should we name them instead?

3. +++ b/core/modules/field/src/Plugin/migrate/source/d7/FieldInstanceOptionTranslation.php
   @@ -0,0 +1,25 @@
   +  public function fields() {
   +    $fields = [
   +      'type_name' => $this->t('Type (article, page, ....)'),
   +    ];
   +    return parent::fields() + $fields;
   +  }
   

   Where does the type_name value actually come from? I am not seeing it in the query.

Looks good to me now. Thanks for the quick update!

+++ b/core/modules/field/src/Plugin/migrate/source/d7/FieldOptionTranslation.php
@@ -0,0 +1,79 @@
+    $query->addField('i18n', 'lid', 'i18n_lid');
+    $query->addField('i18n', 'textgroup');
+    $query->addField('i18n', 'context');
+    $query->addField('i18n', 'objectid');
+    $query->addField('i18n', 'type', 'i18n_type');
+    $query->addField('i18n', 'property');
+    $query->addField('i18n', 'objectindex');
+    $query->addField('i18n', 'format');

I think you can use $query->fields('i18n') here as a shortcut for 'add all the fields form this table'

Same for the 'lt' alias

Other than that, this is over my head 🤔

Looks good to me. #3030937: Ensure language is not Null in translation source queries could either be solved by not null checks or inner join, where inner joins could be made to work. I am fine with either. The rest of the patch I already reviewed before, still looks complete and good.

Committed b4192ba and pushed to 8.8.x. Thanks!

This broke core branch tests in PostgreSQL: https://www.drupal.org/pift-ci-job/1373837

This is usually because of an assertion on the order of items when the order of items wasn't guaranteed in a query. If the order is not guaranteed, then there is no way to assert that the results will be selected in a certain order.

I think this is in an issue in the source plugin, but I can't tell for sure. I'll upload a patch here to see if my hypothesis is correct about the source plugin.

Okay, that looks pretty good. I'm going to re-open as Needs review to confirm the order by column is the correct lid to use.

The fix looks correct, indeterminate sort orders are a common source of issues with postgres (and differences between php versions too.) I'll RTBC the follow up patch for now, and leave it to the committers whether they want to just commit the follow up or revert. If they want to revert we can roll a combined patch to commit later.

Oops! Thanks for catching the postgres regression. I'm going to go ahead and revert this since HEAD is broken (and has been for days, oops -- I missed adding my email notifications for the branch).

After the revert we can combine #25 with the original fix to ensure this works with postgres.

We also should always run any Migrate patch against all three databases prior to commit. :)

Thanks!

Also crediting @mikelutz for the review of the fix.

Hi, I will try to combine the origin patch #21 with patch #25

What we will do is to download both the patches, apply them, and create a new patch. Then we should create an interdiff between the patch in #21 and this new patch, which should confirm that we are only introducing the changes in comment #25.

Edit: this was already outlined by @xjm above in #29 and I missed it. I'm sorry.

I adjusted the issue summary to describe the remaining work and new resolution.

Combined patch #21 and #25, created a new patch and interdiff.

Add PostgreSQL to the issue tag

Fix looks good, RTBC contingent on the last postgres test coming back green.

No need to tag postgreSQL on the issue, it's not really a postgreSQL related issue at all, we just happened to have to make a small adjustment to make sure the tests pass on postgre

Committed ae23684 and pushed to 8.8.x. Thanks!

I don't actually know what field option translations are, but presumably it's a multilingual migrate improvement we might want to list along with the rest. Tagging accordingly. :)