Migrate content type language settings from Drupal 6 & 7

I'm unassigning me from this issu beacuse I can't work on this issue at the moment.

Work in progress...

New patch. Includes custom destination and the migration itself. Migration tests looks good, but needs to be expanded.

Most of the variables we had are not longer necessary because those features are not there (they don't make sense anymore, or they would be at contrib). Not sure if those should be documented on the source (as they are now) or moved to the migration yml.

I just found source test coverage is insufficient too, I thought I had this already covered.

Sorry, no interdiff.

Patch attached has a POC for the use of a Load plugin instead. It's just a fork of LoadEntity and would need some clean up. Posting here for discussion.

Postponed on #2355909: language.settings config is not scalable

I should come back to this one.

Blocked by #2594263: Add translation data to Migrate Drupal's test fixtures.

OK, this is unblocked :) But it's a year old so it positively aches for a re-roll...

Re-blocked by #2596793: Migrate legacy languages to configurable language entites.

Unblocked

This functionality has changed quite a bit since the last patch.
Attached is a new patch based on the work by penyaskito & benjy.

It migrates the D6 content type language settings that are appropriate in D8.
Still needs test, and a check to see if this can be generalised for D7 as well.

Patch attached that migrates from D6 and D7.
Still needs tests.

A test for D7 only, with a schema error. How to fix that?

Also, penyaskito, in #9, talked about where to put comments about the variables that are no longer necessary. I've tried to put that in the yml template. It seems the best place to look to get an overview of what the migration will do.

Don't know what happened but I needed to upload the patch and interdiff again.

I know it is late but the D6 migration looks wrong to me. The D6 migration template is essentially the same as the D7 one. But the D6 i18n variables are different and they are using the same source plugin, which is specific to D7.

1. +++ b/core/modules/language/src/Plugin/migrate/source/LanguageContentSettings.php
   @@ -0,0 +1,73 @@
   +    if ($this->getModuleSchemaVersion('system') >= 7000) {
   

   Seems to be for D7 not D6.

2. +++ b/core/modules/language/src/Plugin/migrate/source/LanguageContentSettings.php
   @@ -0,0 +1,73 @@
   +      $i18n_node_options = $this->variableGet('i18n_node_options_' . $type, NULL);
   

   D6 doesn't have a variable i18n_node_options_

3. +++ b/core/modules/language/src/Plugin/migrate/source/LanguageContentSettings.php
   @@ -0,0 +1,73 @@
   +        $row->setSourceProperty('i18n_lock_node', 1);
   

   The lock variable for D6 is i18n_lock_node_$type

For a bit of clarity the D6 and D7 versions are separated in this patch.

So there are now tests for D6 and D7 that are passing.

Rerolled #26. 3-way merge worked automatically, let's see the results.

MigrateDrupalNTestBase were moved, so need to update the use statement.

These are green locally.

@penyaskito, thanks.

Made two changes:

• Changed the two executeMigration to a single executeMigrations d6/MigrateLanguageContentSettingsTest.php.
• Moved the two migration tests to the Kernel namespace, Drupal\Tests\language\Kernel\Migrate;

Some preliminary notes on review:

For the properties that we're setting, we need to verify that they do the right things
* default_langcode: current_interface?
* language_alterable meaning?
* Is content_translation/enabled really tri-valued? NULL/TRUE/FALSE
* What about the fields themselves, are they marked translatable?

Tests need to check that non-translatable types have correct settings, too
We do a test for system schema >= 7000 in the d7 source, shouldn't that be always true?
Should we separate or coalesce d6/d7?

I did a brief review of the fields we're setting on the language_content_settings config entity:

• default_langcode: We're setting this to 'current_interface' for translated content. Is there any reason for it to be 'current_interface', instead of the default 'site_default'?
• language_alterable: This being false has slightly different semantics than i18n_lock_node_TYPE. The former means you can't choose the language when you create a node; the latter means you can choose it, but not edit it later and change your mind. I think it's close enough.
• third_party_settings/content_translation/enabled: This looks good. NULL is not a real valid value, but in that case (language_content_type_TYPE == 0) the row will be skipped anyhow. I'd like there to be a comment that the value isn't used, so it's clear that we're not really setting that to NULL.

Also, I actually tried to do a migration with this patch, and it worked well! The fields are indeed marked as translatable, since that's the default. So the remaining issues for me are:

• Justify the setting for default_langcode.
• Add a comment for third_party_settings/content_translation/enabled.
• Remove the D6-or-D7 check from the source d7_language_content_settings.
• Add an assertion that language_alterable is TRUE when there's no i18n_lock_node_TYPE.
• Add an assertion that a content type that is not translatable generates no language_content_settings config entity.

@vasi:

default_langcode: We're setting this to 'current_interface' for translated content. Is there any reason for it to be 'current_interface', instead of the default 'site_default'?

We want that content to be in the language negotiated by the page. Using 'site_default' would mean using the default language of the site.

Add a comment for third_party_settings/content_translation/enabled.

Added

Remove the D6-or-D7 check from the source d7_language_content_settings.

Done

Add an assertion that a content type that is not translatable generates no language_content_settings config entity.

We are using all the types as a source, so we would need to modify the patch for that. I'm not sure if it's worth it. I added assertions for ensuring that they have the default configuration instead.

TBD:

Add an assertion that language_alterable is TRUE when there's no i18n_lock_node_TYPE.

NR just for the tests, should be back to NW afterwards.

Tests are green, next steps:

Add an assertion that language_alterable is TRUE when there's no i18n_lock_node_TYPE.

Are there any types already that meets those conditions in d6/d7 dumps?

We are using all the types as a source, so we would need to modify the patch for that. I'm not sure if it's worth it.

But we are using skip_on_empty to skip the ones that are not actually translatable. No patch modifications should be needed.

Are there any types already that meets those conditions in d6/d7 dumps?

I think there's none, you're right. It would be nice if language_alterable = TRUE was tested, but it's probably not worth changing the dumps.

Added the dump modification so we can test that, and added the new test. For that I had to fix a bug in the source, so thanks for asking for that test ;-)

Tagging with the DrupalNorth sprint, sorry for the noise.

phpcs complained about a couple of things, fixed those.

Looks good!

Committed/pushed to 8.2.x and cherry-picked to 8.1.x. Thanks!