Migrate variable language_default to system.site config.

This was originally part of #2225775: Migrate Drupal 6 core node translation to Drupal 8. It is moved here to reduce the scope of that issue. And it conforms to the existing practice of a separate issue for each D6 or D7 variable to config migration.

Credit should go to phenaproxima.

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

quietone created an issue. See original summary.

quietone’s picture

Status: Active » Needs review
FileSize
1.67 KB

And the patch.

quietone’s picture

Status: Needs review » Reviewed & tested by the community

Looks good.

Committers: All credit to phenaproxima.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ 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.

quietone’s picture

Status: Needs work » Needs review
FileSize
2.21 KB
1.66 KB

The migration template now has the correct input to the extract plugin. And the test now checks for French instead of English.

Gábor Hojtsy’s picture

Issue tags: +D8MI, +language-base, +sprint
quietone’s picture

Issue tags: +migrate-d6-d8
Kristen Pol’s picture

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. :/

quietone’s picture

1. Yes, fixed.
2. Is this better?

Thx

Kristen Pol’s picture

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

quietone’s picture

Sorry, IDE had wrong margin (and I didn't notice).

Kristen Pol’s picture

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

quietone’s picture

Thx. To test this will work,
On a D6 source site,

  • enable language module,
  • add at least one other language, /admin/settings/language/add
  • select a non English language as the default, /admin/settings/language

On a D8 source site,

  • use a fresh install.
  • Enable language module

Run the migration. Upgrading from Drupal 6 or 7 to Drupal 8 has all the details.

When the migration is finished, go to /admin/config/regional/language. The default language on D8 will now match the default language set on the D6 source site.

On the topic of testing, migration has an existing issue to Ensure migration tests are meaningful.

Hope that helps.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Status: Needs review » Needs work

The last submitted patch, 11: 2657978-11.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Reviewed & tested by the community

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

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

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

Gábor Hojtsy’s picture

Status: Needs review » Needs work

@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.

maxocub’s picture

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.

maxocub’s picture

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.

Gábor Hojtsy’s picture

@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 :)

maxocub’s picture

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

maxocub’s picture

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.

maxocub’s picture

Status: Needs work » Needs review
FileSize
3.04 KB
3.08 KB

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?

Status: Needs review » Needs work

The last submitted patch, 24: 2657978-24.patch, failed testing.

maxocub’s picture

Status: Needs work » Needs review
FileSize
3.72 KB
692 bytes

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

maxocub’s picture

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

The last submitted patch, 26: 2657978-26.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 27: 2657978-27.patch, failed testing.

Gábor Hojtsy’s picture

@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?

alexpott’s picture

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.

maxocub’s picture

Status: Needs work » Needs review
FileSize
6.32 KB
4.11 KB

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.

Status: Needs review » Needs work

The last submitted patch, 32: 2657978-32.patch, failed testing.

maxocub’s picture

Status: Needs work » Needs review
maxocub’s picture

Gábor Hojtsy’s picture

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

quietone’s picture

