Mark i18n migrations as finished

@Wim Leers, thanks for opening this issue. Unfortunately, the Meta was closed before all the child issues were committed and I didn't remember that there were todos referring to it in core. This needs to wait on at least, #3008028: Migrate D7 i18n menu links.

Sorry, meant that to be just 'postponed'

Let's expand this to update the D6 states as well. And simplified by the title.

Ignore that patch.

Adjusting the tests for the updated states.

I want to review this again.

Not sure how this fits the definition of 'finished' for i18n, but there is #3175477: Migration destination for configuration entity translations is opinionated towards i18n and can only migrate a single property at a time

Needed a reroll.

This is ready for review now.

@idebr: "Complete" does not mean "completely bug free".

1. Why remove i18nstrings?

    +++ b/core/modules/config_translation/migrations/state/config_translation.migrate_drupal.yml
    @@ -1,28 +1,15 @@
    ...
     finished:
    ...
    -    i18nstrings: config_translation

2. Does locale: language belong in this file?

       6:
         locale: language
       7:
    -    locale: language
    +    locale: language

Hi,

Creating a patch as suggested in #18 comment.

Please review the patch.

Thanks.

Patch failed to apply.

In #18, I asked questions. I do not know the answers to those questions. We should not update the patch until the questions are answered.

From 18
1. Very good question. I think it was a simple mistake but it got me looking further. There is a migrate_drupal state file which had i18n entries for d6. Those are now moved to config_translation or content_translation and i18nstrings is restored. That cleans up the migrate_drupal state file to contain a variety of legacy modules that are now in core. It is a nice cleanup.
2. Those lines make no sense. It is supposed to be 'legacy module: destination module' and there is no language module. Those can be deleted.

Point 2, raises the question about what to do when the destination module is not enabled on the destination site. Typically, the destination module provides the migrations but that is not always the case. Or, it could just be a typo in the state file. In any case, the UI checks the state files and plugins etc but there is no check that the destination module is actually enabled. So, the UI could show that it will be upgraded when it will not be. Sigh. Probably needs a follow up for to consider that, adding tag.

But there is a language module, and language.migrate_drupal.yml includes

finished:
  6:
    ...
    locale:
      - language
      - system
    ...
  7:
    ...
    locale:
      - language
      - system
    ...

I would say to move the lines there from config_translation.migrate_drupal.yml, but they are already in the right place so we just have to remove them from the wrong place.

Or maybe the right thing to do is change the lines so that config_translation.migrate_drupal.yml has locale: config_translation.

Since the previous patch, you have moved four lines i18n...: content_translation from the migrate_drupal module to the content_translation module. That makes sense. But you also added i18n_string: config_translation there. That looks like a mistake. That line is already in the file for the config_translation module (D7 only).

Grr! I need another holiday!

I do not know what planet I was visiting when I last worked on this. :-(

First, this needs a reroll.

Ok, now restore the lines in config_translation.migrate_drupal.yml. locale: language. For background that was added in #2936365: Migrate UI - allow modules to declare the state of their migrations. And fix the copy/paste error benjifisher found and pointed out in the last paragraph of #25.

This should now be back on track.

Not sure how I lost this one line change.

@quietone:

Have a cup of tea and upload the patch along with the interdiff for #30. ;)

Lol, you are soo right!

Here is the patch for #30.

I am no longer convinced that a follow up is needed as I was thinking in #24. MigationState does build the migration so all the checkRequirements are done so all should be well. Also, did a test with a destination module with a typo,

finished:
    help: block2

Results in help being listed in the will not be upgraded with a destination module of block2. That is sufficient to alert someone that block2 does not exist.

Removing needs follow up tag.

The patch in #32 (labeled for #30) looks good to me. Quick, commit this before it needs another reroll!

Committed/pushed to 9.2.x, thanks!

This doesn't cherry-pick cleanly to 9.1.x - I'm not sure if the intention here was to backport it to 9.1.x or not (or whether we should), so marking fixed - but if it should go back then please re-open with a re-roll.

Back in #3, this was postponed on #3008028: Migrate D7 i18n menu links. That was committed to 9.1.x so, yes, this can be committed to 9.1.x as well. The question remains, should we do that? What are the reasons not to backport? I can't think of any.

Here is a 9.1 patch in case there is agreement to backport.

Also can't see a reason not to backport. Changing status and kicking off a test run.

I think this gets backport added to the title

Committed ee8c012 and pushed to 9.1.x. Thanks!

@catch, thank you.

Removing [backport] from the title.