Problem/Motivation

Drupal 6 users get their preferred language code migrated properly to Drupal 8 but not their user profile language code.

With Drupal 7, the users langcodes are migrated to Drupal 8, but they might be empty (if the locale module is not used) or of an non existent language (if the language has been deleted on the D6 site).

Proposed resolution

Fix the migration of their user profile language code.

Remaining tasks

Fix it. Add tests. Review.

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#61 interdiff-56-61.txt2.44 KBmaxocub
#61 2751223-61.patch20.37 KBmaxocub
#59 interdiff-56-59.txt4.14 KBjeroenbegyn
#59 2751223-59.patch17.48 KBjeroenbegyn
#56 interdiff-54-56.txt619 bytesmaxocub
#56 2751223-56.patch20.37 KBmaxocub
#54 interdiff-52-54.txt758 bytesmaxocub
#54 2751223-54.patch20.37 KBmaxocub
#52 interdiff-50-52.txt14.36 KBmaxocub
#52 2751223-52.patch20.32 KBmaxocub
#50 interdiff-47-50.txt3.18 KBmaxocub
#50 2751223-50.patch17.54 KBmaxocub
#47 interdiff-39-47.txt7.75 KBmaxocub
#47 2751223-47.patch16.56 KBmaxocub
#39 interdiff-37-39.txt590 bytesmaxocub
#39 2751223-39.patch13.61 KBmaxocub
#37 interdiff-28-37.txt9.43 KBmaxocub
#37 2751223-37.patch13.04 KBmaxocub
#28 interdiff-22-28.txt3.95 KBmaxocub
#28 2751223-28.patch4.29 KBmaxocub
#22 interdiff-19-22.txt464 bytesmaxocub
#22 2751223-22.patch3.4 KBmaxocub
#19 interdiff-17-19.txt2.85 KBmaxocub
#19 2751223-19.patch3.2 KBmaxocub
#17 interdiff-13-17.txt2.35 KBmaxocub
#17 2751223-17.patch2.98 KBmaxocub
#13 interdiff-10-13.txt2.54 KBmaxocub
#13 2751223-13.patch3.31 KBmaxocub
#10 2751223-10.patch1.33 KBmaxocub
#8 users_field_data.png40.14 KBmaxocub
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

vasi created an issue. See original summary.

vasi’s picture

vasi’s picture

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

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now 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.

maxocub’s picture

I did a migration test, and I found that the D6 users' preferred language does get migrated. It is included in core/modules/user/migration_templates/d6_user.yml:

id: d6_user
label: User accounts
migration_tags:
  - Drupal 6
source:
  plugin: d6_user
process:
  uid: uid 
  name: name
  pass: pass
  mail: mail
  created: created
  access: access
  login: login
  status: status
  timezone:
    plugin: user_update_7002
    source: timezone
  preferred_langcode: language
  init: init
  roles:
    plugin: migration
    migration: d6_user_role
    source: roles
  user_picture:
    plugin: migration
    migration: d6_user_picture_file
    source: uid 
    no_stub: true
destination:
  plugin: entity:user
  md5_passwords: true
migration_dependencies:
  required:
    - d6_user_role
  optional:
    - d6_user_picture_file
    - user_picture_entity_display
    - user_picture_entity_form_display

Here is what I tested:

1) Install Drupal 6
2) Enable the locale module
3) Add a language
4) Add some users with different preferred languages

5) Install Drupal 8
6) Enable the language, migrate, migrate_drupal, migrate_drupal_ui modules
7) Go to /upgrade

When the migration is done, I do see the users with the expected preferred language.

Gábor Hojtsy’s picture

@maxocub: is their user profile langcode also set? If so, this issue may be closed cannot reproduce IMHO. @vasi can you elaborate maybe if we are missing something?

maxocub’s picture

FileSize
40.14 KB

@Gábor Hojtsy: Indeed, in the database, in the users_field_data, the preferred_langcode column have the right langcodes, but not the langcode column, it's all 'en'.

