Fix TermTranslation query and add missing source plugin test

Then shall we remove the skip from the migration?

No interdiff because the change is small as is the whole patch.

This looks like a great simplification and hardening at the same time! 🥳

But as a relative outsider, both hunks (the one added by @huzooka and @quietone) are impossible to understand 🙈😅

+++ b/core/modules/taxonomy/src/Plugin/migrate/source/d7/TermTranslation.php
@@ -22,9 +22,16 @@ public function query() {
+    // If there is no 'i18n_mode' column, or it is equal to "0" or "1", then
+    // there is nothing to migrate.

Let's document why 0 or 1.

Refer to the constants at https://git.drupalcode.org/project/i18n/-/blob/7.x-1.x/i18n.module#L15.

@quietone's comment at #3035392-24: Migrate vocabulary translations and taxonomy term references for Drupal 7 node translations also talks about this.

Sibling issue: #3187474: Improve source record count of translation migrate source plugins which use the "i18n_string" table.

Updated comment as per #5.

Thank you!

Wanted to RTBC this, but channeling @alexpott, I think we need to add test coverage for this in \Drupal\Tests\taxonomy\Kernel\Plugin\migrate\source\d7\TermEntityTranslationTest 😅

And you are doing a good job of channeling him!

Turns out there wasn't an existing test for this source plugin, so I added it. I don't know how that happened and I haven't researched it (and don't plan too). The test file you referenced, TermEntityTranslationTest, is for the TermEntityTranslation migration.

Woah, thanks for the super fast turnaround! I think @huzooka is better positioned to review this than I am 🤓

Changing to a bug. The query is incorrect and the test is missing.

Retesting with 9.3.x.

Tagging for Drupal South. This is for someone who has worked with migrations.

Wim Leers already thinks this can be RTBC but recommends another set of eyes on the work.

Fixing the tag.

Now that it's classified as a bug, we need to show that the new test fails without the fix. I ran the new test locally without the other changes and got a wonderful failure: Failed asserting that actual size 7 matches expected size 4. That's exactly what the issue is about, so that's good news.

I have a question about this though.

+++ b/core/modules/taxonomy/src/Plugin/migrate/source/d7/TermTranslation.php
@@ -25,9 +25,16 @@ class TermTranslation extends Term {
+    else {
+      $query->condition('td.tid', '0');
+    }

I just want to confirm that I understand this condition. If the 'i18n_mode' field does not exist, then we're not translating terms. Therefore we want the query to return zero rows. To do that, we put on a condition that will never be met, since there will never be a term with a tid of zero.

Is that right? Could there maybe be a comment added to clarify that?

Or better yet, if I'm understanding the purpose of that condition, it looks like we could use ConditionInterface::alwaysFalse() instead. But I may be misunderstanding things!

@danflanagan8, thank you for the review.

Yes, you are understanding the query correctly. I have changed to using alwaysFalse, which I think is much clearer. I have also added an sentence to the comment at the start of the if block to help explain what is happening. And I have added a test for the situation where there is not an i18n_node column in the taxonomy_vocabulary table.

@quietone, I like the new test case! And thanks for making to other changes to make things clearer.

Woah, very nice indeed, @quietone!

Committed and pushed 4bc7dbb8f9 to 9.3.x and 88bb7758ba to 9.2.x. Thanks!