@maxocub, thx for moving this along. I'm a novice at reviews, but here goes.

  1. +++ b/core/modules/language/src/Plugin/migrate/source/DefaultLanguage.php
    @@ -0,0 +1,83 @@
    +use Drupal\Core\Entity\EntityManagerInterface;
    +use Drupal\Core\State\StateInterface;
    

    Not needed

  2. +++ b/core/modules/language/src/Plugin/migrate/source/DefaultLanguage.php
    @@ -0,0 +1,83 @@
    +  public function __construct(array $configuration, $plugin_id, $plugin_definition, MigrationInterface $migration, StateInterface $state, EntityManagerInterface $entity_manager) {
    

    The constructor can be removed.

  3. +++ b/core/modules/language/src/Plugin/migrate/source/DefaultLanguage.php
    @@ -0,0 +1,83 @@
    +    // Check if the language exists, fail otherwise.
    +    if (ConfigurableLanguage::load($language_default->language) === NULL) {
    +      throw new MigrateException("The language $language_default->language does not exists on this site.");
    +    }
    

    Should we really be looking at the destination in a source plugin? Ideally, the source just gets the data. Making a decision based on the destination site configuration probably should be done in a destination plugin. There certainly aren't exceptions in other source plugins. And s/exists/exist.
    Can someone else comment on this?

  4. +++ b/core/modules/language/src/Tests/Migrate/d6/MigrateDefaultLanguageTest.php
    @@ -0,0 +1,61 @@
    +/**
    + * @file
    + * Contains \Drupal\language\Tests\Migrate\d6\MigrateDefaultLanguageTest.
    + */
    

    Can be removed.

  5. +++ b/core/modules/language/src/Tests/Migrate/d6/MigrateDefaultLanguageTest.php
    @@ -0,0 +1,61 @@
    +namespace Drupal\language\Tests\Migrate\d6;
    

    Namespace for the tests should be Drupal\Tests\language\Kernel\Migrate\d6;

  6. 
    +++ b/core/modules/language/src/Plugin/migrate/source/DefaultLanguage.php
    
  7. If this is the same for d6 and d7, then OK. Otherwise it needs to be in a subdir. If for both, then a test for D7 needs to be added.

And a final note that the docs,https://www.drupal.org/docs/8/upgrade/preparing-an-upgrade, should be updated to state that languages need to be installed on the D8 site before migrating.

Gábor Hojtsy’s picture

+++ 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?

Gábor Hojtsy’s picture

Status: Needs review » Needs work

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’s picture

@Gábor Hojtsy, I thought it was said at the meeting that the docs should be updated. Though, I could have been coughing at the time and got it wrong. But either way, wouldn't it be helpful to explicitly say what to do about languages in the 'Preparing for an upgrade' docs?

Gábor Hojtsy’s picture

@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"?

maxocub’s picture

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

maxocub’s picture

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?

maxocub’s picture

Status: Needs work » Needs review
Issue tags: +migrate-d7-d8
FileSize
7.22 KB
12.82 KB

@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?

Status: Needs review » Needs work

The last submitted patch, 44: 2657978-44.patch, failed testing.

quietone’s picture

Ah, now I see what was niggling me. Of the current migrations dealing with languages or translations this is the only one that checks that the source language exists on the destination site. Should the other migrations that deal with translations, such as d6_i18n_system_maintenance, also verify that the language exists on the destination?

Gábor Hojtsy’s picture

@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’s picture

@Gábor Hojtsy, sorry, I see that now.

+++ b/core/modules/language/tests/src/Kernel/Migrate/d7/MigrateDefaultLanguageTest.php
@@ -0,0 +1,56 @@
+    $default_language = ConfigurableLanguage::load($langcode);
+    $this->assertNotNull($default_language);
+    $this->assertIdentical($langcode, $this->config('system.site')->get('default_langcode'));

How will work for both cases, when the language exists on the destination and when it doesn't? When the language doesn't exist, $default_language would be NULL and the default_language in the configuration would still be the same as before the migration.

maxocub’s picture

@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.

maxocub’s picture

Status: Needs work » Needs review
FileSize
7.75 KB
7.73 KB

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.

Status: Needs review » Needs work

The last submitted patch, 50: 2657978-50.patch, failed testing.

maxocub’s picture

Status: Needs work » Needs review
Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

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 :)

xjm’s picture

Issue tags: +rc eligible

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

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Gábor Hojtsy’s picture

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

  • catch committed 19387f8 on 8.3.x
    Issue #2657978 by maxocub, quietone, Gábor Hojtsy, Kristen Pol, alexpott...

  • catch committed 58afbce on 8.2.x
    Issue #2657978 by maxocub, quietone, Gábor Hojtsy, Kristen Pol, alexpott...

catch’s picture

Status: Reviewed & tested by the community » Fixed
+++ 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...

maxocub’s picture

Title: Variable to config: language_default [d6] » Variable to config: language_default [D6 & D7]

Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

Gábor Hojtsy’s picture

Issue tags: -sprint

Removing from sprint, thanks all again!

maxocub’s picture