And I found two other things while looking at the DB, there's a User ID 0 and the timezones are not right. See screenshot.

users_field_data

Gábor Hojtsy’s picture

Title: D6 users may be migrated into D8 without a langcode » D6 users are migrated into D8 with correct preferred_langcode but incorrect langcode
Category: Task » Bug report
Issue summary: View changes

So the bug is the langcode column does not get migrated right, while the preferred langcode does. Further proof:

18:24 <maxocub> GaborHojtsy: And for https://www.drupal.org/node/2751223, I will try to do it too, but I added a comment with some additional concerns
18:24 <Druplicon> https://www.drupal.org/node/2751223 => D6 users may be migrated into D8 without a langcode #2751223: D6 & D7 users are migrated into D8 with incorrect langcode => 8 comments, 2 IRC mentions
18:26 <GaborHojtsy> maxocub: thanks
18:26 <GaborHojtsy> maxocub: for this later one, are the D6 users having the right langcode on them (not en)?
18:28 <maxocub> GaborHojtsy: No, they have the right preferred_langcode, but not the right langcode, I added a screenshot as demonstration
18:29 <maxocub> GaborHojtsy: Oh, do you meen the langcode on the D? site?
18:29 <maxocub> *D6
18:29 <GaborHojtsy> maxocub: yeah, if their source langcode is "bad" then it is fine, it they are migrated as-is :)
18:31 <maxocub> In my D6 database, the users table have the right langcodes in the language column
18:39 <GaborHojtsy> maxocub: yeah so that is the bug then :)

Retitling to make this clear. Also updated issue summary and made it a bug.

maxocub’s picture

Status: Active » Needs review
FileSize
1.33 KB

I started working on this and I thought that it would be as simple as adding one line in d6_user.yml and one other line in MigrateUserTest.php, but it turns out that it makes the test fails.

The reason the test fails seems to be that the users with a langcode different than 'en' do not get their roles migrated.
That's weird because when I test the migration manually, all roles do get migrated.

Status: Needs review » Needs work

The last submitted patch, 10: 2751223-10.patch, failed testing.

Gábor Hojtsy’s picture

Hm, that is strange. I would have also expected the same changes needed and no test fails :/

maxocub’s picture

Status: Needs work » Needs review
FileSize
3.31 KB
2.54 KB

Duh, now that we are migrating users with the correct langcode, the languages of those users must exist. So I added 'language' as a dependency of the user migration and I added the language module and migration to the failing tests. They should pass now.

One thing I'm not sure, I changed a user's langcode from 'ro' to 'zu' in the fixture because Romanian is not a language in the fixture. Let's see if it breaks other tests.

maxocub’s picture

Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 13: 2751223-13.patch, failed testing.

maxocub’s picture

OK, the two previous failing tests are now passing, now it's the MigrateNodeTest & MigrateNodeRevisionTest tests that are failing. Investigating...

maxocub’s picture

Status: Needs work » Needs review
FileSize
2.98 KB
2.35 KB

I moved the activation of the language module and the language migration to MigrateDrupal6TestBase->migrateUsers() so that other tests that use this method have the users migrated correctly.

Status: Needs review » Needs work

The last submitted patch, 17: 2751223-17.patch, failed testing.

maxocub’s picture

Status: Needs work » Needs review
FileSize
3.2 KB
2.85 KB

Thinking of it, it doesn't make sense to make the d6_user migration dependent on the language migration since not all D6 sites will have users with languages other than English...

So I remove 'language' from the dependencies and I triggered it in the tests where it was needed.

I think it should pass now.

Gábor Hojtsy’s picture

Hm, that makes sense, thanks for investigating. The ultimate question IMHO is how does this solution solve the issue for migrations in practice. If a migration runs languages after users, would that still fail or potentially have weird things on the way? What does the language migration do on a site where language module is not present? I think it would not apply, so if user requires that then user may not apply either? That sounds like a problem. Looks like some conditional way to add a dependency would solve this. Not sure if that is possible somehow with migrations.

