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.
Comment | File | Size | Author |
---|---|---|---|
#61 | interdiff-56-61.txt | 2.44 KB | maxocub |
#61 | 2751223-61.patch | 20.37 KB | maxocub |
#59 | interdiff-56-59.txt | 4.14 KB | jeroenbegyn |
#59 | 2751223-59.patch | 17.48 KB | jeroenbegyn |
#56 | interdiff-54-56.txt | 619 bytes | maxocub |
Comments
Comment #2
vasi CreditAttribution: vasi at Evolving Web commentedComment #3
vasi CreditAttribution: vasi at Evolving Web commentedComment #4
Gábor HojtsyUsers in Drupal 6 do have a language setting:
http://cgit.drupalcode.org/drupal/tree/modules/user/user.install?h=6.x#n231
http://cgit.drupalcode.org/drupal/tree/modules/user/user.module?h=6.x#n2192
http://cgit.drupalcode.org/drupal/tree/modules/locale/locale.module?h=6....
Comment #6
maxocub CreditAttribution: maxocub commentedI 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:
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.
Comment #7
Gábor Hojtsy@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?
Comment #8
maxocub CreditAttribution: maxocub commented@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.
Comment #9
Gábor HojtsySo the bug is the langcode column does not get migrated right, while the preferred langcode does. Further proof:
Retitling to make this clear. Also updated issue summary and made it a bug.
Comment #10
maxocub CreditAttribution: maxocub commentedI 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.
Comment #12
Gábor HojtsyHm, that is strange. I would have also expected the same changes needed and no test fails :/
Comment #13
maxocub CreditAttribution: maxocub commentedDuh, 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.
Comment #14
maxocub CreditAttribution: maxocub commentedComment #16
maxocub CreditAttribution: maxocub commentedOK, the two previous failing tests are now passing, now it's the MigrateNodeTest & MigrateNodeRevisionTest tests that are failing. Investigating...
Comment #17
maxocub CreditAttribution: maxocub commentedI 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.
Comment #19
maxocub CreditAttribution: maxocub commentedThinking 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.
Comment #20
Gábor HojtsyHm, 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.
Comment #21
maxocub CreditAttribution: maxocub commented@Gábor Hojtsy: Good point, some more thinking is needed here. I'll see what I can come up with.
Comment #22
maxocub CreditAttribution: maxocub commentedThanks to @mikeryan, it seems that an optional dependencies might answer our concerns, so here's a new patch.
Comment #23
Gábor HojtsyYay, looks good, thanks for the help @mikeryan!
Comment #24
maxocub CreditAttribution: maxocub commentedShould 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?
Comment #25
Gábor HojtsyUhm, 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.
Comment #26
maxocub CreditAttribution: maxocub commentedBut what kind of solution do we want here if the language of a user no longer exists?
Comment #27
Gábor HojtsyNot 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?
Comment #28
maxocub CreditAttribution: maxocub commentedI 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:
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?
Comment #29
Gábor HojtsyI 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.
Comment #30
maxocub CreditAttribution: maxocub commentedOut 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?
Comment #31
Gábor HojtsyI think that largely depends on the extent of changes required in the D7 migration and the similarity to the D6 one :)
Comment #32
maxocub CreditAttribution: maxocub commentedThe only change needed would be to replace this line:
- langcode: language
by those lines:
in d7_user.yml
And of course to add test coverage.
Comment #33
Gábor HojtsyI think that is fine to include here.
Comment #34
maxocub CreditAttribution: maxocub commentedComment #35
maxocub CreditAttribution: maxocub commentedComment #36
maxocub CreditAttribution: maxocub commentedComment #37
maxocub CreditAttribution: maxocub commentedHmm, 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.
Comment #39
maxocub CreditAttribution: maxocub commentedComment #40
Gábor HojtsyI 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.
Comment #41
Gábor HojtsyOtherwise 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.
Comment #42
jofitz CreditAttribution: jofitz at ComputerMinds commented@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.
Comment #43
Gábor HojtsyAssigning credit then :)
Comment #44
Gábor HojtsyAnd also marking as ready to go in, looks good to me, and credits are now in line.
Comment #45
alexpottDon'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.
Comment #46
maxocub CreditAttribution: maxocub commented@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?
Comment #47
maxocub CreditAttribution: maxocub commentedWhat 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.
Comment #48
Gábor Hojtsy@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:
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 ... :)
Comment #49
phenaproximaGenerally this seems OK; the refactoring of the user test seems to be somewhat out of scope, but I'm not categorically against it.
This should be injected via ContainerFactoryPluginInterface.
Let's make this
empty($value)
.Why are we defaulting to English? I feel like this should be a configuration option.
s/exists/exist
Nit: Should be elseif.
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.
Should use $this->container, not \Drupal.
Let's change this to
empty($source->language)
too.We should be using assertSame(), not the deprecated assertIdentical().
elseif
Again, assertSame(). Feel free to update all the other assertions as well.
Let's use assertNotSame() or assertFalse() here, rather than negating $blocked.
$this->container over \Drupal::service().
Comment #50
maxocub CreditAttribution: maxocub commentedThere 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.
Comment #51
maxocub CreditAttribution: maxocub commentedComment #52
maxocub CreditAttribution: maxocub commented@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?
Comment #53
Gábor Hojtsy@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 :)
Comment #54
maxocub CreditAttribution: maxocub commentedJust added a comment as asked by @phenaproxima at DrupalCon Dublin
Comment #55
phenaproximaShould be 'return', not 'retrun', but this can be fixed on commit! :) +1 RTBC.
Comment #56
maxocub CreditAttribution: maxocub commentedDamn, only one line added and I succeeded to make a typo.
Comment #57
Gábor HojtsyStill looks good :)
Comment #58
penyaskitoThis is issue is a candidate for a novice task. There are some minor typos and grammar errors in comments.
Still a typo in the same line, langode should be langcode.
"are a valid" should be "are valid"
Same here.
Comment #59
jeroenbegyn CreditAttribution: jeroenbegyn at SQLI - Belgium commentedHi,
I have fixed the typo's.
Regards,
Comment #60
penyaskitoThanks for your contribution!
Oops, look like core/modules/user/src/Plugin/migrate/process/UserLangcode.php is missing now, could you fix that?
Comment #61
maxocub CreditAttribution: maxocub commentedComment #62
Gábor HojtsyThanks for fixing the typos :)
Comment #66
catchCommitted/pushed to 8.3.x and cherry-picked to 8.2.x. Thanks!
Retrospectively tagging migrate critical.
Comment #67
Gábor HojtsyYay, amazing, thanks!
Comment #69
maxocub CreditAttribution: maxocub as a volunteer and commented