Migrate D6 comment type language settings

Add tag

This should fulfill the solution in the proposed resolution.

Update the entity counts for MigrateUpgrade6Test.

Tests pass and there are no coding standard errors, so setting NR

This migration is nearly the same as d6_language_content_settings.yml. What is changed is the source of language_alterable, from i18n_lock_node to language_content_type.

+++ b/core/modules/language/migrations/d6_language_content_comment_settings.yml
@@ -0,0 +1,63 @@
+      3: true

This line can be remove

Is this NW for #11?

Remove the unnecessary line, from #11.

Good work, just a few nits:

1. +++ b/core/modules/language/tests/src/Kernel/Migrate/d6/MigrateLanguageContentCommentSettingsTest.php
   @@ -0,0 +1,78 @@
   + * Tests migration of language content comment setting variables.
   

   I don't see any variables being migrated, maybe we should just say "Tests migration of language content comment settings."?

2. +++ b/core/modules/language/tests/src/Kernel/Migrate/d6/MigrateLanguageContentCommentSettingsTest.php
   @@ -0,0 +1,78 @@
   +  public function testLanguageContent() {
   

   This method should be rename to something more specific.

3. +++ b/core/modules/language/tests/src/Kernel/Migrate/d6/MigrateLanguageContentCommentSettingsTest.php
   @@ -0,0 +1,78 @@
   +    $config = ContentLanguageSettings::loadByEntityTypeBundle('comment', 'comment_node_article');
   +    $this->assertSame('comment', $config->get('target_entity_type_id'));
   +    $this->assertSame('comment_node_article', $config->get('target_bundle'));
   +    $this->assertSame('current_interface', $config->get('default_langcode'));
   +    $third_party_settings = [
   +      'content_translation' => [
   +        'enabled' => TRUE,
   +      ],
   +    ];
   +    $this->assertSame($third_party_settings, $config->get('third_party_settings'));
   +
   +    $config = ContentLanguageSettings::loadByEntityTypeBundle('comment', 'comment_node_employee');
   +    $this->assertSame('comment', $config->get('target_entity_type_id'));
   +    $this->assertSame('comment_node_employee', $config->get('target_bundle'));
   +    $this->assertSame('current_interface', $config->get('default_langcode'));
   +    $this->assertSame($third_party_settings, $config->get('third_party_settings'));
   

   We test the same thing twice. Could we test the other language setting?

4. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d6/MigrateUpgrade6Test.php
   @@ -72,8 +72,8 @@ protected function getEntityCounts() {
   -      'migration' => 105,
   ...
   +      'migration' => 106,
   

   I never saw this 'migration' count. I often added migrations without having to update this count. I put it back to 105 and ran the test and it passed. I think this line is not necessary.

Needs a reroll

1. Fixed
2. Fixed
3. Fixed. Thanks I did miss that! Added tests for isLanguageAlterable(). Also changes the tests to use any getters that exist.
4. Interesting. I don't see how it would pass the test when that is changed. If that needs to be removed then it should be a separate issue as it is also in the D7 tests. I'm uploading a test only patch with migrations = 105 instead of 106. Let's see what testbot does with that.

Oh and I ran a full migration and checked that the criteria of the proposed resolution are met.

Darn, when I am sick I make even more silly errors. Patch 2981393-16.patch and 2981393-16-test-only.patch are identical and they should not have been but there it is. So, let's just ignore Comment #16 and move on.

And uploading the correct patch so there is no confusion.

14-1. Fixed
14-2. Fixed
14-3. Fixed. Thanks I did miss that! Added tests for isLanguageAlterable(). Also changes the tests to use any getters that exist.
14-4. Interesting. Created a new issue, since this will affect both d6 and d7, to look into why the migration count changes don't cause test failures, #3009137: Migration entity count not tested in MigrateUpgrade Tests

Oh and I ran a full migration and checked that the criteria of the proposed resolution are met.

@masipila, Thanks! I see you've setup your own d6 an d7 version for testing. Would it be possible for you to add those cases to the d6 and d7 fixtures, if they are not already there?

This patch is only the change to d6_dump suggested in 23 above (changing Migrate test story) content type. I want to see what, if any, tests fail.

Since that failure is on a test of an absent body field, let's try another content type, this time, Sponsor.

Making the changes to the D7 database via the UI are proving impossible. More details on that later.

That is better. Now add back in the patch from #18 plus a fix for the MigrateUpgrade6Test

This changes the content type, Sponsor, to have a multilingual setting of 'Enabled' and updates the test.

I can't get the MigrateUpgrade test to run locally today. Here is an update for the entity count.

Also need to update the count in the Incremental as well.

Triaging the issue queue.

Here is the reroll.

Re the question in #36 about making a follow up for D7. The answer is that I am still on the fence about that. I want to have another look at what is required for D7 and I won't be able to do that for about two days.

Settings NR for the testbot.

Did some more testing and had to make some changes to the migration so the result matches what is in the IS. Updated the IS to identify the Drupal 6 content types associated with each language content settings to help testers.

Silly, forgot to update the test.

Settings to NW to decide about adding the D7 migration here or to do that in a new issue.

This is to test what breaks when changing the Drupal 7 basic page from multilingual Disabled to Enable. I tried to do this with the UI but kept getting an error message "An illegal choice has been detected" so I changed the variable directly. Not sure what problems that may cause.

Not bad, only 4 errors and after a look it appears they can be corrected and not lose any tests. So, I went ahead and added a migration and test for Drupal7. The test passes but I have not tested it from the UI to be sure nor have I fixed the errors reported in #43.

Good, the same tests fail and no new failures. This does look like it will work.

Review again and need to revert changes to Drupal 6 version as I misread the IS. First, lets run the tests on this updated version and then attend to the errors.

Now, work on fixes the failures. This has fixes for two tests plus removes unnecessary changes from the fixture which may also fix the others.

This still needs a migration for this case:
4. If the Drupal 7 content type has multilingual support as 'Enabled, with field translation'.
Drupal 7 fixture: test_content_type.

Ah a new day and I see I got the number wrong on the patch. Uploading the same patch with correct number of this comment (52) to avoid confusion.

Now add in the final migration,
4. If the Drupal 7 content type has multilingual support as 'Enabled, with field translation'.
Drupal 7 fixture: test_content_type.

Assuming this passes tests, it will be ready for review. I do wonder if the new process plugin, content_translation_enabled_setting, show be more generic and handle the other entity type that are translatable with entity translation.

Decided to change the new process plugin to be more flexible. This moves the static map from the migration into the process plugin, but I think I like this better.

This is ready for review

Needs reroll

Phew, that didn't involve editing a fixture, just MigrateUpgrade6Test.php.

Making a few things bold in the IS to make it a bit easier to find.

Right, couple of things.

1) I am not getting the failures that masipila reports. I was working in D7 and using the fixture all was well. I went to add a new content type to the fixture and get this error in the UI 'An illegal choice has been detected. Please contact the site administrator.'. The D7 fixture is just too wonky to work with. That needs to be fixed before any more work on D7 migrations and that needs an issue.

2) This issue will be split into a D6 and D7 version because of 1)