maxocub’s picture

Status: Needs review » Needs work

@Gábor Hojtsy: Good point, some more thinking is needed here. I'll see what I can come up with.

maxocub’s picture

Status: Needs work » Needs review
FileSize
3.4 KB
464 bytes

Thanks to @mikeryan, it seems that an optional dependencies might answer our concerns, so here's a new patch.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Yay, looks good, thanks for the help @mikeryan!

maxocub’s picture

Should we be checking if the language exists on the destination site?

What if someone have a D6 site in English and Spanish, and a user have Spanish as a preferred language, and then the Spanish language gets deleted?

After a migration, that user would possibly be missing roles and field data, since Spanish would not be on the D8 site.

Is that a use case we should cover?

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work

Uhm, yeah if they loose roles and field data, that is a pretty big flaw (or how users are migrated now). If we can figure out a way to solve that, that would be fabulous.

maxocub’s picture

But what kind of solution do we want here if the language of a user no longer exists?

  • Make the migration fail?
  • Migrate that user with the default language of the site?
  • Create the missing language?
  • Make the roles and field data migrate anyway?
  • Something else?
Gábor Hojtsy’s picture

Not sure if we should fail the migration, this sounds like innocent historic problems would then cause bigger havoc then they should. IMHO the user should be migrated with the site default language or if we can still migrate it with an invalid langcode, then do that but make sure the rest of the migrations will not fail on the user. I think loading the user is an issue once it has an invalid language, that is why the roles and fields are not migrated though?

maxocub’s picture

Status: Needs work » Needs review
FileSize
4.29 KB
3.95 KB

I found something else that is wrong with the previous patch. If the locale module is not installed on the D6 site, the user's language will be empty, and also migrated empty, which is bad.

I added a process plugin for the user's langcode that returns:

  • English if the D6 user doesn't have a language,
  • the site's default language if the D6 user's language does not exists,
  • or else, the user's language.

The D6 fixture already contains all those three cases, so I modified the MigrateUserTest to check that their langcodes are valid.

Is this an acceptable solution?

One thing that is missing, is the default_langcode migration in the optional dependencies, since we use the default language if the user's language does not exists. But since that issue, #2657978: Variable to config: language_default [D6 & D7], is not yet commited, I cannot add the dependency here. Should I mark it [PP-1] or something else?

Gábor Hojtsy’s picture

Title: D6 users are migrated into D8 with correct preferred_langcode but incorrect langcode » [PP-1] D6 users are migrated into D8 with correct preferred_langcode but incorrect langcode
Status: Needs review » Postponed

I think these fixes look fantastic. In Drupal 8 one can delete English, but in Drupal 6 that is not possible, so languages migrated from Drupal 6 should include English. Looks like this is postponed on the site default language migration yes.

maxocub’s picture

Out of curiosity, I looked at how D7 users langcode are migrated, and it doesn't seems adequate either. It might migrate non existent or empty langcodes. It does not even depend on the language migration.

Can I apply the solution we have here in this issue (and update the title/summary) or should I open a new one?

Gábor Hojtsy’s picture

I think that largely depends on the extent of changes required in the D7 migration and the similarity to the D6 one :)

maxocub’s picture

The only change needed would be to replace this line:
- langcode: language
by those lines:

+  langcode:
+    plugin: user_langcode
+    source: language

in d7_user.yml

And of course to add test coverage.

Gábor Hojtsy’s picture

I think that is fine to include here.

maxocub’s picture

Title: [PP-1] D6 users are migrated into D8 with correct preferred_langcode but incorrect langcode » [PP-1] D6 & D7 users are migrated into D8 with correct preferred_langcode but incorrect langcode
Issue summary: View changes
maxocub’s picture

Title: [PP-1] D6 & D7 users are migrated into D8 with correct preferred_langcode but incorrect langcode » [PP-1] D6 & D7 users are migrated into D8 with incorrect langcode
Issue tags: +drupal7, +migrate-d7-d8
maxocub’s picture

