Variable to config: language_default [D6 & D7]

+++ b/core/modules/language/src/Tests/Migrate/d6/MigrateDefaultLanguageTest.php
@@ -0,0 +1,31 @@
+    $this->assertIdentical('en', $this->config('system.site')->get('default_langcode'));

Hmmm... but this is the default value - how can we be sure that the migration has worked? Also I have no idea how the migration knows to put the value in the default_langcode config key.

Thanks for the patch! Some feedback:

1. +++ b/core/modules/language/src/Tests/Migrate/d6/MigrateDefaultLanguageTest.php
   @@ -0,0 +1,39 @@
   + * @group migrate_drupal_6
   

   Should there be comments here?

2. +++ b/core/modules/language/src/Tests/Migrate/d6/MigrateDefaultLanguageTest.php
   @@ -0,0 +1,39 @@
   +    // The fixture is set to English as the default. Update it for this test.
   

   This comment isn't very clear to me. Perhaps change to something like:

   The test system defaults to English, so update the default language variable to French so it can be checked.

   Though even what I wrote isn't that clear to me. :/

Yes, that is better but needs to be wrapped to fit in 80 characters. Thanks!

Looks good. Thanks! What's the best way to test this?

Looks great to me :) No problems found on a code review, test seems complete.

So what guarantees that the default language actually exists on the target site? I.e. as a configurable language.

@alexpott: I don't believe we checked that either in #2225775: Migrate Drupal 6 core node translation to Drupal 8 for each node. It can certainly be checked, but then it needs its own variable plugin extension class I guess.

I'm not sure I understand why we should check if the language exists. There seems to be already a migration for the languages, so if on the D6 site, French for example was the default language, then French would be migrated and then made the default language.

Since the languages are migrated, I don't understand why we should look if it exists.

This is the first time I spend some time on a migrate issue, so maybe there something that I don't see.

I tested the patch by following @quietone comment #13 (which doesn't ask to add the language on the D8 site before migrating) and the D6 default language is added to the D8 site and marked as default as expected.

It also seems to work if the default language on D6 was a custom language, not in the language list. The custom language is added to the D8 site and marked as default.

@maxocub: I think it should be checked/tested because the default language on the D6 site may not exist on that D6 site. It may cause more problems in D8 although it was probably not a simple problem in D6 either. Let's see how we can test it:

+++ b/core/modules/language/src/Tests/Migrate/d6/MigrateDefaultLanguageTest.php
@@ -0,0 +1,43 @@
+  public function testMigration() {
+    // The default language of the test fixture is English. Change it to French
+    // before migrating, to be sure that the source site default language is
+    // migrated.
+    $value = 'O:8:"stdClass":11:{s:8:"language";s:2:"fr";s:4:"name";s:6:"French";s:6:"native";s:6:"French";s:9:"direction";s:1:"0";s:7:"enabled";i:1;s:7:"plurals";s:1:"0";s:7:"formula";s:0:"";s:6:"domain";s:0:"";s:6:"prefix";s:0:"";s:6:"weight";s:1:"0";s:10:"javascript";s:0:"";}';
+    $this->sourceDatabase->update('variable')
+      ->fields(array(
+        'value' => $value
+      ))
+      ->condition('name', 'language_default' )
+      ->execute();
+    $this->executeMigration('d6_default_language');
+    $this->assertIdentical('fr', $this->config('system.site')->get('default_langcode'));
+  }

So if you are saying French gets created even if not in the language list, we can add a line of test here to ensure the config has French among the languages too, eg. Language::load('fr') returns a language.

(If that is because the source data dump had French, we should pick a language other than French here to test with or better yet test first with French and then with some other language that is not in the database dump second -- or in another test method).

@maxocub: I think this is something you can do it should not require migrate know-how only tests need to be modified for now :)

How could a default language not exists on a D6 site? By modifying the DB directly, or with drush?

I just tested it manually by modifying the D6 DB. The D8 configuration for the default language is changed to the D6 one (I checked it at admin/config/development/configuration/single/export), but since the language itself doesn't exists, the interface (at admin/config/regional/language) shows English as default.

Now, I'll add a method to the test class.