3) This patch removes the D7 code and fixes the test, unless I messed things up.

So the work remaining here, from #57, is to fix this

3. D6 Content type with multilingual settings 'enabled, with translations' (Article in D6 fixture): TEST FAILED, details below.

Details for 3:

When the D6 content type setting is 'enabled, with translations', the D8 content type setting 'Enable translation' should be false.
This is because the D6 comments are **not** conceptually translations of each other.

Updated the IS to remove references to Drupal 7

I'm unable to reproduce the error results reported in #57 for 'enabled, with translation'

After the migration both article and employee have the following configuration showing the content_translation is indeed false.

 drush cget language.content_settings.comment.comment_node_article
uuid: 86cbdd23-a666-44db-8105-d93aed364a93
langcode: en
status: true
dependencies:
  config:
    - comment.type.comment_node_article
  module:
    - content_translation
third_party_settings:
  content_translation:
    enabled: false
id: comment.comment_node_article
target_entity_type_id: comment
target_bundle: comment_node_article
default_langcode: current_interface
language_alterable: true

I made another content type and set it to 'enabled, with translation' and got the same results.

@masipila, where you using your own test db? Can you provide more details? How can I reproduce the error?

This needs testing from someone else to confirm, or not, the failure reported by masipila.

Let's tag for manual testing since we can't seem to reproduce what Markus found.

OK, rerolled.

Woohoo! Thanks Markus!

Fixes for the two items in #70.

And thanks to you too Markus!

In #72 Markus said this can be RTBC when all tests are passing, they are so I will set it to RTBC.

+++ b/core/modules/language/migrations/d6_language_content_comment_settings.yml
@@ -0,0 +1,62 @@
+migration_tags:
+  - Drupal 6
+  - Configuration
...
+migration_dependencies:
+  required:
+    - d6_comment_type