Title: [PP-1] D6 & D7 users are migrated into D8 with incorrect langcode » D6 & D7 users are migrated into D8 with incorrect langcode
Status: Postponed » Needs work
maxocub’s picture

Status: Needs work » Needs review
FileSize
13.04 KB
9.43 KB

Hmm, I just saw the related issue for D7, #2671312: No default value for User langcode when migrating D7 users with no language, which took a different path. I think it would be best to have the same solution for D6 and for D7. I'll let the reviewers decide which way is preferable.

Anyway, here's the new patch.

Status: Needs review » Needs work

The last submitted patch, 37: 2751223-37.patch, failed testing.

maxocub’s picture

Status: Needs work » Needs review
FileSize
13.61 KB
590 bytes
Gábor Hojtsy’s picture

I closed down #2671312: No default value for User langcode when migrating D7 users with no language as a duplicate of this one because that only dealt with the empty case while this deals with the invalid language case as well, so is more complete. While this issue does not use or introduce a generic solution for default fallback values, the logic is special enough for user languages that it would not be appropriate IMHO to have such a generic fallback plugin.

Gábor Hojtsy’s picture

Otherwise this looks good, and I would RTBC it if not for the lack of credits from #2671312: No default value for User langcode when migrating D7 users with no language, so giving time for the posters there to comment here.

jofitz’s picture

@Gábor Hojtsy: Having reviewed this patch, I agree with your decision to run with this over my attempt. Thanks for giving the opportunity for credits.

Gábor Hojtsy’s picture

Assigning credit then :)

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

And also marking as ready to go in, looks good to me, and credits are now in line.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/user/migration_templates/d6_user.yml
@@ -16,7 +16,11 @@ process:
   preferred_langcode: language
+  preferred_admin_langcode: language

+++ b/core/modules/user/migration_templates/d7_user.yml
@@ -15,7 +15,9 @@ process:
   preferred_langcode: language
   preferred_admin_langcode: language

Don't we have the same issues with these fields being non-existent languages?

Also we add preferred_admin_langcode to the d6 migration with no test afaics.

maxocub’s picture

@alexpott: I thougt so too that it was weird to leave those two fields with a non-existent langcode. But the logic is not the same as for the user langcode, if the field is empty, we should migrate it to the default language, not English, I think. This means that we can't use the same plugin. Should I create a soncond plugin (UserPreferredLangcode) with the correct logic, or should I try to make this one (UserLangcode) be able to take care of the two cases?

maxocub’s picture

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

What about this?

I added a fallback_to_default configuration to the UserLangcode plugin to specify if we should fallback to the default language or not, and I used this plugin for the preferred_langcode and the preferred_admin_langcode.

Also, I added test coverage for D6.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

@alexpott: we already do the cleanup runtime because we don't disallow removing a language when a user/node/etc. has it. So User::getPreferredLangcode() already returns an empty string or the site default langcode if the user had an invalid preferred langcode. That is a situation that can happen in the runtime as well. So while we can clean up the data here, I don't 100% agree it is needed.

Anyway, if this looks better, all for it :)

On actual code review, looks good, only one thing:

+++ b/core/modules/user/migration_templates/d6_user.yml
@@ -19,8 +19,15 @@ process:
+    fallback_to_default: false

If we want to do this, I think this should be named fallback_to_site_default because that makes it clear what default it fallbacks on. Otherwise it also falls back on something so ... :)

phenaproxima’s picture

Issue tags: +Dublin2016