Here's an updated test.
It will fail since it tests the migration with a non-existent language.
I had to trigger the language migration otherwise even the French didn't exist.

Why is i18n-migrate in the tags list? Does this issue have something to do with the i18n module?

This is to get rid of the second 'unrelated' fail.

It's late, can't put things in alphabetical order...

@maxocub: sounds like that exposes the problem that @alexpott was aiming at. Not sure what migrate should do in this case though. The language may not exist due to the language migration not running earlier as you noted. @alexpott what do you think migrate should do here?

Nice work reproducing the issue @maxocub! I think this migration should depend on the language migration and if the language still doesn't exist it needs to report an error and not migrate.

Here's what I came up with so far. Any comments?
Now I've got to find out how to update the test to check if the migration fails if the language doesn't exists.

@maxocub: I also brought this up at the migrate meeting today so hopefully it will get some reviews that way too.

+++ b/core/modules/language/migration_templates/d6_default_language.yml
@@ -0,0 +1,23 @@
+migration_dependencies:
+  required:
+    - language

@quietone: re your final note, this in the patch should already ensure languages happen earlier, so no docs updates required?

BTW good point on the source checking the destination going over the wall of separation of concerns. Looks like the general direction is that the destination plugin should be extended from the config plugin, while the source plugin remains as-is.

@quietone: well, we could explicitly say people don't need to do anything about it, because the migration takes care of that, but is that something we document? "Don't do anything with your languages because they will get migrated"?

The languages do get migrated. What we're doing here is checking for an edge case where a D6 default language does not exist on the D6 site, so then it does not get migrated. Maybe we can say something about that? Ask that people verify that their D6 default language exists on their source site?

Thanks @quietone for you review! I'll move the verification to a destination plugin.

And about your sixth point, yes, it does look like it's the same for D7, so I guess I should also change the file d6_default_language.yml to default_language.yml

Throwing a MigrateException in a destination plugin doesn't seem to mark the migration as failed. The status message tells me that all migrations were succesfull while in the log I see the thrown exception.

Can someone tell me why it worked in the source plugin but not in the destination plugin?

@quietone: I used a destination plugin as you suggested and I made the changes to cover D7 too.

I have two questions about the tests:

1) Since the two tests are almost identical, I thought about making a new class to reduce code duplication, but since they each extend their own base class for D6 and D7, I haven't found how we could do that. So is it OK to leave it as is?

2) I haven't found how we could test that the migration fails if the language doesn't exist. Is there another test in core that does that?

@quietone: well, that would be out of scope for this issue. I also posted that feedback to @alexpott in #18 that we don't check the existence of the language used in node translation migrations either. That said, a nonexistent language there may be a smaller problem compared to here due to the default language being on the container.

@quietone: The test needs to be updated. It needs to check if the migration fails if the language does not exists on the destination. That's where I'm at, and I haven't found yet how to test that. I tried with $this->startCollectingMessages(), but the array $this->migrateMessages is always empty. I'm working on it.

Here's an updated test.

I used the migration messages to check if there's an error due to the non-existing language and I use a trait to avoid code duplication between the D6 and D7 tests. I don't know if there's a better way to accomplish that.

This looks to be resolving all the concerns @alexpott had and nicely tests the situation on both D6 and D7 with both an existing and missing language. Looks fine to me :)

As a non-disruptive addition to an alpha experimental module, this issue is rc eligible per: https://www.drupal.org/core/d8-allowed-changes#rc

Marked #2751223: D6 & D7 users are migrated into D8 with incorrect langcode postponed on this one since that needs this for error resolution.

+++ b/core/modules/language/tests/src/Kernel/Migrate/d6/MigrateDefaultLanguageTest.php
@@ -0,0 +1,38 @@
+  /**
+   * Tests the migration of the language_default variable to the
+   * system.site.default_langcode config key, with an existing language.
+   */

Very minor but these should be one-line. I'm going to change on commit to 'Tests language_default migration with an existing language' (and etc.)

Failed to spot the 'credit should go to phenaproxima' note before committing, but added issue credit...

Thanks!

Removing from sprint, thanks all again!