Hm, how is this limited to only multilingual sites? Eg. it attempts to set a content translation 3rd party setting but is apparently not making content translations a requirement?

Also should it have the Multilingual tag on it?

I looked a bit further and the only language related plugin used, d6_language_content_settings seems to be dependent on locale only. How does this depend on content translation needing to be on the target system?

@Gábor Hojtsy, nice question. I done a little digging and this is what I have found so far.

Content Translation has a listener for the MigrateImportEvent which updates the field storage definitions for the entity type specified in the migrate destination configuration (using property content_translation_update_definitions). This was introduced in #2225775: Migrate Drupal 6 core node translation to Drupal 8 the same patch that brought in d6_language_content_settings.yml. Obviously, if content translation is not enabled the updates will not happen.

I wanted to see what happens when content_translation is not enabled so I commented content_translation out of MigrateLanguageContentCommentSettingsTest and ran it. That results in results in schema errors.

1) Drupal\Tests\language\Kernel\Migrate\d6\MigrateLanguageContentCommentSettingsTest::testLanguageCommentSettings
Schema errors for language.content_settings.comment.comment_node_article with the following errors: language.content_settings.comment.comment_node_article:third_party_settings.content_translation missing schema (/opt/sites/d8/core/lib/Drupal/Core/Config/Development/ConfigSchemaChecker.php:95)

Perhaps this is what vasi is referring to in Issue #2225775- Comment #198 because the next patch in #199 is the first one with the listener. If that is true, there is a dependency on content translation. And even though the source plugin has a source_module of 'locale', it uses i18n variables as well. All indicators that the existing family of '_language_content_settings' migrations are Multilingual and should be moved to content_translations/migrations.

It is now I wish I had more experience with D6/D7 sites. Does this make sense?

TODO:
Another thing to do is to document the destination property, content_translation_update_definitions.

There is definitely a dependency on content translation if we need to set third party settings for content translation :) That would not be valid in configuration unless content translation module is present. Default langcode and language alterable would be settable / migrateable without content translation, but I don't know if there is a way to make the final part only conditional on content translation.

Default langcode and language alterable would be settable / migrateable without content translation, but I don't know if there is a way to make the final part only conditional on content translation

My first reaction is that no, there isn't a way and that two migrations are needed. Ideally, one that does the parts not requiring content translation and one that does that part requiring content translation. This requires more thought than I can give right now.

Based on the source essentially requiring content translation features for these to be there, even though D8 is more flexible and allow part of the features to be present without content translation, this could indeed just expect content translation IMHO. I don't expect that being a problem. IMHO splitting this would be more like hair splitting.

What if we move the migration? The only implication is we'd need to add a hook update to language.module that if migrate_drupal_multilingual is enabled, then enable content_translation module.

We wouldn't be able to easily change the name of the migration.yml, because the name is part of the API and that would be bad. But we can move it. We cannot rename it because the mapping tables and migrate_drupal_ui has the old names already saved. But we can safely move it.

Moving the new migration to content_translation.

Do we really need to add a hook_update for a new migration?

No need for an update if this is a new migration. Ignore that part.

+++ b/core/modules/content_translation/migrations/d6_language_content_comment_settings.yml
@@ -0,0 +1,63 @@
+# Ignore i18n_node_options_[node_type] options not available in Drupal 8,
+# i18n_required_node and i18n_newnode_current

I was about to mark this RTBCed since all other feedback is now addressed. But then I noticed this comment in the yml and said, what does this mean? It don't seem to relate to target_bundle. What is it all about? Can we clarify this comment?

Yes, that is cryptic. It is from d6_language_content_settings.yml which this one started from and it applies to the whole migration. And worse I added it #2225271: Migrate content type language settings from Drupal 6 & 7 Comment #26 and now think it is unhelpful. Not only is there the confusion you experienced, thinking it had to do with the property it is directly above but it doesn't really give any useful information. Comments in the migrations tend to focus on the variables that are used in a migration, not the ones that are ignored.

It is a difficult call to remove a comment but in this case it should go. If we wish to comment about the i18 variables that are ignored then that should be it's own issue and it should cover all the relevant migrations. Also these variables are not retrieved by the source plugin so maybe not worth the effort. Unless, that is, we want to modify all the relevant source plugins to get more variables even though they are not used in core.

This patch removes the comment.

We be good now.

I think a random failure.

Thanks for resolving my concerns! Woot! Committed.