Generally this seems OK; the refactoring of the user test seems to be somewhat out of scope, but I'm not categorically against it.

  1. +++ b/core/modules/user/src/Plugin/migrate/process/UserLangcode.php
    @@ -0,0 +1,48 @@
    +    $language_manager = \Drupal::languageManager();
    

    This should be injected via ContainerFactoryPluginInterface.

  2. +++ b/core/modules/user/src/Plugin/migrate/process/UserLangcode.php
    @@ -0,0 +1,48 @@
    +    if ($value == '') {
    

    Let's make this empty($value).

  3. +++ b/core/modules/user/src/Plugin/migrate/process/UserLangcode.php
    @@ -0,0 +1,48 @@
    +        return 'en';
    

    Why are we defaulting to English? I feel like this should be a configuration option.

  4. +++ b/core/modules/user/src/Plugin/migrate/process/UserLangcode.php
    @@ -0,0 +1,48 @@
    +    // If the user's language does not exists, use the default language.
    

    s/exists/exist

  5. +++ b/core/modules/user/src/Plugin/migrate/process/UserLangcode.php
    @@ -0,0 +1,48 @@
    +    else if ($language_manager->getLanguage($value) === NULL) {
    

    Nit: Should be elseif.

  6. +++ b/core/modules/user/src/Plugin/migrate/process/UserLangcode.php
    @@ -0,0 +1,48 @@
    +    return $value;
    

    Let's hide this behind a 'bypass' configuration option. If that option is set, return $value. Otherwise return NULL so that the migration can skip the row on empty.

  7. +++ b/core/modules/user/tests/src/Kernel/Migrate/d6/MigrateUserTest.php
    @@ -99,15 +105,34 @@ public function testUser() {
    +      $language_manager = \Drupal::languageManager();
    

    Should use $this->container, not \Drupal.

  8. +++ b/core/modules/user/tests/src/Kernel/Migrate/d6/MigrateUserTest.php
    @@ -99,15 +105,34 @@ public function testUser() {
    +      if ($source->language == '') {
    

    Let's change this to empty($source->language) too.

  9. +++ b/core/modules/user/tests/src/Kernel/Migrate/d6/MigrateUserTest.php
    @@ -99,15 +105,34 @@ public function testUser() {
    +        $this->assertIdentical('en', $user->langcode->value);
    +        $this->assertIdentical($default_langcode, $user->preferred_langcode->value);
    +        $this->assertIdentical($default_langcode, $user->preferred_admin_langcode->value);
    

    We should be using assertSame(), not the deprecated assertIdentical().

  10. +++ b/core/modules/user/tests/src/Kernel/Migrate/d6/MigrateUserTest.php
    @@ -99,15 +105,34 @@ public function testUser() {
    +      else if ($language_manager->getLanguage($source->language) === NULL) {
    

    elseif

  11. +++ b/core/modules/user/tests/src/Kernel/Migrate/d7/MigrateUserTest.php
    @@ -61,47 +67,101 @@ protected function setUp() {
    +    $this->assertIdentical($password, $user->getPassword());
    +    $this->assertIdentical($created, $user->getCreatedTime());
    

    Again, assertSame(). Feel free to update all the other assertions as well.

  12. +++ b/core/modules/user/tests/src/Kernel/Migrate/d7/MigrateUserTest.php
    @@ -61,47 +67,101 @@ protected function setUp() {
    +    $this->assertIdentical(!$blocked, $user->isBlocked());
    

    Let's use assertNotSame() or assertFalse() here, rather than negating $blocked.

  13. +++ b/core/modules/user/tests/src/Kernel/Migrate/d7/MigrateUserTest.php
    @@ -61,47 +67,101 @@ protected function setUp() {
    +    foreach ($users as $source) {
    ...
    +      $this->assertEquals($source->uid, \Drupal::service('user.auth')->authenticate($source->name, 'a password'));
    

    $this->container over \Drupal::service().

maxocub’s picture

Status: Needs work » Needs review
FileSize
17.54 KB
3.18 KB

There has been some changes commited today on the D7 MigrateUserTest and the D7 dump, so this patch did apply anymore. First, here's a new patch after a rebase. I'm going to apply the corrections from #48 and #49 in an other patch to make them more apparent.

maxocub’s picture

Status: Needs review » Needs work
maxocub’s picture

Status: Needs work » Needs review
FileSize
20.32 KB
14.36 KB

@alexpott: Re #45, I do agree with Gábor about not needing to clean that data, but if you prefer, why not. Otherwise, I'd be happy to provide a new patch reverting those changes.

@Gábor Hojtsy: Re #48, I changed the configuration name to fallback_to_site_default.

@phenaproxima: Re #49, thanks for the review, I addressed all your comments, except for those two:

3. We default to English because on a D6 site, if the user language column is empty, it means that the locale module was not enabled and that the user is using the default English. Therefore, when we migrate that user, it should be migrated with the 'en' langcode. I don't know if it would be a good thing to make it a configuration option since that plugin is only used for the user langcode (and now preferred langcodes). Hm, thinking of it, it might make sense to get rid of the fallback_to_site_default configuration option and to use something like "fallback: 'en'" or "fallback: 'site_default'", what do you think?

6. I'm not sure I understand what you're suggesting. If the language column is empty, we still need to return a value, since the User entity needs it. Can you elaborate on what you meant?

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

@maxocub, @phenaproxima: yeah rows should not be skipped if the langcode was not there, users should still be migrated in a valid way. Also since this is a special purpose plugin, I don't think there is value in trying to make it try to be more general when it is not. Using fallback_option: site_default would be equally special cased for this, so not sure what helps.

I think this looks good :)

maxocub’s picture

Just added a comment as asked by @phenaproxima at DrupalCon Dublin

phenaproxima’s picture

+++ b/core/modules/user/src/Plugin/migrate/process/UserLangcode.php
@@ -79,6 +79,7 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
+    // If the langode is a valid one, just retrun it.

Should be 'return', not 'retrun', but this can be fixed on commit! :) +1 RTBC.

maxocub’s picture

Damn, only one line added and I succeeded to make a typo.

Gábor Hojtsy’s picture

Still looks good :)

penyaskito’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Novice

This is issue is a candidate for a novice task. There are some minor typos and grammar errors in comments.

  1. +++ b/core/modules/user/src/Plugin/migrate/process/UserLangcode.php
    @@ -0,0 +1,86 @@
    +    // If the langode is a valid one, just return it.
    

    Still a typo in the same line, langode should be langcode.

  2. +++ b/core/modules/user/tests/src/Kernel/Migrate/d6/MigrateUserTest.php
    @@ -91,28 +97,47 @@ public function testUser() {
    +      // Ensure the user's langcode, preferred_langcode,
    +      // preferred_admin_langcode are a valid.
    

    "are a valid" should be "are valid"

  3. +++ b/core/modules/user/tests/src/Kernel/Migrate/d7/MigrateUserTest.php
    @@ -96,37 +101,59 @@ protected function createType($id) {
    +    // Ensure the user's langcode, preferred_langcode,
    +    // preferred_admin_langcode are a valid.
    

    Same here.

jeroenbegyn’s picture

Hi,

I have fixed the typo's.

Regards,

penyaskito’s picture

Thanks for your contribution!

Oops, look like core/modules/user/src/Plugin/migrate/process/UserLangcode.php is missing now, could you fix that?

maxocub’s picture

Status: Needs work » Needs review
Issue tags: -Novice
FileSize
20.37 KB
2.44 KB
Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for fixing the typos :)

The last submitted patch, 59: 2751223-59.patch, failed testing.

  • catch committed 50d30d9 on 8.3.x
    Issue #2751223 by maxocub, jeroenbegyn, Gábor Hojtsy, Jo Fitzgerald: D6...

  • catch committed c40b4ce on 8.2.x
    Issue #2751223 by maxocub, jeroenbegyn, Gábor Hojtsy, Jo Fitzgerald: D6...
catch’s picture

Version: 8.3.x-dev » 8.2.x-dev
Status: Reviewed & tested by the community » Fixed
Issue tags: +Migrate critical

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

Retrospectively tagging migrate critical.

Gábor Hojtsy’s picture

Issue tags: -sprint

Yay, amazing, thanks!

Status: Fixed » Closed (fixed)

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

maxocub’s picture